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:

quoted

Mon, Sep 30, 2019 at 03:38:24PM CEST, andrew@lunn.ch wrote:

quoted

quoted

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

quoted

quoted

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

quoted

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