netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [patch net-next 0/3] net: introduce per-netns netdevice notifiers and use them in mlxsw
@ 2019-09-30  8:15 Jiri Pirko
  2019-09-30  8:15 ` [patch net-next 1/3] net: push loops and nb calls into helper functions Jiri Pirko
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Jiri Pirko @ 2019-09-30  8:15 UTC (permalink / raw)
  To: netdev
  Cc: davem, idosch, pabeni, edumazet, petrm, sd, f.fainelli, stephen, mlxsw

From: Jiri Pirko <jiri@mellanox.com>

Some drivers, like mlxsw, are not interested in notifications coming in
for netdevices from other network namespaces. So introduce per-netns
notifiers and allow to reduce overhead by listening only for
notifications from the same netns.

This is also a preparation for upcoming patchset "devlink: allow devlink
instances to change network namespace". This resolves deadlock during
reload mlxsw into initial netns made possible by
328fbe747ad4 ("net: Close race between {un, }register_netdevice_notifier() and setup_net()/cleanup_net()").

Jiri Pirko (3):
  net: push loops and nb calls into helper functions
  net: introduce per-netns netdevice notifiers
  mlxsw: spectrum: Use per-netns netdevice notifier registration

 .../net/ethernet/mellanox/mlxsw/spectrum.c    |   9 +-
 include/linux/netdevice.h                     |   6 +
 include/net/net_namespace.h                   |   6 +-
 net/core/dev.c                                | 176 +++++++++++++++---
 4 files changed, 165 insertions(+), 32 deletions(-)

-- 
2.21.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [patch net-next 1/3] net: push loops and nb calls into helper functions
  2019-09-30  8:15 [patch net-next 0/3] net: introduce per-netns netdevice notifiers and use them in mlxsw Jiri Pirko
@ 2019-09-30  8:15 ` Jiri Pirko
  2019-09-30  8:15 ` [patch net-next 2/3] net: introduce per-netns netdevice notifiers Jiri Pirko
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 10+ messages in thread
From: Jiri Pirko @ 2019-09-30  8:15 UTC (permalink / raw)
  To: netdev
  Cc: davem, idosch, pabeni, edumazet, petrm, sd, f.fainelli, stephen, mlxsw

From: Jiri Pirko <jiri@mellanox.com>

Push iterations over net namespaces and netdevices from
register_netdevice_notifier() and unregister_netdevice_notifier()
into helper functions. Along with that introduce continue_reverse macros
to make the code a bit nicer allowing to get rid of "last" marks.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 include/linux/netdevice.h   |  3 ++
 include/net/net_namespace.h |  3 +-
 net/core/dev.c              | 89 +++++++++++++++++++++++++------------
 3 files changed, 66 insertions(+), 29 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 9eda1c31d1f7..4f390eec106b 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2564,6 +2564,9 @@ extern rwlock_t				dev_base_lock;		/* Device list lock */
 		list_for_each_entry_safe(d, n, &(net)->dev_base_head, dev_list)
 #define for_each_netdev_continue(net, d)		\
 		list_for_each_entry_continue(d, &(net)->dev_base_head, dev_list)
+#define for_each_netdev_continue_reverse(net, d)		\
+		list_for_each_entry_continue_reverse(d, &(net)->dev_base_head, \
+						     dev_list)
 #define for_each_netdev_continue_rcu(net, d)		\
 	list_for_each_entry_continue_rcu(d, &(net)->dev_base_head, dev_list)
 #define for_each_netdev_in_bond_rcu(bond, slave)	\
diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
index f8712bbeb2e0..c5a98e03591d 100644
--- a/include/net/net_namespace.h
+++ b/include/net/net_namespace.h
@@ -317,7 +317,8 @@ static inline struct net *read_pnet(const possible_net_t *pnet)
 /* Protected by net_rwsem */
 #define for_each_net(VAR)				\
 	list_for_each_entry(VAR, &net_namespace_list, list)
-
+#define for_each_net_continue_reverse(VAR)		\
+	list_for_each_entry_continue_reverse(VAR, &net_namespace_list, list)
 #define for_each_net_rcu(VAR)				\
 	list_for_each_entry_rcu(VAR, &net_namespace_list, list)
 
diff --git a/net/core/dev.c b/net/core/dev.c
index bf3ed413abaf..6a87d0e71201 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1617,6 +1617,62 @@ static int call_netdevice_notifier(struct notifier_block *nb, unsigned long val,
 	return nb->notifier_call(nb, val, &info);
 }
 
+static int call_netdevice_register_notifiers(struct notifier_block *nb,
+					     struct net_device *dev)
+{
+	int err;
+
+	err = call_netdevice_notifier(nb, NETDEV_REGISTER, dev);
+	err = notifier_to_errno(err);
+	if (err)
+		return err;
+
+	if (!(dev->flags & IFF_UP))
+		return 0;
+
+	call_netdevice_notifier(nb, NETDEV_UP, dev);
+	return 0;
+}
+
+static void call_netdevice_unregister_notifiers(struct notifier_block *nb,
+						struct net_device *dev)
+{
+	if (dev->flags & IFF_UP) {
+		call_netdevice_notifier(nb, NETDEV_GOING_DOWN,
+					dev);
+		call_netdevice_notifier(nb, NETDEV_DOWN, dev);
+	}
+	call_netdevice_notifier(nb, NETDEV_UNREGISTER, dev);
+}
+
+static int call_netdevice_register_net_notifiers(struct notifier_block *nb,
+						 struct net *net)
+{
+	struct net_device *dev;
+	int err;
+
+	for_each_netdev(net, dev) {
+		err = call_netdevice_register_notifiers(nb, dev);
+		if (err)
+			goto rollback;
+	}
+	return 0;
+
+rollback:
+	for_each_netdev_continue_reverse(net, dev)
+		call_netdevice_unregister_notifiers(nb, dev);
+	return err;
+}
+
+static void call_netdevice_unregister_net_notifiers(struct notifier_block *nb,
+						    struct net *net)
+{
+	struct net_device *dev;
+
+	for_each_netdev(net, dev)
+		call_netdevice_unregister_notifiers(nb, dev);
+}
+
 static int dev_boot_phase = 1;
 
 /**
@@ -1635,8 +1691,6 @@ static int dev_boot_phase = 1;
 
 int register_netdevice_notifier(struct notifier_block *nb)
 {
-	struct net_device *dev;
-	struct net_device *last;
 	struct net *net;
 	int err;
 
@@ -1649,17 +1703,9 @@ int register_netdevice_notifier(struct notifier_block *nb)
 	if (dev_boot_phase)
 		goto unlock;
 	for_each_net(net) {
-		for_each_netdev(net, dev) {
-			err = call_netdevice_notifier(nb, NETDEV_REGISTER, dev);
-			err = notifier_to_errno(err);
-			if (err)
-				goto rollback;
-
-			if (!(dev->flags & IFF_UP))
-				continue;
-
-			call_netdevice_notifier(nb, NETDEV_UP, dev);
-		}
+		err = call_netdevice_register_net_notifiers(nb, net);
+		if (err)
+			goto rollback;
 	}
 
 unlock:
@@ -1668,22 +1714,9 @@ int register_netdevice_notifier(struct notifier_block *nb)
 	return err;
 
 rollback:
-	last = dev;
-	for_each_net(net) {
-		for_each_netdev(net, dev) {
-			if (dev == last)
-				goto outroll;
-
-			if (dev->flags & IFF_UP) {
-				call_netdevice_notifier(nb, NETDEV_GOING_DOWN,
-							dev);
-				call_netdevice_notifier(nb, NETDEV_DOWN, dev);
-			}
-			call_netdevice_notifier(nb, NETDEV_UNREGISTER, dev);
-		}
-	}
+	for_each_net_continue_reverse(net)
+		call_netdevice_unregister_net_notifiers(nb, net);
 
-outroll:
 	raw_notifier_chain_unregister(&netdev_chain, nb);
 	goto unlock;
 }
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [patch net-next 2/3] net: introduce per-netns netdevice notifiers
  2019-09-30  8:15 [patch net-next 0/3] net: introduce per-netns netdevice notifiers and use them in mlxsw Jiri Pirko
  2019-09-30  8:15 ` [patch net-next 1/3] net: push loops and nb calls into helper functions Jiri Pirko
