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

[Zhi Yong Wu]

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?

>
>> 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.

>
> Jan
>



-- 
Regards,

Zhi Yong Wu