Re: [Qemu-devel] [PATCH v2 for-2.4 00/12] hw/net: Fix .can_receive() for NICs

[Stefan Hajnoczi]

On Wed, Jul 15, 2015 at 06:19:01PM +0800, Fam Zheng wrote:
> v2: Don't use BH in patches 6 and 12. [Jason]
>     Add Stefan's reviewed-by in all others.
>     Drop trace point in patch 8 and move function body upward. [Michael]
> 
> Since a90a742 "tap: Drop tap_can_send", all nics that returns false from
> .can_receive() are required to explicitly flush the incoming queue when the
> status of it is changing back to true, otherwise the backend will sop
> processing more rx packets.
> 
> The purpose of this callback is to tell the peer backend (tap, socket, etc)
> "hold on until guest consumes old data because my buffer is not ready". More
> often than not NICs also do this when driver deactivated the card or disabled
> rx, causing the packets being unnessarily queued, where they should actualy be
> dropped.
> 
> This series adds such missing qemu_flush_queued_packets calls for all NICs, 
> and
> drops such unnecessary conditions in .can_receive(), so that NICs now:
> 
>   - return false from .can_receive when guest buffers are busy.
>   - call qemu_flush_queued_packets when buffers are available again.
>   - return -1 from .receive when rx is not enabled.
> 
> e1000, ne2000, rocker and vmxnet3 are not included because they're fixed by
> other patches on the list and applied to Stefan's tree.
> 
> virtio-net is covered by another series:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2015-06/msg07377.html ^U
> 
> http://patchwork.ozlabs.org/patch/495379/  (the subject should have v2, my 
> bad.)
> 
> All other NICs are okay, as they already do the flush on the state transition
> points.
> 
> Please review.
> 
> Fam Zheng (12):
>   xgmac: Drop packets with eth_can_rx is false.
>   pcnet: Drop pcnet_can_receive
>   eepro100: Drop nic_can_receive
>   usbnet: Drop usbnet_can_receive
>   etsec: Move etsec_can_receive into etsec_receive
>   etsec: Flush queue when rx buffer is consumed
>   mcf_fec: Drop mcf_fec_can_receive
>   milkymist-minimac2: Flush queued packets when link comes up
>   mipsnet: Flush queued packets when receiving is enabled
>   stellaris_enet: Flush queued packets when read done
>   dp8393x: Flush packets when link comes up
>   axienet: Flush queued packets when rx is done
> 
>  hw/net/dp8393x.c            |  8 ++++++++
>  hw/net/eepro100.c           | 11 -----------
>  hw/net/fsl_etsec/etsec.c    | 20 ++++++++++----------
>  hw/net/fsl_etsec/etsec.h    |  4 +++-
>  hw/net/fsl_etsec/rings.c    | 17 +++++++++++------
>  hw/net/lance.c              |  1 -
>  hw/net/mcf_fec.c            |  9 +--------
>  hw/net/milkymist-minimac2.c | 33 ++++++++++++++++-----------------
>  hw/net/mipsnet.c            |  9 +++++++--
>  hw/net/pcnet-pci.c          |  1 -
>  hw/net/pcnet.c              |  9 ---------
>  hw/net/pcnet.h              |  1 -
>  hw/net/stellaris_enet.c     | 14 +++++---------
>  hw/net/xgmac.c              |  8 ++++----
>  hw/net/xilinx_axienet.c     | 17 +++++++++++++----
>  hw/usb/dev-network.c        | 20 ++++----------------
>  trace-events                |  1 -
>  17 files changed, 82 insertions(+), 101 deletions(-)

I can't merge this until net/hub.c is fixed.  Anything that relies on
qemu_can_send_packet() will break if we drop .can_receive() functions.

At this point in the 2.4 release cycle I'm tempted to revert the
relevant qemu_set_fd_handler2() refactoring patches and stage this for
2.5 instead.  That way we know networking works as before and we can
tackle all these fixups in 2.5.

pgp7KUbxkoMc2.pgp
Description: PGP signature