On Mon, Mar 03, 2025 at 04:03:55PM -0800, Jakub Kicinski wrote:

On Mon, 3 Mar 2025 13:33:10 -0500 Joe Damato wrote:

quoted

quoted

quoted

@@ -2880,6 +2880,13 @@ static void refill_work(struct work_struct *work)
        bool still_empty;
        int i;

+       spin_lock(&vi->refill_lock);
+       if (!vi->refill_enabled) {
+               spin_unlock(&vi->refill_lock);
+               return;
+       }
+       spin_unlock(&vi->refill_lock);
+
        for (i = 0; i < vi->curr_queue_pairs; i++) {
                struct receive_queue *rq = &vi->rq[i];
 

Err, I suppose this also doesn't work because:

CPU0                       CPU1
rtnl_lock                  (before CPU0 calls disable_delayed_refill) 
  virtnet_close            refill_work
                             rtnl_lock()
  cancel_sync <= deadlock

Need to give this a bit more thought.  

How about we don't use the API at all from refill_work?

Patch 4 adds consistent NAPI config state and refill_work isn't a
queue resize maybe we don't need to call the netif_queue_set_napi at
all since the NAPI IDs are persisted in the NAPI config state and
refill_work shouldn't change that?

In which case, we could go back to what refill_work was doing
before and avoid the problem entirely.

What do you think ?

Should work, I think. Tho, I suspect someone will want to add queue API
support to virtio sooner or later, and they will run into the same
problem with the netdev instance lock, as all of ndo_close() will then
be covered with netdev->lock.

More thorough and idiomatic way to solve the problem would be to cancel
the work non-sync in ndo_close, add cancel with _sync after netdev is
unregistered (in virtnet_remove()) when the lock is no longer held, then
wrap the entire work with a relevant lock and check if netif_running()
to return early in case of a race.

Thanks for the guidance. I am happy to make an attempt at
implementing this in a future, separate series that follows this
one (probably after netdev conf in a few weeks :).

Middle ground would be to do what you suggested above and just leave 
a well worded comment somewhere that will show up in diffs adding queue
API support?

Jason, Michael, et. al.:  what do you think ? I don't want to spin
up a v6 if you are opposed to proceeding this way. Please let me
know.