Re: [PATCH net 1/3] virtio-net: make refill work a per receive queue work

[Bui Quang Minh <minhquangbui99@gmail.com>]

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@redhat.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@redhat.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) {
          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.

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.