[Qemu-devel] [PATCH v2 0/4] slirp: Fix for requeuing crash, cleanups

[Qemu-devel] [PATCH v2 0/4] slirp: Fix for requeuing crash, cleanups

[Jan Kiszka]

Well, this requeuing bug seems to have a long breath. Previous attempts
to fix it (mine included) neglected the fact that we need to walk the
queue of pending packets, not just restart from the beginning after a
requeue. This version should get it Right(TM).

This also comes with a fix for resource cleanups on slirp shutdown. At
least valgrind is happy now.

Changes in v2:
 - fixed corner case of session list walk that Stefan Weil reported

CC: Fabien Chouteau <chouteau@adacore.com>
CC: Michael S. Tsirkin <mst@redhat.com>
CC: Stefan Weil <sw@weilnetz.de>
CC: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>

Jan Kiszka (4):
  slirp: Keep next_m always valid
  slirp: Fix queue walking in if_start
  slirp: Remove unneeded if_queued
  slirp: Cleanup resources on instance removal

 slirp/if.c       |   64 +++++++++++++++++++++++++++++------------------------
 slirp/ip_icmp.c  |    7 ++++++
 slirp/ip_icmp.h  |    1 +
 slirp/ip_input.c |    7 ++++++
 slirp/mbuf.c     |   21 +++++++++++++++++
 slirp/mbuf.h     |    1 +
 slirp/slirp.c    |   10 +++-----
 slirp/slirp.h    |    3 +-
 slirp/tcp_subr.c |    7 ++++++
 slirp/udp.c      |    8 ++++++
 slirp/udp.h      |    1 +
 11 files changed, 94 insertions(+), 36 deletions(-)

-- 
1.7.3.4

[Qemu-devel] [PATCH v2 1/4] slirp: Keep next_m always valid

[Jan Kiszka]

Make sure that next_m always points to a packet if batchq is non-empty.
This will simplify walking the queues in if_start.

CC: Fabien Chouteau <chouteau@adacore.com>
CC: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
CC: Stefan Weil <sw@weilnetz.de>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 slirp/if.c |   22 ++++++++++++++--------
 1 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/slirp/if.c b/slirp/if.c
index 33f08e1..14fdef1 100644
--- a/slirp/if.c
+++ b/slirp/if.c
@@ -96,8 +96,13 @@ if_output(struct socket *so, struct mbuf *ifm)
 			ifs_insque(ifm, ifq->ifs_prev);
 			goto diddit;
 		}
-	} else
+        } else {
 		ifq = slirp->if_batchq.ifq_prev;
+                /* Set next_m if the queue was empty so far */
+                if (slirp->next_m == &slirp->if_batchq) {
+                    slirp->next_m = ifm;
+                }
+        }
 
 	/* Create a new doubly linked list for this session */
 	ifm->ifq_so = so;
@@ -170,13 +175,8 @@ void if_start(Slirp *slirp)
         if (slirp->if_fastq.ifq_next != &slirp->if_fastq) {
             ifm = slirp->if_fastq.ifq_next;
         } else {
-            /* Nothing on fastq, see if next_m is valid */
-            if (slirp->next_m != &slirp->if_batchq) {
-                ifm = slirp->next_m;
-            } else {
-                ifm = slirp->if_batchq.ifq_next;
-            }
-
+            /* Nothing on fastq, pick up from batchq via next_m */
+            ifm = slirp->next_m;
             from_batchq = true;
         }
 
@@ -202,6 +202,12 @@ void if_start(Slirp *slirp)
         if (ifm->ifs_next != ifm) {
             insque(ifm->ifs_next, ifqt);
             ifs_remque(ifm);
+            /* Set next_m if the session packet is now the only one on
+             * batchq */
+            if (ifqt == &slirp->if_batchq &&
+                slirp->next_m == &slirp->if_batchq) {
+                slirp->next_m = ifm->ifs_next;
+            }
         }
 
         /* Update so_queued */
-- 
1.7.3.4

[Qemu-devel] [PATCH v2 2/4] slirp: Fix queue walking in if_start

[Jan Kiszka]

Another attempt to get this right: We need to carefully walk both the
fastq and the batchq in if_start while trying to send packets to
possibly not yet resolved hosts on the virtual network.

So far we just requeued a delayed packet where it was and then started
walking the queues from the top again - that couldn't work. Now we pre-
calculate the next packet in the queue so that the current one can
safely be removed if it was sent successfully. We also need to take into
account that the next packet can be from the same session if the current
one was sent and there are no other sessions.

CC: Fabien Chouteau <chouteau@adacore.com>
CC: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
CC: Stefan Weil <sw@weilnetz.de>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 slirp/if.c |   46 +++++++++++++++++++++++++++-------------------
 1 files changed, 27 insertions(+), 19 deletions(-)

