From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55341) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZFEyK-0006de-J1 for qemu-devel@nongnu.org; Wed, 15 Jul 2015 01:11:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZFEyG-0004ox-G1 for qemu-devel@nongnu.org; Wed, 15 Jul 2015 01:11:08 -0400 Received: from mx1.redhat.com ([209.132.183.28]:38129) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZFEyG-0004oV-7n for qemu-devel@nongnu.org; Wed, 15 Jul 2015 01:11:04 -0400 Message-ID: <55A5EB61.2040108@redhat.com> Date: Wed, 15 Jul 2015 13:10:57 +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> In-Reply-To: <20150714094840.GD27873@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/14/2015 05:48 PM, Fam Zheng wrote: > On Tue, 07/14 17:33, Jason Wang wrote: >> >> On 07/14/2015 03:53 PM, Fam Zheng wrote: >>> The BH will be scheduled when etsec->rx_buffer_len is becoming 0, which >>> is the condition of queuing. >>> >>> Signed-off-by: Fam Zheng >> I suggest to squash this into patch 05/12 to unbreak bisection. > 05/12 didn't change the logic, it just moved the semantics from returning 0 in > .can_receive() to .receive(). So I think it's OK to split to make reviewing > easier. Ok. > >> 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). > otherwise the call stack could go very deap with a long queue, something like: > > etsec_receive > etsec_rx_ring_write > etsec_walk_rx_ring > etsec_flush_rx_queue > qemu_flush_queued_packets > ... > etsec_receive > etsec_rx_ring_write > etsec_walk_rx_ring > etsec_flush_rx_queue > qemu_flush_queued_packets > /* loop */ > ... I get the point, thanks. >>> --- >>> hw/net/fsl_etsec/etsec.c | 9 +++++++++ >>> hw/net/fsl_etsec/etsec.h | 2 ++ >>> hw/net/fsl_etsec/rings.c | 1 + >>> 3 files changed, 12 insertions(+) >>> >>> diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c >>> index f5170ae..fcde9b4 100644 >>> --- a/hw/net/fsl_etsec/etsec.c >>> +++ b/hw/net/fsl_etsec/etsec.c >>> @@ -366,6 +366,13 @@ static NetClientInfo net_etsec_info = { >>> .link_status_changed = etsec_set_link_status, >>> }; >>> >>> +static void etsec_flush_rx_queue(void *opaque) >>> +{ >>> + eTSEC *etsec = opaque; >>> + >>> + qemu_flush_queued_packets(qemu_get_queue(etsec->nic)); >>> +} >>> + >>> static void etsec_realize(DeviceState *dev, Error **errp) >>> { >>> eTSEC *etsec = ETSEC_COMMON(dev); >>> @@ -378,6 +385,8 @@ static void etsec_realize(DeviceState *dev, Error **errp) >>> etsec->bh = qemu_bh_new(etsec_timer_hit, etsec); >>> etsec->ptimer = ptimer_init(etsec->bh); >>> ptimer_set_freq(etsec->ptimer, 100); >>> + >>> + etsec->flush_bh = qemu_bh_new(etsec_flush_rx_queue, etsec); >>> } >>> >>> static void etsec_instance_init(Object *obj) >>> diff --git a/hw/net/fsl_etsec/etsec.h b/hw/net/fsl_etsec/etsec.h >>> index fc41773..05bb7f8 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; >>> >>> + QEMUBH *flush_bh; >>> + >>> } eTSEC; >>> >>> #define TYPE_ETSEC_COMMON "eTSEC" >>> diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c >>> index a11280b..7aae93e 100644 >>> --- a/hw/net/fsl_etsec/rings.c >>> +++ b/hw/net/fsl_etsec/rings.c >>> @@ -646,6 +646,7 @@ void etsec_walk_rx_ring(eTSEC *etsec, int ring_nbr) >>> } else { >>> etsec->rx_buffer_len = 0; >>> etsec->rx_buffer = NULL; >>> + qemu_bh_schedule(etsec->flush_bh); >>> } >>> >>> RING_DEBUG("eTSEC End of ring_write: remaining_data:%zu\n", remaining_data);