[PATCH net 0/5] Implement ndo_gso_check() for vxlan nics

[PATCH net 0/5] Implement ndo_gso_check() for vxlan nics

[Joe Stringer]

Most NICs that report NETIF_F_GSO_UDP_TUNNEL support VXLAN, and not other
UDP-based encapsulation protocols where the format and size of the header may
differ. This patch series implements ndo_gso_check() for these NICs,
restricting the GSO handling to something that looks and smells like VXLAN.

Implementation shamelessly stolen from Tom Herbert (with minor fixups):
http://thread.gmane.org/gmane.linux.network/332428/focus=333111

If there are particular differences for your driver on actual support, I'd like
to hear about it. I adjusted the i40e driver to report support with tunnel
headers of up to 64 octets, perhaps there are other specifics that I've missed.

Joe Stringer (5):
  be2net: Implement ndo_gso_check()
  i40e: Implement ndo_gso_check()
  fm10k: Implement ndo_gso_check()
  net/mlx4_en: Implement ndo_gso_check()
  qlcnic: Implement ndo_gso_check()

 drivers/net/ethernet/emulex/benet/be_main.c      |   12 ++++++++++++
 drivers/net/ethernet/intel/fm10k/fm10k_netdev.c  |   12 ++++++++++++
 drivers/net/ethernet/intel/i40e/i40e_main.c      |   14 +++++++++++++-
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c   |   12 ++++++++++++
 drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c |   12 ++++++++++++
 5 files changed, 61 insertions(+), 1 deletion(-)

--
1.7.10.4

[PATCH net 1/5] be2net: 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 something that looks
like VXLAN.

Implementation shamelessly stolen from Tom Herbert:
http://thread.gmane.org/gmane.linux.network/332428/focus=333111

Signed-off-by: Joe Stringer <joestringer@nicira.com>
---
 drivers/net/ethernet/emulex/benet/be_main.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c
index 9a18e79..bd52b8d 100644
--- a/drivers/net/ethernet/emulex/benet/be_main.c
+++ b/drivers/net/ethernet/emulex/benet/be_main.c
@@ -4423,6 +4423,17 @@ static void be_del_vxlan_port(struct net_device *netdev, sa_family_t sa_family,
 }
 #endif
 