diff --git a/slirp/if.c b/slirp/if.c
index 14fdef1..6f59620 100644
--- a/slirp/if.c
+++ b/slirp/if.c
@@ -158,26 +158,35 @@ void if_start(Slirp *slirp)
 {
     uint64_t now = qemu_get_clock_ns(rt_clock);
     int requeued = 0;
-    bool from_batchq = false;
-    struct mbuf *ifm, *ifqt;
+    struct mbuf *ifm, *ifm_next, *ifqt;
 
     DEBUG_CALL("if_start");
 
-    while (slirp->if_queued) {
+    if (slirp->if_fastq.ifq_next != &slirp->if_fastq) {
+        ifm_next = slirp->if_fastq.ifq_next;
+    } else if (slirp->next_m != &slirp->if_batchq) {
+        /* Nothing on fastq, pick up from batchq via next_m */
+        ifm_next = slirp->next_m;
+    } else {
+        ifm_next = NULL;
+    }
+
+    while (ifm_next) {
         /* check if we can really output */
-        if (!slirp_can_output(slirp->opaque))
+        if (!slirp_can_output(slirp->opaque)) {
             return;
+        }
 
-        /*
-         * See which queue to get next packet from
-         * If there's something in the fastq, select it immediately
-         */
-        if (slirp->if_fastq.ifq_next != &slirp->if_fastq) {
-            ifm = slirp->if_fastq.ifq_next;
-        } else {
-            /* Nothing on fastq, pick up from batchq via next_m */
-            ifm = slirp->next_m;
-            from_batchq = true;
+        ifm = ifm_next;
+
+        ifm_next = ifm->ifq_next;
+        if (ifm_next == &slirp->if_fastq) {
+            /* No more packets in fastq, switch to batchq */
+            ifm_next = slirp->next_m;
+        }
+        if (ifm_next == &slirp->if_batchq) {
+            /* end of batchq */
+            ifm_next = NULL;
         }
 
         slirp->if_queued--;
@@ -189,7 +198,7 @@ void if_start(Slirp *slirp)
             continue;
         }
 
-        if (from_batchq) {
+        if (ifm == slirp->next_m) {
             /* Set which packet to send on next iteration */
             slirp->next_m = ifm->ifq_next;
         }
@@ -202,11 +211,11 @@ void if_start(Slirp *slirp)
         if (ifm->ifs_next != ifm) {
             insque(ifm->ifs_next, ifqt);
             ifs_remque(ifm);
-            /* Set next_m if the session packet is now the only one on
-             * batchq */
+            /* Set next_m and ifm_next if the session packet is now the only
+             * one on batchq */
             if (ifqt == &slirp->if_batchq &&
                 slirp->next_m == &slirp->if_batchq) {
-                slirp->next_m = ifm->ifs_next;
+                slirp->next_m = ifm_next = ifm->ifs_next;
             }
         }
 
@@ -217,7 +226,6 @@ void if_start(Slirp *slirp)
         }
 
         m_free(ifm);
-
     }
 
     slirp->if_queued = requeued;
-- 
1.7.3.4

[Qemu-devel] [PATCH v2 3/4] slirp: Remove unneeded if_queued

[Jan Kiszka]

There is now a trivial check on entry of if_start for pending packets,
so we can drop the additional tracking via if_queued.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 slirp/if.c    |    8 --------
 slirp/slirp.c |    7 +------
 slirp/slirp.h |    1 -
 3 files changed, 1 insertions(+), 15 deletions(-)

diff --git a/slirp/if.c b/slirp/if.c
index 6f59620..fee745a 100644
--- a/slirp/if.c
+++ b/slirp/if.c
@@ -110,8 +110,6 @@ if_output(struct socket *so, struct mbuf *ifm)
 	insque(ifm, ifq);
 
 diddit:
-	slirp->if_queued++;
-
 	if (so) {
 		/* Update *_queued */
 		so->so_queued++;
@@ -157,7 +155,6 @@ diddit:
 void if_start(Slirp *slirp)
 {
     uint64_t now = qemu_get_clock_ns(rt_clock);
-    int requeued = 0;
     struct mbuf *ifm, *ifm_next, *ifqt;
 
     DEBUG_CALL("if_start");
@@ -189,12 +186,9 @@ void if_start(Slirp *slirp)
             ifm_next = NULL;
         }
 
-        slirp->if_queued--;
-
         /* Try to send packet unless it already expired */
         if (ifm->expiration_date >= now && !if_encap(slirp, ifm)) {
             /* Packet is delayed due to pending ARP resolution */
-            requeued++;
             continue;
         }
 
@@ -227,6 +221,4 @@ void if_start(Slirp *slirp)
 
         m_free(ifm);
     }
-
-    slirp->if_queued = requeued;
 }
diff --git a/slirp/slirp.c b/slirp/slirp.c
index 19d69eb..bcffc34 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -581,12 +581,7 @@ void slirp_select_poll(fd_set *readfds, fd_set *writefds, fd_set *xfds,
                 }
 	}
 
-	/*
-	 * See if we can start outputting
-	 */
-	if (slirp->if_queued) {
-	    if_start(slirp);
-	}
+        if_start(slirp);
     }
 
 	/* clear global file descriptor sets.
diff --git a/slirp/slirp.h b/slirp/slirp.h
index 28a5c03..950eccd 100644
--- a/slirp/slirp.h
+++ b/slirp/slirp.h
@@ -235,7 +235,6 @@ struct Slirp {
     int mbuf_alloced;
 
     /* if states */
-    int if_queued;          /* number of packets queued so far */
     struct mbuf if_fastq;   /* fast queue (for interactive data) */
     struct mbuf if_batchq;  /* queue for non-interactive data */
     struct mbuf *next_m;    /* pointer to next mbuf to output */
-- 
1.7.3.4

[Qemu-devel] [PATCH v2 4/4] slirp: Cleanup resources on instance removal

[Jan Kiszka]

Close & free sockets when shutting down a slirp instance, also release
all buffers.

CC: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 slirp/ip_icmp.c  |    7 +++++++
 slirp/ip_icmp.h  |    1 +
 slirp/ip_input.c |    7 +++++++
 slirp/mbuf.c     |   21 +++++++++++++++++++++
 slirp/mbuf.h     |    1 +
 slirp/slirp.c    |    3 +++
 slirp/slirp.h    |    2 ++
 slirp/tcp_subr.c |    7 +++++++
 slirp/udp.c      |    8 ++++++++
 slirp/udp.h      |    1 +
 10 files changed, 58 insertions(+), 0 deletions(-)

diff --git a/slirp/ip_icmp.c b/slirp/ip_icmp.c
index 5dbf21d..d571fd0 100644
--- a/slirp/ip_icmp.c
+++ b/slirp/ip_icmp.c
@@ -66,6 +66,13 @@ void icmp_init(Slirp *slirp)
     slirp->icmp_last_so = &slirp->icmp;
 }
 
+void icmp_cleanup(Slirp *slirp)
+{
+    while (slirp->icmp.so_next != &slirp->icmp) {
+        icmp_detach(slirp->icmp.so_next);
+    }
+}
+
 static int icmp_send(struct socket *so, struct mbuf *m, int hlen)
 {
     struct ip *ip = mtod(m, struct ip *);
diff --git a/slirp/ip_icmp.h b/slirp/ip_icmp.h
index b3da1f2..1a1af91 100644
--- a/slirp/ip_icmp.h
+++ b/slirp/ip_icmp.h
@@ -154,6 +154,7 @@ struct icmp {
 	(type) == ICMP_MASKREQ || (type) == ICMP_MASKREPLY)
 
 void icmp_init(Slirp *slirp);
+void icmp_cleanup(Slirp *slirp);
 void icmp_input(struct mbuf *, int);
 void icmp_error(struct mbuf *msrc, u_char type, u_char code, int minsize,
                 const char *message);
diff --git a/slirp/ip_input.c b/slirp/ip_input.c
index c7b3eb4..ce24faf 100644
--- a/slirp/ip_input.c
+++ b/slirp/ip_input.c
@@ -61,6 +61,13 @@ ip_init(Slirp *slirp)
     icmp_init(slirp);
 }
 
+void ip_cleanup(Slirp *slirp)
+{
+    udp_cleanup(slirp);
+    tcp_cleanup(slirp);
+    icmp_cleanup(slirp);
+}
+
 /*
  * Ip input routine.  Checksum and byte swap header.  If fragmented
  * try to reassemble.  Process options.  Pass to next level.
diff --git a/slirp/mbuf.c b/slirp/mbuf.c
index c699c75..4fefb04 100644
--- a/slirp/mbuf.c
+++ b/slirp/mbuf.c
@@ -32,6 +32,27 @@ m_init(Slirp *slirp)
     slirp->m_usedlist.m_next = slirp->m_usedlist.m_prev = &slirp->m_usedlist;
 }
 
+void m_cleanup(Slirp *slirp)
+{
+    struct mbuf *m, *next;
+
+    m = slirp->m_usedlist.m_next;
+    while (m != &slirp->m_usedlist) {
+        next = m->m_next;
+        if (m->m_flags & M_EXT) {
+            free(m->m_ext);
+        }
+        free(m);
+        m = next;
+    }
+    m = slirp->m_freelist.m_next;
+    while (m != &slirp->m_freelist) {
+        next = m->m_next;
+        free(m);
+        m = next;
+    }
+}
+
 /*
  * Get an mbuf from the free list, if there are none
  * malloc one
diff --git a/slirp/mbuf.h b/slirp/mbuf.h
index 8d7951f..3f3ab09 100644
--- a/slirp/mbuf.h
+++ b/slirp/mbuf.h
@@ -116,6 +116,7 @@ struct mbuf {
 					 * it rather than putting it on the free list */
 
 void m_init(Slirp *);
