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 <f...@redhat.com> >> 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);