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

From: "Michael S. Tsirkin" <[email-masked]>
Date: Sun, 4 Jan 2026 08:24:15 -0500

On Sun, Jan 04, 2026 at 11:33:20AM +0700, Bui Quang Minh wrote:
> On 1/3/26 23:57, Michael S. Tsirkin wrote:
> > 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. 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 <[email-masked]>
> > > Closes: https://netdev-ctrl.bots.linux.dev/logs/vmksft/drv-hw-dbg/results/400961/3-xdp-py/stderr
> > > Cc: [email-masked]
> > > Suggested-by: Xuan Zhuo <[email-masked]>
> > > Signed-off-by: Bui Quang Minh <[email-masked]>
> > > ---
> > >   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);
> > > @@ -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;
> > >   }
> > >   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
> > hmm do we even need this, then?
> > 
> > > +			 */
> > > +			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);
> > 
> > hmm do we even need this, then?
> > 
> > > +
> > >   	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.
> > > +		 */
> > I'd drop this comment, it does not clarify much.
> 
> Here I want to note on why we can't use try_fill_recv like in other places.

It does not really answer the question why, you could just disable napi
if you wanted.

> > 
> > > +		local_bh_disable();
> > > +		for (i = 0; i < vi->curr_queue_pairs; i++)
> > > +			virtqueue_napi_schedule(&vi->rq[i].napi, vi->rq[i].vq);
> > > +
> > > +		local_bh_enable();
> > > +	}
> > >   	return 0;
> > >   }
> > > -- 
> > > 2.43.0