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: > > > > 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. > > 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? Seems like Jason agrees that leaving refill_work unmodified will work [1]. I think leaving a comment is a good idea and am happy to do so. Not sure where would be a good spot for it. Two spots that come to mind are: - in virtnet_probe where all the other netdev ops are plumbed through, or - above virtnet_disable_queue_pair which I assume a future queue API implementor would need to call for ndo_queue_stop I get the feeling you have a much better suggestion in mind though :) [1]: https://lore.kernel.org/netdev/CACGkMEvWuRjBbc3PvOUpDFkjcby5QNLw5hA_FpNSPyWjkEXD_Q@xxxxxxxxxxxxxx/
