Re: [Qemu-devel] [PATCH] virtio-net: Drop net_virtio_info.can_receive

[Michael S. Tsirkin]

On Fri, Jul 03, 2015 at 12:17:38PM +0800, Fam Zheng wrote:
> On Fri, 07/03 09:12, Fam Zheng wrote:
> > On Thu, 07/02 18:46, Michael S. Tsirkin wrote:
> > > On Thu, Jul 02, 2015 at 01:46:26PM +0100, Stefan Hajnoczi wrote:
> > > > On Tue, Jun 30, 2015 at 04:35:24PM +0800, Jason Wang wrote:
> > > > > On 06/30/2015 11:06 AM, Fam Zheng wrote:
> > > > > > virtio_net_receive still does the check by calling
> > > > > > virtio_net_can_receive, if the device or driver is not ready, the 
> > > > > > packet
> > > > > > is dropped.
> > > > > >
> > > > > > This is necessary because returning false from can_receive 
> > > > > > complicates
> > > > > > things: the peer would disable sending until we explicitly flush the
> > > > > > queue.
> > > > > >
> > > > > > Signed-off-by: Fam Zheng <address@hidden>
> > > > > > ---
> > > > > >  hw/net/virtio-net.c | 1 -
> > > > > >  1 file changed, 1 deletion(-)
> > > > > >
> > > > > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > > > > > index d728233..dbef0d0 100644
> > > > > > --- a/hw/net/virtio-net.c
> > > > > > +++ b/hw/net/virtio-net.c
> > > > > > @@ -1503,7 +1503,6 @@ static int 
> > > > > > virtio_net_load_device(VirtIODevice *vdev, QEMUFile *f,
> > > > > >  static NetClientInfo net_virtio_info = {
> > > > > >      .type = NET_CLIENT_OPTIONS_KIND_NIC,
> > > > > >      .size = sizeof(NICState),
> > > > > > -    .can_receive = virtio_net_can_receive,
> > > > > >      .receive = virtio_net_receive,
> > > > > >      .link_status_changed = virtio_net_set_link_status,
> > > > > >      .query_rx_filter = virtio_net_query_rxfilter,
> > > > > 
> > > > > A side effect of this patch is it will read and then drop packet is
> > > > > guest driver is no ok.
> > > > 
> > > > I think that the semantics of .can_receive() and .receive() return
> > > > values are currently incorrect in many NICs.  They have .can_receive()
> > > > functions that return false for conditions where .receive() would
> > > > discard the packet.  So what happens is that packets get queued when
> > > > they should actually be discarded.
> > > > 
> > > > The purpose of the flow control (queuing) mechanism is to tell the
> > > > sender to hold off until the receiver has more rx buffers available.
> > > > It's a short-term thing that doesn't included link down, rx disable, or
> > > > NIC reset states.
> > > > 
> > > > Therefore, I think this patch will not introduce a regression.  It is
> > > > adjusting the code to stop queuing packets when they should actually be
> > > > dropped.
> > > > 
> > > > Thoughts?
> > > > 
> > > > Reviewed-by: Stefan Hajnoczi <address@hidden>
> > > 
> > > OK, but we do have transient out of buffers states too, and
> > > virtio_net_can_receive checks these as well.
> 
> I'm a bit confused, do you mean virtio_queue_ready()? That is buffer presence
> check, and the free buffer check virtio_net_has_buffers() is only in
> virtio_net_receive(). Am I missing things?
> 
> Fam

Interesting. You are right.

See also commit cdd5cc12ba8cf0c068da319370bdd3ba45eaf7ac.


> > > 
> > > To me, it looks like we should change virtio_net_can_receive to
> > > avoid checking link down state, etc.
> > > But I don't see why we should remove it completely.
> > 
> > OK, it makse sense to check buffer state.
> > 
> > Fam
> >