| Message ID | 20250203215816.1294081-1-kuba@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 9dd05df8403bda5b68178b795c554b3940628bb6 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: warn if NAPI instance wasn't shut down | expand |
On Mon, Feb 03, 2025 at 01:58:16PM -0800, Jakub Kicinski wrote: > Drivers should always disable a NAPI instance before removing it. > If they don't the instance may be queued for polling. > Since commit 86e25f40aa1e ("net: napi: Add napi_config") > we also remove the NAPI from the busy polling hash table > in napi_disable(), so not disabling would leave a stale > entry there. > > Use of busy polling is relatively uncommon so bugs may be lurking > in the drivers. Add an explicit warning. > > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > --- > net/core/dev.c | 3 +++ > 1 file changed, 3 insertions(+) > Let's find out how many bugs are lurking: Reviewed-by: Joe Damato <jdamato@fastly.com>
On Mon, Feb 3, 2025 at 10:58 PM Jakub Kicinski <kuba@kernel.org> wrote: > > Drivers should always disable a NAPI instance before removing it. > If they don't the instance may be queued for polling. > Since commit 86e25f40aa1e ("net: napi: Add napi_config") > we also remove the NAPI from the busy polling hash table > in napi_disable(), so not disabling would leave a stale > entry there. > > Use of busy polling is relatively uncommon so bugs may be lurking > in the drivers. Add an explicit warning. > > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > --- > net/core/dev.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/net/core/dev.c b/net/core/dev.c > index c0021cbd28fc..2b141f20b13b 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -7071,6 +7071,9 @@ void __netif_napi_del_locked(struct napi_struct *napi) > if (!test_and_clear_bit(NAPI_STATE_LISTED, &napi->state)) > return; > > + /* Make sure NAPI is disabled (or was never enabled). */ > + WARN_ON(!test_bit(NAPI_STATE_SCHED, &napi->state)); > + > if (napi->config) { > napi->index = -1; > napi->config = NULL; > -- > 2.48.1 > This makes sense. Although a WARN_ON_ONCE() might avoid some noise. Reviewed-by: Eric Dumazet <edumazet@google.com>
Hello: This patch was applied to netdev/net-next.git (main) by Jakub Kicinski <kuba@kernel.org>: On Mon, 3 Feb 2025 13:58:16 -0800 you wrote: > Drivers should always disable a NAPI instance before removing it. > If they don't the instance may be queued for polling. > Since commit 86e25f40aa1e ("net: napi: Add napi_config") > we also remove the NAPI from the busy polling hash table > in napi_disable(), so not disabling would leave a stale > entry there. > > [...] Here is the summary with links: - [net-next] net: warn if NAPI instance wasn't shut down https://git.kernel.org/netdev/net-next/c/9dd05df8403b You are awesome, thank you!
diff --git a/net/core/dev.c b/net/core/dev.c index c0021cbd28fc..2b141f20b13b 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -7071,6 +7071,9 @@ void __netif_napi_del_locked(struct napi_struct *napi) if (!test_and_clear_bit(NAPI_STATE_LISTED, &napi->state)) return; + /* Make sure NAPI is disabled (or was never enabled). */ + WARN_ON(!test_bit(NAPI_STATE_SCHED, &napi->state)); + if (napi->config) { napi->index = -1; napi->config = NULL;
Drivers should always disable a NAPI instance before removing it. If they don't the instance may be queued for polling. Since commit 86e25f40aa1e ("net: napi: Add napi_config") we also remove the NAPI from the busy polling hash table in napi_disable(), so not disabling would leave a stale entry there. Use of busy polling is relatively uncommon so bugs may be lurking in the drivers. Add an explicit warning. Signed-off-by: Jakub Kicinski <kuba@kernel.org> --- net/core/dev.c | 3 +++ 1 file changed, 3 insertions(+)