[PATCHv3 net] i40e: Implement ndo_gso_check()

[PATCHv3 net] i40e: Implement ndo_gso_check()

[Joe Stringer]

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;
+
+#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

Re: [PATCHv3 net] i40e: Implement ndo_gso_check()

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 869 bytes --]

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.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCHv3 net] i40e: Implement ndo_gso_check()

[Sergei Shtylyov]

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

Re: [linux-nics] [PATCHv3 net] i40e: Implement ndo_gso_check()

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 2491 bytes --]

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



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [linux-nics] [PATCHv3 net] i40e: Implement ndo_gso_check()

[Joe Stringer]

>> > +#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.

Re: [PATCHv3 net] i40e: Implement ndo_gso_check()

[Tom Herbert]

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
>

Re: [PATCHv3 net] i40e: Implement ndo_gso_check()

[Joe Stringer]

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.