Message-ID: <CACGkMEuasGDh=wT0n5b5QFDSNNBK7muipBKHb2v5eoKCU0NWDw@mail.gmail.com>
Date: Wed, 31 Dec 2025 15:30:06 +0800
From: Jason Wang <jasowang@...hat.com>
To: Bui Quang Minh <minhquangbui99@...il.com>
Cc: "Michael S. Tsirkin" <mst@...hat.com>, Xuan Zhuo <xuanzhuo@...ux.alibaba.com>, netdev@...r.kernel.org, 
	Eugenio Pérez <eperezma@...hat.com>, 
	Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>, 
	Jesper Dangaard Brouer <hawk@...nel.org>, John Fastabend <john.fastabend@...il.com>, 
	Stanislav Fomichev <sdf@...ichev.me>, virtualization@...ts.linux.dev, 
	linux-kernel@...r.kernel.org, bpf@...r.kernel.org
Subject: Re: [PATCH net 1/3] virtio-net: make refill work a per receive queue work

On Wed, Dec 31, 2025 at 12:29 AM Bui Quang Minh
<minhquangbui99@...il.com> wrote:
>
> On 12/26/25 14:37, Michael S. Tsirkin wrote:
> > On Fri, Dec 26, 2025 at 09:31:26AM +0800, Jason Wang wrote:
> >> On Fri, Dec 26, 2025 at 12:27 AM Michael S. Tsirkin <mst@...hat.com> wrote:
> >>> On Thu, Dec 25, 2025 at 03:33:29PM +0800, Jason Wang wrote:
> >>>> On Wed, Dec 24, 2025 at 9:48 AM Michael S. Tsirkin <mst@...hat.com> wrote:
> >>>>> On Wed, Dec 24, 2025 at 09:37:14AM +0800, Xuan Zhuo wrote:
> >>>>>> Hi Jason,
> >>>>>>
> >>>>>> I'm wondering why we even need this refill work. Why not simply let NAPI retry
> >>>>>> the refill on its next run if the refill fails? That would seem much simpler.
> >>>>>> This refill work complicates maintenance and often introduces a lot of
> >>>>>> concurrency issues and races.
> >>>>>>
> >>>>>> Thanks.
> >>>>> refill work can refill from GFP_KERNEL, napi only from ATOMIC.
> >>>>>
> >>>>> And if GFP_ATOMIC failed, aggressively retrying might not be a great idea.
> >>>> Btw, I see some drivers are doing things as Xuan said. E.g
> >>>> mlx5e_napi_poll() did:
> >>>>
> >>>> busy |= INDIRECT_CALL_2(rq->post_wqes,
> >>>>                                  mlx5e_post_rx_mpwqes,
> >>>>                                  mlx5e_post_rx_wqes,
> >>>>
> >>>> ...
> >>>>
> >>>> if (busy) {
> >>>>           if (likely(mlx5e_channel_no_affinity_change(c))) {
> >>>>                  work_done = budget;
> >>>>                  goto out;
> >>>> ...
> >>>
> >>> is busy a GFP_ATOMIC allocation failure?
> >> Yes, and I think the logic here is to fallback to ksoftirqd if the
> >> allocation fails too much.
> >>
> >> Thanks
> >
> > True. I just don't know if this works better or worse than the
> > current design, but it is certainly simpler and we never actually
> > worried about the performance of the current one.
> >
> >
> > So you know, let's roll with this approach.
> >
> > I do however ask that some testing is done on the patch forcing these OOM
> > situations just to see if we are missing something obvious.
> >
> >
> > the beauty is the patch can be very small:
> > 1. patch 1 do not schedule refill ever, just retrigger napi
> > 2. remove all the now dead code
> >
> > this way patch 1 will be small and backportable to stable.
>
> I've tried 1. with this patch
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 1bb3aeca66c6..9e890aff2d95 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -3035,7 +3035,7 @@ static int virtnet_receive_packets(struct virtnet_info *vi,
>   }
>
>   static int virtnet_receive(struct receive_queue *rq, int budget,
> -               unsigned int *xdp_xmit)
> +               unsigned int *xdp_xmit, bool *retry_refill)
>   {
>       struct virtnet_info *vi = rq->vq->vdev->priv;
>       struct virtnet_rq_stats stats = {};
> @@ -3047,12 +3047,8 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
>           packets = virtnet_receive_packets(vi, rq, budget, xdp_xmit, &stats);
>
>       if (rq->vq->num_free > min((unsigned int)budget, virtqueue_get_vring_size(rq->vq)) / 2) {
> -        if (!try_fill_recv(vi, rq, GFP_ATOMIC)) {
> -            spin_lock(&vi->refill_lock);
> -            if (vi->refill_enabled)
> -                schedule_delayed_work(&vi->refill, 0);
> -            spin_unlock(&vi->refill_lock);
> -        }
> +        if (!try_fill_recv(vi, rq, GFP_ATOMIC))
> +            *retry_refill = true;
>       }
>
>       u64_stats_set(&stats.packets, packets);
> @@ -3129,18 +3125,18 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
>       struct send_queue *sq;
>       unsigned int received;
>       unsigned int xdp_xmit = 0;
> -    bool napi_complete;
> +    bool napi_complete, retry_refill = false;
>
>       virtnet_poll_cleantx(rq, budget);
>
> -    received = virtnet_receive(rq, budget, &xdp_xmit);
> +    received = virtnet_receive(rq, budget, &xdp_xmit, &retry_refill);
>       rq->packets_in_napi += received;
>
>       if (xdp_xmit & VIRTIO_XDP_REDIR)
>           xdp_do_flush();
>
>       /* Out of packets? */
> -    if (received < budget) {
> +    if (received < budget && !retry_refill) {

But you didn't return the budget when we need to retry here?

>           napi_complete = virtqueue_napi_complete(napi, rq->vq, received);
>           /* Intentionally not taking dim_lock here. This may result in a
>            * spurious net_dim call. But if that happens virtnet_rx_dim_work
> @@ -3230,9 +3226,11 @@ static int virtnet_open(struct net_device *dev)
>
>       for (i = 0; i < vi->max_queue_pairs; i++) {
>           if (i < vi->curr_queue_pairs)
> -            /* Make sure we have some buffers: if oom use wq. */
> -            if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL))
> -                schedule_delayed_work(&vi->refill, 0);
> +            /* If this fails, we will retry later in
> +             * NAPI poll, which is scheduled in the below
> +             * virtnet_enable_queue_pair
> +             */
> +            try_fill_recv(vi, &vi->rq[i], GFP_KERNEL);
>
>           err = virtnet_enable_queue_pair(vi, i);
>           if (err < 0)
> @@ -3473,15 +3471,15 @@ static void __virtnet_rx_resume(struct virtnet_info *vi,
>                   bool refill)
>   {
>       bool running = netif_running(vi->dev);
> -    bool schedule_refill = false;
>
> -    if (refill && !try_fill_recv(vi, rq, GFP_KERNEL))
> -        schedule_refill = true;
> +    if (refill)
> +        /* If this fails, we will retry later in NAPI poll, which is
> +         * scheduled in the below virtnet_napi_enable
> +         */
> +        try_fill_recv(vi, rq, GFP_KERNEL);
> +
>       if (running)
>           virtnet_napi_enable(rq);
> -
> -    if (schedule_refill)
> -        schedule_delayed_work(&vi->refill, 0);
>   }
>
>   static void virtnet_rx_resume_all(struct virtnet_info *vi)
> @@ -3777,6 +3775,7 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
>       struct virtio_net_rss_config_trailer old_rss_trailer;
>       struct net_device *dev = vi->dev;
>       struct scatterlist sg;
> +    int i;
>
>       if (!vi->has_cvq || !virtio_has_feature(vi->vdev, VIRTIO_NET_F_MQ))
>           return 0;
> @@ -3829,11 +3828,8 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
>       }
>   succ:
>       vi->curr_queue_pairs = queue_pairs;
> -    /* virtnet_open() will refill when device is going to up. */
> -    spin_lock_bh(&vi->refill_lock);
> -    if (dev->flags & IFF_UP && vi->refill_enabled)
> -        schedule_delayed_work(&vi->refill, 0);
> -    spin_unlock_bh(&vi->refill_lock);
> +    for (i = 0; i < vi->curr_queue_pairs; i++)
> +        try_fill_recv(vi, &vi->rq[i], GFP_KERNEL);
>
>       return 0;
>   }
>
>
> But I got an issue with selftests/drivers/net/hw/xsk_reconfig.py. This
> test sets up XDP zerocopy (Xsk) but does not provide any descriptors to
> the fill ring. So xsk_pool does not have any descriptors and
> try_fill_recv will always fail. The RX NAPI keeps polling. Later, when
> we want to disable the xsk_pool, in virtnet_xsk_pool_disable path,
>
> virtnet_xsk_pool_disable
> -> virtnet_rq_bind_xsk_pool
>    -> virtnet_rx_pause
>      -> __virtnet_rx_pause
>        -> virtnet_napi_disable
>          -> napi_disable
>
> We get stuck in napi_disable because the RX NAPI is still polling.

napi_disable will set NAPI_DISABLE bit, no?

>
> In drivers/net/ethernet/mellanox/mlx5, AFAICS, it uses state bit for
> synchronization between xsk setup (mlx5e_xsk_setup_pool) with RX NAPI
> (mlx5e_napi_poll) without using napi_disable/enable. However, in
> drivers/net/ethernet/intel/ice,
>
> ice_xsk_pool_setup
> -> ice_qp_dis
>    -> ice_qvec_toggle_napi
>      -> napi_disable
>
> it still uses napi_disable. Did I miss something in the above patch?
> I'll try to look into using another synchronization instead of
> napi_disable/enable in xsk_pool setup path too.
>
> Thanks,
> Quang Minh.
>

Thanks

Powered by blists - more mailing lists