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

[Zhi Yong Wu]

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])

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