Re: [patch net-next 2/3] net: introduce per-netns netdevice notifiers

[Jiri Pirko <jiri@resnulli.us>]

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