On Thu, Dec 4, 2025 at 11:08 PM Bui Quang Minh [off-list ref] wrote:

On 12/3/25 13:37, Jason Wang wrote:

quoted

On Tue, Dec 2, 2025 at 11:29 PM Bui Quang Minh [off-list ref] wrote:

quoted

On 12/2/25 13:03, Jason Wang wrote:

quoted

On Mon, Dec 1, 2025 at 11:04 PM Bui Quang Minh [off-list ref] wrote:

quoted

On 11/28/25 09:20, Jason Wang wrote:

quoted

On Fri, Nov 28, 2025 at 1:47 AM Bui Quang Minh [off-list ref] wrote:

quoted

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.

quoted

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()")?

Yes, I've tried to fix it in 4bc12818b363 ("virtio-net: disable delayed
refill when pausing rx"), but it has flaws.

I wonder if a simplified version is just restoring the behaviour
before 413f0271f3966 by using napi_enable_locked() but maybe I miss
something.

As far as I understand, before 413f0271f3966 ("net: protect NAPI
enablement with netdev_lock()"), the napi is protected by the

I guess you meant napi enable/disable actually.

quoted

rtnl_lock(). But in the refill_work, we don't acquire the rtnl_lock(),

Any reason we need to hold rtnl_lock() there?

Correct me if I'm wrong here. Before 413f0271f3966 ("net: protect NAPI
enablement with netdev_lock()"), napi_disable and napi_enable are not
safe to be called concurrently.

The example race is

napi_disable -> napi_save_config -> write to n->config->defer_hard_irqs
napi_enable -> napi_restore_config -> read n->config->defer_hard_irqs

In refill_work, we don't hold any locks so the race scenario can happen.

Ok, I get you, so it occurs after we introduced the NAPI config to virtio-net.

Maybe I misunderstand what you mean by restoring the behavior before
413f0271f3966. Do you mean that we use this pattern

     In virtnet_xdp_se;

     netdev_lock(dev);
     virtnet_rx_pause_all()
         -> napi_disable_locked

     virtnet_rx_resume_all()
         -> napi_disable_locked
     netdev_unlock(dev);

And in other places where we pause the rx too. It will hold the
netdev_lock during the time napi is disabled so that even when
refill_work happens concurrently, napi_disable cannot acquire the
netdev_lock and gets stuck inside.

It might work but I think it would be easy to either

1) use locked version everywhere

or

2) use the unlocked version everywhere

Mix using locked and unlocked may cause the code hard to be maintained

Thanks

quoted

quoted

so it seems like we will have race condition before 413f0271f3966 ("net:
protect NAPI enablement with netdev_lock()").

Thanks,
Quang Minh.

Thanks

Thanks,
Quang Minh.