On Thu, Oct 13, 2022 at 04:00:57PM +0200, Laurent Vivier wrote:
> When virtio-net is used with the socket netdev backend, the backend
> can be busy and not able to collect new packets.
>
> In this case, net_socket_receive() returns 0 and registers a poll function
> to detect when the socket is ready again.
>
> In virtio_net_tx_bh(), virtio_net_flush_tx() forwards the 0, the virtio
> notifications are disabled and the function is not rescheduled, waiting
> for the backend to be ready.
>
> When the socket netdev backend is again able to send packets, the poll
> function re-starts to flush remaining packets. This is done by
> calling virtio_net_tx_complete(). It re-enables notifications and calls
> again virtio_net_flush_tx().
>
> But it seems if virtio_net_flush_tx() reaches the tx_burst value all
> the queue is not flushed and no new notification is sent to reschedule
> virtio_net_tx_bh(). Nothing re-start to flush the queue and remaining packets
> are stuck in the queue.
>
> To fix that, detect in virtio_net_tx_complete() if virtio_net_flush_tx()
> has been stopped by tx_burst and if yes reschedule the bottom half
> function virtio_net_tx_bh() to flush the remaining packets.
>
> This is what is done in virtio_net_tx_bh() when the virtio_net_flush_tx()
> is synchronous, and completely by-passed when the operation needs to be
> asynchronous.
>
> RFC RFC RFC RFC RFC RFC RFC RFC RFC RFC RFC RFC RFC RFC RFC RFC RFC RFC
>
> Do we need to reschedule the function in the other case managed
> in virtio_net_tx_bh() and by-passed when the completion is asynchronous?
I am guessing no.
> /* If less than a full burst, re-enable notification and flush
> * anything that may have come in while we weren't looking. If
> * we find something, assume the guest is still active and reschedule */
> virtio_queue_set_notification(q->tx_vq, 1);
> ret = virtio_net_flush_tx(q);
> if (ret == -EINVAL) {
> return;
> } else if (ret > 0) {
> virtio_queue_set_notification(q->tx_vq, 0);
> qemu_bh_schedule(q->tx_bh);
> q->tx_waiting = 1;
> }
>
> RFC RFC RFC RFC RFC RFC RFC RFC RFC RFC RFC RFC RFC RFC RFC RFC RFC RFC
>
> Fixes: a697a334b3c4 ("virtio-net: Introduce a new bottom half packet TX")
> Cc: alex.william...@redhat.com
> Signed-off-by: Laurent Vivier <lviv...@redhat.com>
Looks ok superficially
Acked-by: Michael S. Tsirkin <m...@redhat.com>
Jason, your area.
> ---
> hw/net/virtio-net.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index e9f696b4cfeb..1fbf2f3e19a7 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -2526,6 +2526,7 @@ static void virtio_net_tx_complete(NetClientState *nc,
> ssize_t len)
> VirtIONet *n = qemu_get_nic_opaque(nc);
> VirtIONetQueue *q = virtio_net_get_subqueue(nc);
> VirtIODevice *vdev = VIRTIO_DEVICE(n);
> + int ret;
>
> virtqueue_push(q->tx_vq, q->async_tx.elem, 0);
> virtio_notify(vdev, q->tx_vq);
> @@ -2534,7 +2535,17 @@ static void virtio_net_tx_complete(NetClientState *nc,
> ssize_t len)
> q->async_tx.elem = NULL;
>
> virtio_queue_set_notification(q->tx_vq, 1);
> - virtio_net_flush_tx(q);
> + ret = virtio_net_flush_tx(q);
> + if (q->tx_bh && ret >= n->tx_burst) {
> + /*
> + * the flush has been stopped by tx_burst
> + * we will not receive notification for the
> + * remainining part, so re-schedule
> + */
> + virtio_queue_set_notification(q->tx_vq, 0);
> + qemu_bh_schedule(q->tx_bh);
> + q->tx_waiting = 1;
> + }
> }
>
> /* TX */
> --
> 2.37.3
