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

[Zhi Yong Wu]

On Wed, Feb 15, 2012 at 4:30 PM, Jan Kiszka <address@hidden> wrote:
> On 2012-02-15 09:13, address@hidden wrote:
>> From: Zhi Yong Wu <address@hidden>
>>
>> This patch fixes the slirp crash in current QEMU upstream.
>>
>> Signed-off-by: Zhi Yong Wu <address@hidden>
>> ---
>>  slirp/if.c   |   37 ++++++++++++++++++++++++++++++-------
>>  slirp/mbuf.c |    3 +--
>>  2 files changed, 31 insertions(+), 9 deletions(-)
>>
>> diff --git a/slirp/if.c b/slirp/if.c
>> index 8e0cac2..f7f8577 100644
>> --- a/slirp/if.c
>> +++ b/slirp/if.c
>> @@ -20,8 +20,15 @@ ifs_insque(struct mbuf *ifm, struct mbuf *ifmhead)
>>  static void
>>  ifs_remque(struct mbuf *ifm)
>>  {
>> -     ifm->ifs_prev->ifs_next = ifm->ifs_next;
>> -     ifm->ifs_next->ifs_prev = ifm->ifs_prev;
>> +        if (ifm->ifs_next->ifs_next == ifm
>> +            && ifm->ifs_next->ifs_prev == ifm) {
>> +            ifs_init(ifm->ifs_next);
>> +        } else {
>> +            ifm->ifs_prev->ifs_next = ifm->ifs_next;
>> +            ifm->ifs_next->ifs_prev = ifm->ifs_prev;
>> +        }
>> +
>> +        ifs_init(ifm);
>
> This looks, well, interesting. Can you explain the logic? We really need
> to document the queuing mechanics.
          ifm->ifs_prev->ifs_next = ifm->ifs_next;
          ifm->ifs_next->ifs_prev = ifm->ifs_prev;
+        ifs_init(ifm);
Sorry, actually we only need to add one line of code.
>
>>  }
>>
>>  void
>> @@ -154,14 +161,18 @@ 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");
>> +    DEBUG_CALL("if_start");
>>
>> -     if (slirp->if_queued == 0)
>> -        return; /* Nothing to do */
>> +    if (slirp->if_queued == 0)
>> +        return; /* Nothing to do */
>
> Even if the result looks funny, let's not touch lines just for indention
> (and braces would be missing anyway).
OK. undo them to their original state.
>
>> +
>> +    slirp->next_m = &slirp->if_batchq;
>
> Have you understood the difference between the natural order of
> if_batchq and next_m? I still wonder what the latter is good for.
Sorry, the line of code should be removed. next_m will point to next
packet which will be handled, if there's multiple session, it will
point to the head packet for next session; if there's one session, it
will point to batchq.

>
>>
>>   again:
>> +        ifm_next = NULL;
>> +
>>          /* check if we can really output */
>>          if (!slirp_can_output(slirp->opaque))
>>              return;
>> @@ -190,6 +201,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 +221,18 @@ if_start(Slirp *slirp)
>>                  m_free(ifm);
>>              } else {
>>                  /* re-queue */
>> -                insque(ifm, ifqt);
>> +                if (ifm_next) {
>> +                    /*restore the original state of bachq*/
>> +                    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;
>
> So is this only about the correct ordering or also about pointer
> correctness?
The former. If ifm need to be re-queued, it will be put to where it
originally is.
>
>> +                } 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:
>
> Thanks for digging into this, but I really think it needs more comments
> and potentially even some cleanups.
>
> Jan
>



-- 
Regards,

Zhi Yong Wu