+static bool be_gso_check(struct sk_buff *skb, struct net_device *dev)
+{
+	if ((skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) &&
+	    (skb->inner_protocol_type != ENCAP_TYPE_ETHER ||
+	     skb->inner_protocol != htons(ETH_P_TEB) ||
+	     skb_inner_mac_header(skb) - skb_transport_header(skb) != 16))
+		return false;
+
+	return true;
+}
+
 static const struct net_device_ops be_netdev_ops = {
 	.ndo_open		= be_open,
 	.ndo_stop		= be_close,
@@ -4451,6 +4462,7 @@ static const struct net_device_ops be_netdev_ops = {
 	.ndo_add_vxlan_port	= be_add_vxlan_port,
 	.ndo_del_vxlan_port	= be_del_vxlan_port,
 #endif
+	.ndo_gso_check		= be_gso_check,
 };
 
 static void be_netdev_init(struct net_device *netdev)
-- 
1.7.10.4

[PATCH net 2/5] 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 tunnel headers over UDP
of up to 64 octets in length.

Implementation shamelessly stolen from Tom Herbert:
http://thread.gmane.org/gmane.linux.network/332428/focus=333111

Signed-off-by: Joe Stringer <joestringer@nicira.com>
---
 drivers/net/ethernet/intel/i40e/i40e_main.c |   14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index c3a7f4a..21829b5 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -7444,9 +7444,20 @@ static int i40e_ndo_fdb_dump(struct sk_buff *skb,
 
 	return idx;
 }
-
 #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) &&
+	    (skb->inner_protocol_type != ENCAP_TYPE_ETHER ||
+	     skb->inner_protocol != htons(ETH_P_TEB) ||
+	     skb_inner_mac_header(skb) - skb_transport_header(skb) > 64))
+		return false;
+
+	return true;
+}
+
 static const struct net_device_ops i40e_netdev_ops = {
 	.ndo_open		= i40e_open,
 	.ndo_stop		= i40e_close,
@@ -7487,6 +7498,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: [PATCH net 2/5] i40e: Implement ndo_gso_check()

[Jesse Gross]

On Tue, Nov 4, 2014 at 1:56 PM, Joe Stringer <joestringer@nicira.com> wrote:
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index c3a7f4a..21829b5 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> +static bool i40e_gso_check(struct sk_buff *skb, struct net_device *dev)
> +{
> +       if ((skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) &&
> +           (skb->inner_protocol_type != ENCAP_TYPE_ETHER ||
> +            skb->inner_protocol != htons(ETH_P_TEB) ||
> +            skb_inner_mac_header(skb) - skb_transport_header(skb) > 64))
> +               return false;

I think it may be possible to even support a few more things here.
According to the datasheet here:
http://www.intel.com/content/dam/www/public/us/en/documents/datasheets/xl710-10-40-controller-datasheet.pdf

This can actually support 64 bytes beyond the tunnel header, which
would make for a total of 80 bytes. It looks like it can also support
IPv4 or IPv6 beyond just Ethernet as the encapsulated protocol.

Intel guys, can you confirm that this is correct?

Re: [PATCH net 2/5] i40e: Implement ndo_gso_check()

[Jeff Kirsher]

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

On Tue, 2014-11-04 at 15:45 -0800, Jesse Gross wrote:
> On Tue, Nov 4, 2014 at 1:56 PM, Joe Stringer <joestringer@nicira.com> wrote:
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> > index c3a7f4a..21829b5 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> > +static bool i40e_gso_check(struct sk_buff *skb, struct net_device *dev)
> > +{
> > +       if ((skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) &&
> > +           (skb->inner_protocol_type != ENCAP_TYPE_ETHER ||
> > +            skb->inner_protocol != htons(ETH_P_TEB) ||
> > +            skb_inner_mac_header(skb) - skb_transport_header(skb) > 64))
> > +               return false;
> 
> I think it may be possible to even support a few more things here.
> According to the datasheet here:
> http://www.intel.com/content/dam/www/public/us/en/documents/datasheets/xl710-10-40-controller-datasheet.pdf
> 
> This can actually support 64 bytes beyond the tunnel header, which
> would make for a total of 80 bytes. It looks like it can also support
> IPv4 or IPv6 beyond just Ethernet as the encapsulated protocol.
> 
> Intel guys, can you confirm that this is correct?

I believe you are correct Jesse, but I will let Shannon Nelson or Jesse
Brandeburg respond since they are the i40e maintainers.

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

Re: [PATCH net 2/5] i40e: Implement ndo_gso_check()

[Joe Stringer]

On Tuesday, November 04, 2014 15:45:22 Jesse Gross wrote:
> On Tue, Nov 4, 2014 at 1:56 PM, Joe Stringer <joestringer@nicira.com> wrote:
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c
> > b/drivers/net/ethernet/intel/i40e/i40e_main.c index c3a7f4a..21829b5
> > 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> > +static bool i40e_gso_check(struct sk_buff *skb, struct net_device *dev)
> > +{
> > +       if ((skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) &&
> > +           (skb->inner_protocol_type != ENCAP_TYPE_ETHER ||
> > +            skb->inner_protocol != htons(ETH_P_TEB) ||
> > +            skb_inner_mac_header(skb) - skb_transport_header(skb) > 64))
> > +               return false;
> 
> I think it may be possible to even support a few more things here.
> According to the datasheet here:
> http://www.intel.com/content/dam/www/public/us/en/documents/datasheets/xl71
> 0-10-40-controller-datasheet.pdf
> 
> This can actually support 64 bytes beyond the tunnel header, which
> would make for a total of 80 bytes. It looks like it can also support
> IPv4 or IPv6 beyond just Ethernet as the encapsulated protocol.
> 
> Intel guys, can you confirm that this is correct?

I'm just respinning this for v4/6 beyond GRE/UDP tunnel and IPIP, and I found 
the description of max protocol parsing size of 480B (with individual header 
limit of 255B). I couldn't find where you get this 64/80 number or which 
headers it maps to. Could you (or one of the intel guys) expand on this?

Re: [PATCH net 2/5] i40e: Implement ndo_gso_check()

[Jesse Gross]

On Thu, Nov 20, 2014 at 11:16 AM, Joe Stringer <joestringer@nicira.com> wrote:
> On Tuesday, November 04, 2014 15:45:22 Jesse Gross wrote:
>> On Tue, Nov 4, 2014 at 1:56 PM, Joe Stringer <joestringer@nicira.com> wrote:
>> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c
>> > b/drivers/net/ethernet/intel/i40e/i40e_main.c index c3a7f4a..21829b5
>> > 100644
>> > --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
>> > +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
>> > +static bool i40e_gso_check(struct sk_buff *skb, struct net_device *dev)
>> > +{
>> > +       if ((skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) &&
>> > +           (skb->inner_protocol_type != ENCAP_TYPE_ETHER ||
>> > +            skb->inner_protocol != htons(ETH_P_TEB) ||
>> > +            skb_inner_mac_header(skb) - skb_transport_header(skb) > 64))
>> > +               return false;
>>
>> I think it may be possible to even support a few more things here.
>> According to the datasheet here:
>> http://www.intel.com/content/dam/www/public/us/en/documents/datasheets/xl71
>> 0-10-40-controller-datasheet.pdf
>>
>> This can actually support 64 bytes beyond the tunnel header, which
>> would make for a total of 80 bytes. It looks like it can also support
>> IPv4 or IPv6 beyond just Ethernet as the encapsulated protocol.
>>
>> Intel guys, can you confirm that this is correct?
>
> I'm just respinning this for v4/6 beyond GRE/UDP tunnel and IPIP, and I found
> the description of max protocol parsing size of 480B (with individual header
> limit of 255B). I couldn't find where you get this 64/80 number or which
> headers it maps to. Could you (or one of the intel guys) expand on this?

The number that I gave was from the section on Geneve support (on page
708), which says that it can support up to 64 bytes of options (this
was also my understanding from previous conversations with Intel
guys). I searched for 480 byte limit and it seems like it for receive
instead of transmit, which could conceivably be different.

Re: [PATCH net 2/5] i40e: Implement ndo_gso_check()

[Jeff Kirsher]

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

On Tue, 2014-11-04 at 13:56 -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 tunnel headers over
> UDP
> of up to 64 octets in length.
> 
> Implementation shamelessly stolen from Tom Herbert:
> http://thread.gmane.org/gmane.linux.network/332428/focus=333111
> 
> Signed-off-by: Joe Stringer <joestringer@nicira.com>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_main.c |   14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)

Thanks again Joe, I will add your patch to my queue.

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

[PATCH net 3/5] fm10k: 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 something that looks
like VXLAN.

Implementation shamelessly stolen from Tom Herbert:
http://thread.gmane.org/gmane.linux.network/332428/focus=333111

Signed-off-by: Joe Stringer <joestringer@nicira.com>
---
Should this driver report support for GSO on packets with tunnel headers
up to 64B like the i40e driver does?
---
 drivers/net/ethernet/intel/fm10k/fm10k_netdev.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c b/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c
index 8811364..b9ef622 100644
--- a/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c
+++ b/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c
@@ -1350,6 +1350,17 @@ static void fm10k_dfwd_del_station(struct net_device *dev, void *priv)
 	}
 }
 
+static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev)
+{
+	if ((skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) &&
+	    (skb->inner_protocol_type != ENCAP_TYPE_ETHER ||
+	     skb->inner_protocol != htons(ETH_P_TEB) ||
+	     skb_inner_mac_header(skb) - skb_transport_header(skb) != 16))
+		return false;
+
+	return true;
+}
+
 static const struct net_device_ops fm10k_netdev_ops = {
 	.ndo_open		= fm10k_open,
 	.ndo_stop		= fm10k_close,
@@ -1372,6 +1383,7 @@ static const struct net_device_ops fm10k_netdev_ops = {
 	.ndo_do_ioctl		= fm10k_ioctl,
 	.ndo_dfwd_add_station	= fm10k_dfwd_add_station,
 	.ndo_dfwd_del_station	= fm10k_dfwd_del_station,
+	.ndo_gso_check		= fm10k_gso_check,
 };
 
 #define DEFAULT_DEBUG_LEVEL_SHIFT 3
-- 
1.7.10.4

Re: [linux-nics] [PATCH net 3/5] fm10k: Implement ndo_gso_check()

[Jeff Kirsher]

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

On Tue, 2014-11-04 at 13:56 -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 something that looks
> like VXLAN.
> 
> Implementation shamelessly stolen from Tom Herbert:
> http://thread.gmane.org/gmane.linux.network/332428/focus=333111
> 
> Signed-off-by: Joe Stringer <joestringer@nicira.com>
> ---
> Should this driver report support for GSO on packets with tunnel
> headers
> up to 64B like the i40e driver does?
> ---
>  drivers/net/ethernet/intel/fm10k/fm10k_netdev.c |   12 ++++++++++++
>  1 file changed, 12 insertions(+)

Thanks Joe, I will add your patch to my queue.

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

Re: [linux-nics] [PATCH net 3/5] fm10k: Implement ndo_gso_check()

[Or Gerlitz]

On Wed, Nov 5, 2014 at 2:34 PM, Jeff Kirsher
<jeffrey.t.kirsher@intel.com> wrote:
> On Tue, 2014-11-04 at 13:56 -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 something that looks
>> like VXLAN.
>>
>> Implementation shamelessly stolen from Tom Herbert:
>> http://thread.gmane.org/gmane.linux.network/332428/focus=333111
>>
>> Signed-off-by: Joe Stringer <joestringer@nicira.com>
>> ---
>> Should this driver report support for GSO on packets with tunnel
>> headers
>> up to 64B like the i40e driver does?
>> ---
>>  drivers/net/ethernet/intel/fm10k/fm10k_netdev.c |   12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>
> Thanks Joe, I will add your patch to my queue.

Hi Jeff, please see my comment on patch 0/5, we're essentially
replicating the same helper four different times (fm10k, mlx4, benet,
qlgc) - I don't see the point in doing so. I asked Joe to come up with
one generic helper and then to pick it up by the four drivers, makes
sense?

Re: [linux-nics] [PATCH net 3/5] fm10k: Implement ndo_gso_check()

[Jeff Kirsher]

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

On Wed, 2014-11-05 at 14:44 +0200, Or Gerlitz wrote:
> On Wed, Nov 5, 2014 at 2:34 PM, Jeff Kirsher
> <jeffrey.t.kirsher@intel.com> wrote:
> > On Tue, 2014-11-04 at 13:56 -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 something that looks
> >> like VXLAN.
> >>
> >> Implementation shamelessly stolen from Tom Herbert:
> >> http://thread.gmane.org/gmane.linux.network/332428/focus=333111
> >>
> >> Signed-off-by: Joe Stringer <joestringer@nicira.com>
> >> ---
> >> Should this driver report support for GSO on packets with tunnel
> >> headers
> >> up to 64B like the i40e driver does?
> >> ---
> >>  drivers/net/ethernet/intel/fm10k/fm10k_netdev.c |   12 ++++++++++++
> >>  1 file changed, 12 insertions(+)
> >
> > Thanks Joe, I will add your patch to my queue.
> 
> Hi Jeff, please see my comment on patch 0/5, we're essentially
> replicating the same helper four different times (fm10k, mlx4, benet,
> qlgc) - I don't see the point in doing so. I asked Joe to come up with
> one generic helper and then to pick it up by the four drivers, makes
> sense?

Yeah, I just saw your reply Or.  Ok, I will await an update to Joe's
series, thanks!

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

Re: [linux-nics] [PATCH net 3/5] fm10k: Implement ndo_gso_check()

[Jeff Kirsher]

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

On Wed, 2014-11-05 at 10:26 -0800, Joe Stringer wrote:
> On 5 November 2014 04:47, Jeff Kirsher <jeffrey.t.kirsher@intel.com> wrote:
> >
> > On Wed, 2014-11-05 at 14:44 +0200, Or Gerlitz wrote:
> > > On Wed, Nov 5, 2014 at 2:34 PM, Jeff Kirsher
> > > <jeffrey.t.kirsher@intel.com> wrote:
> > > > On Tue, 2014-11-04 at 13:56 -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 something that looks
> > > >> like VXLAN.
> > > >>
> > > >> Implementation shamelessly stolen from Tom Herbert:
> > > >> http://thread.gmane.org/gmane.linux.network/332428/focus=333111
> > > >>
> > > >> Signed-off-by: Joe Stringer <joestringer@nicira.com>
> > > >> ---
> > > >> Should this driver report support for GSO on packets with tunnel
> > > >> headers
> > > >> up to 64B like the i40e driver does?
> > > >> ---
> > > >>  drivers/net/ethernet/intel/fm10k/fm10k_netdev.c |   12 ++++++++++++
> > > >>  1 file changed, 12 insertions(+)
> > > >
> > > > Thanks Joe, I will add your patch to my queue.
> > >
> > > Hi Jeff, please see my comment on patch 0/5, we're essentially
> > > replicating the same helper four different times (fm10k, mlx4, benet,
> > > qlgc) - I don't see the point in doing so. I asked Joe to come up with
> > > one generic helper and then to pick it up by the four drivers, makes
> > > sense?
> >
> > Yeah, I just saw your reply Or.  Ok, I will await an update to Joe's
> > series, thanks!
> 
> Thanks Or/Jeff.
> 
> There is also the question in the commit message above, perhaps fm10k
> support is a bit different - wasn't sure who to ask regarding that.

Matthew Vick is the fm10k maintainer now and can answer any fm10k
questions you may have.

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

Re: [linux-nics] [PATCH net 3/5] fm10k: Implement ndo_gso_check()

[Joe Stringer]

On Thu, Nov 06, 2014 at 11:58:32PM +0000, Vick, Matthew wrote:
> On 11/5/14, 11:36 AM, "Jeff Kirsher" <jeffrey.t.kirsher@intel.com> wrote:
>
> >On Wed, 2014-11-05 at 10:26 -0800, Joe Stringer wrote:
> >> On 5 November 2014 04:47, Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> >>wrote:
> >> >
> >> > On Wed, 2014-11-05 at 14:44 +0200, Or Gerlitz wrote:
> >> > > On Wed, Nov 5, 2014 at 2:34 PM, Jeff Kirsher
> >> > > <jeffrey.t.kirsher@intel.com> wrote:
> >> > > > On Tue, 2014-11-04 at 13:56 -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 something that
> >>looks
> >> > > >> like VXLAN.
> >> > > >>
> >> > > >> Implementation shamelessly stolen from Tom Herbert:
> >> > > >> http://thread.gmane.org/gmane.linux.network/332428/focus=333111
> >> > > >>
> >> > > >> Signed-off-by: Joe Stringer <joestringer@nicira.com>
> >> > > >> ---
> >> > > >> Should this driver report support for GSO on packets with tunnel
> >> > > >> headers
> >> > > >> up to 64B like the i40e driver does?
> >> > > >> ---
> >> > > >>  drivers/net/ethernet/intel/fm10k/fm10k_netdev.c |   12
> >>++++++++++++
> >> > > >>  1 file changed, 12 insertions(+)
> >> > > >
> >> > > > Thanks Joe, I will add your patch to my queue.
> >> > >
> >> > > Hi Jeff, please see my comment on patch 0/5, we're essentially
> >> > > replicating the same helper four different times (fm10k, mlx4,
> >>benet,
> >> > > qlgc) - I don't see the point in doing so. I asked Joe to come up
> >>with
> >> > > one generic helper and then to pick it up by the four drivers, makes
> >> > > sense?
> >> >
> >> > Yeah, I just saw your reply Or.  Ok, I will await an update to Joe's
> >> > series, thanks!
> >>
> >> Thanks Or/Jeff.
> >>
> >> There is also the question in the commit message above, perhaps fm10k
> >> support is a bit different - wasn't sure who to ask regarding that.
> >
> >Matthew Vick is the fm10k maintainer now and can answer any fm10k
> >questions you may have.
>
> Hi Joe, fm10k's hardware is pretty lax about the header size. As long as
> the total header length (outer+inner) is 184 bytes or less we're golden,
> so if I'm not mistaken that leaves us with a max of 130 bytes beyond the
> tunnel header.

Oh, okay. To be more explicit, in the case of UDP tunnels I take it that
you're talking about L2+L3+(L4+)tunnel+L2+L3+L4 <= 184? (L4 perhaps
optional depending on the tunnel protocol used)

In that case, the fm10k_gso_check would use something closer to
"skb_inner_transport_header(skb) - skb_mac_header(skb) > 184", or
perhaps 164 to allow for inner L4 header (?).

Joe

Re: [linux-nics] [PATCH net 3/5] fm10k: Implement ndo_gso_check()

[Joe Stringer]

On Fri, 07 Nov 2014 14:20:08 Vick, Matthew wrote:
> On 11/6/14, 4:55 PM, "Joe Stringer" <joestringer@nicira.com> wrote:
> >On Thu, Nov 06, 2014 at 11:58:32PM +0000, Vick, Matthew wrote:
> >> On 11/5/14, 11:36 AM, "Jeff Kirsher" <jeffrey.t.kirsher@intel.com>
> >>
> >>wrote:
> >> Hi Joe, fm10k's hardware is pretty lax about the header size. As long as
> >> the total header length (outer+inner) is 184 bytes or less we're golden,
> >> so if I'm not mistaken that leaves us with a max of 130 bytes beyond the
> >> tunnel header.
> >
> >Oh, okay. To be more explicit, in the case of UDP tunnels I take it that
> >you're talking about L2+L3+(L4+)tunnel+L2+L3+L4 <= 184? (L4 perhaps
> >optional depending on the tunnel protocol used)
> >
> >In that case, the fm10k_gso_check would use something closer to
> >"skb_inner_transport_header(skb) - skb_mac_header(skb) > 184", or
> >perhaps 164 to allow for inner L4 header (?).
> >
> >Joe
> 
> Yes, I'm talking about the full shebang.
> 
> I like the 164 check, personally (with appropriate #define for
> readability).

