Per previous discussion we don't want to generalize this current
offload interface. Can we just NETDEV_UP as the notifier?
The problem with only using NETDEV_UP/REGISTER is that some drivers need
to
reconfigure their offloads during operation while keeping the netdevice
in
UP state. One example is ixgbe. This was my first idea also.
It's configuration. Just get it once and save the port number(s) in
the driver. Configure the device only when
IXGBE_FLAG_VXLAN_OFFLOAD_CAPABLE is set.
I don't see much value that each driver has to hold a database of ports to
offload, the kernel knows much better and can easily be queried via this
mechanism.
This is not only about ixgbe, but also about benet, which f.e. needs the
list of offloads available again during resume. Of course, drivers can do
their own stateful handling of ports, but this seems to much more generate
problems. I rather would like to see more logic moved into the core and
less
quoted
code in the drivers.
All the drivers that support VXLAN already save the ports in a stateful
fashion. Some support a single port, some and array, some a list. The only
question is whether they bother to save the information when the offload
feature is disabled. ixgbe does not for instance, but it looks like fm10k
does since it doesn't check any feature flags. Fixing those that don't save
the information all the times should be straightforward.
Yes, would probably be straight forward to add another list plus extra
reference counting and maybe a lock, but I don't see why we need to
duplicate the policy in the drivers? I personally think ixgbe does ok in
this regard.
Anyway, this series merely tries to decouple 10gigbit cards from vxlan
and/or geneve. It will be no problem to deal with this later on.
Bye,
Hannes
Per previous discussion we don't want to generalize this current
offload interface. Can we just NETDEV_UP as the notifier?
The problem with only using NETDEV_UP/REGISTER is that some drivers
need
to
reconfigure their offloads during operation while keeping the netdevice
in
UP state. One example is ixgbe. This was my first idea also.
It's configuration. Just get it once and save the port number(s) in
the driver. Configure the device only when
IXGBE_FLAG_VXLAN_OFFLOAD_CAPABLE is set.
I don't see much value that each driver has to hold a database of ports
to
offload, the kernel knows much better and can easily be queried via this
mechanism.
This is not only about ixgbe, but also about benet, which f.e. needs the
list of offloads available again during resume. Of course, drivers can do
their own stateful handling of ports, but this seems to much more
generate
problems. I rather would like to see more logic moved into the core and
less
quoted
code in the drivers.
All the drivers that support VXLAN already save the ports in a stateful
fashion. Some support a single port, some and array, some a list. The only
question is whether they bother to save the information when the offload
feature is disabled. ixgbe does not for instance, but it looks like fm10k
does since it doesn't check any feature flags. Fixing those that don't
save
the information all the times should be straightforward.
Yes, would probably be straight forward to add another list plus extra
reference counting and maybe a lock, but I don't see why we need to
duplicate the policy in the drivers? I personally think ixgbe does ok in
this regard.
Again, to reiterate, _all_ the drivers that support VXLAN already save
the ports in a stateful fashion. Adding a new notifier is unnecessary
and makes things more complicated, not less. Also to reiterate, this
is not generic functionality that the driver is providing; we should
not need to add anything for this into the core APIs outside of the
two ndo functions that already exist.
Tom
Anyway, this series merely tries to decouple 10gigbit cards from vxlan
and/or geneve. It will be no problem to deal with this later on.
Bye,
Hannes
Per previous discussion we don't want to generalize this current
offload interface. Can we just NETDEV_UP as the notifier?
The problem with only using NETDEV_UP/REGISTER is that some drivers
need
to
reconfigure their offloads during operation while keeping the netdevice
in
UP state. One example is ixgbe. This was my first idea also.
It's configuration. Just get it once and save the port number(s) in
the driver. Configure the device only when
IXGBE_FLAG_VXLAN_OFFLOAD_CAPABLE is set.
I don't see much value that each driver has to hold a database of ports
to
offload, the kernel knows much better and can easily be queried via this
mechanism.
This is not only about ixgbe, but also about benet, which f.e. needs the
list of offloads available again during resume. Of course, drivers can do
their own stateful handling of ports, but this seems to much more
generate
problems. I rather would like to see more logic moved into the core and
less
quoted
code in the drivers.
All the drivers that support VXLAN already save the ports in a stateful
fashion. Some support a single port, some and array, some a list. The only
question is whether they bother to save the information when the offload
feature is disabled. ixgbe does not for instance, but it looks like fm10k
does since it doesn't check any feature flags. Fixing those that don't
save
the information all the times should be straightforward.
Yes, would probably be straight forward to add another list plus extra
reference counting and maybe a lock, but I don't see why we need to
duplicate the policy in the drivers? I personally think ixgbe does ok in
this regard.
Again, to reiterate, _all_ the drivers that support VXLAN already save
the ports in a stateful fashion. Adding a new notifier is unnecessary
and makes things more complicated, not less. Also to reiterate, this
is not generic functionality that the driver is providing; we should
not need to add anything for this into the core APIs outside of the
two ndo functions that already exist.
I consider the offloading interface work in progress:
I agree this interface can be much improved. But this series focuses on
firstly removing the dependency on the modules without changing too much
otherwise. No new callback is added just two ones are migrated into one.
Currently drivers don't handle a list of all ports, they discard new
ones as soon as they don't fit into the hardware anymore (mostly only
one). So currently the state is not completely replicated in the
drivers. We could call the ndo_add_*_ports again after we deleted some
ports and try to refresh from the core kernel side. I think instead of
adding this logic to all drivers to fix this, it is easier to callback
into the kernel instead of adding this logic to almost all network drivers.
Lot's of drivers refresh the ports list by calling ndo_open function
again because of error recovery or resuming and thus get a new port list
(here some drivers actually currently increment the refcounter for
already installed ports by mistake and I am looking into how to fix
this, probably by throwing away the list of currently installed port
numbers first and then refreshing all ports). It is even worse: it seems
drivers actually increment the port refcounter by mistake when you
change ring settings (or other settings causing a device reset) after
installing the vxlan offload.
I agree it would be great to get away with the callback completely but I
haven't yet fully figured out how error handling should be done then. It
seems also not useful to put the error handling in the ndo_add_*_port
functions as most simply configure the structs and call a worker
afterwards to process them. So the correct propagation of errors can
also be tricky.
I just don't want to mess with the state in the current drivers but as a
first step remove the dependency.
The reason why I want to keep the callback is that without completely
understanding the rest of problems I don't know if we need it or not.
Thoughts?
Thanks,
Hannes
Per previous discussion we don't want to generalize this current
offload interface. Can we just NETDEV_UP as the notifier?
The problem with only using NETDEV_UP/REGISTER is that some drivers
need
to
reconfigure their offloads during operation while keeping the
netdevice
in
UP state. One example is ixgbe. This was my first idea also.
It's configuration. Just get it once and save the port number(s) in
the driver. Configure the device only when
IXGBE_FLAG_VXLAN_OFFLOAD_CAPABLE is set.
I don't see much value that each driver has to hold a database of ports
to
offload, the kernel knows much better and can easily be queried via
this
mechanism.
This is not only about ixgbe, but also about benet, which f.e. needs
the
list of offloads available again during resume. Of course, drivers can
do
their own stateful handling of ports, but this seems to much more
generate
problems. I rather would like to see more logic moved into the core and
less
quoted
code in the drivers.
All the drivers that support VXLAN already save the ports in a stateful
fashion. Some support a single port, some and array, some a list. The
only
question is whether they bother to save the information when the offload
feature is disabled. ixgbe does not for instance, but it looks like
fm10k
does since it doesn't check any feature flags. Fixing those that don't
save
the information all the times should be straightforward.
Yes, would probably be straight forward to add another list plus extra
reference counting and maybe a lock, but I don't see why we need to
duplicate the policy in the drivers? I personally think ixgbe does ok in
this regard.
Again, to reiterate, _all_ the drivers that support VXLAN already save
the ports in a stateful fashion. Adding a new notifier is unnecessary
and makes things more complicated, not less. Also to reiterate, this
is not generic functionality that the driver is providing; we should
not need to add anything for this into the core APIs outside of the
two ndo functions that already exist.
I consider the offloading interface work in progress:
I consider it a closed interface. New devices should implement
protocol generic offloads so that we don't need to extend the protocol
specific offload interfaces any further. We do need to break the
dependency between drivers and VXLAN but that can be done without any
additional APIs.
I agree this interface can be much improved. But this series focuses on
firstly removing the dependency on the modules without changing too much
otherwise. No new callback is added just two ones are migrated into one.
Currently drivers don't handle a list of all ports, they discard new ones as
soon as they don't fit into the hardware anymore (mostly only one). So
currently the state is not completely replicated in the drivers. We could
call the ndo_add_*_ports again after we deleted some ports and try to
refresh from the core kernel side. I think instead of adding this logic to
all drivers to fix this, it is easier to callback into the kernel instead of
adding this logic to almost all network drivers.
Almost all network drivers??? I count only nine that are setting
.ndo_add_vxlan_port.
Lot's of drivers refresh the ports list by calling ndo_open function again
because of error recovery or resuming and thus get a new port list (here
some drivers actually currently increment the refcounter for already
installed ports by mistake and I am looking into how to fix this, probably
by throwing away the list of currently installed port numbers first and then
refreshing all ports). It is even worse: it seems drivers actually increment
the port refcounter by mistake when you change ring settings (or other
settings causing a device reset) after installing the vxlan offload.
I agree it would be great to get away with the callback completely but I
haven't yet fully figured out how error handling should be done then. It
seems also not useful to put the error handling in the ndo_add_*_port
functions as most simply configure the structs and call a worker afterwards
to process them. So the correct propagation of errors can also be tricky.
I just don't want to mess with the state in the current drivers but as a
first step remove the dependency.
The reason why I want to keep the callback is that without completely
understanding the rest of problems I don't know if we need it or not.
It is true that the drivers are all over the board on how they
implement this. They need to be fixed regardless (as you point ref
counting is a mess). A new notifier does not address this and just
maintains what is a bad model in the first place. Inflicting more
complexity into core APIs for this simply isn't justified, let's try
to fix the drivers in question.
From: Hannes Frederic Sowa <hidden> Date: 2016-01-11 21:09:07
Hi Tom,
On 11.01.2016 21:46, Tom Herbert wrote:
On Mon, Jan 11, 2016 at 10:56 AM, Hannes Frederic Sowa
[off-list ref] wrote:
quoted
I consider the offloading interface work in progress:
I consider it a closed interface. New devices should implement
protocol generic offloads so that we don't need to extend the protocol
specific offload interfaces any further. We do need to break the
dependency between drivers and VXLAN but that can be done without any
additional APIs.
I am totally with you here.
The reason why I did this series is that I don't want to have vxlan and
geneve modules autoloaded when I modprobe any of those drivers which
implement ndo_add_*_port and use the current style of callbacks. So I
think this is a step to reduce this entanglement already. I solely
switch from vxlan_get_rx_port and geneve_get_rx_port to
netdev_refresh_offloads.
quoted
I agree this interface can be much improved. But this series focuses on
firstly removing the dependency on the modules without changing too much
otherwise. No new callback is added just two ones are migrated into one.
Currently drivers don't handle a list of all ports, they discard new ones as
soon as they don't fit into the hardware anymore (mostly only one). So
currently the state is not completely replicated in the drivers. We could
call the ndo_add_*_ports again after we deleted some ports and try to
refresh from the core kernel side. I think instead of adding this logic to
all drivers to fix this, it is easier to callback into the kernel instead of
adding this logic to almost all network drivers.
Almost all network drivers??? I count only nine that are setting
.ndo_add_vxlan_port.
I was referring to the drivers which already implement ndo_add_{vxlan,
geneve}_port, of course. Sorry for not being specific enough here. All
the other drivers don't need to be considered at all.
quoted
Lot's of drivers refresh the ports list by calling ndo_open function again
because of error recovery or resuming and thus get a new port list (here
some drivers actually currently increment the refcounter for already
installed ports by mistake and I am looking into how to fix this, probably
by throwing away the list of currently installed port numbers first and then
refreshing all ports). It is even worse: it seems drivers actually increment
the port refcounter by mistake when you change ring settings (or other
settings causing a device reset) after installing the vxlan offload.
I agree it would be great to get away with the callback completely but I
haven't yet fully figured out how error handling should be done then. It
seems also not useful to put the error handling in the ndo_add_*_port
functions as most simply configure the structs and call a worker afterwards
to process them. So the correct propagation of errors can also be tricky.
I just don't want to mess with the state in the current drivers but as a
first step remove the dependency.
The reason why I want to keep the callback is that without completely
understanding the rest of problems I don't know if we need it or not.
It is true that the drivers are all over the board on how they
implement this. They need to be fixed regardless (as you point ref
counting is a mess). A new notifier does not address this and just
maintains what is a bad model in the first place. Inflicting more
complexity into core APIs for this simply isn't justified, let's try
to fix the drivers in question.
I think we have a core entanglement between the drivers that offload
vxlan and geneve to the tunnel modules which is hurting most right now
without dealing too much with the offloading API.
As net-next is closed now do you think the series which I actually send
hopefully in time is still worth being considered to at least get the
autoloading fixed. It just replaces the callback by a different callback
which does not depend on the modules.
Quite some drivers need to be touched by that (which implement those
offloads) and I am not sure if requesting a new port list from the
kernel in case of error conditions and the reinitialization of the NIC
during its UP operation without state is still the way to go and easier
to implement. It feels that we have to add more code to the drivers
otherwise. By looking at the current code it might make sense to just
remove the reference counting and ask the kernel to just send down the
list of new ports.
As soon as an interface for generic offloads is found it can be favored
by the kernel stack and the vxlan specific code can be ifdefed out and
later on removed as soon as all features are adequately replaced by the
generic offloading interface.
Does that make sense?
Thanks,
Hannes