On Thu, Feb 16, 2012 at 5:21 PM, Zhi Yong Wu <zwu.ker...@gmail.com> wrote: > On Thu, Feb 16, 2012 at 4:48 PM, Jan Kiszka <jan.kis...@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.kis...@web.de> wrote: >>>> On 2012-02-16 09:07, zwu.ker...@gmail.com wrote: >>>>> From: Zhi Yong Wu <wu...@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 <wu...@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