From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:58182) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Rxvr2-0003a5-Rp for qemu-devel@nongnu.org; Thu, 16 Feb 2012 02:34:18 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Rxvqw-0008BL-DK for qemu-devel@nongnu.org; Thu, 16 Feb 2012 02:34:12 -0500 Received: from mail-pz0-f45.google.com ([209.85.210.45]:57309) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Rxvqw-0008B3-5C for qemu-devel@nongnu.org; Thu, 16 Feb 2012 02:34:06 -0500 Received: by dadp14 with SMTP id p14so2125954dad.4 for ; Wed, 15 Feb 2012 23:34:04 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <4F3B6D36.1030808@web.de> References: <1329293587-16246-1-git-send-email-zwu.kernel@gmail.com> <4F3B6D36.1030808@web.de> Date: Thu, 16 Feb 2012 15:34:04 +0800 Message-ID: From: Zhi Yong Wu Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 2/2] slirp: fix packet requeue issue in batchq List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jan Kiszka Cc: aliguori@us.ibm.com, Zhi Yong Wu , qemu-devel@nongnu.org, stefanha@linux.vnet.ibm.com, mst@redhat.com On Wed, Feb 15, 2012 at 4:30 PM, Jan Kiszka wrote: > On 2012-02-15 09:13, zwu.kernel@gmail.com wrote: >> From: Zhi Yong Wu >> >> This patch fixes the slirp crash in current QEMU upstream. >> >> Signed-off-by: Zhi Yong Wu >> --- >> =A0slirp/if.c =A0 | =A0 37 ++++++++++++++++++++++++++++++------- >> =A0slirp/mbuf.c | =A0 =A03 +-- >> =A02 files changed, 31 insertions(+), 9 deletions(-) >> >> diff --git a/slirp/if.c b/slirp/if.c >> index 8e0cac2..f7f8577 100644 >> --- a/slirp/if.c >> +++ b/slirp/if.c >> @@ -20,8 +20,15 @@ ifs_insque(struct mbuf *ifm, struct mbuf *ifmhead) >> =A0static void >> =A0ifs_remque(struct mbuf *ifm) >> =A0{ >> - =A0 =A0 ifm->ifs_prev->ifs_next =3D ifm->ifs_next; >> - =A0 =A0 ifm->ifs_next->ifs_prev =3D ifm->ifs_prev; >> + =A0 =A0 =A0 =A0if (ifm->ifs_next->ifs_next =3D=3D ifm >> + =A0 =A0 =A0 =A0 =A0 =A0&& ifm->ifs_next->ifs_prev =3D=3D ifm) { >> + =A0 =A0 =A0 =A0 =A0 =A0ifs_init(ifm->ifs_next); >> + =A0 =A0 =A0 =A0} else { >> + =A0 =A0 =A0 =A0 =A0 =A0ifm->ifs_prev->ifs_next =3D ifm->ifs_next; >> + =A0 =A0 =A0 =A0 =A0 =A0ifm->ifs_next->ifs_prev =3D ifm->ifs_prev; >> + =A0 =A0 =A0 =A0} >> + >> + =A0 =A0 =A0 =A0ifs_init(ifm); > > This looks, well, interesting. Can you explain the logic? We really need > to document the queuing mechanics. ifm->ifs_prev->ifs_next =3D ifm->ifs_next; ifm->ifs_next->ifs_prev =3D ifm->ifs_prev; + ifs_init(ifm); Sorry, actually we only need to add one line of code. > >> =A0} >> >> =A0void >> @@ -154,14 +161,18 @@ if_start(Slirp *slirp) >> =A0{ >> =A0 =A0 =A0uint64_t now =3D qemu_get_clock_ns(rt_clock); >> =A0 =A0 =A0int requeued =3D 0; >> - =A0 =A0 struct mbuf *ifm, *ifqt; >> + =A0 =A0struct mbuf *ifm, *ifqt, *ifm_next; >> >> - =A0 =A0 DEBUG_CALL("if_start"); >> + =A0 =A0DEBUG_CALL("if_start"); >> >> - =A0 =A0 if (slirp->if_queued =3D=3D 0) >> - =A0 =A0 =A0 =A0return; /* Nothing to do */ >> + =A0 =A0if (slirp->if_queued =3D=3D 0) >> + =A0 =A0 =A0 =A0return; /* Nothing to do */ > > Even if the result looks funny, let's not touch lines just for indention > (and braces would be missing anyway). OK. undo them to their original state. > >> + >> + =A0 =A0slirp->next_m =3D &slirp->if_batchq; > > Have you understood the difference between the natural order of > if_batchq and next_m? I still wonder what the latter is good for. Sorry, the line of code should be removed. next_m will point to next packet which will be handled, if there's multiple session, it will point to the head packet for next session; if there's one session, it will point to batchq. > >> >> =A0 again: >> + =A0 =A0 =A0 =A0ifm_next =3D NULL; >> + >> =A0 =A0 =A0 =A0 =A0/* check if we can really output */ >> =A0 =A0 =A0 =A0 =A0if (!slirp_can_output(slirp->opaque)) >> =A0 =A0 =A0 =A0 =A0 =A0 =A0return; >> @@ -190,6 +201,7 @@ if_start(Slirp *slirp) >> =A0 =A0 =A0 /* If there are more packets for this session, re-queue them= */ >> =A0 =A0 =A0 if (ifm->ifs_next !=3D /* ifm->ifs_prev !=3D */ ifm) { >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 insque(ifm->ifs_next, ifqt); >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ifm_next =3D ifm->ifs_next; >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 ifs_remque(ifm); >> =A0 =A0 =A0 } >> >> @@ -209,7 +221,18 @@ if_start(Slirp *slirp) >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0m_free(ifm); >> =A0 =A0 =A0 =A0 =A0 =A0 =A0} else { >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0/* re-queue */ >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0insque(ifm, ifqt); >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (ifm_next) { >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0/*restore the original state of= bachq*/ >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0remque(ifm_next); >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0insque(ifm, ifqt); >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ifm_next->ifs_prev->ifs_next = =3D ifm; >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ifm->ifs_prev =3D ifm_next->ifs= _prev; >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ifm->ifs_next =3D ifm_next; >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ifm_next->ifs_prev =3D ifm; > > So is this only about the correct ordering or also about pointer > correctness? The former. If ifm need to be re-queued, it will be put to where it originally is. > >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0} else { >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0insque(ifm, ifqt); >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0} >> + >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0requeued++; >> =A0 =A0 =A0 =A0 =A0 =A0 =A0} >> =A0 =A0 =A0 =A0 =A0} >> diff --git a/slirp/mbuf.c b/slirp/mbuf.c >> index c699c75..f429c0a 100644 >> --- a/slirp/mbuf.c >> +++ b/slirp/mbuf.c >> @@ -68,8 +68,7 @@ m_get(Slirp *slirp) >> =A0 =A0 =A0 m->m_size =3D SLIRP_MSIZE - offsetof(struct mbuf, m_dat); >> =A0 =A0 =A0 m->m_data =3D m->m_dat; >> =A0 =A0 =A0 m->m_len =3D 0; >> - =A0 =A0 =A0 =A0m->m_nextpkt =3D NULL; >> - =A0 =A0 =A0 =A0m->m_prevpkt =3D NULL; >> + =A0 =A0 =A0 =A0ifs_init(m); >> =A0 =A0 =A0 =A0 =A0m->arp_requested =3D false; >> =A0 =A0 =A0 =A0 =A0m->expiration_date =3D (uint64_t)-1; >> =A0end_error: > > Thanks for digging into this, but I really think it needs more comments > and potentially even some cleanups. > > Jan > --=20 Regards, Zhi Yong Wu