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

On 08.01.2016 22:12, Jesse Gross wrote:

On Fri, Jan 8, 2016 at 12:47 PM, Hannes Frederic Sowa
[off-list ref] wrote:

quoted

On 07.01.2016 01:18, Jesse Gross wrote:

quoted

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