* [Qemu-devel] [PATCH v2] slirp: fix packet requeue issue in batchq @ 2012-02-16 8:07 zwu.kernel 2012-02-16 8:37 ` Jan Kiszka 0 siblings, 1 reply; 7+ messages in thread From: zwu.kernel @ 2012-02-16 8:07 UTC (permalink / raw) To: qemu-devel; +Cc: aliguori, Zhi Yong Wu, jan.kiszka, stefanha, mst From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> --- slirp/if.c | 19 +++++++++++++++++-- slirp/mbuf.c | 3 +-- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/slirp/if.c b/slirp/if.c index 8e0cac2..57350d5 100644 --- a/slirp/if.c +++ b/slirp/if.c @@ -22,6 +22,7 @@ ifs_remque(struct mbuf *ifm) { ifm->ifs_prev->ifs_next = ifm->ifs_next; ifm->ifs_next->ifs_prev = ifm->ifs_prev; + ifs_init(ifm); } void @@ -154,7 +155,7 @@ 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"); @@ -162,6 +163,8 @@ if_start(Slirp *slirp) return; /* Nothing to do */ again: + ifm_next = NULL; + /* check if we can really output */ if (!slirp_can_output(slirp->opaque)) return; @@ -190,6 +193,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 +213,18 @@ if_start(Slirp *slirp) m_free(ifm); } else { /* re-queue */ - insque(ifm, ifqt); + if (ifm_next) { + /*restore the original state of batchq*/ + 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] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v2] slirp: fix packet requeue issue in batchq 2012-02-16 8:07 [Qemu-devel] [PATCH v2] slirp: fix packet requeue issue in batchq zwu.kernel @ 2012-02-16 8:37 ` Jan Kiszka 2012-02-16 8:45 ` Zhi Yong Wu 0 siblings, 1 reply; 7+ messages in thread From: Jan Kiszka @ 2012-02-16 8:37 UTC (permalink / raw) To: zwu.kernel; +Cc: aliguori, Zhi Yong Wu, qemu-devel, stefanha, mst [-- Attachment #1: Type: text/plain, Size: 2721 bytes --] On 2012-02-16 09:07, zwu.kernel@gmail.com wrote: > From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> > Please summarize in a bit more details what was broken. > Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> > --- > slirp/if.c | 19 +++++++++++++++++-- > slirp/mbuf.c | 3 +-- > 2 files changed, 18 insertions(+), 4 deletions(-) > > diff --git a/slirp/if.c b/slirp/if.c > index 8e0cac2..57350d5 100644 > --- a/slirp/if.c > +++ b/slirp/if.c > @@ -22,6 +22,7 @@ ifs_remque(struct mbuf *ifm) > { > ifm->ifs_prev->ifs_next = ifm->ifs_next; > ifm->ifs_next->ifs_prev = ifm->ifs_prev; > + ifs_init(ifm); > } > > void > @@ -154,7 +155,7 @@ 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"); > > @@ -162,6 +163,8 @@ if_start(Slirp *slirp) > return; /* Nothing to do */ > > again: > + ifm_next = NULL; > + > /* check if we can really output */ > if (!slirp_can_output(slirp->opaque)) > return; > @@ -190,6 +193,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 +213,18 @@ if_start(Slirp *slirp) > m_free(ifm); > } else { > /* re-queue */ > - insque(ifm, ifqt); > + if (ifm_next) { > + /*restore the original state of batchq*/ > + 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: Wondering now: Is this hunk required or a cleanup? Jan [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 262 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v2] slirp: fix packet requeue issue in batchq 2012-02-16 8:37 ` Jan Kiszka @ 2012-02-16 8:45 ` Zhi Yong Wu 2012-02-16 8:46 ` Zhi Yong Wu 2012-02-16 8:48 ` Jan Kiszka 0 siblings, 2 replies; 7+ messages in thread From: Zhi Yong Wu @ 2012-02-16 8:45 UTC (permalink / raw) To: Jan Kiszka; +Cc: aliguori, Zhi Yong Wu, qemu-devel, stefanha, mst On Thu, Feb 16, 2012 at 4:37 PM, Jan Kiszka <jan.kiszka@web.de> wrote: > On 2012-02-16 09:07, zwu.kernel@gmail.com wrote: >> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> >> > > Please summarize in a bit more details what was broken. Should those bits be put in the message part of this part? or here? > >> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> >> --- >> slirp/if.c | 19 +++++++++++++++++-- >> slirp/mbuf.c | 3 +-- >> 2 files changed, 18 insertions(+), 4 deletions(-) >> >> diff --git a/slirp/if.c b/slirp/if.c >> index 8e0cac2..57350d5 100644 >> --- a/slirp/if.c >> +++ b/slirp/if.c >> @@ -22,6 +22,7 @@ ifs_remque(struct mbuf *ifm) >> { >> ifm->ifs_prev->ifs_next = ifm->ifs_next; >> ifm->ifs_next->ifs_prev = ifm->ifs_prev; >> + ifs_init(ifm); >> } >> >> void >> @@ -154,7 +155,7 @@ 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"); >> >> @@ -162,6 +163,8 @@ if_start(Slirp *slirp) >> return; /* Nothing to do */ >> >> again: >> + ifm_next = NULL; >> + >> /* check if we can really output */ >> if (!slirp_can_output(slirp->opaque)) >> return; >> @@ -190,6 +193,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 +213,18 @@ if_start(Slirp *slirp) >> m_free(ifm); >> } else { >> /* re-queue */ >> - insque(ifm, ifqt); >> + if (ifm_next) { >> + /*restore the original state of batchq*/ >> + 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: > > Wondering now: Is this hunk required or a cleanup? The former. I think that the pointer of one raw mbuf which are used to link the packets for the same session should default to itself, not NULL. > > Jan > -- Regards, Zhi Yong Wu ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v2] slirp: fix packet requeue issue in batchq 2012-02-16 8:45 ` Zhi Yong Wu @ 2012-02-16 8:46 ` Zhi Yong Wu 2012-02-16 8:48 ` Jan Kiszka 1 sibling, 0 replies; 7+ messages in thread From: Zhi Yong Wu @ 2012-02-16 8:46 UTC (permalink / raw) To: Jan Kiszka; +Cc: aliguori, Zhi Yong Wu, qemu-devel, stefanha, mst On Thu, Feb 16, 2012 at 4:45 PM, Zhi Yong Wu <zwu.kernel@gmail.com> wrote: > On Thu, Feb 16, 2012 at 4:37 PM, Jan Kiszka <jan.kiszka@web.de> wrote: >> On 2012-02-16 09:07, zwu.kernel@gmail.com wrote: >>> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> >>> >> >> Please summarize in a bit more details what was broken. > Should those bits be put in the message part of this part? or here? s/this part/this patch/ > >> >>> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> >>> --- >>> slirp/if.c | 19 +++++++++++++++++-- >>> slirp/mbuf.c | 3 +-- >>> 2 files changed, 18 insertions(+), 4 deletions(-) >>> >>> diff --git a/slirp/if.c b/slirp/if.c >>> index 8e0cac2..57350d5 100644 >>> --- a/slirp/if.c >>> +++ b/slirp/if.c >>> @@ -22,6 +22,7 @@ ifs_remque(struct mbuf *ifm) >>> { >>> ifm->ifs_prev->ifs_next = ifm->ifs_next; >>> ifm->ifs_next->ifs_prev = ifm->ifs_prev; >>> + ifs_init(ifm); >>> } >>> >>> void >>> @@ -154,7 +155,7 @@ 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"); >>> >>> @@ -162,6 +163,8 @@ if_start(Slirp *slirp) >>> return; /* Nothing to do */ >>> >>> again: >>> + ifm_next = NULL; >>> + >>> /* check if we can really output */ >>> if (!slirp_can_output(slirp->opaque)) >>> return; >>> @@ -190,6 +193,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 +213,18 @@ if_start(Slirp *slirp) >>> m_free(ifm); >>> } else { >>> /* re-queue */ >>> - insque(ifm, ifqt); >>> + if (ifm_next) { >>> + /*restore the original state of batchq*/ >>> + 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: >> >> Wondering now: Is this hunk required or a cleanup? > The former. I think that the pointer of one raw mbuf which are used to > link the packets for the same session should default to itself, not > NULL. > >> >> Jan >> > > > > -- > Regards, > > Zhi Yong Wu -- Regards, Zhi Yong Wu ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v2] slirp: fix packet requeue issue in batchq 2012-02-16 8:45 ` Zhi Yong Wu 2012-02-16 8:46 ` Zhi Yong Wu @ 2012-02-16 8:48 ` Jan Kiszka 2012-02-16 9:21 ` Zhi Yong Wu 1 sibling, 1 reply; 7+ messages in thread From: Jan Kiszka @ 2012-02-16 8:48 UTC (permalink / raw) To: Zhi Yong Wu; +Cc: aliguori, Zhi Yong Wu, qemu-devel, stefanha, mst [-- Attachment #1: Type: text/plain, Size: 3421 bytes --] On 2012-02-16 09:45, Zhi Yong Wu wrote: > On Thu, Feb 16, 2012 at 4:37 PM, Jan Kiszka <jan.kiszka@web.de> wrote: >> On 2012-02-16 09:07, zwu.kernel@gmail.com wrote: >>> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> >>> >> >> Please summarize in a bit more details what was broken. > Should those bits be put in the message part of this part? or here? Here, as a commit log. > >> >>> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> >>> --- >>> slirp/if.c | 19 +++++++++++++++++-- >>> slirp/mbuf.c | 3 +-- >>> 2 files changed, 18 insertions(+), 4 deletions(-) >>> >>> diff --git a/slirp/if.c b/slirp/if.c >>> index 8e0cac2..57350d5 100644 >>> --- a/slirp/if.c >>> +++ b/slirp/if.c >>> @@ -22,6 +22,7 @@ ifs_remque(struct mbuf *ifm) >>> { >>> ifm->ifs_prev->ifs_next = ifm->ifs_next; >>> ifm->ifs_next->ifs_prev = ifm->ifs_prev; >>> + ifs_init(ifm); >>> } >>> >>> void >>> @@ -154,7 +155,7 @@ 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"); >>> >>> @@ -162,6 +163,8 @@ if_start(Slirp *slirp) >>> return; /* Nothing to do */ >>> >>> again: >>> + ifm_next = NULL; >>> + >>> /* check if we can really output */ >>> if (!slirp_can_output(slirp->opaque)) >>> return; >>> @@ -190,6 +193,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 +213,18 @@ if_start(Slirp *slirp) >>> m_free(ifm); >>> } else { >>> /* re-queue */ >>> - insque(ifm, ifqt); >>> + if (ifm_next) { >>> + /*restore the original state of batchq*/ >>> + 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: >> >> Wondering now: Is this hunk required or a cleanup? > The former. I think that the pointer of one raw mbuf which are used to > link the packets for the same session should default to itself, not > NULL. OK. Out of curiosity, is that an older issue or just related to the requeuing we now practice? Jan [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 262 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v2] slirp: fix packet requeue issue in batchq 2012-02-16 8:48 ` Jan Kiszka @ 2012-02-16 9:21 ` Zhi Yong Wu 2012-02-16 9:30 ` Zhi Yong Wu 0 siblings, 1 reply; 7+ messages in thread From: Zhi Yong Wu @ 2012-02-16 9:21 UTC (permalink / raw) To: Jan Kiszka; +Cc: aliguori, Zhi Yong Wu, mst, qemu-devel, stefanha On Thu, Feb 16, 2012 at 4:48 PM, Jan Kiszka <jan.kiszka@web.de> wrote: > On 2012-02-16 09:45, Zhi Yong Wu wrote: >> On Thu, Feb 16, 2012 at 4:37 PM, Jan Kiszka <jan.kiszka@web.de> wrote: >>> On 2012-02-16 09:07, zwu.kernel@gmail.com wrote: >>>> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> >>>> >>> >>> Please summarize in a bit more details what was broken. >> Should those bits be put in the message part of this part? or here? > > Here, as a commit log. In slirp, The condition ifm->ifs_next == ifm is used to determine if one session only has one packet to handled. But sometime its pointers is default to NULL. Moreover, if one session has multiple packet to be handled, when one packet need to re-queued, the origianl code only simply inserted back to batchq, this will cause that one socket will correspone to multiple doubly linked list of mbuf, but ifs_next[prev] pointer of this re-queued mbuf still keeps its original value, this is wrong. batchq<-ifq_next[prev]--->mbuf1[socket1] <-ifq_next[prev]--->-->mbuf4[socket2]<--->... ^| (ifs_next[prev]) mbuf2[socket1] ^| (ifs_next[prev])) mbuf3[socket1] ^| (ifs_next[prev])) After re-queue batchq<--ifs_next[prev])-->mbuf1[socket1] <--ifs_next[prev])->mbuf2[socket1]<--ifs_next[prev])->mbuf4[socket2]<--->... ^| (ifs_next[prev]) ^| (ifs_next[prev]) mbuf3[socket1] ^| (ifs_next[prev]) This is wrong. This patch fixes this, when one packet is re-queued, the packets for the same session will be put to its original location > >> >>> >>>> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> >>>> --- >>>> slirp/if.c | 19 +++++++++++++++++-- >>>> slirp/mbuf.c | 3 +-- >>>> 2 files changed, 18 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/slirp/if.c b/slirp/if.c >>>> index 8e0cac2..57350d5 100644 >>>> --- a/slirp/if.c >>>> +++ b/slirp/if.c >>>> @@ -22,6 +22,7 @@ ifs_remque(struct mbuf *ifm) >>>> { >>>> ifm->ifs_prev->ifs_next = ifm->ifs_next; >>>> ifm->ifs_next->ifs_prev = ifm->ifs_prev; >>>> + ifs_init(ifm); >>>> } >>>> >>>> void >>>> @@ -154,7 +155,7 @@ 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"); >>>> >>>> @@ -162,6 +163,8 @@ if_start(Slirp *slirp) >>>> return; /* Nothing to do */ >>>> >>>> again: >>>> + ifm_next = NULL; >>>> + >>>> /* check if we can really output */ >>>> if (!slirp_can_output(slirp->opaque)) >>>> return; >>>> @@ -190,6 +193,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 +213,18 @@ if_start(Slirp *slirp) >>>> m_free(ifm); >>>> } else { >>>> /* re-queue */ >>>> - insque(ifm, ifqt); >>>> + if (ifm_next) { >>>> + /*restore the original state of batchq*/ >>>> + 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: >>> >>> Wondering now: Is this hunk required or a cleanup? >> The former. I think that the pointer of one raw mbuf which are used to >> link the packets for the same session should default to itself, not >> NULL. > > OK. Out of curiosity, is that an older issue or just related to the > requeuing we now practice? I don't know if it is an older issue, but i make sure that it relative the requeuing stuff. For example, you can see some stuff in ifs_remque(). ifm->ifs_prev->ifs_next = ifm->ifs_next; ifm->ifs_next->ifs_prev = ifm->ifs_prev; The pointer of one pointer easily casues segment error if this pointer is NULL. This is only one example. > > Jan > -- Regards, Zhi Yong Wu ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v2] slirp: fix packet requeue issue in batchq 2012-02-16 9:21 ` Zhi Yong Wu @ 2012-02-16 9:30 ` Zhi Yong Wu 0 siblings, 0 replies; 7+ messages in thread From: Zhi Yong Wu @ 2012-02-16 9:30 UTC (permalink / raw) To: Jan Kiszka; +Cc: aliguori, Zhi Yong Wu, mst, qemu-devel, stefanha On Thu, Feb 16, 2012 at 5:21 PM, Zhi Yong Wu <zwu.kernel@gmail.com> wrote: > On Thu, Feb 16, 2012 at 4:48 PM, Jan Kiszka <jan.kiszka@web.de> wrote: >> On 2012-02-16 09:45, Zhi Yong Wu wrote: >>> On Thu, Feb 16, 2012 at 4:37 PM, Jan Kiszka <jan.kiszka@web.de> wrote: >>>> On 2012-02-16 09:07, zwu.kernel@gmail.com wrote: >>>>> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> >>>>> >>>> >>>> Please summarize in a bit more details what was broken. >>> Should those bits be put in the message part of this part? or here? >> >> Here, as a commit log. > In slirp, The condition ifm->ifs_next == ifm is used to determine if > one session only has one packet to handled. But sometime its pointers > is default to NULL. Moreover, if one session has multiple packet to be > handled, when one packet need to re-queued, the origianl code only > simply inserted back to batchq, this will cause that one socket will > correspone to multiple doubly linked list of mbuf, but ifs_next[prev] > pointer of this re-queued mbuf still keeps its original value, this is > wrong. > > batchq<-ifq_next[prev]--->mbuf1[socket1] > <-ifq_next[prev]--->-->mbuf4[socket2]<--->... > ^| (ifs_next[prev]) > mbuf2[socket1] > ^| (ifs_next[prev])) > mbuf3[socket1] > ^| (ifs_next[prev])) > > After re-queue > > batchq<--ifs_next[prev])-->mbuf1[socket1] > <--ifs_next[prev])->mbuf2[socket1]<--ifs_next[prev])->mbuf4[socket2]<--->... > ^| (ifs_next[prev]) > ^| (ifs_next[prev]) > > mbuf3[socket1] > > ^| (ifs_next[prev]) Sorry, there is wrong with this above figure. It seems to be messy after i sent out this mail. batchq<--ifq_next[prev])-->mbuf1[socket1] <--ifq_next[prev])->mbuf2[socket1]<--ifq_next[prev])->mbuf4[socket2]<--->... ^| (ifs_next[prev]) ^| (ifs_next[prev]) mbuf3[socket1] ^| (ifs_next[prev]) > > This is wrong. > > This patch fixes this, when one packet is re-queued, the packets for > the same session will be put to its original location > >> >>> >>>> >>>>> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> >>>>> --- >>>>> slirp/if.c | 19 +++++++++++++++++-- >>>>> slirp/mbuf.c | 3 +-- >>>>> 2 files changed, 18 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/slirp/if.c b/slirp/if.c >>>>> index 8e0cac2..57350d5 100644 >>>>> --- a/slirp/if.c >>>>> +++ b/slirp/if.c >>>>> @@ -22,6 +22,7 @@ ifs_remque(struct mbuf *ifm) >>>>> { >>>>> ifm->ifs_prev->ifs_next = ifm->ifs_next; >>>>> ifm->ifs_next->ifs_prev = ifm->ifs_prev; >>>>> + ifs_init(ifm); >>>>> } >>>>> >>>>> void >>>>> @@ -154,7 +155,7 @@ 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"); >>>>> >>>>> @@ -162,6 +163,8 @@ if_start(Slirp *slirp) >>>>> return; /* Nothing to do */ >>>>> >>>>> again: >>>>> + ifm_next = NULL; >>>>> + >>>>> /* check if we can really output */ >>>>> if (!slirp_can_output(slirp->opaque)) >>>>> return; >>>>> @@ -190,6 +193,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 +213,18 @@ if_start(Slirp *slirp) >>>>> m_free(ifm); >>>>> } else { >>>>> /* re-queue */ >>>>> - insque(ifm, ifqt); >>>>> + if (ifm_next) { >>>>> + /*restore the original state of batchq*/ >>>>> + 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: >>>> >>>> Wondering now: Is this hunk required or a cleanup? >>> The former. I think that the pointer of one raw mbuf which are used to >>> link the packets for the same session should default to itself, not >>> NULL. >> >> OK. Out of curiosity, is that an older issue or just related to the >> requeuing we now practice? > I don't know if it is an older issue, but i make sure that it relative > the requeuing stuff. > > For example, you can see some stuff in ifs_remque(). > ifm->ifs_prev->ifs_next = ifm->ifs_next; > ifm->ifs_next->ifs_prev = ifm->ifs_prev; > > The pointer of one pointer easily casues segment error if this pointer > is NULL. This is only one example. > >> >> Jan >> > > > > -- > Regards, > > Zhi Yong Wu -- Regards, Zhi Yong Wu ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-02-16 9:30 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-02-16 8:07 [Qemu-devel] [PATCH v2] slirp: fix packet requeue issue in batchq zwu.kernel 2012-02-16 8:37 ` Jan Kiszka 2012-02-16 8:45 ` Zhi Yong Wu 2012-02-16 8:46 ` Zhi Yong Wu 2012-02-16 8:48 ` Jan Kiszka 2012-02-16 9:21 ` Zhi Yong Wu 2012-02-16 9:30 ` 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).