Thread (19 messages) 19 messages, 3 authors, 2026-01-05

Re: [PATCH net v2 1/3] virtio-net: don't schedule delayed refill worker

From: "Michael S. Tsirkin" <mst@redhat.com>
Date: 2026-01-03 00:16:09
Also in: bpf, lkml, stable, virtualization

On Fri, Jan 02, 2026 at 10:20:21PM +0700, Bui Quang Minh wrote:
When we fail to refill the receive buffers, we schedule a delayed worker
to retry later. However, this worker creates some concurrency issues
such as races and deadlocks.
include at least one example here, pls.
quoted hunk ↗ jump to hunk
To simplify the logic and avoid further
problems, we will instead retry refilling in the next NAPI poll.

Fixes: 4bc12818b363 ("virtio-net: disable delayed refill when pausing rx")
Reported-by: Paolo Abeni <pabeni@redhat.com>
Closes: https://netdev-ctrl.bots.linux.dev/logs/vmksft/drv-hw-dbg/results/400961/3-xdp-py/stderr
Cc: stable@vger.kernel.org
Suggested-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Signed-off-by: Bui Quang Minh <redacted>
---
 drivers/net/virtio_net.c | 55 ++++++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 25 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 1bb3aeca66c6..ac514c9383ae 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);
So this function sets retry_refill to true but assumes caller
will set it to false? seems unnecessarily complex.
just have to always set retry_refill correctly
and not rely on the caller.

quoted hunk ↗ jump to hunk
@@ -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
@@ -3160,7 +3156,7 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
 		virtnet_xdp_put_sq(vi, sq);
 	}
 
-	return received;
+	return retry_refill ? budget : received;
a comment can't hurt here, to document what is going on.
quoted hunk ↗ jump to hunk
 }
 
 static void virtnet_disable_queue_pair(struct virtnet_info *vi, int qp_index)
@@ -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,17 @@ 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);
+	if (dev->flags & IFF_UP) {
+		/* Let the NAPI poll refill the receive buffer for us. We can't
+		 * safely call try_fill_recv() here because the NAPI might be
+		 * enabled already.
+		 */
+		local_bh_disable();
+		for (i = 0; i < vi->curr_queue_pairs; i++)
you cam declare i here in the for loop.
and ++i is a bit clearer.
+			virtqueue_napi_schedule(&vi->rq[i].napi, vi->rq[i].vq);
+
+		local_bh_enable();
+	}
 
 	return 0;
 }
-- 
2.43.0
  
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help