@ 2019-09-30  8:15 ` Jiri Pirko
  2019-09-30 13:38   ` Andrew Lunn
  2019-10-02 15:47   ` David Miller
  2019-09-30  8:15 ` [patch net-next 3/3] mlxsw: spectrum: Use per-netns netdevice notifier registration Jiri Pirko
  2019-10-02 15:52 ` [patch net-next 0/3] net: introduce per-netns netdevice notifiers and use them in mlxsw David Miller
  3 siblings, 2 replies; 10+ messages in thread
From: Jiri Pirko @ 2019-09-30  8:15 UTC (permalink / raw)
  To: netdev
  Cc: davem, idosch, pabeni, edumazet, petrm, sd, f.fainelli, stephen, mlxsw

From: Jiri Pirko <jiri@mellanox.com>

Often the code for example in drivers is interested in getting notifier
call only from certain network namespace. In addition to the existing
global netdevice notifier chain introduce per-netns chains and allow
users to register to that. Eventually this would eliminate unnecessary
overhead in case there are many netdevices in many network namespaces.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 include/linux/netdevice.h   |  3 ++
 include/net/net_namespace.h |  3 ++
 net/core/dev.c              | 87 +++++++++++++++++++++++++++++++++++++
 3 files changed, 93 insertions(+)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 4f390eec106b..184f54f1b9e1 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2494,6 +2494,9 @@ const char *netdev_cmd_to_name(enum netdev_cmd cmd);
 
 int register_netdevice_notifier(struct notifier_block *nb);
 int unregister_netdevice_notifier(struct notifier_block *nb);
+int register_netdevice_notifier_net(struct net *net, struct notifier_block *nb);
+int unregister_netdevice_notifier_net(struct net *net,
+				      struct notifier_block *nb);
 
 struct netdev_notifier_info {
 	struct net_device	*dev;
diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
index c5a98e03591d..5ac2bb16d4b3 100644
--- a/include/net/net_namespace.h
+++ b/include/net/net_namespace.h
@@ -36,6 +36,7 @@
 #include <linux/ns_common.h>
 #include <linux/idr.h>
 #include <linux/skbuff.h>
+#include <linux/notifier.h>
 
 struct user_namespace;
 struct proc_dir_entry;
@@ -96,6 +97,8 @@ struct net {
 	struct list_head 	dev_base_head;
 	struct hlist_head 	*dev_name_head;
 	struct hlist_head	*dev_index_head;
+	struct raw_notifier_head	netdev_chain;
+
 	unsigned int		dev_base_seq;	/* protected by rtnl_mutex */
 	int			ifindex;
 	unsigned int		dev_unreg_count;
diff --git a/net/core/dev.c b/net/core/dev.c
index 6a87d0e71201..3302cefd3041 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1766,6 +1766,80 @@ int unregister_netdevice_notifier(struct notifier_block *nb)
 }
 EXPORT_SYMBOL(unregister_netdevice_notifier);
 
+/**
+ * register_netdevice_notifier_net - register a per-netns network notifier block
+ * @net: network namespace
+ * @nb: notifier
+ *
+ * Register a notifier to be called when network device events occur.
+ * The notifier passed is linked into the kernel structures and must
+ * not be reused until it has been unregistered. A negative errno code
+ * is returned on a failure.
+ *
+ * When registered all registration and up events are replayed
+ * to the new notifier to allow device to have a race free
+ * view of the network device list.
+ */
+
+int register_netdevice_notifier_net(struct net *net, struct notifier_block *nb)
+{
+	int err;
+
+	rtnl_lock();
+	err = raw_notifier_chain_register(&net->netdev_chain, nb);
+	if (err)
+		goto unlock;
+	if (dev_boot_phase)
+		goto unlock;
+
+	err = call_netdevice_register_net_notifiers(nb, net);
+	if (err)
+		goto chain_unregister;
+
+unlock:
+	rtnl_unlock();
+	return err;
+
+chain_unregister:
+	raw_notifier_chain_unregister(&netdev_chain, nb);
+	goto unlock;
+}
+EXPORT_SYMBOL(register_netdevice_notifier_net);
+
+/**
+ * unregister_netdevice_notifier_net - unregister a per-netns
+ *                                     network notifier block
+ * @net: network namespace
+ * @nb: notifier
+ *
+ * Unregister a notifier previously registered by
+ * register_netdevice_notifier(). The notifier is unlinked into the
+ * kernel structures and may then be reused. A negative errno code
+ * is returned on a failure.
+ *
+ * After unregistering unregister and down device events are synthesized
+ * for all devices on the device list to the removed notifier to remove
+ * the need for special case cleanup code.
+ */
+
+int unregister_netdevice_notifier_net(struct net *net,
+				      struct notifier_block *nb)
+{
+	int err;
+
+	rtnl_lock();
+	err = raw_notifier_chain_unregister(&net->netdev_chain, nb);
+	if (err)
+		goto unlock;
+
+	call_netdevice_unregister_net_notifiers(nb, net);
+
+unlock:
+	rtnl_unlock();
+	return err;
+}
+EXPORT_SYMBOL(unregister_netdevice_notifier_net);
+
 /**
  *	call_netdevice_notifiers_info - call all network notifier blocks
  *	@val: value passed unmodified to notifier function
@@ -1778,7 +1852,18 @@ EXPORT_SYMBOL(unregister_netdevice_notifier);
 static int call_netdevice_notifiers_info(unsigned long val,
 					 struct netdev_notifier_info *info)
 {
+	struct net *net = dev_net(info->dev);
+	int ret;
+
 	ASSERT_RTNL();
+
+	/* Run per-netns notifier block chain first, then run the global one.
+	 * Hopefully, one day, the global one is going to be removed after
+	 * all notifier block registrators get converted to be per-netns.
+	 */
+	ret = raw_notifier_call_chain(&net->netdev_chain, val, info);
+	if (ret & NOTIFY_STOP_MASK)
+		return ret;
 	return raw_notifier_call_chain(&netdev_chain, val, info);
 }
 
@@ -9668,6 +9753,8 @@ static int __net_init netdev_init(struct net *net)
 	if (net->dev_index_head == NULL)
 		goto err_idx;
 
+	RAW_INIT_NOTIFIER_HEAD(&net->netdev_chain);
+
 	return 0;
 
 err_idx:
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [patch net-next 3/3] mlxsw: spectrum: Use per-netns netdevice notifier registration
  2019-09-30  8:15 [patch net-next 0/3] net: introduce per-netns netdevice notifiers and use them in mlxsw Jiri Pirko
  2019-09-30  8:15 ` [patch net-next 1/3] net: push loops and nb calls into helper functions Jiri Pirko
  2019-09-30  8:15 ` [patch net-next 2/3] net: introduce per-netns netdevice notifiers Jiri Pirko
@ 2019-09-30  8:15 ` Jiri Pirko
  2019-10-02 15:52 ` [patch net-next 0/3] net: introduce per-netns netdevice notifiers and use them in mlxsw David Miller
  3 siblings, 0 replies; 10+ messages in thread
From: Jiri Pirko @ 2019-09-30  8:15 UTC (permalink / raw)
  To: netdev
  Cc: davem, idosch, pabeni, edumazet, petrm, sd, f.fainelli, stephen, mlxsw

From: Jiri Pirko <jiri@mellanox.com>

The mlxsw_sp instance is not interested in events happening in other
network namespaces. So use "_net" variants for netdevice notifier
registration/unregistration and get only events which are happening in
the net the instance is in.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlxsw/spectrum.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
index dcf9562bce8a..a54a0dc82ff2 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
@@ -4864,7 +4864,8 @@ static int mlxsw_sp_init(struct mlxsw_core *mlxsw_core,
 	 * respin.
 	 */
 	mlxsw_sp->netdevice_nb.notifier_call = mlxsw_sp_netdevice_event;
-	err = register_netdevice_notifier(&mlxsw_sp->netdevice_nb);
+	err = register_netdevice_notifier_net(&init_net,
+					      &mlxsw_sp->netdevice_nb);
 	if (err) {
 		dev_err(mlxsw_sp->bus_info->dev, "Failed to register netdev notifier\n");
 		goto err_netdev_notifier;
@@ -4887,7 +4888,8 @@ static int mlxsw_sp_init(struct mlxsw_core *mlxsw_core,
 err_ports_create:
 	mlxsw_sp_dpipe_fini(mlxsw_sp);
 err_dpipe_init:
-	unregister_netdevice_notifier(&mlxsw_sp->netdevice_nb);
+	unregister_netdevice_notifier_net(&init_net,
+					  &mlxsw_sp->netdevice_nb);
 err_netdev_notifier:
 	if (mlxsw_sp->clock)
 		mlxsw_sp->ptp_ops->fini(mlxsw_sp->ptp_state);
@@ -4973,7 +4975,8 @@ static void mlxsw_sp_fini(struct mlxsw_core *mlxsw_core)
 
 	mlxsw_sp_ports_remove(mlxsw_sp);
 	mlxsw_sp_dpipe_fini(mlxsw_sp);
-	unregister_netdevice_notifier(&mlxsw_sp->netdevice_nb);
+	unregister_netdevice_notifier_net(&init_net,
+					  &mlxsw_sp->netdevice_nb);
 	if (mlxsw_sp->clock) {
 		mlxsw_sp->ptp_ops->fini(mlxsw_sp->ptp_state);
 		mlxsw_sp->ptp_ops->clock_fini(mlxsw_sp->clock);
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [patch net-next 2/3] net: introduce per-netns netdevice notifiers
  2019-09-30  8:15 ` [patch net-next 2/3] net: introduce per-netns netdevice notifiers Jiri Pirko