Thanks for the feedback, I take it that this approach is preferable over the 
other one involving an skb_gso_check() + fm10k_tx_encap_offload() call?

Re: [PATCH net 3/5] fm10k: Implement ndo_gso_check()

[Alexander Duyck]

On 11/04/2014 01:56 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 something that looks
> like VXLAN.
>
> Implementation shamelessly stolen from Tom Herbert:
> http://thread.gmane.org/gmane.linux.network/332428/focus=333111
>
> Signed-off-by: Joe Stringer <joestringer@nicira.com>
> ---
> Should this driver report support for GSO on packets with tunnel headers
> up to 64B like the i40e driver does?
> ---
>  drivers/net/ethernet/intel/fm10k/fm10k_netdev.c |   12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c b/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c
> index 8811364..b9ef622 100644
> --- a/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c
> +++ b/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c
> @@ -1350,6 +1350,17 @@ static void fm10k_dfwd_del_station(struct net_device *dev, void *priv)
>  	}
>  }
>  
> +static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev)
> +{
> +	if ((skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) &&
> +	    (skb->inner_protocol_type != ENCAP_TYPE_ETHER ||
> +	     skb->inner_protocol != htons(ETH_P_TEB) ||
> +	     skb_inner_mac_header(skb) - skb_transport_header(skb) != 16))
> +		return false;
> +
> +	return true;
> +}
> +
>  static const struct net_device_ops fm10k_netdev_ops = {
>  	.ndo_open		= fm10k_open,
>  	.ndo_stop		= fm10k_close,
> @@ -1372,6 +1383,7 @@ static const struct net_device_ops fm10k_netdev_ops = {
>  	.ndo_do_ioctl		= fm10k_ioctl,
>  	.ndo_dfwd_add_station	= fm10k_dfwd_add_station,
>  	.ndo_dfwd_del_station	= fm10k_dfwd_del_station,
> +	.ndo_gso_check		= fm10k_gso_check,
>  };
>  
>  #define DEFAULT_DEBUG_LEVEL_SHIFT 3

I'm thinking this check is far too simplistic.  If you look the fm10k
driver already has fm10k_tx_encap_offload() in the TSO function for
verifying if it can support offloading tunnels or not.  I would
recommend starting there or possibly even just adapting that function to
suit your purpose.

Thanks,

Alex

Re: [PATCH net 3/5] fm10k: Implement ndo_gso_check()

[Joe Stringer]

On Wed, Nov 05, 2014 at 06:54:00PM -0800, Alexander Duyck wrote:
> On 11/04/2014 01:56 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 something that looks
> > like VXLAN.
> >
> > Implementation shamelessly stolen from Tom Herbert:
> > http://thread.gmane.org/gmane.linux.network/332428/focus=333111
> >
> > Signed-off-by: Joe Stringer <joestringer@nicira.com>
> > ---
> > Should this driver report support for GSO on packets with tunnel headers
> > up to 64B like the i40e driver does?
> > ---
> >  drivers/net/ethernet/intel/fm10k/fm10k_netdev.c |   12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c b/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c
> > index 8811364..b9ef622 100644
> > --- a/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c
> > +++ b/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c
> > @@ -1350,6 +1350,17 @@ static void fm10k_dfwd_del_station(struct net_device *dev, void *priv)
> >  	}
> >  }
> >  
> > +static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev)
> > +{
> > +	if ((skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) &&
> > +	    (skb->inner_protocol_type != ENCAP_TYPE_ETHER ||
> > +	     skb->inner_protocol != htons(ETH_P_TEB) ||
> > +	     skb_inner_mac_header(skb) - skb_transport_header(skb) != 16))
> > +		return false;
> > +
> > +	return true;
> > +}
> > +
> >  static const struct net_device_ops fm10k_netdev_ops = {
> >  	.ndo_open		= fm10k_open,
> >  	.ndo_stop		= fm10k_close,
> > @@ -1372,6 +1383,7 @@ static const struct net_device_ops fm10k_netdev_ops = {
> >  	.ndo_do_ioctl		= fm10k_ioctl,
> >  	.ndo_dfwd_add_station	= fm10k_dfwd_add_station,
> >  	.ndo_dfwd_del_station	= fm10k_dfwd_del_station,
> > +	.ndo_gso_check		= fm10k_gso_check,
> >  };
> >  
> >  #define DEFAULT_DEBUG_LEVEL_SHIFT 3
> 
> I'm thinking this check is far too simplistic.  If you look the fm10k
> driver already has fm10k_tx_encap_offload() in the TSO function for
> verifying if it can support offloading tunnels or not.  I would
> recommend starting there or possibly even just adapting that function to
> suit your purpose.
> 
> Thanks,
> 
> Alex

Would it be enough to just call fm10k_tx_encap_offload() in a way that echoes fm10k_tso()?

+static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev)
+{
+       if (skb->encapsulation && !fm10k_tx_encap_offload(skb))
+               return false;
+
+       return true;
+}

