| Message ID | 1417718366-14310-1-git-send-email-joestringer@nicira.com |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, 2014-12-04 at 10:39 -0800, Joe Stringer wrote: > ndo_gso_check() was recently introduced to allow NICs to report the > offloading support that they have on a per-skb basis. Add an > implementation for this driver which checks for IPIP, GRE, UDP > tunnels. > > Signed-off-by: Joe Stringer <joestringer@nicira.com> > --- > v3: Drop IPIP and GRE (no driver support even though hw supports it). > Check for UDP outer protocol for UDP tunnels. > v2: Expand to include IP in IP and IPv4/IPv6 inside GRE/UDP tunnels. > Add MAX_INNER_LENGTH (as 80). > --- > drivers/net/ethernet/intel/i40e/i40e_main.c | 26 > ++++++++++++++++++++++++++ > 1 file changed, 26 insertions(+) Thanks Joe, I will add your patch to my queue. Jesse Gross/Tom- If you guys are OK with the latest patch, I will move forward with adding this patch to my queue.
Hello. On 12/04/2014 09:39 PM, Joe Stringer wrote: > ndo_gso_check() was recently introduced to allow NICs to report the > offloading support that they have on a per-skb basis. Add an > implementation for this driver which checks for IPIP, GRE, UDP tunnels. > Signed-off-by: Joe Stringer <joestringer@nicira.com> > --- > v3: Drop IPIP and GRE (no driver support even though hw supports it). > Check for UDP outer protocol for UDP tunnels. > v2: Expand to include IP in IP and IPv4/IPv6 inside GRE/UDP tunnels. > Add MAX_INNER_LENGTH (as 80). > --- > drivers/net/ethernet/intel/i40e/i40e_main.c | 26 ++++++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c > index c3a7f4a..0d6493a 100644 > --- a/drivers/net/ethernet/intel/i40e/i40e_main.c > +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c > @@ -7447,6 +7447,31 @@ static int i40e_ndo_fdb_dump(struct sk_buff *skb, > > #endif /* USE_DEFAULT_FDB_DEL_DUMP */ > #endif /* HAVE_FDB_OPS */ Need empty line here, I think. > +static bool i40e_gso_check(struct sk_buff *skb, struct net_device *dev) > +{ > + if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) { > + unsigned char *ihdr; > + > + if (skb->protocol != IPPROTO_UDP || > + skb->inner_protocol_type != ENCAP_TYPE_ETHER) > + return false; > + > + if (skb->inner_protocol == htons(ETH_P_TEB)) > + ihdr = skb_inner_mac_header(skb); > + else if (skb->inner_protocol == htons(ETH_P_IP) || > + skb->inner_protocol == htons(ETH_P_IPV6)) > + ihdr = skb_inner_network_header(skb); > + else > + return false; The above is asking to be a *switch* instead, no? > + > +#define MAX_TUNNEL_HDR_LEN 80 I'd #define this just above the function, if not at the start of the file... [...] WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2014-12-04 at 22:11 +0300, Sergei Shtylyov wrote: > Hello. > > On 12/04/2014 09:39 PM, Joe Stringer wrote: > > > ndo_gso_check() was recently introduced to allow NICs to report the > > offloading support that they have on a per-skb basis. Add an > > implementation for this driver which checks for IPIP, GRE, UDP tunnels. > > > Signed-off-by: Joe Stringer <joestringer@nicira.com> > > --- > > v3: Drop IPIP and GRE (no driver support even though hw supports it). > > Check for UDP outer protocol for UDP tunnels. > > v2: Expand to include IP in IP and IPv4/IPv6 inside GRE/UDP tunnels. > > Add MAX_INNER_LENGTH (as 80). > > --- > > drivers/net/ethernet/intel/i40e/i40e_main.c | 26 ++++++++++++++++++++++++++ > > 1 file changed, 26 insertions(+) > > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c > > index c3a7f4a..0d6493a 100644 > > --- a/drivers/net/ethernet/intel/i40e/i40e_main.c > > +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c > > @@ -7447,6 +7447,31 @@ static int i40e_ndo_fdb_dump(struct sk_buff *skb, > > > > #endif /* USE_DEFAULT_FDB_DEL_DUMP */ > > #endif /* HAVE_FDB_OPS */ > > Need empty line here, I think. Nope, look above the #endif's, that is the blank line that you are wanting if the above #ifdefs are defined and if not, the blank line is before the #ifdef's. So if you were to add a blank line as you requested, there is a possibility of 2 blank lines in a row. So it is not needed. > > > +static bool i40e_gso_check(struct sk_buff *skb, struct net_device *dev) > > +{ > > + if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) { > > + unsigned char *ihdr; > > + > > + if (skb->protocol != IPPROTO_UDP || > > + skb->inner_protocol_type != ENCAP_TYPE_ETHER) > > + return false; > > + > > + if (skb->inner_protocol == htons(ETH_P_TEB)) > > + ihdr = skb_inner_mac_header(skb); > > + else if (skb->inner_protocol == htons(ETH_P_IP) || > > + skb->inner_protocol == htons(ETH_P_IPV6)) > > + ihdr = skb_inner_network_header(skb); > > + else > > + return false; > > The above is asking to be a *switch* instead, no? > > > + > > +#define MAX_TUNNEL_HDR_LEN 80 > > I'd #define this just above the function, if not at the start of the file... > > [...] > > WBR, Sergei > > _______________________________________________ > Linux-nics mailing list > Linux-nics@intel.com
On Thu, Dec 4, 2014 at 10:39 AM, Joe Stringer <joestringer@nicira.com> wrote: > ndo_gso_check() was recently introduced to allow NICs to report the > offloading support that they have on a per-skb basis. Add an > implementation for this driver which checks for IPIP, GRE, UDP tunnels. > > Signed-off-by: Joe Stringer <joestringer@nicira.com> > --- > v3: Drop IPIP and GRE (no driver support even though hw supports it). > Check for UDP outer protocol for UDP tunnels. > v2: Expand to include IP in IP and IPv4/IPv6 inside GRE/UDP tunnels. > Add MAX_INNER_LENGTH (as 80). > --- > drivers/net/ethernet/intel/i40e/i40e_main.c | 26 ++++++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c > index c3a7f4a..0d6493a 100644 > --- a/drivers/net/ethernet/intel/i40e/i40e_main.c > +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c > @@ -7447,6 +7447,31 @@ static int i40e_ndo_fdb_dump(struct sk_buff *skb, > > #endif /* USE_DEFAULT_FDB_DEL_DUMP */ > #endif /* HAVE_FDB_OPS */ > +static bool i40e_gso_check(struct sk_buff *skb, struct net_device *dev) > +{ > + if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) { > + unsigned char *ihdr; > + > + if (skb->protocol != IPPROTO_UDP || > + skb->inner_protocol_type != ENCAP_TYPE_ETHER) > + return false; > + > + if (skb->inner_protocol == htons(ETH_P_TEB)) > + ihdr = skb_inner_mac_header(skb); > + else if (skb->inner_protocol == htons(ETH_P_IP) || > + skb->inner_protocol == htons(ETH_P_IPV6)) > + ihdr = skb_inner_network_header(skb); > + else > + return false; > + Wow, this is getting complicated! :-( It's not clear that the protocol specific checks are needed here since it looks like the header length is being passed to the device later on. Also, I think we need skb_inner_mac_header(skb) - skb_transport_header(skb) to always work to give the length of the encapsulation headers (in case there is no real inner mac header, then that offset should be for the network header). So would a simple check like this work: if (skb->encapsulation && (skb_inner_mac_header(skb) - skb_transport_header(skb)) > MAX_TUNNEL_HDR_LEN) return false; > +#define MAX_TUNNEL_HDR_LEN 80 > + if (ihdr - skb_transport_header(skb) > MAX_TUNNEL_HDR_LEN) > + return false; > + } > + > + return true; > +} > + > static const struct net_device_ops i40e_netdev_ops = { > .ndo_open = i40e_open, > .ndo_stop = i40e_close, > @@ -7487,6 +7512,7 @@ static const struct net_device_ops i40e_netdev_ops = { > .ndo_fdb_dump = i40e_ndo_fdb_dump, > #endif > #endif > + .ndo_gso_check = i40e_gso_check, > }; > > /** > -- > 1.7.10.4 > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> > +#define MAX_TUNNEL_HDR_LEN 80 >> >> I'd #define this just above the function, if not at the start of the file... Right, the style for most of this file is to place the #define like this above the function. I'll do that. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 4 December 2014 at 12:17, Tom Herbert <therbert@google.com> wrote: > On Thu, Dec 4, 2014 at 10:39 AM, Joe Stringer <joestringer@nicira.com> wrote: >> ndo_gso_check() was recently introduced to allow NICs to report the >> offloading support that they have on a per-skb basis. Add an >> implementation for this driver which checks for IPIP, GRE, UDP tunnels. >> >> Signed-off-by: Joe Stringer <joestringer@nicira.com> >> --- >> v3: Drop IPIP and GRE (no driver support even though hw supports it). >> Check for UDP outer protocol for UDP tunnels. >> v2: Expand to include IP in IP and IPv4/IPv6 inside GRE/UDP tunnels. >> Add MAX_INNER_LENGTH (as 80). >> --- >> drivers/net/ethernet/intel/i40e/i40e_main.c | 26 ++++++++++++++++++++++++++ >> 1 file changed, 26 insertions(+) >> >> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c >> index c3a7f4a..0d6493a 100644 >> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c >> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c >> @@ -7447,6 +7447,31 @@ static int i40e_ndo_fdb_dump(struct sk_buff *skb, >> >> #endif /* USE_DEFAULT_FDB_DEL_DUMP */ >> #endif /* HAVE_FDB_OPS */ >> +static bool i40e_gso_check(struct sk_buff *skb, struct net_device *dev) >> +{ >> + if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) { >> + unsigned char *ihdr; >> + >> + if (skb->protocol != IPPROTO_UDP || >> + skb->inner_protocol_type != ENCAP_TYPE_ETHER) >> + return false; >> + >> + if (skb->inner_protocol == htons(ETH_P_TEB)) >> + ihdr = skb_inner_mac_header(skb); >> + else if (skb->inner_protocol == htons(ETH_P_IP) || >> + skb->inner_protocol == htons(ETH_P_IPV6)) >> + ihdr = skb_inner_network_header(skb); >> + else >> + return false; >> + > > Wow, this is getting complicated! :-( It's not clear that the protocol > specific checks are needed here since it looks like the header length > is being passed to the device later on. Also, I think we need > skb_inner_mac_header(skb) - skb_transport_header(skb) to always work > to give the length of the encapsulation headers (in case there is no > real inner mac header, then that offset should be for the network > header). > > So would a simple check like this work: > > if (skb->encapsulation && > (skb_inner_mac_header(skb) - skb_transport_header(skb)) > > MAX_TUNNEL_HDR_LEN) > return false; Ah, I didn't realise that the inner_mac_header() would be equivalent to inner network when inner mac doesn't exist. This is much tidier, thanks Tom. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c index c3a7f4a..0d6493a 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_main.c +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c @@ -7447,6 +7447,31 @@ static int i40e_ndo_fdb_dump(struct sk_buff *skb, #endif /* USE_DEFAULT_FDB_DEL_DUMP */ #endif /* HAVE_FDB_OPS */ +static bool i40e_gso_check(struct sk_buff *skb, struct net_device *dev) +{ + if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) { + unsigned char *ihdr; + + if (skb->protocol != IPPROTO_UDP || + skb->inner_protocol_type != ENCAP_TYPE_ETHER) + return false; + + if (skb->inner_protocol == htons(ETH_P_TEB)) + ihdr = skb_inner_mac_header(skb); + else if (skb->inner_protocol == htons(ETH_P_IP) || + skb->inner_protocol == htons(ETH_P_IPV6)) + ihdr = skb_inner_network_header(skb); + else + return false; + +#define MAX_TUNNEL_HDR_LEN 80 + if (ihdr - skb_transport_header(skb) > MAX_TUNNEL_HDR_LEN) + return false; + } + + return true; +} + static const struct net_device_ops i40e_netdev_ops = { .ndo_open = i40e_open, .ndo_stop = i40e_close, @@ -7487,6 +7512,7 @@ static const struct net_device_ops i40e_netdev_ops = { .ndo_fdb_dump = i40e_ndo_fdb_dump, #endif #endif + .ndo_gso_check = i40e_gso_check, }; /**
ndo_gso_check() was recently introduced to allow NICs to report the offloading support that they have on a per-skb basis. Add an implementation for this driver which checks for IPIP, GRE, UDP tunnels. Signed-off-by: Joe Stringer <joestringer@nicira.com> --- v3: Drop IPIP and GRE (no driver support even though hw supports it). Check for UDP outer protocol for UDP tunnels. v2: Expand to include IP in IP and IPv4/IPv6 inside GRE/UDP tunnels. Add MAX_INNER_LENGTH (as 80). --- drivers/net/ethernet/intel/i40e/i40e_main.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)