+void m_cleanup(Slirp *slirp);
 struct mbuf * m_get(Slirp *);
 void m_free(struct mbuf *);
 void m_cat(register struct mbuf *, register struct mbuf *);
diff --git a/slirp/slirp.c b/slirp/slirp.c
index bcffc34..1502830 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -246,6 +246,9 @@ void slirp_cleanup(Slirp *slirp)
 
     unregister_savevm(NULL, "slirp", slirp);
 
+    ip_cleanup(slirp);
+    m_cleanup(slirp);
+
     g_free(slirp->tftp_prefix);
     g_free(slirp->bootp_filename);
     g_free(slirp);
diff --git a/slirp/slirp.h b/slirp/slirp.h
index 950eccd..013a3b3 100644
--- a/slirp/slirp.h
+++ b/slirp/slirp.h
@@ -314,6 +314,7 @@ void if_output(struct socket *, struct mbuf *);
 
 /* ip_input.c */
 void ip_init(Slirp *);
+void ip_cleanup(Slirp *);
 void ip_input(struct mbuf *);
 void ip_slowtimo(Slirp *);
 void ip_stripoptions(register struct mbuf *, struct mbuf *);
@@ -331,6 +332,7 @@ void tcp_setpersist(register struct tcpcb *);
 
 /* tcp_subr.c */
 void tcp_init(Slirp *);
+void tcp_cleanup(Slirp *);
 void tcp_template(struct tcpcb *);
 void tcp_respond(struct tcpcb *, register struct tcpiphdr *, register struct mbuf *, tcp_seq, tcp_seq, int);
 struct tcpcb * tcp_newtcpcb(struct socket *);
diff --git a/slirp/tcp_subr.c b/slirp/tcp_subr.c
index 143a238..6f6585a 100644
--- a/slirp/tcp_subr.c
+++ b/slirp/tcp_subr.c
@@ -55,6 +55,13 @@ tcp_init(Slirp *slirp)
     slirp->tcp_last_so = &slirp->tcb;
 }
 
