Message ID | 4F4D5AA1.5050309@web.de |
---|---|
State | New |
Headers | show |
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
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;