Re: [PATCH net-next v3 8/8] geneve: break dependency to network drivers

[Hannes Frederic Sowa <hannes@stressinduktion.org>]

Hi Jesse,

On 07.01.2016 01:18, Jesse Gross wrote:
> On Wed, Jan 6, 2016 at 3:39 PM, Hannes Frederic Sowa
> <hannes@stressinduktion.org> wrote:
>> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
>> ---
>>   drivers/net/geneve.c | 30 +++++++++++++++++++++++++++---
>>   include/net/geneve.h |  7 +++----
>>   2 files changed, 30 insertions(+), 7 deletions(-)
>
> Thanks a lot for going through all the drivers! You found more things
> than I thought. I noticed just a couple new things with this version
> but overall I think it is a good direction.
>
>> diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
>> index 24b077a32c1c9c..5b8d728b1204be 100644
>> --- a/drivers/net/geneve.c
>> +++ b/drivers/net/geneve.c
>> @@ -1110,7 +1110,7 @@ static struct device_type geneve_type = {
>>    * supply the listening GENEVE udp ports. Callers are expected
>>    * to implement the ndo_add_geneve_port.
>>    */
>> -void geneve_get_rx_port(struct net_device *dev)
>> +static void geneve_notify_refresh_netdev(struct net_device *dev)
>>   {
>>          struct net *net = dev_net(dev);
>>          struct geneve_net *gn = net_generic(net, geneve_net_id);
>
> Both this function and the corresponding one in VXLAN assume that the
> add port NDO is implemented and blindly dereference the pointer. Now
> that all protocols get refreshed at the same time, we should check
> first. There's also a comment above the function that says this, which
> we can remove now.

Absolutely, will fix this in the next version. With the separate 
offloading types this wouldn't be possible. Thanks for seeing this!

I actually had this in a follow-up patch because I want to call this on 
NETDEV_REGISTER, too, so we don't need to add the geneve/vxlan functions 
to the drivers in ndo_start anymore.

> I also think we can update the comments in netdevice.h for the
> VXLAN/Geneve add/remove NDOs to say that they are protected by RTNL.

Will do. Thanks!

>> diff --git a/include/net/geneve.h b/include/net/geneve.h
>> index e6c23dc765f7ec..7d52077b72faa3 100644
>> --- a/include/net/geneve.h
>> +++ b/include/net/geneve.h
>> -#if IS_ENABLED(CONFIG_GENEVE)
>> -void geneve_get_rx_port(struct net_device *netdev);
>> -#else
>>   static inline void geneve_get_rx_port(struct net_device *netdev)
>>   {
>> +       call_netdevice_notifiers(NETDEV_REFRESH_OFFLOADS, netdev);
>>   }
>> -#endif
>
> I think we should merge vxlan_get_rx_port() and geneve_get_rx_port()
> into a single generic function. For drivers that support both, they
> now have two calls to get notified for all offloaded ports. This
> actually can cause problems related to duplicates, similar to what you
> feared before.

Agreed. I add comments to the offloading functions in netdevice.h so 
they need to be implemented in a idempotent way and review the drivers 
how they deal with it.

Thanks for the review,
Hannes