@ 2019-09-30 13:38   ` Andrew Lunn
  2019-09-30 14:23     ` Jiri Pirko
  2019-10-02 15:47   ` David Miller
  1 sibling, 1 reply; 10+ messages in thread
From: Andrew Lunn @ 2019-09-30 13:38 UTC (permalink / raw)
  To: Jiri Pirko
  Cc: netdev, davem, idosch, pabeni, edumazet, petrm, sd, f.fainelli,
	stephen, mlxsw

>  static int call_netdevice_notifiers_info(unsigned long val,
>  					 struct netdev_notifier_info *info)
>  {
> +	struct net *net = dev_net(info->dev);
> +	int ret;
> +
>  	ASSERT_RTNL();
> +
> +	/* Run per-netns notifier block chain first, then run the global one.
> +	 * Hopefully, one day, the global one is going to be removed after
> +	 * all notifier block registrators get converted to be per-netns.
> +	 */

Hi Jiri

Is that really going to happen? register_netdevice_notifier() is used
in 130 files. Do you plan to spend the time to make it happen?

> +	ret = raw_notifier_call_chain(&net->netdev_chain, val, info);
> +	if (ret & NOTIFY_STOP_MASK)
> +		return ret;
>  	return raw_notifier_call_chain(&netdev_chain, val, info);
>  }

Humm. I wonder about NOTIFY_STOP_MASK here. These are two separate
chains. Should one chain be able to stop the other chain? Are there
other examples where NOTIFY_STOP_MASK crosses a chain boundary?

      Andrew

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [patch net-next 2/3] net: introduce per-netns netdevice notifiers
  2019-09-30 13:38   ` Andrew Lunn
@ 2019-09-30 14:23     ` Jiri Pirko
  2019-09-30 15:33       ` Andrew Lunn
  0 siblings, 1 reply; 10+ messages in thread
From: Jiri Pirko @ 2019-09-30 14:23 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: netdev, davem, idosch, pabeni, edumazet, petrm, sd, f.fainelli,
	stephen, mlxsw

Mon, Sep 30, 2019 at 03:38:24PM CEST, andrew@lunn.ch wrote:
>>  static int call_netdevice_notifiers_info(unsigned long val,
>>  					 struct netdev_notifier_info *info)
>>  {
>> +	struct net *net = dev_net(info->dev);
>> +	int ret;
>> +
>>  	ASSERT_RTNL();
>> +
>> +	/* Run per-netns notifier block chain first, then run the global one.
>> +	 * Hopefully, one day, the global one is going to be removed after
>> +	 * all notifier block registrators get converted to be per-netns.
>> +	 */
>
>Hi Jiri
>
>Is that really going to happen? register_netdevice_notifier() is used
>in 130 files. Do you plan to spend the time to make it happen?

That's why I prepended the sentency with "Hopefully, one day"...


>
>> +	ret = raw_notifier_call_chain(&net->netdev_chain, val, info);
>> +	if (ret & NOTIFY_STOP_MASK)
>> +		return ret;
>>  	return raw_notifier_call_chain(&netdev_chain, val, info);
>>  }
>
>Humm. I wonder about NOTIFY_STOP_MASK here. These are two separate
>chains. Should one chain be able to stop the other chain? Are there

Well if the failing item would be in the second chain, at the beginning
of it, it would be stopped too. Does not matter where the stop happens,
the point is that the whole processing stops. That is why I added the
check here.


>other examples where NOTIFY_STOP_MASK crosses a chain boundary?

Not aware of it, no. Could you please describe what is wrong?


>
>      Andrew

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [patch net-next 2/3] net: introduce per-netns netdevice notifiers
  2019-09-30 14:23     ` Jiri Pirko
@ 2019-09-30 15:33       ` Andrew Lunn
  2019-09-30 18:01         ` Jiri Pirko
  0 siblings, 1 reply; 10+ messages in thread
From: Andrew Lunn @ 2019-09-30 15:33 UTC (permalink / raw)
  To: Jiri Pirko
  Cc: netdev, davem, idosch, pabeni, edumazet, petrm, sd, f.fainelli,
	stephen, mlxsw

On Mon, Sep 30, 2019 at 04:23:49PM +0200, Jiri Pirko wrote:
> Mon, Sep 30, 2019 at 03:38:24PM CEST, andrew@lunn.ch wrote:
> >>  static int call_netdevice_notifiers_info(unsigned long val,
> >>  					 struct netdev_notifier_info *info)
> >>  {
> >> +	struct net *net = dev_net(info->dev);
> >> +	int ret;
> >> +
> >>  	ASSERT_RTNL();
> >> +
> >> +	/* Run per-netns notifier block chain first, then run the global one.
> >> +	 * Hopefully, one day, the global one is going to be removed after
> >> +	 * all notifier block registrators get converted to be per-netns.
> >> +	 */
> >
> >Hi Jiri
> >
> >Is that really going to happen? register_netdevice_notifier() is used
> >in 130 files. Do you plan to spend the time to make it happen?
> 
> That's why I prepended the sentency with "Hopefully, one day"...
> 
> 
> >
> >> +	ret = raw_notifier_call_chain(&net->netdev_chain, val, info);
> >> +	if (ret & NOTIFY_STOP_MASK)
> >> +		return ret;
> >>  	return raw_notifier_call_chain(&netdev_chain, val, info);
> >>  }
> >
> >Humm. I wonder about NOTIFY_STOP_MASK here. These are two separate
> >chains. Should one chain be able to stop the other chain? Are there
> 
> Well if the failing item would be in the second chain, at the beginning
> of it, it would be stopped too. Does not matter where the stop happens,
> the point is that the whole processing stops. That is why I added the
> check here.
> 
> 
> >other examples where NOTIFY_STOP_MASK crosses a chain boundary?
> 
> Not aware of it, no. Could you please describe what is wrong?

You are expanding the meaning of NOTIFY_STOP_MASK. It now can stop
some other chain. If this was one chain with a filter, i would not be
asking. But this is two different chains, and one chain can stop
another? At minimum, i think this needs to be reviewed by the core
kernel people.

But i'm also wondering if you are solving the problem at the wrong
level. Are there other notifier chains which would benefit from
respecting name space boundaries? Would a better solution be to extend
struct notifier_block with some sort of filter?

Do you have some performance numbers? Where are you getting your
performance gains from? By the fact you are doing NOTIFY_STOP_MASK
earlier, so preventing a long chain being walked? I notice
notifer_block has a priority field. Did you try using that to put your
notified earlier on the chain?

	 Andrew

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [patch net-next 2/3] net: introduce per-netns netdevice notifiers
  2019-09-30 15:33       ` Andrew Lunn