Thanks,
Joe

Re: [PATCH net 3/5] fm10k: Implement ndo_gso_check()

[Joe Stringer]

On Thu, Nov 06, 2014 at 10:41:15AM -0800, Joe Stringer wrote:
> On Wed, Nov 05, 2014 at 06:54:00PM -0800, Alexander Duyck wrote:
> > On 11/04/2014 01:56 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 something that looks
> > > like VXLAN.
> > >
> > > Implementation shamelessly stolen from Tom Herbert:
> > > http://thread.gmane.org/gmane.linux.network/332428/focus=333111
> > >
> > > Signed-off-by: Joe Stringer <joestringer@nicira.com>
> > > ---
> > > Should this driver report support for GSO on packets with tunnel headers
> > > up to 64B like the i40e driver does?
> > > ---
> > >  drivers/net/ethernet/intel/fm10k/fm10k_netdev.c |   12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > >
> > > diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c b/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c
> > > index 8811364..b9ef622 100644
> > > --- a/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c
> > > +++ b/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c
> > > @@ -1350,6 +1350,17 @@ static void fm10k_dfwd_del_station(struct net_device *dev, void *priv)
> > >  	}
> > >  }
> > >  
> > > +static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev)
> > > +{
> > > +	if ((skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) &&
> > > +	    (skb->inner_protocol_type != ENCAP_TYPE_ETHER ||
> > > +	     skb->inner_protocol != htons(ETH_P_TEB) ||
> > > +	     skb_inner_mac_header(skb) - skb_transport_header(skb) != 16))
> > > +		return false;
> > > +
> > > +	return true;
> > > +}
> > > +
> > >  static const struct net_device_ops fm10k_netdev_ops = {
> > >  	.ndo_open		= fm10k_open,
> > >  	.ndo_stop		= fm10k_close,
> > > @@ -1372,6 +1383,7 @@ static const struct net_device_ops fm10k_netdev_ops = {
> > >  	.ndo_do_ioctl		= fm10k_ioctl,
> > >  	.ndo_dfwd_add_station	= fm10k_dfwd_add_station,
> > >  	.ndo_dfwd_del_station	= fm10k_dfwd_del_station,
> > > +	.ndo_gso_check		= fm10k_gso_check,
> > >  };
> > >  
> > >  #define DEFAULT_DEBUG_LEVEL_SHIFT 3
> > 
> > I'm thinking this check is far too simplistic.  If you look the fm10k
> > driver already has fm10k_tx_encap_offload() in the TSO function for
> > verifying if it can support offloading tunnels or not.  I would
> > recommend starting there or possibly even just adapting that function to
> > suit your purpose.
> > 
> > Thanks,
> > 
> > Alex
> 
> Would it be enough to just call fm10k_tx_encap_offload() in a way that echoes fm10k_tso()?
> 
> +static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev)
> +{
> +       if (skb->encapsulation && !fm10k_tx_encap_offload(skb))
> +               return false;
> +
> +       return true;
> +}

Oh, I suppose we need to check the gso_type too. More like this?

+static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev)
+{
+       if ((skb_shinfo(skb)->gso_type & (SKB_GSO_UDP_TUNNEL | SKB_GSO_GRE)) &&
+           !fm10k_tx_encap_offload(skb))
+               return false;
+
+       return true;
+}

Re: [PATCH net 3/5] fm10k: Implement ndo_gso_check()

[Vick, Matthew]

On 11/6/14, 1:15 PM, "Joe Stringer" <joestringer@nicira.com> wrote:

>Oh, I suppose we need to check the gso_type too. More like this?
>
>+static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev)
>+{
>+       if ((skb_shinfo(skb)->gso_type & (SKB_GSO_UDP_TUNNEL |
>SKB_GSO_GRE)) &&
>+           !fm10k_tx_encap_offload(skb))
>+               return false;
>+
>+       return true;
>+}

It seems like the skb_shinfo(skb)->gso_type check should be in some more
generic ndo_gso_check that drivers can default to/extend. Then, we could
end up with something like

static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev)
{
	if (skb_gso_check(skb, dev) && !fm10k_tx_encap_offload(skb))
		return false;

	return true;
}

This could even be simplified and still legible as

static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev)
{
	return !(skb_gso_check(skb, dev) && !fm10k_tx_encap_offload(skb));
}

