| Message ID | 1329379634-1498-1-git-send-email-zwu.kernel@gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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 >
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
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
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 >
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
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: