[net-next,v2] gtp: add notification mechanism

Like all other network functions, let's notify gtp context on creation and
deletion.

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Tested-by: Gabriel Ganne <gabriel.ganne@6wind.com>
---

v1 -> v2:
 - fix typo in the commit title
 - fix indentation of GTP_GENL_MCGRP

 drivers/net/gtp.c        | 58 +++++++++++++++++++++++++++++++++-------
 include/uapi/linux/gtp.h |  2 ++
 2 files changed, 51 insertions(+), 9 deletions(-)

Hi Nicolas,

thanks a lot for your patch.

On Tue, Aug 25, 2020 at 05:57:15PM +0200, Nicolas Dichtel wrote:
> Like all other network functions, let's notify gtp context on creation and
> deletion.

While this may be in-line with typical kernel tunnel device practises, I am not
convinced it is the right way to go for GTP.

Contrary to other tunneling mechansims, GTP doesn't have a 1:1 rlationship between
tunnels and netdev's.  You can easily have tens of thousands - or even many more -
PDP contexts (at least one per subscriber) within one "gtp0" netdev.  Also, the state
is highly volatile.  Every time a subscriber registers/deregisters, goes in or out of
coverage, in or out of airplane mode, etc. those PDP contexts go up and down.

Sending (unsolicited) notifications about all of those seems quite heavyweight to me.

Le 25/08/2020 à 19:01, Harald Welte a écrit :
> Hi Nicolas,
> 
> thanks a lot for your patch.
> 
> On Tue, Aug 25, 2020 at 05:57:15PM +0200, Nicolas Dichtel wrote:
>> Like all other network functions, let's notify gtp context on creation and
>> deletion.
> 
> While this may be in-line with typical kernel tunnel device practises, I am not
> convinced it is the right way to go for GTP.
> 
> Contrary to other tunneling mechansims, GTP doesn't have a 1:1 rlationship between
> tunnels and netdev's.  You can easily have tens of thousands - or even many more -
> PDP contexts (at least one per subscriber) within one "gtp0" netdev.  Also, the state
> is highly volatile.  Every time a subscriber registers/deregisters, goes in or out of
> coverage, in or out of airplane mode, etc. those PDP contexts go up and down.
> 
> Sending (unsolicited) notifications about all of those seems quite heavyweight to me.
There is no 'unsolicited' notifications with this patch. Notifications are sent
only if a userspace application has subscribed to the gtp mcast group.
ip routes or conntrack entries are notified in the same way and there could a
lot of them also (more than 100k conntrack entries for example).

Hi Nicolas,

On Wed, Aug 26, 2020 at 09:47:54AM +0200, Nicolas Dichtel wrote:
> > Sending (unsolicited) notifications about all of those seems quite heavyweight to me.
>
> There is no 'unsolicited' notifications with this patch. Notifications are sent
> only if a userspace application has subscribed to the gtp mcast group.
> ip routes or conntrack entries are notified in the same way and there could a
> lot of them also (more than 100k conntrack entries for example).

Ok, thanks for reminding me of that.  However, even if those events are
not sent/multicasted, it still looks like the proposed patch is
unconditionally allocating a netlink message and filling it with
information about the PDP.  That alone looks like adding significant
overhead to every user - even the majority of current use cases where
nobody is listening/subscribing to that multicast group.

Wouldn't it make sense to only allocate + fill those messages if we
actually knew a subscriber existed?

Le 26/08/2020 à 20:52, Harald Welte a écrit :
> Hi Nicolas,
> 
> On Wed, Aug 26, 2020 at 09:47:54AM +0200, Nicolas Dichtel wrote:
>>> Sending (unsolicited) notifications about all of those seems quite heavyweight to me.
>>
>> There is no 'unsolicited' notifications with this patch. Notifications are sent
>> only if a userspace application has subscribed to the gtp mcast group.
>> ip routes or conntrack entries are notified in the same way and there could a
>> lot of them also (more than 100k conntrack entries for example).
> 
> Ok, thanks for reminding me of that.  However, even if those events are
> not sent/multicasted, it still looks like the proposed patch is
> unconditionally allocating a netlink message and filling it with
> information about the PDP.  That alone looks like adding significant
> overhead to every user - even the majority of current use cases where
> nobody is listening/subscribing to that multicast group.
I don't think that this is a significant overhead. This is added in the control
path. When a PDP context is added, the rtnl lock is took, this is another
magnitude of overhead than a kmalloc().

> 
> Wouldn't it make sense to only allocate + fill those messages if we
> actually knew a subscriber existed?
In fact, this is actually how the netlink framework works.

Hi Nicolas,

On Thu, Aug 27, 2020 at 12:36:24AM +0200, Nicolas Dichtel wrote:
> Le 26/08/2020 à 20:52, Harald Welte a écrit :

> > Wouldn't it make sense to only allocate + fill those messages if we
> > actually knew a subscriber existed?
>
> In fact, this is actually how the netlink framework works.

Well, as you can tell from my responses, I've not been doing kernel work
for a decade now, so I'm looking at things from a more distant and
ignorant perspective.  To me it seems odd to allocate memory and copy
data to it (cache misses, ...) if nobody every requested that data, and
nobody will ever use it.  But if this is how it is supposed to work,
then I will of course defer to that.  All netlink would have to expose
is a function that returns whether or not there are any subscribers
to the given multicast group.  Then all of the allocation +
initialization would disappear in a branch that is not executed most of
the time, at least for current, existing gtpnl systems.  Yes, that means
one more branch, of course.  But that branch will happen later on
anyway, event today: Only after the allocation + initialization.

So having said the above, if this is how it is supposed to work with
netlink:

Acked-by: Harald Welte <laforge@gnumonks.org>

Hi Harald,

Le 27/08/2020 à 11:00, Harald Welte a écrit :
> Hi Nicolas,
> 
> On Thu, Aug 27, 2020 at 12:36:24AM +0200, Nicolas Dichtel wrote:
>> Le 26/08/2020 à 20:52, Harald Welte a écrit :
> 
>>> Wouldn't it make sense to only allocate + fill those messages if we
>>> actually knew a subscriber existed?
>>
>> In fact, this is actually how the netlink framework works.
> 
> Well, as you can tell from my responses, I've not been doing kernel work
> for a decade now, so I'm looking at things from a more distant and
> ignorant perspective.  To me it seems odd to allocate memory and copy
> data to it (cache misses, ...) if nobody every requested that data, and
> nobody will ever use it.  But if this is how it is supposed to work,
> then I will of course defer to that.  All netlink would have to expose
> is a function that returns whether or not there are any subscribers
> to the given multicast group.  Then all of the allocation +
> initialization would disappear in a branch that is not executed most of
> the time, at least for current, existing gtpnl systems.  Yes, that means
> one more branch, of course.  But that branch will happen later on
> anyway, event today: Only after the allocation + initialization.
I agree, but I didn't find a good solution for this right now. The lookup is not
straight forward.

> 
> So having said the above, if this is how it is supposed to work with
> netlink:
> 
> Acked-by: Harald Welte <laforge@gnumonks.org>
> 
Thank you.