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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > ---
> > 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
> [email protected]
On Thu, Dec 4, 2014 at 10:39 AM, Joe Stringer <[email protected]> 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 <[email protected]>
> ---
> 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
>
>> > +#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.
On 4 December 2014 at 12:17, Tom Herbert <[email protected]> wrote:
> On Thu, Dec 4, 2014 at 10:39 AM, Joe Stringer <[email protected]> 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 <[email protected]>
>> ---
>> 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.