Re: [PATCH RFC] virtio_net: gate delayed refill scheduling
From: Jason Wang <jasowang@redhat.com>
Date: 2025-11-28 02:20:36
Also in:
bpf, lkml, virtualization
On Fri, Nov 28, 2025 at 1:47 AM Bui Quang Minh [off-list ref] wrote:
I think the the requeue in refill_work is not the problem here. In virtnet_rx_pause[_all](), we use cancel_work_sync() which is safe to use "even if the work re-queues itself". AFAICS, cancel_work_sync() will disable work -> flush work -> enable again. So if the work requeue itself in flush work, the requeue will fail because the work is already disabled.
Right.
I think what triggers the deadlock here is a bug in virtnet_rx_resume_all(). virtnet_rx_resume_all() calls to __virtnet_rx_resume() which calls napi_enable() and may schedule refill. It schedules the refill work right after napi_enable the first receive queue. The correct way must be napi_enable all receive queues before scheduling refill work.
So what you meant is that the napi_disable() is called for a queue
whose NAPI has been disabled?
cpu0] enable_delayed_refill()
cpu0] napi_enable(queue0)
cpu0] schedule_delayed_work(&vi->refill)
cpu1] napi_disable(queue0)
cpu1] napi_enable(queue0)
cpu1] napi_disable(queue1)
In this case cpu1 waits forever while holding the netdev lock. This
looks like a bug since the netdev_lock 413f0271f3966 ("net: protect
NAPI enablement with netdev_lock()")?
quoted hunk ↗ jump to hunk
The fix is like this (there are quite duplicated code, I will clean up and send patches later if it is correct)diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 8e04adb57f52..892aa0805d1b 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c@@ -3482,20 +3482,25 @@ static void __virtnet_rx_resume(struct virtnet_info *vi, static void virtnet_rx_resume_all(struct virtnet_info *vi) { int i; + bool schedule_refill = false; + + for (i = 0; i < vi->max_queue_pairs; i++) + __virtnet_rx_resume(vi, &vi->rq[i], false); enable_delayed_refill(vi); - for (i = 0; i < vi->max_queue_pairs; i++) { - if (i < vi->curr_queue_pairs) - __virtnet_rx_resume(vi, &vi->rq[i], true); - else - __virtnet_rx_resume(vi, &vi->rq[i], false); - } + + for (i = 0; i < vi->curr_queue_pairs; i++) + if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL)) + schedule_refill = true; + + if (schedule_refill) + schedule_delayed_work(&vi->refill, 0); } static void virtnet_rx_resume(struct virtnet_info *vi, struct receive_queue *rq) { - enable_delayed_refill(vi); __virtnet_rx_resume(vi, rq, true); + enable_delayed_refill(vi);
This seems to be odd. I think at least we need to move this before:
+ if (schedule_refill) + schedule_delayed_work(&vi->refill, 0);
?
} static int virtnet_rx_resize(struct virtnet_info *vi, I also move the enable_delayed_refill() after we __virtnet_rx_resume() to ensure no refill is scheduled before napi_enable(). What do you think?
This has been implemented in your patch or I may miss something. Thanks
Thanks, Quang Minh