LinuxLists.cc - [PATCH net 0/5] Implement ndo_gso

[permalink] [raw]

Subject: [PATCH net 4/5] net/mlx4_en: 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 <[email protected]>
---
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

2014-11-04 22:03:47

[permalink] [raw]

Subject: [PATCH net 1/5] be2net: 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 <[email protected]>
---
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

2014-11-04 22:03:57

[permalink] [raw]

Subject: [PATCH net 2/5] i40e: 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 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 <[email protected]>
---
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

2014-11-04 22:04:07

[permalink] [raw]

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 <[email protected]>
---
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

2014-11-04 22:04:27

[permalink] [raw]

Subject: [PATCH net 3/5] fm10k: 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 <[email protected]>
---
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

2014-11-04 23:53:22

by Jesse Gross

[permalink] [raw]

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

On Tue, Nov 4, 2014 at 1:56 PM, Joe Stringer <[email protected]> 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?

2014-11-05 09:17:41

by Shahed Shaikh

[permalink] [raw]

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

> -----Original Message-----
> From: Joe Stringer [mailto:[email protected]]
> Sent: Wednesday, November 05, 2014 3:27 AM
> To: netdev
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; 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 <[email protected]>
> ---
> 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 <[email protected]>

Thanks,
Shahed

2014-11-05 12:34:38

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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.

Attachments:

signature.asc (819.00 B)
This is a digitally signed message part

2014-11-05 12:35:29

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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.

Attachments:

signature.asc (819.00 B)
This is a digitally signed message part

2014-11-05 12:37:16

[permalink] [raw]

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

On Tue, 2014-11-04 at 15:45 -0800, Jesse Gross wrote:
> On Tue, Nov 4, 2014 at 1:56 PM, Joe Stringer <[email protected]> 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.

Attachments:

signature.asc (819.00 B)
This is a digitally signed message part

2014-11-05 12:38:02

[permalink] [raw]

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

On Tue, Nov 4, 2014 at 11:56 PM, Joe Stringer <[email protected]> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2014-11-05 12:42:00

[permalink] [raw]

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

On Tue, Nov 4, 2014 at 11:56 PM, 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 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 <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2014-11-05 12:44:47

[permalink] [raw]

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

On Wed, Nov 5, 2014 at 2:34 PM, Jeff Kirsher
<[email protected]> 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 <[email protected]>
>> ---
>> 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?

2014-11-05 12:47:10

[permalink] [raw]

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

On Wed, 2014-11-05 at 14:44 +0200, Or Gerlitz wrote:
> On Wed, Nov 5, 2014 at 2:34 PM, Jeff Kirsher
> <[email protected]> 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 <[email protected]>
> >> ---
> >> 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!

Attachments:

signature.asc (819.00 B)
This is a digitally signed message part

2014-11-05 18:00:57

by Tom Herbert

[permalink] [raw]

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

On Wed, Nov 5, 2014 at 9:50 AM, Joe Stringer <[email protected]> wrote:
>
> On 5 November 2014 04:38, Or Gerlitz <[email protected]> wrote:
>>
>> On Tue, Nov 4, 2014 at 11:56 PM, Joe Stringer <[email protected]> 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.

2014-11-05 21:32:47

[permalink] [raw]

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

On Wed, Nov 5, 2014 at 8:00 PM, Tom Herbert <[email protected]> wrote:
> On Wed, Nov 5, 2014 at 9:50 AM, Joe Stringer <[email protected]> wrote:
>>
>> On 5 November 2014 04:38, Or Gerlitz <[email protected]> wrote:
>>>
>>> On Tue, Nov 4, 2014 at 11:56 PM, Joe Stringer <[email protected]> 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.

2014-11-05 21:38:33

by David Miller

[permalink] [raw]

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

From: Or Gerlitz <[email protected]>
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.

2014-11-06 01:06:58

[permalink] [raw]

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

On Wed, Nov 05, 2014 at 04:38:25PM -0500, David Miller wrote:
> From: Or Gerlitz <[email protected]>
> 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?

2014-11-06 02:16:08

by David Miller

[permalink] [raw]

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

From: Joe Stringer <[email protected]>
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.

2014-11-06 02:44:10

by Tom Herbert

[permalink] [raw]

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

On Wed, Nov 5, 2014 at 6:15 PM, David Miller <[email protected]> wrote:
> From: Joe Stringer <[email protected]>
> 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.

2014-11-06 02:54:05

by Alexander Duyck

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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

2014-11-06 08:23:33

[permalink] [raw]

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

On Thu, Nov 6, 2014 at 4:44 AM, Tom Herbert <[email protected]> wrote:
> On Wed, Nov 5, 2014 at 6:15 PM, David Miller <[email protected]> wrote:
>> From: Joe Stringer <[email protected]>
>> 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?

2014-11-06 16:06:11

by Tom Herbert

[permalink] [raw]

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

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 <[email protected]> wrote:
>> -----Original Message-----
>> From: Tom Herbert [mailto:[email protected]]
>>
>> On Wed, Nov 5, 2014 at 6:15 PM, David Miller <[email protected]>
>> wrote:
>> > From: Joe Stringer <[email protected]>
>> > 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

2014-11-06 16:22:33

by Jesse Gross

[permalink] [raw]

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

On Thu, Nov 6, 2014 at 8:06 AM, Tom Herbert <[email protected]> wrote:
> On Wed, Nov 5, 2014 at 10:16 PM, Sathya Perla <[email protected]> wrote:
>>> -----Original Message-----
>>> From: Tom Herbert [mailto:[email protected]]
>>>
>>> On Wed, Nov 5, 2014 at 6:15 PM, David Miller <[email protected]>
>>> wrote:
>>> > From: Joe Stringer <[email protected]>
>>> > 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

2014-11-06 18:48:27

[permalink] [raw]

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

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 <[email protected]>
> > ---
> > 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

2014-11-06 21:15:41

[permalink] [raw]

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

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 <[email protected]>
> > > ---
> > > 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;
+}

2014-11-07 01:07:54

by Vick, Matthew

[permalink] [raw]

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

On 11/6/14, 1:15 PM, "Joe Stringer" <[email protected]> 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?

2014-11-07 05:05:31

[permalink] [raw]

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

On Fri, 07 Nov 2014 14:07:36 Vick, Matthew wrote:
> On 11/6/14, 1:15 PM, "Joe Stringer" <[email protected]> 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

2014-11-07 19:50:30

by Vick, Matthew

[permalink] [raw]

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

On 11/6/14, 9:05 PM, "Joe Stringer" <[email protected]> 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

2014-11-07 23:11:23

[permalink] [raw]

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

On Friday, November 07, 2014 11:49:38 Vick, Matthew wrote:
> On 11/6/14, 9:05 PM, "Joe Stringer" <[email protected]> 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

2014-11-08 00:51:17

by Vick, Matthew

[permalink] [raw]

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

On 11/7/14, 2:35 PM, "Joe Stringer" <[email protected]> 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

2014-11-20 19:16:10