What do you think of this approach?

Re: [PATCH net 3/5] fm10k: Implement ndo_gso_check()

[Joe Stringer]

On Fri, 07 Nov 2014 14:07:36 Vick, Matthew wrote:
> On 11/6/14, 1:15 PM, "Joe Stringer" <joestringer@nicira.com> wrote:
> >Oh, I suppose we need to check the gso_type too. More like this?
> >
> >+static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev)
> >+{
> >+       if ((skb_shinfo(skb)->gso_type & (SKB_GSO_UDP_TUNNEL |
> >SKB_GSO_GRE)) &&
> >+           !fm10k_tx_encap_offload(skb))
> >+               return false;
> >+
> >+       return true;
> >+}
> 
> It seems like the skb_shinfo(skb)->gso_type check should be in some more
> generic ndo_gso_check that drivers can default to/extend. Then, we could
> end up with something like
> 
> static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev)
> {
> 	if (skb_gso_check(skb, dev) && !fm10k_tx_encap_offload(skb))
> 		return false;
> 
> 	return true;
> }
> 
> This could even be simplified and still legible as
> 
> static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev)
> {
> 	return !(skb_gso_check(skb, dev) && !fm10k_tx_encap_offload(skb));
> }
> 
> What do you think of this approach?

Let's merge both discussions into one thread (pick here or there). We have 
this suggestion or the one which simply checks for tunnels and inner+outer 
header lengths. Do you have a preference between them?

We could introduce an "skb_is_gso_encap()" or similar for this purpose. 
Checking for SKB_GSO_UDP_TUNNEL or SKB_GSO_GRE is pretty closely tied to what 
fm10k_tx_encap_offload() checks for though; it might not even make sense to call 
it if some of the other SKB_GSO_* flags are raised.

Joe

Re: [PATCH net 3/5] fm10k: Implement ndo_gso_check()

[Vick, Matthew]

On 11/6/14, 9:05 PM, "Joe Stringer" <joestringer@nicira.com> wrote:

>Let's merge both discussions into one thread (pick here or there). We
>have 
>this suggestion or the one which simply checks for tunnels and
>inner+outer 
>header lengths. Do you have a preference between them?

Agreed with merging the thread--consider it merged!

Reflecting on this some more, I prefer the latter option (checking tunnels
and header lengths). I'm leaning towards pushing the header length check
into fm10k_tx_encap_offload and then making fm10k_gso_check call that with
the gso_type. So, it's really the most recent version of the patch you
proposed:

static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev)
{
	if ((skb_shinfo(skb)->gso_type & (SKB_GSO_UDP_TUNNEL | SKB_GSO_GRE)) &&
	    !fm10k_tx_encap_offload(skb))
		return false;

	return true;
}


plus the header length being checked in fm10k_tx_encap_offload. The only
nit would be that I'd just return the conditional instead of having
"return true" or "return false" lines.

The tunnel length check really should be there in fm10k_tx_encap_offload
anyway, so I'll get a patch together for that one.

>We could introduce an "skb_is_gso_encap()" or similar for this purpose.
>Checking for SKB_GSO_UDP_TUNNEL or SKB_GSO_GRE is pretty closely tied to
>what 
>fm10k_tx_encap_offload() checks for though; it might not even make sense
>to call 
>it if some of the other SKB_GSO_* flags are raised.

A fair point. On the other hand, we have to check the header length both
in the GSO and non-GSO cases anyway, so only having the check in
fm10k_tx_encap_offload and calling it from fm10k_gso_check wouldn't be as
duplicative. What do you think about that approach?

As an aside: the more I think about this, the more I think Tom's right and
that each driver really should have it's own ndo_gso_check() for this.
With fm10k and i40e being different, we're already at 40% of the current
drivers being different. I'll leave it to Or to comment on whether the
other drivers could share the check in some manner.

Cheers,
Matthew

Re: [PATCH net 3/5] fm10k: Implement ndo_gso_check()

[Joe Stringer]

On Friday, November 07, 2014 11:49:38 Vick, Matthew wrote:
> On 11/6/14, 9:05 PM, "Joe Stringer" <joestringer@nicira.com> wrote:
> >Let's merge both discussions into one thread (pick here or there). We
> >have
> >this suggestion or the one which simply checks for tunnels and
> >inner+outer
> >header lengths. Do you have a preference between them?
> 
> Agreed with merging the thread--consider it merged!
> 
> Reflecting on this some more, I prefer the latter option (checking tunnels
> and header lengths). I'm leaning towards pushing the header length check
> into fm10k_tx_encap_offload and then making fm10k_gso_check call that with
> the gso_type. So, it's really the most recent version of the patch you
> proposed:
> 
> static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev)
> {
> 	if ((skb_shinfo(skb)->gso_type & (SKB_GSO_UDP_TUNNEL | SKB_GSO_GRE)) &&
> 	    !fm10k_tx_encap_offload(skb))
> 		return false;
> 
> 	return true;
> }
> 
> 
> plus the header length being checked in fm10k_tx_encap_offload. The only
> nit would be that I'd just return the conditional instead of having
> "return true" or "return false" lines.

OK, that sounds reasonable.

> The tunnel length check really should be there in fm10k_tx_encap_offload
> anyway, so I'll get a patch together for that one.

Thanks.

> >We could introduce an "skb_is_gso_encap()" or similar for this purpose.
> >Checking for SKB_GSO_UDP_TUNNEL or SKB_GSO_GRE is pretty closely tied to
> >what
> >fm10k_tx_encap_offload() checks for though; it might not even make sense
> >to call
> >it if some of the other SKB_GSO_* flags are raised.
> 
> A fair point. On the other hand, we have to check the header length both
> in the GSO and non-GSO cases anyway, so only having the check in
> fm10k_tx_encap_offload and calling it from fm10k_gso_check wouldn't be as
> duplicative. What do you think about that approach?

Sure, I think fm10k_tx_encap_offload() is a good place for the header length 
check. Separately, my question above was regarding the idea of a helper for 
SKB_GSO_{GRE,UDP_TUNNEL}. The only reason it might be useful for the fm10k 
driver is because all encap is checked in the fm10k_tx_encap_offload() function. 
Other drivers don't seem to handle different tunnels together like this though, 
so I'm inclined to stick with the below for now.
                                                                                
static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev)        
{                                                                               
        return (!(skb_shinfo(skb)->gso_type &                                   
                  (SKB_GSO_UDP_TUNNEL | SKB_GSO_GRE)) ||                        
                fm10k_tx_encap_offload(skb));                                   
}

Cheers,
Joe

Re: [PATCH net 3/5] fm10k: Implement ndo_gso_check()

[Vick, Matthew]

On 11/7/14, 2:35 PM, "Joe Stringer" <joestringer@nicira.com> wrote:

>Sure, I think fm10k_tx_encap_offload() is a good place for the header
>length 
>check. Separately, my question above was regarding the idea of a helper
>for 
>SKB_GSO_{GRE,UDP_TUNNEL}. The only reason it might be useful for the
>fm10k 
>driver is because all encap is checked in the fm10k_tx_encap_offload()
>function. 
>Other drivers don't seem to handle different tunnels together like this
>though, 
>so I'm inclined to stick with the below for now.
>                  
>      
>static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev)
>      
>{                 
>      
>        return (!(skb_shinfo(skb)->gso_type &
>      
>                  (SKB_GSO_UDP_TUNNEL | SKB_GSO_GRE)) ||
>      
>                fm10k_tx_encap_offload(skb));
>      
>}
>
>Cheers,
>Joe

I agree. I think that makes the most sense.

Cheers,
Matthew

[PATCH net 4/5] net/mlx4_en: 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 something that looks
like VXLAN.

Implementation shamelessly stolen from Tom Herbert:
http://thread.gmane.org/gmane.linux.network/332428/focus=333111

