Mon, Sep 30, 2019 at 05:33:43PM CEST, and...@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, and...@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
