From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40988) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZFHGi-0005Yt-8A for qemu-devel@nongnu.org; Wed, 15 Jul 2015 03:38:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZFHGf-0001SH-1O for qemu-devel@nongnu.org; Wed, 15 Jul 2015 03:38:16 -0400 Received: from mx1.redhat.com ([209.132.183.28]:49870) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZFHGe-0001S0-PS for qemu-devel@nongnu.org; Wed, 15 Jul 2015 03:38:12 -0400 Message-ID: <55A60DDE.3070804@redhat.com> Date: Wed, 15 Jul 2015 15:38:06 +0800 From: Jason Wang MIME-Version: 1.0 References: <1436860421-4604-1-git-send-email-famz@redhat.com> <1436860421-4604-7-git-send-email-famz@redhat.com> <55A4D775.40006@redhat.com> <20150714094840.GD27873@ad.nay.redhat.com> <55A5EB61.2040108@redhat.com> <20150715060108.GC2412@ad.nay.redhat.com> In-Reply-To: <20150715060108.GC2412@ad.nay.redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH for-2.4 06/12] etsec: Flush queue when rx buffer is consumed List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng Cc: Peter Maydell , Peter Crosthwaite , Rob Herring , qemu-devel@nongnu.org, Michael Walle , Gerd Hoffmann , stefanha@redhat.com, "Edgar E. Iglesias" On 07/15/2015 02:01 PM, Fam Zheng wrote: > On Wed, 07/15 13:10, Jason Wang wrote: >>>> And can we do this without a bh? Otherwise, we may need to stop and >>>> restart the bh during vm stop and start? >>> A bh doesn't hurt when vm stop and restart (we get superfluous flush), >> The problem is qemu_flush_queued_packets() does not check runstate. So >> it may call .receive() which may modify guest state (DMA or registers). > You're right, .can_receive will be called incorrectly if the following sequence > of events is processed by main loop right after we schedule it: > > 1) QMP 'stop' command: > Runstate is changed to STOP. > > 2) tap read: > A new packet is read in, but since qemu_can_send_packet is false, it will > be queued. > > 3) aio_dispatch: > This BH is called too late here, and the queue is flushed, which calls > .receive(). > > An ideal fix would be stopping tap with a vmstate handler, but for this patch, > does the following work? Looks good for me. How about axienet then since in your series it also uses a bh? > > diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c > index f5170ae..0f5cf44 100644 > --- a/hw/net/fsl_etsec/etsec.c > +++ b/hw/net/fsl_etsec/etsec.c > @@ -342,13 +342,22 @@ static ssize_t etsec_receive(NetClientState *nc, > const uint8_t *buf, > size_t size) > { > + ssize_t ret; > eTSEC *etsec = qemu_get_nic_opaque(nc); > > #if defined(HEX_DUMP) > fprintf(stderr, "%s receive size:%d\n", etsec->nic->nc.name, size); > qemu_hexdump(buf, stderr, "", size); > #endif > - return etsec_rx_ring_write(etsec, buf, size); > + /* Flush is unnecessary as are already in receiving path */ > + etsec->need_flush = false; > + ret = etsec_rx_ring_write(etsec, buf, size); > + if (ret == 0) { > + /* The packet will be queued, let's flush it when buffer is avilable > + * again. */ > + etsec->need_flush = true; > + } > + return ret; > } > > > diff --git a/hw/net/fsl_etsec/etsec.h b/hw/net/fsl_etsec/etsec.h > index fc41773..e7dc0a4 100644 > --- a/hw/net/fsl_etsec/etsec.h > +++ b/hw/net/fsl_etsec/etsec.h > @@ -144,6 +144,8 @@ typedef struct eTSEC { > QEMUBH *bh; > struct ptimer_state *ptimer; > > + /* Whether we should flush the rx queue when buffer becomes available. */ > + bool need_flush; > } eTSEC; > > #define TYPE_ETSEC_COMMON "eTSEC" > diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c > index a11280b..68e7b6d 100644 > --- a/hw/net/fsl_etsec/rings.c > +++ b/hw/net/fsl_etsec/rings.c > @@ -646,6 +646,9 @@ void etsec_walk_rx_ring(eTSEC *etsec, int ring_nbr) > } else { > etsec->rx_buffer_len = 0; > etsec->rx_buffer = NULL; > + if (etsec->need_flush) { > + qemu_flush_queued_packets(qemu_get_queue(etsec->nic)); > + } > } > > RING_DEBUG("eTSEC End of ring_write: remaining_data:%zu\n", remaining_data); > >