+void tcp_cleanup(Slirp *slirp)
+{
+    while (slirp->tcb.so_next != &slirp->tcb) {
+        tcp_close(sototcpcb(slirp->tcb.so_next));
+    }
+}
+
 /*
  * Create template to be used to send tcp packets on a connection.
  * Call after host entry created, fills
diff --git a/slirp/udp.c b/slirp/udp.c
index 5b060f3..ced5096 100644
--- a/slirp/udp.c
+++ b/slirp/udp.c
@@ -49,6 +49,14 @@ udp_init(Slirp *slirp)
     slirp->udb.so_next = slirp->udb.so_prev = &slirp->udb;
     slirp->udp_last_so = &slirp->udb;
 }
+
+void udp_cleanup(Slirp *slirp)
+{
+    while (slirp->udb.so_next != &slirp->udb) {
+        udp_detach(slirp->udb.so_next);
+    }
+}
+
 /* m->m_data  points at ip packet header
  * m->m_len   length ip packet
  * ip->ip_len length data (IPDU)
diff --git a/slirp/udp.h b/slirp/udp.h
index 9b5c3cf..9bf31fe 100644
--- a/slirp/udp.h
+++ b/slirp/udp.h
@@ -74,6 +74,7 @@ struct udpiphdr {
 struct mbuf;
 
 void udp_init(Slirp *);
+void udp_cleanup(Slirp *);
 void udp_input(register struct mbuf *, int);
 int udp_output(struct socket *, struct mbuf *, struct sockaddr_in *);
 int udp_attach(struct socket *);
-- 
1.7.3.4

Re: [Qemu-devel] [PATCH v2 0/4] slirp: Fix for requeuing crash, cleanups

[Stefan Weil]

Am 02.03.2012 19:57, schrieb Jan Kiszka:
> Well, this requeuing bug seems to have a long breath. Previous attempts
> to fix it (mine included) neglected the fact that we need to walk the
> queue of pending packets, not just restart from the beginning after a
> requeue. This version should get it Right(TM).
>
> This also comes with a fix for resource cleanups on slirp shutdown. At
> least valgrind is happy now.
>
> Changes in v2:
> - fixed corner case of session list walk that Stefan Weil reported
>
> CC: Fabien Chouteau <chouteau@adacore.com>
> CC: Michael S. Tsirkin <mst@redhat.com>
> CC: Stefan Weil <sw@weilnetz.de>
> CC: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>
> Jan Kiszka (4):
> slirp: Keep next_m always valid
> slirp: Fix queue walking in if_start
> slirp: Remove unneeded if_queued
> slirp: Cleanup resources on instance removal
>
> slirp/if.c | 64 +++++++++++++++++++++++++++++------------------------
> slirp/ip_icmp.c | 7 ++++++
> slirp/ip_icmp.h | 1 +
> slirp/ip_input.c | 7 ++++++
> slirp/mbuf.c | 21 +++++++++++++++++
> slirp/mbuf.h | 1 +
> slirp/slirp.c | 10 +++-----
> slirp/slirp.h | 3 +-
> slirp/tcp_subr.c | 7 ++++++
> slirp/udp.c | 8 ++++++
> slirp/udp.h | 1 +
> 11 files changed, 94 insertions(+), 36 deletions(-)

Hi Jan,

this is what I get with your new patch series.

Regards,
Stefan


Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffe9bf0700 (LWP 5863)]
0x00005555557781bf in slirp_remque (a=0x5555569916b0) at 
/home/stefan/src/qemu/repo.or.cz/qemu/ar7/slirp/misc.c:39
39        ((struct quehead *)(element->qh_rlink))->qh_link = 
element->qh_link;
(gdb) i s
#0  0x00005555557781bf in slirp_remque (a=0x5555569916b0) at 
/home/stefan/src/qemu/repo.or.cz/qemu/ar7/slirp/misc.c:39
#1  0x0000555555777b00 in m_get (slirp=0x5555562bdb80) at 
/home/stefan/src/qemu/repo.or.cz/qemu/ar7/slirp/mbuf.c:81
#2  0x000055555577abdf in slirp_input (slirp=0x5555562bdb80, 
pkt=0x555556305d58 "RU\n", pkt_len=54) at 
/home/stefan/src/qemu/repo.or.cz/qemu/ar7/slirp/slirp.c:673
#3  0x0000555555730f8b in net_slirp_receive (nc=0x5555562bd950, 
buf=0x555556305d58 "RU\n", size=54) at 
/home/stefan/src/qemu/repo.or.cz/qemu/ar7/net/slirp.c:116
#4  0x000055555572dc11 in qemu_vlan_deliver_packet 
(sender=0x5555563074c0, flags=0, buf=0x555556305d58 "RU\n", size=54, 
opaque=0x5555562bd8b0)
     at /home/stefan/src/qemu/repo.or.cz/qemu/ar7/net.c:451
#5  0x0000555555730938 in qemu_net_queue_deliver (queue=0x5555562bd8f0, 
sender=0x5555563074c0, flags=0, data=0x555556305d58 "RU\n", size=54)
     at /home/stefan/src/qemu/repo.or.cz/qemu/ar7/net/queue.c:154
#6  0x0000555555730a78 in qemu_net_queue_send (queue=0x5555562bd8f0, 
sender=0x5555563074c0, flags=0, data=0x555556305d58 "RU\n", size=54, 
sent_cb=0)
     at /home/stefan/src/qemu/repo.or.cz/qemu/ar7/net/queue.c:188
#7  0x000055555572de30 in qemu_send_packet_async_with_flags 
(sender=0x5555563074c0, flags=0, buf=0x555556305d58 "RU\n", size=54, 
sent_cb=0)
     at /home/stefan/src/qemu/repo.or.cz/qemu/ar7/net.c:519
#8  0x000055555572de8b in qemu_send_packet_async (sender=0x5555563074c0, 
buf=0x555556305d58 "RU\n", size=54, sent_cb=0) at 
/home/stefan/src/qemu/repo.or.cz/qemu/ar7/net.c:526
#9  0x000055555572dedb in qemu_send_packet (vc=0x5555563074c0, 
buf=0x555556305d58 "RU\n", size=54) at 
/home/stefan/src/qemu/repo.or.cz/qemu/ar7/net.c:532
#10 0x00005555556e9daa in pcnet_transmit (s=0x555556305af8) at 
/home/stefan/src/qemu/repo.or.cz/qemu/ar7/hw/pcnet.c:1258
#11 0x00005555556ea0fd in pcnet_poll_timer (opaque=0x555556305af8) at 
/home/stefan/src/qemu/repo.or.cz/qemu/ar7/hw/pcnet.c:1321
#12 0x00005555556ea8e9 in pcnet_ioport_writew (opaque=0x555556305af8, 
addr=18, val=0) at /home/stefan/src/qemu/repo.or.cz/qemu/ar7/hw/pcnet.c:1571
#13 0x00005555556e62b3 in pcnet_ioport_write (opaque=0x555556305af8, 
addr=18, data=0, size=2) at 
/home/stefan/src/qemu/repo.or.cz/qemu/ar7/hw/pcnet-pci.c:120
#14 0x0000555555801c8b in memory_region_write_accessor 
(opaque=0x555556306d80, addr=18, value=0x7fffe9bef690, size=2, shift=0, 
mask=65535)
     at /home/stefan/src/qemu/repo.or.cz/qemu/ar7/memory.c:329
#15 0x0000555555801d6d in access_with_adjusted_size (addr=18, 
value=0x7fffe9bef690, size=2, access_size_min=1, access_size_max=4,
     access=0x555555801c13 <memory_region_write_accessor>, 
opaque=0x555556306d80) at 
/home/stefan/src/qemu/repo.or.cz/qemu/ar7/memory.c:359
#16 0x000055555580217d in memory_region_iorange_write 
(iorange=0x555556306dc0, offset=18, width=2, data=0) at 
/home/stefan/src/qemu/repo.or.cz/qemu/ar7/memory.c:428
#17 0x00005555557fb41c in ioport_writew_thunk (opaque=0x555556306dc0, 
addr=4146, data=0) at /home/stefan/src/qemu/repo.or.cz/qemu/ar7/ioport.c:218
#18 0x00005555557facb5 in ioport_write (index=1, address=4146, data=0) 
at /home/stefan/src/qemu/repo.or.cz/qemu/ar7/ioport.c:82
#19 0x00005555557fb8a3 in cpu_outw (addr=4146, val=0) at 
/home/stefan/src/qemu/repo.or.cz/qemu/ar7/ioport.c:281
#20 0x00005555556c7ae4 in isa_mmio_writew (opaque=0x0, addr=4146, val=0) 
at /home/stefan/src/qemu/repo.or.cz/qemu/ar7/hw/isa_mmio.c:38
#21 0x000055555580477f in memory_region_dispatch_write 
(mr=0x5555562ffc38, addr=4146, data=0, size=2) at 
/home/stefan/src/qemu/repo.or.cz/qemu/ar7/memory.c:913
#22 0x0000555555807184 in io_mem_write (io_index=38, addr=4146, val=0, 
size=2) at /home/stefan/src/qemu/repo.or.cz/qemu/ar7/memory.c:1502
#23 0x000055555581d4e3 in io_writew (physaddr=4146, val=0, 
addr=3087011890, retaddr=0x4034685f) at 
/home/stefan/src/qemu/repo.or.cz/qemu/ar7/softmmu_template.h:225
#24 0x000055555581d5cc in __stw_mmu (addr=3087011890, val=0, mmu_idx=0) 
at /home/stefan/src/qemu/repo.or.cz/qemu/ar7/softmmu_template.h:257
#25 0x0000000040346860 in ?? ()
#26 0x0000000000000000 in ?? ()
(gdb) p ((struct quehead *)(element->qh_rlink))
$1 = (struct quehead *) 0x0

Re: [Qemu-devel] [PATCH v2 0/4] slirp: Fix for requeuing crash, cleanups

[Jan Kiszka]

On 2012-03-02 22:05, Stefan Weil wrote:
> Am 02.03.2012 19:57, schrieb Jan Kiszka:
>> Well, this requeuing bug seems to have a long breath. Previous attempts
>> to fix it (mine included) neglected the fact that we need to walk the
>> queue of pending packets, not just restart from the beginning after a
>> requeue. This version should get it Right(TM).
>>
>> This also comes with a fix for resource cleanups on slirp shutdown. At
>> least valgrind is happy now.
>>
>> Changes in v2:
>> - fixed corner case of session list walk that Stefan Weil reported
>>
>> CC: Fabien Chouteau <chouteau@adacore.com>
>> CC: Michael S. Tsirkin <mst@redhat.com>
>> CC: Stefan Weil <sw@weilnetz.de>
>> CC: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>>
>> Jan Kiszka (4):
>> slirp: Keep next_m always valid
>> slirp: Fix queue walking in if_start
>> slirp: Remove unneeded if_queued
>> slirp: Cleanup resources on instance removal
>>
>> slirp/if.c | 64 +++++++++++++++++++++++++++++------------------------
>> slirp/ip_icmp.c | 7 ++++++
>> slirp/ip_icmp.h | 1 +
>> slirp/ip_input.c | 7 ++++++
>> slirp/mbuf.c | 21 +++++++++++++++++
>> slirp/mbuf.h | 1 +
>> slirp/slirp.c | 10 +++-----
>> slirp/slirp.h | 3 +-
>> slirp/tcp_subr.c | 7 ++++++
>> slirp/udp.c | 8 ++++++
>> slirp/udp.h | 1 +
>> 11 files changed, 94 insertions(+), 36 deletions(-)
> 
> Hi Jan,
> 
> this is what I get with your new patch series.
> 
> Regards,
> Stefan
> 
> 
> Program received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 0x7fffe9bf0700 (LWP 5863)]
> 0x00005555557781bf in slirp_remque (a=0x5555569916b0) at 
> /home/stefan/src/qemu/repo.or.cz/qemu/ar7/slirp/misc.c:39
> 39        ((struct quehead *)(element->qh_rlink))->qh_link = 
> element->qh_link;
> (gdb) i s
> #0  0x00005555557781bf in slirp_remque (a=0x5555569916b0) at 
> /home/stefan/src/qemu/repo.or.cz/qemu/ar7/slirp/misc.c:39
> #1  0x0000555555777b00 in m_get (slirp=0x5555562bdb80) at 
> /home/stefan/src/qemu/repo.or.cz/qemu/ar7/slirp/mbuf.c:81
> #2  0x000055555577abdf in slirp_input (slirp=0x5555562bdb80, 
> pkt=0x555556305d58 "RU\n", pkt_len=54) at 
> /home/stefan/src/qemu/repo.or.cz/qemu/ar7/slirp/slirp.c:673
> #3  0x0000555555730f8b in net_slirp_receive (nc=0x5555562bd950, 
> buf=0x555556305d58 "RU\n", size=54) at 
> /home/stefan/src/qemu/repo.or.cz/qemu/ar7/net/slirp.c:116
> #4  0x000055555572dc11 in qemu_vlan_deliver_packet 
> (sender=0x5555563074c0, flags=0, buf=0x555556305d58 "RU\n", size=54, 
> opaque=0x5555562bd8b0)
>      at /home/stefan/src/qemu/repo.or.cz/qemu/ar7/net.c:451
> #5  0x0000555555730938 in qemu_net_queue_deliver (queue=0x5555562bd8f0, 
> sender=0x5555563074c0, flags=0, data=0x555556305d58 "RU\n", size=54)
>      at /home/stefan/src/qemu/repo.or.cz/qemu/ar7/net/queue.c:154
> #6  0x0000555555730a78 in qemu_net_queue_send (queue=0x5555562bd8f0, 
> sender=0x5555563074c0, flags=0, data=0x555556305d58 "RU\n", size=54, 
> sent_cb=0)
>      at /home/stefan/src/qemu/repo.or.cz/qemu/ar7/net/queue.c:188
> #7  0x000055555572de30 in qemu_send_packet_async_with_flags 
> (sender=0x5555563074c0, flags=0, buf=0x555556305d58 "RU\n", size=54, 
> sent_cb=0)
>      at /home/stefan/src/qemu/repo.or.cz/qemu/ar7/net.c:519
> #8  0x000055555572de8b in qemu_send_packet_async (sender=0x5555563074c0, 
> buf=0x555556305d58 "RU\n", size=54, sent_cb=0) at 
> /home/stefan/src/qemu/repo.or.cz/qemu/ar7/net.c:526
> #9  0x000055555572dedb in qemu_send_packet (vc=0x5555563074c0, 
> buf=0x555556305d58 "RU\n", size=54) at 
> /home/stefan/src/qemu/repo.or.cz/qemu/ar7/net.c:532
> #10 0x00005555556e9daa in pcnet_transmit (s=0x555556305af8) at 
> /home/stefan/src/qemu/repo.or.cz/qemu/ar7/hw/pcnet.c:1258
> #11 0x00005555556ea0fd in pcnet_poll_timer (opaque=0x555556305af8) at 
> /home/stefan/src/qemu/repo.or.cz/qemu/ar7/hw/pcnet.c:1321
> #12 0x00005555556ea8e9 in pcnet_ioport_writew (opaque=0x555556305af8, 
> addr=18, val=0) at /home/stefan/src/qemu/repo.or.cz/qemu/ar7/hw/pcnet.c:1571
> #13 0x00005555556e62b3 in pcnet_ioport_write (opaque=0x555556305af8, 
> addr=18, data=0, size=2) at 
> /home/stefan/src/qemu/repo.or.cz/qemu/ar7/hw/pcnet-pci.c:120
> #14 0x0000555555801c8b in memory_region_write_accessor 
> (opaque=0x555556306d80, addr=18, value=0x7fffe9bef690, size=2, shift=0, 
> mask=65535)
>      at /home/stefan/src/qemu/repo.or.cz/qemu/ar7/memory.c:329
> #15 0x0000555555801d6d in access_with_adjusted_size (addr=18, 
> value=0x7fffe9bef690, size=2, access_size_min=1, access_size_max=4,
>      access=0x555555801c13 <memory_region_write_accessor>, 
> opaque=0x555556306d80) at 
> /home/stefan/src/qemu/repo.or.cz/qemu/ar7/memory.c:359
> #16 0x000055555580217d in memory_region_iorange_write 
> (iorange=0x555556306dc0, offset=18, width=2, data=0) at 
> /home/stefan/src/qemu/repo.or.cz/qemu/ar7/memory.c:428
> #17 0x00005555557fb41c in ioport_writew_thunk (opaque=0x555556306dc0, 
> addr=4146, data=0) at /home/stefan/src/qemu/repo.or.cz/qemu/ar7/ioport.c:218
> #18 0x00005555557facb5 in ioport_write (index=1, address=4146, data=0) 
> at /home/stefan/src/qemu/repo.or.cz/qemu/ar7/ioport.c:82
> #19 0x00005555557fb8a3 in cpu_outw (addr=4146, val=0) at 
> /home/stefan/src/qemu/repo.or.cz/qemu/ar7/ioport.c:281
> #20 0x00005555556c7ae4 in isa_mmio_writew (opaque=0x0, addr=4146, val=0) 
> at /home/stefan/src/qemu/repo.or.cz/qemu/ar7/hw/isa_mmio.c:38
> #21 0x000055555580477f in memory_region_dispatch_write 
> (mr=0x5555562ffc38, addr=4146, data=0, size=2) at 
> /home/stefan/src/qemu/repo.or.cz/qemu/ar7/memory.c:913
> #22 0x0000555555807184 in io_mem_write (io_index=38, addr=4146, val=0, 
> size=2) at /home/stefan/src/qemu/repo.or.cz/qemu/ar7/memory.c:1502
> #23 0x000055555581d4e3 in io_writew (physaddr=4146, val=0, 
> addr=3087011890, retaddr=0x4034685f) at 
> /home/stefan/src/qemu/repo.or.cz/qemu/ar7/softmmu_template.h:225
> #24 0x000055555581d5cc in __stw_mmu (addr=3087011890, val=0, mmu_idx=0) 
> at /home/stefan/src/qemu/repo.or.cz/qemu/ar7/softmmu_template.h:257
> #25 0x0000000040346860 in ?? ()
> #26 0x0000000000000000 in ?? ()
> (gdb) p ((struct quehead *)(element->qh_rlink))
> $1 = (struct quehead *) 0x0
> 

Grmbl. Was very hard to reproduce here (triggered once every few hours
with lots of interaction beforehand), but now I think I got the point
(recursion of if_start due to if_encap). Will rework the code to address
this.

Thanks for testing so far, will likely need your services again soon.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH v2 0/4] slirp: Fix for requeuing crash, cleanups

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 535 bytes --]

On 2012-03-05 10:06, Jan Kiszka wrote:
> Grmbl. Was very hard to reproduce here (triggered once every few hours
> with lots of interaction beforehand), but now I think I got the point
> (recursion of if_start due to if_encap). Will rework the code to address
> this.
> 
> Thanks for testing so far, will likely need your services again soon.

I'm careful now with claiming it's fixed, but this version

git://git.kiszka.org/qemu.git queues/slirp

passes all tests that broke in the past. Please have another try.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] [PATCH v2 0/4] slirp: Fix for requeuing crash, cleanups

[Stefan Weil]

Am 06.03.2012 08:41, schrieb Jan Kiszka:
> On 2012-03-05 10:06, Jan Kiszka wrote:
>> Grmbl. Was very hard to reproduce here (triggered once every few hours
>> with lots of interaction beforehand), but now I think I got the point
>> (recursion of if_start due to if_encap). Will rework the code to address
>> this.
>>
>> Thanks for testing so far, will likely need your services again soon.
>
> I'm careful now with claiming it's fixed, but this version
>
> git://git.kiszka.org/qemu.git queues/slirp
>
> passes all tests that broke in the past. Please have another try.
>
> Jan

Tested-by: Stefan Weil <sw@weilnetz.de>

Hi Jan,

this time it looks good. I booted all my 4 MIPS Malta configurations
(32/64 bit little/big endian) via NFS root and used apt-get without
any QEMU crash.

I got a LINUX kernel crash in the emulation, but only once (not
reproducible), and it did not look network related (crash in kfree).

Regards,

Stefan