Re: [Qemu-devel] [PATCH for-2.4 06/12] etsec: Flush queue when rx buffer is consumed

[Fam Zheng <famz@redhat.com>]

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 <famz@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.

> 
> 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),
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 */
                            ...

> 
> > ---
> >  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);
>