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

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

On 08.01.2016 22:12, Jesse Gross wrote:
> On Fri, Jan 8, 2016 at 12:47 PM, Hannes Frederic Sowa
> <hannes@stressinduktion.org> wrote:
>> On 07.01.2016 01:18, Jesse Gross wrote:
>>>
>>> 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.
>>
>>
>> Checking out the ndo_add_vxlan_port callbacks, we cannot change how we
>> refresh the ports unfortunately. For some drivers it might be easy to
>> update, some look a little bit more complicated.
>>
>> I guess we should try to keep more complexity in the core and make sure we
>> only call those functions if necessary for the particular type of offload
>> than to replicate the logic in each driver.
>>
>> If you agree I prepare this series with the changes to have separate
>> callbacks, address your other feedback and send it out.
>
> I'm not sure that I totally understand, do you have an example of a
> problematic driver?

bnx2x e.g. does refcounting on the port. So when it will implement 
geneve offloading, it will refcount the vxlan port multiple times with 
the current schema. Currently this is safe, as bnx2x doesn't care about 
geneve and thus no further callbacks will happen. But I fear drivers 
being broken as soon as they implement geneve additionally with the 
current schema.

> I think what I was proposing is fairly simple, so maybe I wasn't
> totally clear. Here's what I think we can do:
>
> In your most recent series, vxlan_get_rx_port() and
> geneve_get_rx_port() have the same implementation. I think we can
> merge them into a single function (i.e. udp_tunnel_get_rx_port()). For
> all of the drivers that only implement VXLAN offloads, this is just a
> 1:1 switch so there should be no functionality change. There's only
> one driver than currently implements both (i40e) and it calls both
> vxlan_get_rx_port() and geneve_get_rx_port() back to back. We can just
> collapse this into a single call and since that will trigger both
> offloads to refresh, the behavior should again be the same as we have.

I totally understood that. I am just fearing future additions of offload 
while not updating the current vxlan offloading then as well.

My current series ATM implements just what you proposed.

> I do know that there are so drivers that try to do clever things as
> far as refcounting, etc. However, since the callbacks are the same as
> we did before, it shouldn't introduce any issues.

Agreed. I can push the series out now as you proposed and we have to 
intercept all upcoming driver changes in regard to geneve offloading and 
review them. :)

Bye,
Hannes