Signed-off-by: Joe Stringer <joestringer@nicira.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
index f3032fe..aca9908 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
@@ -2344,6 +2344,17 @@ static void mlx4_en_del_vxlan_port(struct  net_device *dev,
 }
 #endif
 
+static bool mlx4_en_gso_check(struct sk_buff *skb, struct net_device *dev)
+{
+	if ((skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) &&
+	    (skb->inner_protocol_type != ENCAP_TYPE_ETHER ||
+	     skb->inner_protocol != htons(ETH_P_TEB) ||
+	     skb_inner_mac_header(skb) - skb_transport_header(skb) != 16))
+		return false;
+
+	return true;
+}
+
 static const struct net_device_ops mlx4_netdev_ops = {
 	.ndo_open		= mlx4_en_open,
 	.ndo_stop		= mlx4_en_close,
@@ -2374,6 +2385,7 @@ static const struct net_device_ops mlx4_netdev_ops = {
 	.ndo_add_vxlan_port	= mlx4_en_add_vxlan_port,
 	.ndo_del_vxlan_port	= mlx4_en_del_vxlan_port,
 #endif
+	.ndo_gso_check		= mlx4_en_gso_check,
 };
 
 static const struct net_device_ops mlx4_netdev_ops_master = {
-- 
1.7.10.4

Re: [PATCH net 4/5] net/mlx4_en: Implement ndo_gso_check()

[Or Gerlitz]

On Tue, Nov 4, 2014 at 11:56 PM, 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 something that looks
> like VXLAN.
>
> Implementation shamelessly stolen from Tom Herbert:
> http://thread.gmane.org/gmane.linux.network/332428/focus=333111
>
> Signed-off-by: Joe Stringer <joestringer@nicira.com>
> ---
>  drivers/net/ethernet/mellanox/mlx4/en_netdev.c |   12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> index f3032fe..aca9908 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> @@ -2344,6 +2344,17 @@ static void mlx4_en_del_vxlan_port(struct  net_device *dev,
>  }
>  #endif
>
> +static bool mlx4_en_gso_check(struct sk_buff *skb, struct net_device *dev)
> +{
> +       if ((skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) &&
> +           (skb->inner_protocol_type != ENCAP_TYPE_ETHER ||
> +            skb->inner_protocol != htons(ETH_P_TEB) ||
> +            skb_inner_mac_header(skb) - skb_transport_header(skb) != 16))
> +               return false;

Let's have this 16 constant to be more clear... e.g make it the sum of
sizeof struct udphdr and struct vxlanhdr - you would need to move the
latter from vxlan.c into a public header. All for the general patch I
suggested

Or.

> +
> +       return true;
> +}
> +
>  static const struct net_device_ops mlx4_netdev_ops = {
>         .ndo_open               = mlx4_en_open,
>         .ndo_stop               = mlx4_en_close,
> @@ -2374,6 +2385,7 @@ static const struct net_device_ops mlx4_netdev_ops = {
>         .ndo_add_vxlan_port     = mlx4_en_add_vxlan_port,
>         .ndo_del_vxlan_port     = mlx4_en_del_vxlan_port,
>  #endif
> +       .ndo_gso_check          = mlx4_en_gso_check,
>  };
>
>  static const struct net_device_ops mlx4_netdev_ops_master = {
> --
> 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

[PATCH net 5/5] qlcnic: 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 something that looks
like VXLAN.

Implementation shamelessly stolen from Tom Herbert:
http://thread.gmane.org/gmane.linux.network/332428/focus=333111

Signed-off-by: Joe Stringer <joestringer@nicira.com>
---
 drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
index f5e29f7..6184f47 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
@@ -505,6 +505,17 @@ static void qlcnic_del_vxlan_port(struct net_device *netdev,
 }
 #endif
 
+static bool qlcnic_gso_check(struct sk_buff *skb, struct net_device *dev)
+{
+	if ((skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) &&
+	    (skb->inner_protocol_type != ENCAP_TYPE_ETHER ||
+	     skb->inner_protocol != htons(ETH_P_TEB) ||
+	     skb_inner_mac_header(skb) - skb_transport_header(skb) != 16))
+		return false;
+
+	return true;
+}
+
 static const struct net_device_ops qlcnic_netdev_ops = {
 	.ndo_open	   = qlcnic_open,
 	.ndo_stop	   = qlcnic_close,
@@ -537,6 +548,7 @@ static const struct net_device_ops qlcnic_netdev_ops = {
 	.ndo_set_vf_vlan	= qlcnic_sriov_set_vf_vlan,
 	.ndo_set_vf_spoofchk	= qlcnic_sriov_set_vf_spoofchk,
 #endif
+	.ndo_gso_check		= qlcnic_gso_check,
 };
 
 static const struct net_device_ops qlcnic_netdev_failed_ops = {
-- 
1.7.10.4

RE: [PATCH net 5/5] qlcnic: Implement ndo_gso_check()

[Shahed Shaikh]

> -----Original Message-----
> From: Joe Stringer [mailto:joestringer@nicira.com]
> Sent: Wednesday, November 05, 2014 3:27 AM
> To: netdev
> Cc: sathya.perla@emulex.com; jeffrey.t.kirsher@intel.com;
> linux.nics@intel.com; amirv@mellanox.com; Shahed Shaikh; Dept-GE Linux
> NIC Dev; Tom Herbert (Partner - google); linux-kernel
> Subject: [PATCH net 5/5] qlcnic: Implement ndo_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 something that looks like VXLAN.
> 
> Implementation shamelessly stolen from Tom Herbert:
> http://thread.gmane.org/gmane.linux.network/332428/focus=333111
> 
> Signed-off-by: Joe Stringer <joestringer@nicira.com>
> ---
>  drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c |   12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
> b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
> index f5e29f7..6184f47 100644
> --- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
> +++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
> @@ -505,6 +505,17 @@ static void qlcnic_del_vxlan_port(struct net_device
> *netdev,  }  #endif
> 
> +static bool qlcnic_gso_check(struct sk_buff *skb, struct net_device
> +*dev) {
> +	if ((skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) &&
> +	    (skb->inner_protocol_type != ENCAP_TYPE_ETHER ||
> +	     skb->inner_protocol != htons(ETH_P_TEB) ||
> +	     skb_inner_mac_header(skb) - skb_transport_header(skb) != 16))
> +		return false;
> +

Hi Joe,

Yes, qlcnic driver only supports VXLAN offload.
It would be good to put a comment about value 16 to make it more intuitive. 
e.g. 16 is the size of outer UDP header + VXLAN header.

Anyway, patch looks good to me.

Acked-by: Shahed Shaikh <shahed.shaikh@qlogic.com>

Thanks,
Shahed

Re: [PATCH net 0/5] Implement ndo_gso_check() for vxlan nics

[Or Gerlitz]

On Tue, Nov 4, 2014 at 11:56 PM, Joe Stringer <joestringer@nicira.com> wrote:
> Most NICs that report NETIF_F_GSO_UDP_TUNNEL support VXLAN, and not other
> UDP-based encapsulation protocols where the format and size of the header may
> differ. This patch series implements ndo_gso_check() for these NICs,
> restricting the GSO handling to something that looks and smells like VXLAN.
>
> Implementation shamelessly stolen from Tom Herbert (with minor fixups):
> http://thread.gmane.org/gmane.linux.network/332428/focus=333111


Hi Joe,

1st, thanks for picking this task...2nd, for drivers that currently
support only pure VXLAN, I don't see the point
to replicate the helper suggested by Tom (good catch on the size check
to be 16 and not 12) four times and who know how more in the future.
Let's just have one generic helper and make the mlx4/be/fm10k/benet
drivers to have it as their ndo, OK?

Or.

>
> If there are particular differences for your driver on actual support, I'd like
> to hear about it. I adjusted the i40e driver to report support with tunnel
> headers of up to 64 octets, perhaps there are other specifics that I've missed.
>
> Joe Stringer (5):
>   be2net: Implement ndo_gso_check()
>   i40e: Implement ndo_gso_check()
>   fm10k: Implement ndo_gso_check()
>   net/mlx4_en: Implement ndo_gso_check()
>   qlcnic: Implement ndo_gso_check()
>
>  drivers/net/ethernet/emulex/benet/be_main.c      |   12 ++++++++++++
>  drivers/net/ethernet/intel/fm10k/fm10k_netdev.c  |   12 ++++++++++++
>  drivers/net/ethernet/intel/i40e/i40e_main.c      |   14 +++++++++++++-
>  drivers/net/ethernet/mellanox/mlx4/en_netdev.c   |   12 ++++++++++++
>  drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c |   12 ++++++++++++
>  5 files changed, 61 insertions(+), 1 deletion(-)
>
> --
> 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

Re: [PATCH net 0/5] Implement ndo_gso_check() for vxlan nics

[Tom Herbert]

On Wed, Nov 5, 2014 at 9:50 AM, Joe Stringer <joestringer@nicira.com> wrote:
>
> On 5 November 2014 04:38, Or Gerlitz <gerlitz.or@gmail.com> wrote:
>>
>> On Tue, Nov 4, 2014 at 11:56 PM, Joe Stringer <joestringer@nicira.com> wrote:
>> > Most NICs that report NETIF_F_GSO_UDP_TUNNEL support VXLAN, and not other
>> > UDP-based encapsulation protocols where the format and size of the header may
>> > differ. This patch series implements ndo_gso_check() for these NICs,
>> > restricting the GSO handling to something that looks and smells like VXLAN.
>> >
>> > Implementation shamelessly stolen from Tom Herbert (with minor fixups):
>> > http://thread.gmane.org/gmane.linux.network/332428/focus=333111
>>
>>
>> Hi Joe,
>>
>> 1st, thanks for picking this task...2nd, for drivers that currently
>> support only pure VXLAN, I don't see the point
>> to replicate the helper suggested by Tom (good catch on the size check
>> to be 16 and not 12) four times and who know how more in the future.
>> Let's just have one generic helper and make the mlx4/be/fm10k/benet
>> drivers to have it as their ndo, OK?
>
>
> Thanks for taking a look.
>
> I had debated whether to do this or not as the actual support on each NIC may differ, and each implementation may morph over time to match these capabilities better. Obviously the vendors will know better than me on this, so I'm posing this series to prod them for more information. At this point I've had just one maintainer come back and confirm that this helper is a good fit for their hardware, so I'd like to confirm that multiple drivers will use a ndo_gso_check_vxlan_helper() function before I go and create it.


Thanks for implementing this fix!

Personally, I would rather not have the helper. This is already a
small number of drivers, and each driver owner should consider what
limitations are of their device and try to enable to allow the maximum
number of use cases possible. I'm also hoping that new devices will
implement the more generic mechanism so that VXLAN is just one
supported protocol.

Re: [PATCH net 0/5] Implement ndo_gso_check() for vxlan nics

[Or Gerlitz]

On Wed, Nov 5, 2014 at 8:00 PM, Tom Herbert <therbert@google.com> wrote:
> On Wed, Nov 5, 2014 at 9:50 AM, Joe Stringer <joestringer@nicira.com> wrote:
>>
>> On 5 November 2014 04:38, Or Gerlitz <gerlitz.or@gmail.com> wrote:
>>>
>>> On Tue, Nov 4, 2014 at 11:56 PM, Joe Stringer <joestringer@nicira.com> wrote:
>>> > Most NICs that report NETIF_F_GSO_UDP_TUNNEL support VXLAN, and not other
>>> > UDP-based encapsulation protocols where the format and size of the header may
>>> > differ. This patch series implements ndo_gso_check() for these NICs,
>>> > restricting the GSO handling to something that looks and smells like VXLAN.
>>> >
>>> > Implementation shamelessly stolen from Tom Herbert (with minor fixups):
>>> > http://thread.gmane.org/gmane.linux.network/332428/focus=333111
>>>
>>>
>>> Hi Joe,
>>>
>>> 1st, thanks for picking this task...2nd, for drivers that currently
>>> support only pure VXLAN, I don't see the point
>>> to replicate the helper suggested by Tom (good catch on the size check
>>> to be 16 and not 12) four times and who know how more in the future.
>>> Let's just have one generic helper and make the mlx4/be/fm10k/benet
>>> drivers to have it as their ndo, OK?
>>
>>
>> Thanks for taking a look.
>>
>> I had debated whether to do this or not as the actual support on each NIC may differ, and each implementation may morph over time to match these capabilities better. Obviously the vendors will know better than me on this, so I'm posing this series to prod them for more information. At this point I've had just one maintainer come back and confirm that this helper is a good fit for their hardware, so I'd like to confirm that multiple drivers will use a ndo_gso_check_vxlan_helper() function before I go and create it.
>
>
> Thanks for implementing this fix!
>
> Personally, I would rather not have the helper. This is already a
> small number of drivers, and each driver owner should consider what
> limitations are of their device and try to enable to allow the maximum
> number of use cases possible. I'm also hoping that new devices will
> implement the more generic mechanism so that VXLAN is just one
> supported protocol.

but fact is that the proposed patch series has the --same-- helper for
four drivers, so why not start with a that limited helper which would
be picked up by these drivers and we'll take it from there.

Re: [PATCH net 0/5] Implement ndo_gso_check() for vxlan nics

[David Miller]

From: Or Gerlitz <gerlitz.or@gmail.com>
Date: Wed, 5 Nov 2014 23:32:44 +0200

> but fact is that the proposed patch series has the --same-- helper for
> four drivers, so why not start with a that limited helper which would
> be picked up by these drivers and we'll take it from there.

I'm in favor of the helper, duplication is error prone.

And in fact, any differences a driver ends up needing might be
integratable into the helper.

Re: [PATCH net 0/5] Implement ndo_gso_check() for vxlan nics

[Joe Stringer]

On Wed, Nov 05, 2014 at 04:38:25PM -0500, David Miller wrote:
> From: Or Gerlitz <gerlitz.or@gmail.com>
> Date: Wed, 5 Nov 2014 23:32:44 +0200
> 
> > but fact is that the proposed patch series has the --same-- helper for
> > four drivers, so why not start with a that limited helper which would
> > be picked up by these drivers and we'll take it from there.
> 
> I'm in favor of the helper, duplication is error prone.
> 
> And in fact, any differences a driver ends up needing might be
> integratable into the helper.

My impression was that the changes are more likely to be
hardware-specific (like the i40e changes) rather than software-specific,
like changes that might be integrated into the helper.

That said, I can rework for one helper. The way I see it would be the
same code as these patches, as "vxlan_gso_check(struct sk_buff *)" in
drivers/net/vxlan.c which would be called from each driver. Is that what
you had in mind?

Re: [PATCH net 0/5] Implement ndo_gso_check() for vxlan nics

[David Miller]

From: Joe Stringer <joestringer@nicira.com>
Date: Wed, 5 Nov 2014 17:06:46 -0800

> My impression was that the changes are more likely to be
> hardware-specific (like the i40e changes) rather than software-specific,
> like changes that might be integrated into the helper.

I think there is more commonality amongst hardware capabilities,
and this is why I want the helper to play itself out.

> That said, I can rework for one helper. The way I see it would be the
> same code as these patches, as "vxlan_gso_check(struct sk_buff *)" in
> drivers/net/vxlan.c which would be called from each driver. Is that what
> you had in mind?

Yes.

Re: [PATCH net 0/5] Implement ndo_gso_check() for vxlan nics

[Tom Herbert]

On Wed, Nov 5, 2014 at 6:15 PM, David Miller <davem@davemloft.net> wrote:
> From: Joe Stringer <joestringer@nicira.com>
> Date: Wed, 5 Nov 2014 17:06:46 -0800
>
>> My impression was that the changes are more likely to be
>> hardware-specific (like the i40e changes) rather than software-specific,
>> like changes that might be integrated into the helper.
>
> I think there is more commonality amongst hardware capabilities,
> and this is why I want the helper to play itself out.
>
>> That said, I can rework for one helper. The way I see it would be the
>> same code as these patches, as "vxlan_gso_check(struct sk_buff *)" in
>> drivers/net/vxlan.c which would be called from each driver. Is that what
>> you had in mind?
>
> Yes.

Note that this code is not VXLAN specific, it will also accept NVGRE
and GRE/UDP with keyid and TEB. I imagine all these cases should be
indistinguishable to the hardware so they probably just work (which
would be cool!). It might be better to name and locate the helper
function to reflect that.

Re: [PATCH net 0/5] Implement ndo_gso_check() for vxlan nics

[Or Gerlitz]

On Thu, Nov 6, 2014 at 4:44 AM, Tom Herbert <therbert@google.com> wrote:
> On Wed, Nov 5, 2014 at 6:15 PM, David Miller <davem@davemloft.net> wrote:
>> From: Joe Stringer <joestringer@nicira.com>
>> Date: Wed, 5 Nov 2014 17:06:46 -0800
>>
>>> My impression was that the changes are more likely to be
>>> hardware-specific (like the i40e changes) rather than software-specific,
>>> like changes that might be integrated into the helper.
>>
>> I think there is more commonality amongst hardware capabilities,
>> and this is why I want the helper to play itself out.
>>
>>> That said, I can rework for one helper. The way I see it would be the
>>> same code as these patches, as "vxlan_gso_check(struct sk_buff *)" in
>>> drivers/net/vxlan.c which would be called from each driver. Is that what
>>> you had in mind?
>>
>> Yes.
>
> Note that this code is not VXLAN specific, it will also accept NVGRE
> and GRE/UDP with keyid and TEB. I imagine all these cases should be
> indistinguishable to the hardware so they probably just work (which
> would be cool!). It might be better to name and locate the helper
> function to reflect that.

Unlike the VXLAN case, currently there's no indication from the
network stack towards the driver that an NVGRE tunnel was set, so in
our case we're not arming the HW offloads for NVGRE. I'll look into
that, maybe we can make them work always. Also re the math there to be
the same for VXLAN/NVGRE  -- skb_inner_mac_header(skb) -
skb_transport_header(skb) is exactly 8 (sizeof struct gre_base_hdr),
isn't that?

Re: [PATCH net 0/5] Implement ndo_gso_check() for vxlan nics

[Tom Herbert]

The inner headers are reset in iptunnel_handle_offloads. This called
in the xmit encapsulation function (GRE, fou, VXLAN, etc.) before
added in encapsulation headers, so the inner headers will point to the
encapsulation payload, i.e. the encapsulated packet. The headers are
only on the first encapsulation, so with nested tunnels the inner
headers point to encapsulated packet. Since VXLAN and NVGRE have same
size of encapsulation (8 UDP + 8 header),   skb_inner_mac_header(skb)
- skb_transport_header(skb) should always be 16.


On Wed, Nov 5, 2014 at 10:16 PM, Sathya Perla <Sathya.Perla@emulex.com> wrote:
>> -----Original Message-----
>> From: Tom Herbert [mailto:therbert@google.com]
>>
>> On Wed, Nov 5, 2014 at 6:15 PM, David Miller <davem@davemloft.net>
>> wrote:
>> > From: Joe Stringer <joestringer@nicira.com>
>> > Date: Wed, 5 Nov 2014 17:06:46 -0800
>> >
>> >> My impression was that the changes are more likely to be
>> >> hardware-specific (like the i40e changes) rather than software-specific,
>> >> like changes that might be integrated into the helper.
>> >
>> > I think there is more commonality amongst hardware capabilities,
>> > and this is why I want the helper to play itself out.
>> >
>> >> That said, I can rework for one helper. The way I see it would be the
>> >> same code as these patches, as "vxlan_gso_check(struct sk_buff *)" in
>> >> drivers/net/vxlan.c which would be called from each driver. Is that what
>> >> you had in mind?
>> >
>> > Yes.
>>
>> Note that this code is not VXLAN specific, it will also accept NVGRE
>> and GRE/UDP with keyid and TEB. I imagine all these cases should be
>> indistinguishable to the hardware so they probably just work (which
>> would be cool!). It might be better to name and locate the helper
>> function to reflect that.
>
> Tom, I'm confused as to how the value of (skb_inner_mac_header(skb) - skb_transport_header(skb))
> would be the same for VxLAN and NVGRE encapsulated packets. Wouldn't this value be 16 for VxLAN
> and 8 for NVGRE?
>
The inner headers are reset in iptunnel_handle_offloads. This is
called in the xmit encapsulation functions (GRE, fou, VXLAN, etc.)
before adding in encapsulation headers (skb_push), so the
mac_inner_header will point to the encapsulation payload, i.e. the
encapsulated packet. This should not change after being set, although
inner network and inner transport can. The headers are only set on the
first encapsulation, so with nested tunnels the inner headers point to
the innermost encapsulated packet. Since VXLAN and NVGRE have same
size of encapsulation (8 UDP + 8 header),   skb_inner_mac_header(skb)
- skb_transport_header(skb) should always be 16.

Tom

> thks,
> -Sathya

Re: [PATCH net 0/5] Implement ndo_gso_check() for vxlan nics

[Jesse Gross]

On Thu, Nov 6, 2014 at 8:06 AM, Tom Herbert <therbert@google.com> wrote:
> On Wed, Nov 5, 2014 at 10:16 PM, Sathya Perla <Sathya.Perla@emulex.com> wrote:
>>> -----Original Message-----
>>> From: Tom Herbert [mailto:therbert@google.com]
>>>
>>> On Wed, Nov 5, 2014 at 6:15 PM, David Miller <davem@davemloft.net>
>>> wrote:
>>> > From: Joe Stringer <joestringer@nicira.com>
>>> > Date: Wed, 5 Nov 2014 17:06:46 -0800
>>> >
>>> >> My impression was that the changes are more likely to be
>>> >> hardware-specific (like the i40e changes) rather than software-specific,
>>> >> like changes that might be integrated into the helper.
>>> >
>>> > I think there is more commonality amongst hardware capabilities,
>>> > and this is why I want the helper to play itself out.
>>> >
>>> >> That said, I can rework for one helper. The way I see it would be the
>>> >> same code as these patches, as "vxlan_gso_check(struct sk_buff *)" in
>>> >> drivers/net/vxlan.c which would be called from each driver. Is that what
>>> >> you had in mind?
>>> >
>>> > Yes.
>>>
>>> Note that this code is not VXLAN specific, it will also accept NVGRE
>>> and GRE/UDP with keyid and TEB. I imagine all these cases should be
>>> indistinguishable to the hardware so they probably just work (which
>>> would be cool!). It might be better to name and locate the helper
>>> function to reflect that.
>>
>> Tom, I'm confused as to how the value of (skb_inner_mac_header(skb) - skb_transport_header(skb))
>> would be the same for VxLAN and NVGRE encapsulated packets. Wouldn't this value be 16 for VxLAN
>> and 8 for NVGRE?
>>
> The inner headers are reset in iptunnel_handle_offloads. This is
> called in the xmit encapsulation functions (GRE, fou, VXLAN, etc.)
> before adding in encapsulation headers (skb_push), so the
> mac_inner_header will point to the encapsulation payload, i.e. the
> encapsulated packet. This should not change after being set, although
> inner network and inner transport can. The headers are only set on the
> first encapsulation, so with nested tunnels the inner headers point to
> the innermost encapsulated packet. Since VXLAN and NVGRE have same
> size of encapsulation (8 UDP + 8 header),   skb_inner_mac_header(skb)
> - skb_transport_header(skb) should always be 16.

Tom, NVGRE is not encapsulated in UDP and it is not 16 bytes.

http://tools.ietf.org/html/draft-sridharan-virtualization-nvgre-06