[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/3] slirp: Fix requeuing of batchq packets in i
From: |
Jan Kiszka |
Subject: |
Re: [Qemu-devel] [PATCH 2/3] slirp: Fix requeuing of batchq packets in if_start |
Date: |
Wed, 29 Feb 2012 08:58:03 +0100 |
User-agent: |
Mozilla/5.0 (X11; U; Linux i686 (x86_64); de; rv:1.8.1.12) Gecko/20080226 SUSE/2.0.0.12-1.1 Thunderbird/2.0.0.12 Mnenhy/0.7.5.666 |
On 2012-02-28 23:52, Jan Kiszka wrote:
> On 2012-02-28 23:18, Stefan Weil wrote:
>> Am 17.02.2012 16:45, schrieb Jan Kiszka:
>>> In case we requeued a packet that was the head of a longer session
>>> queue, we failed to restore this ordering. Also, we did not properly
>>> deal with changes to Slirp::next_m.
>>>
>>> Instead of a cumbersome roll back, this fix simply avoids any changes
>>> until we know if the packet was actually sent. Both fixes crashes due
>>> to inconsistent queues and simplifies the logic.
>>>
>>> Thanks to Zhi Yong Wu who found the reason for these crashes.
>>>
>>> CC: Zhi Yong Wu <address@hidden>
>>> CC: Fabien Chouteau <address@hidden>
>>> Signed-off-by: Jan Kiszka <address@hidden>
>>> ---
>>> slirp/if.c | 35 +++++++++++++++++++----------------
>>> 1 files changed, 19 insertions(+), 16 deletions(-)
>>
>> Latest QEMU crashed here 4 times with MIPS Malta
>> when I tried 'apt-get update' in the guest. See gdb output
>> below for details.
>>
>> I only got the crash with big endian MIPS, not with little
>> endian which is strange.
>>
>> After I reverted the above patch, MIPS Malta worked
>> again as before.
>>
>> So maybe we changed one crash against a new one.
>
> Embarrassing.
>
> Does this help? Specifically expired packet handling is broken.
>
> diff --git a/slirp/if.c b/slirp/if.c
> index 33f08e1..954ef1e 100644
> --- a/slirp/if.c
> +++ b/slirp/if.c
> @@ -153,7 +153,7 @@ void if_start(Slirp *slirp)
> {
> uint64_t now = qemu_get_clock_ns(rt_clock);
> int requeued = 0;
> - bool from_batchq = false;
> + bool from_batchq;
> struct mbuf *ifm, *ifqt;
>
> DEBUG_CALL("if_start");
> @@ -169,6 +169,7 @@ void if_start(Slirp *slirp)
> */
> if (slirp->if_fastq.ifq_next != &slirp->if_fastq) {
> ifm = slirp->if_fastq.ifq_next;
> + from_batchq = false;
> } else {
> /* Nothing on fastq, see if next_m is valid */
> if (slirp->next_m != &slirp->if_batchq) {
> @@ -176,14 +177,17 @@ void if_start(Slirp *slirp)
> } else {
> ifm = slirp->if_batchq.ifq_next;
> }
> -
> from_batchq = true;
> }
>
> slirp->if_queued--;
>
> /* Try to send packet unless it already expired */
> - if (ifm->expiration_date >= now && !if_encap(slirp, ifm)) {
> + if (ifm->expiration_date < now) {
> + m_free(ifm);
> + continue;
> + }
> + if (!if_encap(slirp, ifm)) {
> /* Packet is delayed due to pending ARP resolution */
> requeued++;
> continue;
>
> Jan
Nonsense. We need to walk the queues properly and carefully. Will come
up with a better version.
Jan
signature.asc
Description: OpenPGP digital signature