* [PATCH net] net: xdp: don't call notifiers during driver init
@ 2023-03-16 0:29 Jakub Kicinski
2023-03-16 5:31 ` John Fastabend
2023-03-16 6:49 ` Tariq Toukan
0 siblings, 2 replies; 6+ messages in thread
From: Jakub Kicinski @ 2023-03-16 0:29 UTC (permalink / raw)
To: davem
Cc: netdev, edumazet, pabeni, Jakub Kicinski, ast, daniel, hawk,
john.fastabend, lorenzo, tariqt, bpf
Drivers will commonly perform feature setting during init, if they use
the xdp_set_features_flag() helper they'll likely run into an ASSERT_RTNL()
inside call_netdevice_notifiers_info().
Don't call the notifier until the device is actually registered.
Nothing should be tracking the device until its registered.
Fixes: 4d5ab0ad964d ("net/mlx5e: take into account device reconfiguration for xdp_features flag")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: ast@kernel.org
CC: daniel@iogearbox.net
CC: hawk@kernel.org
CC: john.fastabend@gmail.com
CC: lorenzo@kernel.org
CC: tariqt@nvidia.com
CC: bpf@vger.kernel.org
---
net/core/xdp.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/core/xdp.c b/net/core/xdp.c
index 87e654b7d06c..5722a1fc6e9e 100644
--- a/net/core/xdp.c
+++ b/net/core/xdp.c
@@ -781,6 +781,9 @@ void xdp_set_features_flag(struct net_device *dev, xdp_features_t val)
return;
dev->xdp_features = val;
+
+ if (dev->reg_state < NETREG_REGISTERED)
+ return;
call_netdevice_notifiers(NETDEV_XDP_FEAT_CHANGE, dev);
}
EXPORT_SYMBOL_GPL(xdp_set_features_flag);
--
2.39.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* RE: [PATCH net] net: xdp: don't call notifiers during driver init
2023-03-16 0:29 [PATCH net] net: xdp: don't call notifiers during driver init Jakub Kicinski
@ 2023-03-16 5:31 ` John Fastabend
2023-03-16 6:49 ` Tariq Toukan
1 sibling, 0 replies; 6+ messages in thread
From: John Fastabend @ 2023-03-16 5:31 UTC (permalink / raw)
To: Jakub Kicinski, davem
Cc: netdev, edumazet, pabeni, Jakub Kicinski, ast, daniel, hawk,
john.fastabend, lorenzo, tariqt, bpf
Jakub Kicinski wrote:
> Drivers will commonly perform feature setting during init, if they use
> the xdp_set_features_flag() helper they'll likely run into an ASSERT_RTNL()
> inside call_netdevice_notifiers_info().
>
> Don't call the notifier until the device is actually registered.
> Nothing should be tracking the device until its registered.
>
> Fixes: 4d5ab0ad964d ("net/mlx5e: take into account device reconfiguration for xdp_features flag")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: ast@kernel.org
> CC: daniel@iogearbox.net
> CC: hawk@kernel.org
> CC: john.fastabend@gmail.com
> CC: lorenzo@kernel.org
> CC: tariqt@nvidia.com
> CC: bpf@vger.kernel.org
> ---
> net/core/xdp.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/net/core/xdp.c b/net/core/xdp.c
> index 87e654b7d06c..5722a1fc6e9e 100644
> --- a/net/core/xdp.c
> +++ b/net/core/xdp.c
> @@ -781,6 +781,9 @@ void xdp_set_features_flag(struct net_device *dev, xdp_features_t val)
> return;
>
> dev->xdp_features = val;
> +
> + if (dev->reg_state < NETREG_REGISTERED)
> + return;
> call_netdevice_notifiers(NETDEV_XDP_FEAT_CHANGE, dev);
> }
> EXPORT_SYMBOL_GPL(xdp_set_features_flag);
> --
> 2.39.2
>
LGTM
Acked-by: John Fastabend <john.fastabend@gmail.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: xdp: don't call notifiers during driver init
2023-03-16 0:29 [PATCH net] net: xdp: don't call notifiers during driver init Jakub Kicinski
2023-03-16 5:31 ` John Fastabend
@ 2023-03-16 6:49 ` Tariq Toukan
2023-03-16 11:03 ` Lorenzo Bianconi
1 sibling, 1 reply; 6+ messages in thread
From: Tariq Toukan @ 2023-03-16 6:49 UTC (permalink / raw)
To: Jakub Kicinski, davem
Cc: netdev, edumazet, pabeni, ast, daniel, hawk, john.fastabend,
lorenzo, tariqt, bpf, Tariq Toukan
On 16/03/2023 2:29, Jakub Kicinski wrote:
> Drivers will commonly perform feature setting during init, if they use
> the xdp_set_features_flag() helper they'll likely run into an ASSERT_RTNL()
> inside call_netdevice_notifiers_info().
>
> Don't call the notifier until the device is actually registered.
> Nothing should be tracking the device until its registered.
>
> Fixes: 4d5ab0ad964d ("net/mlx5e: take into account device reconfiguration for xdp_features flag")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: ast@kernel.org
> CC: daniel@iogearbox.net
> CC: hawk@kernel.org
> CC: john.fastabend@gmail.com
> CC: lorenzo@kernel.org
> CC: tariqt@nvidia.com
> CC: bpf@vger.kernel.org
> ---
> net/core/xdp.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/net/core/xdp.c b/net/core/xdp.c
> index 87e654b7d06c..5722a1fc6e9e 100644
> --- a/net/core/xdp.c
> +++ b/net/core/xdp.c
> @@ -781,6 +781,9 @@ void xdp_set_features_flag(struct net_device *dev, xdp_features_t val)
> return;
>
> dev->xdp_features = val;
> +
> + if (dev->reg_state < NETREG_REGISTERED)
> + return;
I maybe need to dig deeper, but, it looks strange to still
call_netdevice_notifiers in cases > NETREG_REGISTERED.
Isn't it problematic to call it with NETREG_UNREGISTERED ?
For comparison, netif_set_real_num_tx_queues has this ASSERT_RTNL() only
under dev->reg_state == NETREG_REGISTERED || dev->reg_state ==
NETREG_UNREGISTERING.
> call_netdevice_notifiers(NETDEV_XDP_FEAT_CHANGE, dev);
> }
> EXPORT_SYMBOL_GPL(xdp_set_features_flag);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: xdp: don't call notifiers during driver init
2023-03-16 6:49 ` Tariq Toukan
@ 2023-03-16 11:03 ` Lorenzo Bianconi
2023-03-16 21:54 ` Jakub Kicinski
0 siblings, 1 reply; 6+ messages in thread
From: Lorenzo Bianconi @ 2023-03-16 11:03 UTC (permalink / raw)
To: Tariq Toukan
Cc: Jakub Kicinski, davem, netdev, edumazet, pabeni, ast, daniel,
hawk, john.fastabend, tariqt, bpf
[-- Attachment #1: Type: text/plain, Size: 1995 bytes --]
>
>
> On 16/03/2023 2:29, Jakub Kicinski wrote:
> > Drivers will commonly perform feature setting during init, if they use
> > the xdp_set_features_flag() helper they'll likely run into an ASSERT_RTNL()
> > inside call_netdevice_notifiers_info().
> >
> > Don't call the notifier until the device is actually registered.
> > Nothing should be tracking the device until its registered.
> >
> > Fixes: 4d5ab0ad964d ("net/mlx5e: take into account device reconfiguration for xdp_features flag")
> > Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> > ---
> > CC: ast@kernel.org
> > CC: daniel@iogearbox.net
> > CC: hawk@kernel.org
> > CC: john.fastabend@gmail.com
> > CC: lorenzo@kernel.org
> > CC: tariqt@nvidia.com
> > CC: bpf@vger.kernel.org
> > ---
> > net/core/xdp.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/net/core/xdp.c b/net/core/xdp.c
> > index 87e654b7d06c..5722a1fc6e9e 100644
> > --- a/net/core/xdp.c
> > +++ b/net/core/xdp.c
> > @@ -781,6 +781,9 @@ void xdp_set_features_flag(struct net_device *dev, xdp_features_t val)
> > return;
> > dev->xdp_features = val;
> > +
> > + if (dev->reg_state < NETREG_REGISTERED)
> > + return;
>
> I maybe need to dig deeper, but, it looks strange to still
> call_netdevice_notifiers in cases > NETREG_REGISTERED.
>
> Isn't it problematic to call it with NETREG_UNREGISTERED ?
>
> For comparison, netif_set_real_num_tx_queues has this ASSERT_RTNL() only
> under dev->reg_state == NETREG_REGISTERED || dev->reg_state ==
> NETREG_UNREGISTERING.
does it make sense to run call_netdevice_notifiers() in xdp_set_features_flag()
just if dev->reg_state is NETREG_REGISTERED?
Moreover, looking at the code it seems netdev code can run with dev->reg_state
set to NETREG_UNREGISTERED and without holding RTNL lock, right?
Regards,
Lorenzo
>
> > call_netdevice_notifiers(NETDEV_XDP_FEAT_CHANGE, dev);
> > }
> > EXPORT_SYMBOL_GPL(xdp_set_features_flag);
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: xdp: don't call notifiers during driver init
2023-03-16 11:03 ` Lorenzo Bianconi
@ 2023-03-16 21:54 ` Jakub Kicinski
2023-03-16 21:59 ` Jakub Kicinski
0 siblings, 1 reply; 6+ messages in thread
From: Jakub Kicinski @ 2023-03-16 21:54 UTC (permalink / raw)
To: Lorenzo Bianconi
Cc: Tariq Toukan, davem, netdev, edumazet, pabeni, ast, daniel, hawk,
john.fastabend, tariqt, bpf
On Thu, 16 Mar 2023 12:03:57 +0100 Lorenzo Bianconi wrote:
> > I maybe need to dig deeper, but, it looks strange to still
> > call_netdevice_notifiers in cases > NETREG_REGISTERED.
> >
> > Isn't it problematic to call it with NETREG_UNREGISTERED ?
> >
> > For comparison, netif_set_real_num_tx_queues has this ASSERT_RTNL() only
> > under dev->reg_state == NETREG_REGISTERED || dev->reg_state ==
> > NETREG_UNREGISTERING.
>
> does it make sense to run call_netdevice_notifiers() in xdp_set_features_flag()
> just if dev->reg_state is NETREG_REGISTERED?
I was thinking - we'll adjust it if someone complains, but indeed
the detection is somewhat weak, a call on a dead device but under
rtnl_lock won't warn. Let me just copy what the queue helpers do,
exactly, then.
> Moreover, looking at the code it seems netdev code can run with dev->reg_state
> set to NETREG_UNREGISTERED and without holding RTNL lock, right?
You mean - part of unregistration is done without rtnl lock held?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: xdp: don't call notifiers during driver init
2023-03-16 21:54 ` Jakub Kicinski
@ 2023-03-16 21:59 ` Jakub Kicinski
0 siblings, 0 replies; 6+ messages in thread
From: Jakub Kicinski @ 2023-03-16 21:59 UTC (permalink / raw)
To: Lorenzo Bianconi
Cc: Tariq Toukan, davem, netdev, edumazet, pabeni, ast, daniel, hawk,
john.fastabend, tariqt, bpf
On Thu, 16 Mar 2023 14:54:18 -0700 Jakub Kicinski wrote:
> > does it make sense to run call_netdevice_notifiers() in xdp_set_features_flag()
> > just if dev->reg_state is NETREG_REGISTERED?
>
> I was thinking - we'll adjust it if someone complains, but indeed
> the detection is somewhat weak, a call on a dead device but under
> rtnl_lock won't warn. Let me just copy what the queue helpers do,
> exactly, then.
It's just a notifier, I think I'll go with == REGISTERED.
Let me send v2.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-03-16 21:59 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-03-16 0:29 [PATCH net] net: xdp: don't call notifiers during driver init Jakub Kicinski
2023-03-16 5:31 ` John Fastabend
2023-03-16 6:49 ` Tariq Toukan
2023-03-16 11:03 ` Lorenzo Bianconi
2023-03-16 21:54 ` Jakub Kicinski
2023-03-16 21:59 ` Jakub Kicinski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).