@ 2019-09-30 18:01         ` Jiri Pirko
  0 siblings, 0 replies; 10+ messages in thread
From: Jiri Pirko @ 2019-09-30 18:01 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: netdev, davem, idosch, pabeni, edumazet, petrm, sd, f.fainelli,
	stephen, mlxsw

Mon, Sep 30, 2019 at 05:33:43PM CEST, andrew@lunn.ch wrote:
>On Mon, Sep 30, 2019 at 04:23:49PM +0200, Jiri Pirko wrote:
>> Mon, Sep 30, 2019 at 03:38:24PM CEST, andrew@lunn.ch wrote:
>> >>  static int call_netdevice_notifiers_info(unsigned long val,
>> >>  					 struct netdev_notifier_info *info)
>> >>  {
>> >> +	struct net *net = dev_net(info->dev);
>> >> +	int ret;
>> >> +
>> >>  	ASSERT_RTNL();
>> >> +
>> >> +	/* Run per-netns notifier block chain first, then run the global one.
>> >> +	 * Hopefully, one day, the global one is going to be removed after
>> >> +	 * all notifier block registrators get converted to be per-netns.
>> >> +	 */
>> >
>> >Hi Jiri
>> >
>> >Is that really going to happen? register_netdevice_notifier() is used
>> >in 130 files. Do you plan to spend the time to make it happen?
>> 
>> That's why I prepended the sentency with "Hopefully, one day"...
>> 
>> 
>> >
>> >> +	ret = raw_notifier_call_chain(&net->netdev_chain, val, info);
>> >> +	if (ret & NOTIFY_STOP_MASK)
>> >> +		return ret;
>> >>  	return raw_notifier_call_chain(&netdev_chain, val, info);
>> >>  }
>> >
>> >Humm. I wonder about NOTIFY_STOP_MASK here. These are two separate
>> >chains. Should one chain be able to stop the other chain? Are there
>> 
>> Well if the failing item would be in the second chain, at the beginning
>> of it, it would be stopped too. Does not matter where the stop happens,
>> the point is that the whole processing stops. That is why I added the
>> check here.
>> 
>> 
>> >other examples where NOTIFY_STOP_MASK crosses a chain boundary?
>> 
>> Not aware of it, no. Could you please describe what is wrong?
>
>You are expanding the meaning of NOTIFY_STOP_MASK. It now can stop
>some other chain. If this was one chain with a filter, i would not be

Well, it was originally a single chain, so the semantics stays intact.
Again, it is not some other independent chain. It's just netns one and
general one, both serve the same purpose.


>asking. But this is two different chains, and one chain can stop
>another? At minimum, i think this needs to be reviewed by the core
>kernel people.
>
>But i'm also wondering if you are solving the problem at the wrong
>level. Are there other notifier chains which would benefit from
>respecting name space boundaries? Would a better solution be to extend
>struct notifier_block with some sort of filter?

I mentioned my primary motivation in the cover letter. What I want to
avoid is need of taking &pernet_ops_rwsem during registration of tne
notifier and avoid deadlock in my usecase.

Plus it seems very clear that if a notifier knows what netns is he
interested in, he just registers in that particular netns chain.
Having one fat generic chain with filters is basically what we have
right now.


>
>Do you have some performance numbers? Where are you getting your
>performance gains from? By the fact you are doing NOTIFY_STOP_MASK
>earlier, so preventing a long chain being walked? I notice
>notifer_block has a priority field. Did you try using that to put your
>notified earlier on the chain?

It is not about stopping the chain earlier, not at all. It is the fact
that with many netdevices in many network namespaces you gat a lot of
wasted calls to notifiers registators that does not care.



>
>	 Andrew

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [patch net-next 2/3] net: introduce per-netns netdevice notifiers
  2019-09-30  8:15 ` [patch net-next 2/3] net: introduce per-netns netdevice notifiers Jiri Pirko
  2019-09-30 13:38   ` Andrew Lunn
@ 2019-10-02 15:47   ` David Miller
  1 sibling, 0 replies; 10+ messages in thread
