[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2] slirp: fix packet requeue issue in batchq
From: |
Zhi Yong Wu |
Subject: |
Re: [Qemu-devel] [PATCH v2] slirp: fix packet requeue issue in batchq |
Date: |
Thu, 16 Feb 2012 17:30:29 +0800 |
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