| Message ID | 20160613174806.15186.64702.stgit@localhost.localdomain |
|---|---|
| State | Superseded |
| Headers | show |
Hi Alex, very cool series! On 13.06.2016 19:48, Alexander Duyck wrote: > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > index d101e4d904ba..e959b6348f91 100644 > --- a/include/linux/netdevice.h > +++ b/include/linux/netdevice.h > @@ -1269,6 +1269,14 @@ struct net_device_ops { > void (*ndo_del_geneve_port)(struct net_device *dev, > sa_family_t sa_family, > __be16 port); > + void (*ndo_add_udp_enc_port)(struct net_device *dev, > + sa_family_t sa_family, > + __be16 port, > + unsigned int type); > + void (*ndo_del_udp_enc_port)(struct net_device *dev, > + sa_family_t sa_family, > + __be16 port, > + unsigned int type); > void* (*ndo_dfwd_add_station)(struct net_device *pdev, > struct net_device *dev); > void (*ndo_dfwd_del_station)(struct net_device *pdev, What do you think about adding a struct as argument to ndo_*_udp_enc_port? As a result we can much easier add new fields in case future NICs allow us to e.g. specify a bound ip address? Thanks, Hannes
On Mon, Jun 13, 2016 at 10:57 AM, Hannes Frederic Sowa <hannes@redhat.com> wrote: > Hi Alex, > > very cool series! > > On 13.06.2016 19:48, Alexander Duyck wrote: >> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h >> index d101e4d904ba..e959b6348f91 100644 >> --- a/include/linux/netdevice.h >> +++ b/include/linux/netdevice.h >> @@ -1269,6 +1269,14 @@ struct net_device_ops { >> void (*ndo_del_geneve_port)(struct net_device *dev, >> sa_family_t sa_family, >> __be16 port); >> + void (*ndo_add_udp_enc_port)(struct net_device *dev, >> + sa_family_t sa_family, >> + __be16 port, >> + unsigned int type); >> + void (*ndo_del_udp_enc_port)(struct net_device *dev, >> + sa_family_t sa_family, >> + __be16 port, >> + unsigned int type); >> void* (*ndo_dfwd_add_station)(struct net_device *pdev, >> struct net_device *dev); >> void (*ndo_dfwd_del_station)(struct net_device *pdev, > > What do you think about adding a struct as argument to > ndo_*_udp_enc_port? As a result we can much easier add new fields in > case future NICs allow us to e.g. specify a bound ip address? > This is why I think we should be using ntuple filtering for encapsulation instead perpetuating the notion that encapsulation offload can only be done based on receive ports. Tom > Thanks, > Hannes >
On Mon, Jun 13, 2016 at 10:57 AM, Hannes Frederic Sowa <hannes@redhat.com> wrote: > Hi Alex, > > very cool series! > > On 13.06.2016 19:48, Alexander Duyck wrote: >> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h >> index d101e4d904ba..e959b6348f91 100644 >> --- a/include/linux/netdevice.h >> +++ b/include/linux/netdevice.h >> @@ -1269,6 +1269,14 @@ struct net_device_ops { >> void (*ndo_del_geneve_port)(struct net_device *dev, >> sa_family_t sa_family, >> __be16 port); >> + void (*ndo_add_udp_enc_port)(struct net_device *dev, >> + sa_family_t sa_family, >> + __be16 port, >> + unsigned int type); >> + void (*ndo_del_udp_enc_port)(struct net_device *dev, >> + sa_family_t sa_family, >> + __be16 port, >> + unsigned int type); >> void* (*ndo_dfwd_add_station)(struct net_device *pdev, >> struct net_device *dev); >> void (*ndo_dfwd_del_station)(struct net_device *pdev, > > What do you think about adding a struct as argument to > ndo_*_udp_enc_port? As a result we can much easier add new fields in > case future NICs allow us to e.g. specify a bound ip address? Actually that is probably a good idea. Suggestions on the name are welcome. Otherwise I will try to come up with something in a bit as I am currently going through and flushing out all the driver specific VXLAN and GENEVE build flags. - Alex
Hi,
[auto build test ERROR on net/master]
[also build test ERROR on v4.7-rc3 next-20160609]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Alexander-Duyck/Future-proof-tunnel-offload-handlers/20160614-020534
config: x86_64-randconfig-x015-06140233 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
In file included from include/net/vxlan.h:11:0,
from drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:53:
include/net/udp_tunnel.h: In function 'udp_tunnel_handle_offloads':
>> include/net/udp_tunnel.h:130:9: error: implicit declaration of function 'iptunnel_handle_offloads' [-Werror=implicit-function-declaration]
return iptunnel_handle_offloads(skb, type);
^~~~~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/iptunnel_handle_offloads +130 include/net/udp_tunnel.h
c29a70d2 Pravin B Shelar 2015-08-26 124 int md_size);
c29a70d2 Pravin B Shelar 2015-08-26 125
aed069df Alexander Duyck 2016-04-14 126 static inline int udp_tunnel_handle_offloads(struct sk_buff *skb, bool udp_csum)
6a93cc90 Andy Zhou 2014-09-16 127 {
6a93cc90 Andy Zhou 2014-09-16 128 int type = udp_csum ? SKB_GSO_UDP_TUNNEL_CSUM : SKB_GSO_UDP_TUNNEL;
6a93cc90 Andy Zhou 2014-09-16 129
6fa79666 Edward Cree 2016-02-11 @130 return iptunnel_handle_offloads(skb, type);
6a93cc90 Andy Zhou 2014-09-16 131 }
6a93cc90 Andy Zhou 2014-09-16 132
6a93cc90 Andy Zhou 2014-09-16 133 static inline void udp_tunnel_encap_enable(struct socket *sock)
:::::: The code at line 130 was first introduced by commit
:::::: 6fa79666e24d32be1b709f5269af41ed9e829e7e net: ip_tunnel: remove 'csum_help' argument to iptunnel_handle_offloads
:::::: TO: Edward Cree <ecree@solarflare.com>
:::::: CC: David S. Miller <davem@davemloft.net>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On 13.06.2016 21:47, Alexander Duyck wrote: > On Mon, Jun 13, 2016 at 10:57 AM, Hannes Frederic Sowa > <hannes@redhat.com> wrote: >> Hi Alex, >> >> very cool series! >> >> On 13.06.2016 19:48, Alexander Duyck wrote: >>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h >>> index d101e4d904ba..e959b6348f91 100644 >>> --- a/include/linux/netdevice.h >>> +++ b/include/linux/netdevice.h >>> @@ -1269,6 +1269,14 @@ struct net_device_ops { >>> void (*ndo_del_geneve_port)(struct net_device *dev, >>> sa_family_t sa_family, >>> __be16 port); >>> + void (*ndo_add_udp_enc_port)(struct net_device *dev, >>> + sa_family_t sa_family, >>> + __be16 port, >>> + unsigned int type); >>> + void (*ndo_del_udp_enc_port)(struct net_device *dev, >>> + sa_family_t sa_family, >>> + __be16 port, >>> + unsigned int type); >>> void* (*ndo_dfwd_add_station)(struct net_device *pdev, >>> struct net_device *dev); >>> void (*ndo_dfwd_del_station)(struct net_device *pdev, >> >> What do you think about adding a struct as argument to >> ndo_*_udp_enc_port? As a result we can much easier add new fields in >> case future NICs allow us to e.g. specify a bound ip address? > > Actually that is probably a good idea. Suggestions on the name are > welcome. Otherwise I will try to come up with something in a bit as I > am currently going through and flushing out all the driver specific > VXLAN and GENEVE build flags. Hmmm... struct net_device_hw_offload, to be most generic? Maybe we can even drop the udp_enc in the name and go completely generic: int (*ndo_apply_offload)(..., struct hw_offload). (enc reminded me too much at encryption) Another idea, should we add error indications also for the future? We can signal if a specific card was not able to enable offloading. Different situations can be signaled: port list depleted, protocol unsupported etc. Might make sense for later postprocessing and signaling to user space. Thanks, Hannes
On Mon, Jun 13, 2016 at 2:08 PM, Hannes Frederic Sowa <hannes@redhat.com> wrote: > On 13.06.2016 21:47, Alexander Duyck wrote: >> On Mon, Jun 13, 2016 at 10:57 AM, Hannes Frederic Sowa >> <hannes@redhat.com> wrote: >>> Hi Alex, >>> >>> very cool series! >>> >>> On 13.06.2016 19:48, Alexander Duyck wrote: >>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h >>>> index d101e4d904ba..e959b6348f91 100644 >>>> --- a/include/linux/netdevice.h >>>> +++ b/include/linux/netdevice.h >>>> @@ -1269,6 +1269,14 @@ struct net_device_ops { >>>> void (*ndo_del_geneve_port)(struct net_device *dev, >>>> sa_family_t sa_family, >>>> __be16 port); >>>> + void (*ndo_add_udp_enc_port)(struct net_device *dev, >>>> + sa_family_t sa_family, >>>> + __be16 port, >>>> + unsigned int type); >>>> + void (*ndo_del_udp_enc_port)(struct net_device *dev, >>>> + sa_family_t sa_family, >>>> + __be16 port, >>>> + unsigned int type); >>>> void* (*ndo_dfwd_add_station)(struct net_device *pdev, >>>> struct net_device *dev); >>>> void (*ndo_dfwd_del_station)(struct net_device *pdev, >>> >>> What do you think about adding a struct as argument to >>> ndo_*_udp_enc_port? As a result we can much easier add new fields in >>> case future NICs allow us to e.g. specify a bound ip address? >> >> Actually that is probably a good idea. Suggestions on the name are >> welcome. Otherwise I will try to come up with something in a bit as I >> am currently going through and flushing out all the driver specific >> VXLAN and GENEVE build flags. > > Hmmm... struct net_device_hw_offload, to be most generic? Maybe we can > even drop the udp_enc in the name and go completely generic: > > int (*ndo_apply_offload)(..., struct hw_offload). The only probably with generically using the offload keyword is it is not very clear about what is going on. For now I am just going with udp_enc_endpoint_info since that is basically what we are passing. Then I just use the pointer variable ei for passing it back and forth between the functions. > (enc reminded me too much at encryption) I don't know. In a way that isn't too far off since we are looking at packet data buried inside of a UDP packet. I thought it worked based on the fact that we have hw_enc_features which is what is used to indicate the hw features when a packet is encapsulated. I could add a few more letters and move things over to "encap" if you prefer. It just means adding 2 more letters. > Another idea, should we add error indications also for the future? We > can signal if a specific card was not able to enable offloading. > Different situations can be signaled: port list depleted, protocol > unsupported etc. > > Might make sense for later postprocessing and signaling to user space. The problem is we are using a notifier type setup. As such we cannot really exit out if an error occurs on one of the ports. - Alex
diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c index f5ce41532cf4..f1d06bef1c55 100644 --- a/drivers/net/geneve.c +++ b/drivers/net/geneve.c @@ -1508,7 +1508,7 @@ static int geneve_netdevice_event(struct notifier_block *unused, { struct net_device *dev = netdev_notifier_info_to_dev(ptr); - if (event == NETDEV_OFFLOAD_PUSH_GENEVE) + if (event == NETDEV_OFFLOAD_PUSH_UDP_ENC_OFFLOAD) geneve_push_rx_ports(dev); return NOTIFY_DONE; diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c index 43f634282726..72da056abdf4 100644 --- a/drivers/net/vxlan.c +++ b/drivers/net/vxlan.c @@ -3266,7 +3266,7 @@ static int vxlan_netdevice_event(struct notifier_block *unused, if (event == NETDEV_UNREGISTER) vxlan_handle_lowerdev_unregister(vn, dev); - else if (event == NETDEV_OFFLOAD_PUSH_VXLAN) + else if (event == NETDEV_OFFLOAD_PUSH_UDP_ENC_OFFLOAD) vxlan_push_rx_ports(dev); return NOTIFY_DONE; diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index d101e4d904ba..e959b6348f91 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -1269,6 +1269,14 @@ struct net_device_ops { void (*ndo_del_geneve_port)(struct net_device *dev, sa_family_t sa_family, __be16 port); + void (*ndo_add_udp_enc_port)(struct net_device *dev, + sa_family_t sa_family, + __be16 port, + unsigned int type); + void (*ndo_del_udp_enc_port)(struct net_device *dev, + sa_family_t sa_family, + __be16 port, + unsigned int type); void* (*ndo_dfwd_add_station)(struct net_device *pdev, struct net_device *dev); void (*ndo_dfwd_del_station)(struct net_device *pdev, @@ -2251,8 +2259,7 @@ struct netdev_lag_lower_state_info { #define NETDEV_BONDING_INFO 0x0019 #define NETDEV_PRECHANGEUPPER 0x001A #define NETDEV_CHANGELOWERSTATE 0x001B -#define NETDEV_OFFLOAD_PUSH_VXLAN 0x001C -#define NETDEV_OFFLOAD_PUSH_GENEVE 0x001D +#define NETDEV_OFFLOAD_PUSH_UDP_ENC_OFFLOAD 0x001C int register_netdevice_notifier(struct notifier_block *nb); int unregister_netdevice_notifier(struct notifier_block *nb); diff --git a/include/net/geneve.h b/include/net/geneve.h index cb544a530146..7638ec62c5e1 100644 --- a/include/net/geneve.h +++ b/include/net/geneve.h @@ -1,10 +1,7 @@ #ifndef __NET_GENEVE_H #define __NET_GENEVE_H 1 -#ifdef CONFIG_INET #include <net/udp_tunnel.h> -#endif - /* Geneve Header: * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ @@ -64,8 +61,7 @@ struct genevehdr { static inline void geneve_get_rx_port(struct net_device *netdev) { - ASSERT_RTNL(); - call_netdevice_notifiers(NETDEV_OFFLOAD_PUSH_GENEVE, netdev); + udp_tunnel_get_rx_port(netdev); } #ifdef CONFIG_INET diff --git a/include/net/udp_tunnel.h b/include/net/udp_tunnel.h index 704f931fd9ad..9ea813740231 100644 --- a/include/net/udp_tunnel.h +++ b/include/net/udp_tunnel.h @@ -96,6 +96,12 @@ void udp_tunnel_push_rx_port(struct net_device *dev, struct socket *sock, void udp_tunnel_notify_add_rx_port(struct socket *sock, unsigned int type); void udp_tunnel_notify_del_rx_port(struct socket *sock, unsigned int type); +static inline void udp_tunnel_get_rx_port(struct net_device *dev) +{ + ASSERT_RTNL(); + call_netdevice_notifiers(NETDEV_OFFLOAD_PUSH_UDP_ENC_OFFLOAD, dev); +} + /* Transmit the skb using UDP encapsulation. */ void udp_tunnel_xmit_skb(struct rtable *rt, struct sock *sk, struct sk_buff *skb, __be32 src, __be32 dst, __u8 tos, __u8 ttl, diff --git a/include/net/vxlan.h b/include/net/vxlan.h index b8803165df91..2c4f8fcd5a3b 100644 --- a/include/net/vxlan.h +++ b/include/net/vxlan.h @@ -8,6 +8,7 @@ #include <linux/netdevice.h> #include <linux/udp.h> #include <net/dst_metadata.h> +#include <net/udp_tunnel.h> /* VXLAN protocol (RFC 7348) header: * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ @@ -394,8 +395,7 @@ static inline __be32 vxlan_compute_rco(unsigned int start, unsigned int offset) static inline void vxlan_get_rx_port(struct net_device *netdev) { - ASSERT_RTNL(); - call_netdevice_notifiers(NETDEV_OFFLOAD_PUSH_VXLAN, netdev); + udp_tunnel_get_rx_port(netdev); } static inline unsigned short vxlan_get_sk_family(struct vxlan_sock *vs) diff --git a/net/ipv4/udp_tunnel.c b/net/ipv4/udp_tunnel.c index d575b70644e3..a22677cd41d8 100644 --- a/net/ipv4/udp_tunnel.c +++ b/net/ipv4/udp_tunnel.c @@ -83,6 +83,12 @@ void udp_tunnel_push_rx_port(struct net_device *dev, struct socket *sock, sa_family_t sa_family = sk->sk_family; __be16 port = inet_sk(sk)->inet_sport; + if (dev->netdev_ops->ndo_add_udp_enc_port) { + dev->netdev_ops->ndo_add_udp_enc_port(dev, sa_family, + port, type); + return; + } + switch (type) { case UDP_ENC_OFFLOAD_TYPE_VXLAN: if (!dev->netdev_ops->ndo_add_vxlan_port) @@ -122,6 +128,12 @@ static void udp_tunnel_pull_rx_port(struct net_device *dev, sa_family_t sa_family = sk->sk_family; __be16 port = inet_sk(sk)->inet_sport; + if (dev->netdev_ops->ndo_del_udp_enc_port) { + dev->netdev_ops->ndo_del_udp_enc_port(dev, sa_family, + port, type); + return; + } + switch (type) { case UDP_ENC_OFFLOAD_TYPE_VXLAN: if (!dev->netdev_ops->ndo_del_vxlan_port)
This patch merges the notifiers for VXLAN and GENEVE into a single UDP encapsulation notifier. The idea is that we will want to only have to make one notifier call to receive the list of ports for VXLAN and GENEVE tunnels that need to be offloaded. Signed-off-by: Alexander Duyck <aduyck@mirantis.com> --- drivers/net/geneve.c | 2 +- drivers/net/vxlan.c | 2 +- include/linux/netdevice.h | 11 +++++++++-- include/net/geneve.h | 6 +----- include/net/udp_tunnel.h | 6 ++++++ include/net/vxlan.h | 4 ++-- net/ipv4/udp_tunnel.c | 12 ++++++++++++ 7 files changed, 32 insertions(+), 11 deletions(-)