From: David Miller @ 2019-10-02 15:47 UTC (permalink / raw)
  To: jiri
  Cc: netdev, idosch, pabeni, edumazet, petrm, sd, f.fainelli, stephen,
	mlxsw, andrew

From: Jiri Pirko <jiri@resnulli.us>
Date: Mon, 30 Sep 2019 10:15:10 +0200

> From: Jiri Pirko <jiri@mellanox.com>
> 
> Often the code for example in drivers is interested in getting notifier
> call only from certain network namespace. In addition to the existing
> global netdevice notifier chain introduce per-netns chains and allow
> users to register to that. Eventually this would eliminate unnecessary
> overhead in case there are many netdevices in many network namespaces.
> 
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>

Ok, so there was a discussion about stop semantics.

Honestly, I think that's fine.

Stop means the operation cannot be performed and whoever is firing off
the notifier will have to fail and undo the config change being
attempted.

In that context, it doesn't matter who or where in the chain we
trigger the stop.

Given all of that I am pretty sure this change is fine and I will
add it to net-next.  We can fix any actual semantic problems this
might introduce as a follow-on.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [patch net-next 0/3] net: introduce per-netns netdevice notifiers and use them in mlxsw
  2019-09-30  8:15 [patch net-next 0/3] net: introduce per-netns netdevice notifiers and use them in mlxsw Jiri Pirko
                   ` (2 preceding siblings ...)
  2019-09-30  8:15 ` [patch net-next 3/3] mlxsw: spectrum: Use per-netns netdevice notifier registration Jiri Pirko
