Re: [Qemu-devel] [PATCH 2/3] slirp: Fix requeuing of batchq packets in if_start

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 2866 bytes --]

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 <wuzhy@linux.vnet.ibm.com>
>>> CC: Fabien Chouteau <chouteau@adacore.com>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>> ---
>>> 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]