* [Qemu-devel] [PATCH 2/2] slirp: fix packet requeue issue in batchq @ 2012-02-15 8:13 zwu.kernel 2012-02-15 8:30 ` Jan Kiszka 0 siblings, 1 reply; 3+ messages in thread From: zwu.kernel @ 2012-02-15 8:13 UTC (permalink / raw) To: qemu-devel; +Cc: aliguori, Zhi Yong Wu, jan.kiszka, stefanha, mst From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> This patch fixes the slirp crash in current QEMU upstream. Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> --- slirp/if.c | 37 ++++++++++++++++++++++++++++++------- slirp/mbuf.c | 3 +-- 2 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) static void ifs_remque(struct mbuf *ifm) { - ifm->ifs_prev->ifs_next = ifm->ifs_next; - ifm->ifs_next->ifs_prev = ifm->ifs_prev; + if (ifm->ifs_next->ifs_next == ifm + && ifm->ifs_next->ifs_prev == ifm) { + ifs_init(ifm->ifs_next); + } else { + ifm->ifs_prev->ifs_next = ifm->ifs_next; + ifm->ifs_next->ifs_prev = ifm->ifs_prev; + } + + ifs_init(ifm); } void @@ -154,14 +161,18 @@ if_start(Slirp *slirp) { uint64_t now = qemu_get_clock_ns(rt_clock); int requeued = 0; - struct mbuf *ifm, *ifqt; + struct mbuf *ifm, *ifqt, *ifm_next; - DEBUG_CALL("if_start"); + DEBUG_CALL("if_start"); - if (slirp->if_queued == 0) - return; /* Nothing to do */ + if (slirp->if_queued == 0) + return; /* Nothing to do */ + + slirp->next_m = &slirp->if_batchq; again: + ifm_next = NULL; + /* check if we can really output */ if (!slirp_can_output(slirp->opaque)) return; @@ -190,6 +201,7 @@ if_start(Slirp *slirp) /* If there are more packets for this session, re-queue them */ if (ifm->ifs_next != /* ifm->ifs_prev != */ ifm) { insque(ifm->ifs_next, ifqt); + ifm_next = ifm->ifs_next; ifs_remque(ifm); } @@ -209,7 +221,18 @@ if_start(Slirp *slirp) m_free(ifm); } else { /* re-queue */ - insque(ifm, ifqt); + if (ifm_next) { + /*restore the original state of bachq*/ + remque(ifm_next); + insque(ifm, ifqt); + ifm_next->ifs_prev->ifs_next = ifm; + ifm->ifs_prev = ifm_next->ifs_prev; + ifm->ifs_next = ifm_next; + ifm_next->ifs_prev = ifm; + } else { + insque(ifm, ifqt); + } + requeued++; } } 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) m->m_size = SLIRP_MSIZE - offsetof(struct mbuf, m_dat); m->m_data = m->m_dat; m->m_len = 0; - m->m_nextpkt = NULL; - m->m_prevpkt = NULL; + ifs_init(m); m->arp_requested = false; m->expiration_date = (uint64_t)-1; end_error: -- 1.7.6 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] slirp: fix packet requeue issue in batchq 2012-02-15 8:13 [Qemu-devel] [PATCH 2/2] slirp: fix packet requeue issue in batchq zwu.kernel @ 2012-02-15 8:30 ` Jan Kiszka 2012-02-16 7:34 ` Zhi Yong Wu 0 siblings, 1 reply; 3+ messages in thread From: Jan Kiszka @ 2012-02-15 8:30 UTC (permalink / raw) To: zwu.kernel; +Cc: aliguori, Zhi Yong Wu, qemu-devel, stefanha, mst [-- Attachment #1: Type: text/plain, Size: 3762 bytes --] On 2012-02-15 09:13, zwu.kernel@gmail.com wrote: > From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> > > This patch fixes the slirp crash in current QEMU upstream. > > Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> > --- > slirp/if.c | 37 ++++++++++++++++++++++++++++++------- > slirp/mbuf.c | 3 +-- > 2 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) > static void > ifs_remque(struct mbuf *ifm) > { > - ifm->ifs_prev->ifs_next = ifm->ifs_next; > - ifm->ifs_next->ifs_prev = ifm->ifs_prev; > + if (ifm->ifs_next->ifs_next == ifm > + && ifm->ifs_next->ifs_prev == ifm) { > + ifs_init(ifm->ifs_next); > + } else { > + ifm->ifs_prev->ifs_next = ifm->ifs_next; > + ifm->ifs_next->ifs_prev = ifm->ifs_prev; > + } > + > + ifs_init(ifm); This looks, well, interesting. Can you explain the logic? We really need to document the queuing mechanics. > } > > void > @@ -154,14 +161,18 @@ if_start(Slirp *slirp) > { > uint64_t now = qemu_get_clock_ns(rt_clock); > int requeued = 0; > - struct mbuf *ifm, *ifqt; > + struct mbuf *ifm, *ifqt, *ifm_next; > > - DEBUG_CALL("if_start"); > + DEBUG_CALL("if_start"); > > - if (slirp->if_queued == 0) > - return; /* Nothing to do */ > + if (slirp->if_queued == 0) > + return; /* Nothing to do */ Even if the result looks funny, let's not touch lines just for indention (and braces would be missing anyway). > + > + slirp->next_m = &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. > > again: > + ifm_next = NULL; > + > /* check if we can really output */ > if (!slirp_can_output(slirp->opaque)) > return; > @@ -190,6 +201,7 @@ if_start(Slirp *slirp) > /* If there are more packets for this session, re-queue them */ > if (ifm->ifs_next != /* ifm->ifs_prev != */ ifm) { > insque(ifm->ifs_next, ifqt); > + ifm_next = ifm->ifs_next; > ifs_remque(ifm); > } > > @@ -209,7 +221,18 @@ if_start(Slirp *slirp) > m_free(ifm); > } else { > /* re-queue */ > - insque(ifm, ifqt); > + if (ifm_next) { > + /*restore the original state of bachq*/ > + remque(ifm_next); > + insque(ifm, ifqt); > + ifm_next->ifs_prev->ifs_next = ifm; > + ifm->ifs_prev = ifm_next->ifs_prev; > + ifm->ifs_next = ifm_next; > + ifm_next->ifs_prev = ifm; So is this only about the correct ordering or also about pointer correctness? > + } else { > + insque(ifm, ifqt); > + } > + > requeued++; > } > } > 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) > m->m_size = SLIRP_MSIZE - offsetof(struct mbuf, m_dat); > m->m_data = m->m_dat; > m->m_len = 0; > - m->m_nextpkt = NULL; > - m->m_prevpkt = NULL; > + ifs_init(m); > m->arp_requested = false; > m->expiration_date = (uint64_t)-1; > end_error: Thanks for digging into this, but I really think it needs more comments and potentially even some cleanups. Jan [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 262 bytes --] ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] slirp: fix packet requeue issue in batchq 2012-02-15 8:30 ` Jan Kiszka @ 2012-02-16 7:34 ` Zhi Yong Wu 0 siblings, 0 replies; 3+ messages in thread From: Zhi Yong Wu @ 2012-02-16 7:34 UTC (permalink / raw) To: Jan Kiszka; +Cc: aliguori, Zhi Yong Wu, qemu-devel, stefanha, mst On Wed, Feb 15, 2012 at 4:30 PM, Jan Kiszka <jan.kiszka@web.de> wrote: > On 2012-02-15 09:13, zwu.kernel@gmail.com wrote: >> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> >> >> This patch fixes the slirp crash in current QEMU upstream. >> >> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> >> --- >> slirp/if.c | 37 ++++++++++++++++++++++++++++++------- >> slirp/mbuf.c | 3 +-- >> 2 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) >> static void >> ifs_remque(struct mbuf *ifm) >> { >> - ifm->ifs_prev->ifs_next = ifm->ifs_next; >> - ifm->ifs_next->ifs_prev = ifm->ifs_prev; >> + if (ifm->ifs_next->ifs_next == ifm >> + && ifm->ifs_next->ifs_prev == ifm) { >> + ifs_init(ifm->ifs_next); >> + } else { >> + ifm->ifs_prev->ifs_next = ifm->ifs_next; >> + ifm->ifs_next->ifs_prev = ifm->ifs_prev; >> + } >> + >> + ifs_init(ifm); > > This looks, well, interesting. Can you explain the logic? We really need > to document the queuing mechanics. ifm->ifs_prev->ifs_next = ifm->ifs_next; ifm->ifs_next->ifs_prev = ifm->ifs_prev; + ifs_init(ifm); Sorry, actually we only need to add one line of code. > >> } >> >> void >> @@ -154,14 +161,18 @@ if_start(Slirp *slirp) >> { >> uint64_t now = qemu_get_clock_ns(rt_clock); >> int requeued = 0; >> - struct mbuf *ifm, *ifqt; >> + struct mbuf *ifm, *ifqt, *ifm_next; >> >> - DEBUG_CALL("if_start"); >> + DEBUG_CALL("if_start"); >> >> - if (slirp->if_queued == 0) >> - return; /* Nothing to do */ >> + if (slirp->if_queued == 0) >> + return; /* 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. > >> + >> + slirp->next_m = &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. > >> >> again: >> + ifm_next = NULL; >> + >> /* check if we can really output */ >> if (!slirp_can_output(slirp->opaque)) >> return; >> @@ -190,6 +201,7 @@ if_start(Slirp *slirp) >> /* If there are more packets for this session, re-queue them */ >> if (ifm->ifs_next != /* ifm->ifs_prev != */ ifm) { >> insque(ifm->ifs_next, ifqt); >> + ifm_next = ifm->ifs_next; >> ifs_remque(ifm); >> } >> >> @@ -209,7 +221,18 @@ if_start(Slirp *slirp) >> m_free(ifm); >> } else { >> /* re-queue */ >> - insque(ifm, ifqt); >> + if (ifm_next) { >> + /*restore the original state of bachq*/ >> + remque(ifm_next); >> + insque(ifm, ifqt); >> + ifm_next->ifs_prev->ifs_next = ifm; >> + ifm->ifs_prev = ifm_next->ifs_prev; >> + ifm->ifs_next = ifm_next; >> + ifm_next->ifs_prev = 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. > >> + } else { >> + insque(ifm, ifqt); >> + } >> + >> requeued++; >> } >> } >> 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) >> m->m_size = SLIRP_MSIZE - offsetof(struct mbuf, m_dat); >> m->m_data = m->m_dat; >> m->m_len = 0; >> - m->m_nextpkt = NULL; >> - m->m_prevpkt = NULL; >> + ifs_init(m); >> m->arp_requested = false; >> m->expiration_date = (uint64_t)-1; >> end_error: > > Thanks for digging into this, but I really think it needs more comments > and potentially even some cleanups. > > Jan > -- Regards, Zhi Yong Wu ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2012-02-16 7:34 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-02-15 8:13 [Qemu-devel] [PATCH 2/2] slirp: fix packet requeue issue in batchq zwu.kernel 2012-02-15 8:30 ` Jan Kiszka 2012-02-16 7:34 ` Zhi Yong Wu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).