@ 2019-10-02 15:52 ` David Miller
  3 siblings, 0 replies; 10+ messages in thread
From: David Miller @ 2019-10-02 15:52 UTC (permalink / raw)
  To: jiri
  Cc: netdev, idosch, pabeni, edumazet, petrm, sd, f.fainelli, stephen, mlxsw

From: Jiri Pirko <jiri@resnulli.us>
Date: Mon, 30 Sep 2019 10:15:08 +0200

> From: Jiri Pirko <jiri@mellanox.com>
> 
> Some drivers, like mlxsw, are not interested in notifications coming in
> for netdevices from other network namespaces. So introduce per-netns
> notifiers and allow to reduce overhead by listening only for
> notifications from the same netns.
> 
> This is also a preparation for upcoming patchset "devlink: allow devlink
> instances to change network namespace". This resolves deadlock during
> reload mlxsw into initial netns made possible by
> 328fbe747ad4 ("net: Close race between {un, }register_netdevice_notifier() and setup_net()/cleanup_net()").

Series applied.

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2019-10-02 15:52 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-30  8:15 [patch net-next 0/3] net: introduce per-netns netdevice notifiers and use them in mlxsw Jiri Pirko
2019-09-30  8:15 ` [patch net-next 1/3] net: push loops and nb calls into helper functions Jiri Pirko
2019-09-30  8:15 ` [patch net-next 2/3] net: introduce per-netns netdevice notifiers Jiri Pirko
2019-09-30 13:38   ` Andrew Lunn
2019-09-30 14:23     ` Jiri Pirko
2019-09-30 15:33       ` Andrew Lunn
2019-09-30 18:01         ` Jiri Pirko
2019-10-02 15:47   ` David Miller
2019-09-30  8:15 ` [patch net-next 3/3] mlxsw: spectrum: Use per-netns netdevice notifier registration Jiri Pirko
2019-10-02 15:52 ` [patch net-next 0/3] net: introduce per-netns netdevice notifiers and use them in mlxsw David Miller

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).