Re: [Qemu-devel] [PATCH v2] slirp: fix packet requeue issue in batchq

[Zhi Yong Wu]

On Thu, Feb 16, 2012 at 5:21 PM, Zhi Yong Wu <address@hidden> wrote:
> On Thu, Feb 16, 2012 at 4:48 PM, Jan Kiszka <address@hidden> wrote:
>> On 2012-02-16 09:45, Zhi Yong Wu wrote:
>>> On Thu, Feb 16, 2012 at 4:37 PM, Jan Kiszka <address@hidden> wrote:
>>>> On 2012-02-16 09:07, address@hidden wrote:
>>>>> From: Zhi Yong Wu <address@hidden>
>>>>>
>>>>
>>>> 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 <address@hidden>
>>>>> ---
>>>>>  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