On Thu, Oct 13, 2022 at 10:48 PM Michael S. Tsirkin <m...@redhat.com> wrote:
>
> 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;
> > + }
Do we need to fix the case of tx timer or it doesn't suffer from this issue?
Thanks
> > }
> >
> > /* TX */
> > --
> > 2.37.3
>
