[PATCH] bonding: allow TSO being set on bonding master

[PATCH] bonding: allow TSO being set on bonding master

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

In some situations, we need to disable TSO on bonding slaves.

bonding device automatically unset TSO in bond_fix_features(), and
performance is not good because :

1) We consume more cpu cycles.

2) GSO segmentation has some bugs leading to out of order TCP packets
if this segmentation is done before virtual device. This particular
problem will be addressed in a separate patch.

This patch allows TSO being set/unset on the bonding master,
so that GSO segmentation is done after bonding layer.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Jay Vosburgh <fubar@us.ibm.com>
Cc: Andy Gospodarek <andy@greyhouse.net>
Cc: Maciej Żenczykowski <maze@google.com>
Cc: Tom Herbert <therbert@google.com>
Cc: Neal Cardwell <ncardwell@google.com>
Cc: Yuchung Cheng <ycheng@google.com>
---
 drivers/net/bonding/bond_main.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index d0aade0..b429bb8 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1362,6 +1362,11 @@ static netdev_features_t bond_fix_features(struct net_device *dev,
 						     slave->dev->features,
 						     mask);
 	}
+	/* Allow TSO being used on bonding device :
+	 * Performing the GSO segmentation after bonding transmit
+	 * is a performance improvement.
+	 */
+	features |= mask & NETIF_F_ALL_TSO;
 
 out:
 	read_unlock(&bond->lock);

Re: [PATCH] bonding: allow TSO being set on bonding master

[Michał Mirosław]

2013/5/15 Eric Dumazet <eric.dumazet@gmail.com>:
> From: Eric Dumazet <edumazet@google.com>
>
> In some situations, we need to disable TSO on bonding slaves.
>
> bonding device automatically unset TSO in bond_fix_features(), and
> performance is not good because :
>
> 1) We consume more cpu cycles.
>
> 2) GSO segmentation has some bugs leading to out of order TCP packets
> if this segmentation is done before virtual device. This particular
> problem will be addressed in a separate patch.
>
> This patch allows TSO being set/unset on the bonding master,
> so that GSO segmentation is done after bonding layer.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Jay Vosburgh <fubar@us.ibm.com>
> Cc: Andy Gospodarek <andy@greyhouse.net>
> Cc: Maciej Żenczykowski <maze@google.com>
> Cc: Tom Herbert <therbert@google.com>
> Cc: Neal Cardwell <ncardwell@google.com>
> Cc: Yuchung Cheng <ycheng@google.com>
> ---
>  drivers/net/bonding/bond_main.c |    5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index d0aade0..b429bb8 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1362,6 +1362,11 @@ static netdev_features_t bond_fix_features(struct net_device *dev,
>                                                      slave->dev->features,
>                                                      mask);
>         }
> +       /* Allow TSO being used on bonding device :
> +        * Performing the GSO segmentation after bonding transmit
> +        * is a performance improvement.
> +        */
> +       features |= mask & NETIF_F_ALL_TSO;
>
>  out:
>         read_unlock(&bond->lock);

Have you tried adding it to NETIF_F_ONE_FOR_ALL set? 'Team' and bridge
could then also use it.

Best Regards,
Michał Mirosław

Re: [PATCH] bonding: allow TSO being set on bonding master

[Eric Dumazet]

On Thu, 2013-05-16 at 00:55 +0200, Michał Mirosław wrote:

> Have you tried adding it to NETIF_F_ONE_FOR_ALL set? 'Team' and bridge
> could then also use it.

Good point, I am testing this, thanks !

Re: [PATCH] bonding: allow TSO being set on bonding master

[Eric Dumazet]

On Wed, 2013-05-15 at 16:18 -0700, Eric Dumazet wrote:
> On Thu, 2013-05-16 at 00:55 +0200, Michał Mirosław wrote:
> 
> > Have you tried adding it to NETIF_F_ONE_FOR_ALL set? 'Team' and bridge
> > could then also use it.
> 
> Good point, I am testing this, thanks !
> 

Hmm, this doesnt work.


# ethtool -k bond0 | grep tcp-segmentation-offload
tcp-segmentation-offload: off
# ethtool -K bond0 tso on
# ethtool -k bond0 | grep tcp-segmentation-offload
tcp-segmentation-offload: off

Do you have something different in mind ?

diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
index 77f5202..c8d9db0 100644
--- a/include/linux/netdev_features.h
+++ b/include/linux/netdev_features.h
@@ -132,7 +132,7 @@ enum {
  * for all in netdev_increment_features.
  */
 #define NETIF_F_ONE_FOR_ALL    (NETIF_F_GSO_SOFTWARE | NETIF_F_GSO_ROBUST | \
-                                NETIF_F_SG | NETIF_F_HIGHDMA |         \
+                                NETIF_F_SG | NETIF_F_HIGHDMA | NETIF_F_ALL_TSO |       \
                                 NETIF_F_FRAGLIST | NETIF_F_VLAN_CHALLENGED)
 /*
  * If one device doesn't support one of these features, then disable it

Re: [PATCH] bonding: allow TSO being set on bonding master

[Maciej Żenczykowski]

I think it should always be possible to enable both TSO and GSO on the
bond master - regardless of what the slaves support and/or currently
have enabled.
At the last moment as we're passing a packet to a slave, we should
check whether the slave will like it and if not GSO it.
This means TSO or GSO enabled on the master can effectively almost
force (or force emulation of) GSO on slaves.
Even if the slaves themselves don't support GSO.

There's a lot more room for argument with regards to the default state
of TSO/GSO on the master.
I would argue for default to on for both.
One could argue that if none of the slaves support GSO or TSO then
maybe it should default to off, but I don't buy that.

I don't really understand the point of passing GSO/TSO up from the
slaves to the master.
Although the maximum size and/or number of segs for a packet probably
needs to be propagated from TSO capable devices,
unless one were to add a "split big GSO packet into smaller (but still
larger than mtu) TSO packets" step as well.

On Wed, May 15, 2013 at 4:30 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Wed, 2013-05-15 at 16:18 -0700, Eric Dumazet wrote:
>> On Thu, 2013-05-16 at 00:55 +0200, Michał Mirosław wrote:
>>
>> > Have you tried adding it to NETIF_F_ONE_FOR_ALL set? 'Team' and bridge
>> > could then also use it.
>>
>> Good point, I am testing this, thanks !
>>
>
> Hmm, this doesnt work.
>
>
> # ethtool -k bond0 | grep tcp-segmentation-offload
> tcp-segmentation-offload: off
> # ethtool -K bond0 tso on
> # ethtool -k bond0 | grep tcp-segmentation-offload
> tcp-segmentation-offload: off
>
> Do you have something different in mind ?
>
> diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
> index 77f5202..c8d9db0 100644
> --- a/include/linux/netdev_features.h
> +++ b/include/linux/netdev_features.h
> @@ -132,7 +132,7 @@ enum {
>   * for all in netdev_increment_features.
>   */
>  #define NETIF_F_ONE_FOR_ALL    (NETIF_F_GSO_SOFTWARE | NETIF_F_GSO_ROBUST | \
> -                                NETIF_F_SG | NETIF_F_HIGHDMA |         \
> +                                NETIF_F_SG | NETIF_F_HIGHDMA | NETIF_F_ALL_TSO |       \
>                                  NETIF_F_FRAGLIST | NETIF_F_VLAN_CHALLENGED)
>  /*
>   * If one device doesn't support one of these features, then disable it
>
>
> --
> 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] bonding: allow TSO being set on bonding master

[Eric Dumazet]

Please do not top post on netdev.

On Wed, 2013-05-15 at 21:31 -0700, Maciej Żenczykowski wrote:
> I think it should always be possible to enable both TSO and GSO on the
> bond master - regardless of what the slaves support and/or currently
> have enabled.
> At the last moment as we're passing a packet to a slave, we should
> check whether the slave will like it and if not GSO it.
> This means TSO or GSO enabled on the master can effectively almost
> force (or force emulation of) GSO on slaves.
> Even if the slaves themselves don't support GSO.

GSO is software provided in core network.

> 
> There's a lot more room for argument with regards to the default state
> of TSO/GSO on the master.
> I would argue for default to on for both.
> One could argue that if none of the slaves support GSO or TSO then
> maybe it should default to off, but I don't buy that.
> 
> I don't really understand the point of passing GSO/TSO up from the
> slaves to the master.
> Although the maximum size and/or number of segs for a packet probably
> needs to be propagated from TSO capable devices,
> unless one were to add a "split big GSO packet into smaller (but still
> larger than mtu) TSO packets" step as well.

We could always build GSO packets in TCP stack and if these packets land
on a non SG/TSO capable device, segment them, but it would be more
expensive than building non GSO packets at the beginning.
Its also risky because this segmentation uses GFP_ATOMIC allocations and
therefore can easily fail (especially if SG is lacking)

So bonding has heuristics : 

- If at least one slave supports TSO, then the master supports TSO 
- If at least one slave support GSO, then the master supports GSO
...

Point is : as we do have software fallback, we should allow the admin to
set/unset TSO on the bonding master, regardless of slaves settings.

Depending on the netfilter/qdisc setups, an admin might know better than
the kernel heuristics.

Re: [PATCH] bonding: allow TSO being set on bonding master

[Michał Mirosław]

2013/5/16 Eric Dumazet <eric.dumazet@gmail.com>:
> On Wed, 2013-05-15 at 16:18 -0700, Eric Dumazet wrote:
>> On Thu, 2013-05-16 at 00:55 +0200, Michał Mirosław wrote:
>>
>> > Have you tried adding it to NETIF_F_ONE_FOR_ALL set? 'Team' and bridge
>> > could then also use it.
>>
>> Good point, I am testing this, thanks !
> Hmm, this doesnt work.
>
> # ethtool -k bond0 | grep tcp-segmentation-offload
> tcp-segmentation-offload: off
> # ethtool -K bond0 tso on
> # ethtool -k bond0 | grep tcp-segmentation-offload
> tcp-segmentation-offload: off
>
> Do you have something different in mind ?

This should enable GSO if at least one slave has it on. What features
(tx-tcp*) the slaves have?

Best Regards,
Michał Mirosław

Re: [PATCH] bonding: allow TSO being set on bonding master

[Eric Dumazet]

On Thu, 2013-05-16 at 07:45 +0200, Michał Mirosław wrote:

> This should enable GSO if at least one slave has it on. What features
> (tx-tcp*) the slaves have?

Both slaves have tso off, gso on.

(Let's say for some tricky hardware bug on the NIC)

We want to not do tso on the NIC, but do the GSO segmentation right
before NIC.

It brings about 30% improvement.

Re: [PATCH] bonding: allow TSO being set on bonding master

[Michał Mirosław]

2013/5/16 Eric Dumazet <eric.dumazet@gmail.com>:
> On Thu, 2013-05-16 at 07:45 +0200, Michał Mirosław wrote:
>
>> This should enable GSO if at least one slave has it on. What features
>> (tx-tcp*) the slaves have?
>
> Both slaves have tso off, gso on.
>
> (Let's say for some tricky hardware bug on the NIC)
>
> We want to not do tso on the NIC, but do the GSO segmentation right
> before NIC.
>
> It brings about 30% improvement.

Then your patch is the way to go.

BTW, the fix_features callbacks of bridge, team and bonding are so
similar it calls for some refactoring. What they want also seems very
close.

Best Regards,
Michał Mirosław

[PATCH v2] bonding: allow TSO being set on bonding master

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

In some situations, we need to disable TSO on bonding slaves.

bonding device automatically unset TSO in bond_fix_features(), and
performance is not good because :

1) We consume more cpu cycles.

2) GSO segmentation has some bugs leading to out of order TCP packets
if this segmentation is done before virtual device. This particular
problem will be addressed in a separate patch.

This patch allows TSO being set/unset on the bonding master,
so that GSO segmentation is done after bonding layer.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Michał Mirosław <mirqus@gmail.com>
Cc: Jay Vosburgh <fubar@us.ibm.com>
Cc: Andy Gospodarek <andy@greyhouse.net>
Cc: Maciej Żenczykowski <maze@google.com>
Cc: Tom Herbert <therbert@google.com>
Cc: Neal Cardwell <ncardwell@google.com>
Cc: Yuchung Cheng <ycheng@google.com>
---
v2: add a documented helper so that team/bridge can use it

 drivers/net/bonding/bond_main.c |    1 +
 include/linux/netdevice.h       |   11 +++++++++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index d0aade0..449ad9b 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1362,6 +1362,7 @@ static netdev_features_t bond_fix_features(struct net_device *dev,
 						     slave->dev->features,
 						     mask);
 	}
+	features = netdev_add_tso_features(features, mask);
 
 out:
 	read_unlock(&bond->lock);
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index a94a5a0..095945c 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2733,6 +2733,17 @@ static inline netdev_features_t netdev_get_wanted_features(
 }
 netdev_features_t netdev_increment_features(netdev_features_t all,
 	netdev_features_t one, netdev_features_t mask);
+
+/* Allow TSO being used on stacked device :
+ * Performing the GSO segmentation before last device
+ * is a performance improvement.
+ */
+static inline netdev_features_t netdev_add_tso_features(netdev_features_t features,
+	netdev_features_t mask)
+{
+	return netdev_increment_features(features, NETIF_F_ALL_TSO, mask);
+}
+
 int __netdev_update_features(struct net_device *dev);
 void netdev_update_features(struct net_device *dev);
 void netdev_change_features(struct net_device *dev);

Re: [PATCH v2] bonding: allow TSO being set on bonding master

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 16 May 2013 10:34:53 -0700

> From: Eric Dumazet <edumazet@google.com>
> 
> In some situations, we need to disable TSO on bonding slaves.
> 
> bonding device automatically unset TSO in bond_fix_features(), and
> performance is not good because :
> 
> 1) We consume more cpu cycles.
> 
> 2) GSO segmentation has some bugs leading to out of order TCP packets
> if this segmentation is done before virtual device. This particular
> problem will be addressed in a separate patch.
> 
> This patch allows TSO being set/unset on the bonding master,
> so that GSO segmentation is done after bonding layer.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Applied, but:

> +static inline netdev_features_t netdev_add_tso_features(netdev_features_t features,
> +	netdev_features_t mask)

I fixed the mask argument indentation when I commited this.

Thakns.

Re: [PATCH v2] bonding: allow TSO being set on bonding master

[Eric Dumazet]

On Thu, 2013-05-16 at 15:02 -0700, David Miller wrote:

> Applied, but:
> 
> > +static inline netdev_features_t netdev_add_tso_features(netdev_features_t features,
> > +	netdev_features_t mask)
> 
> I fixed the mask argument indentation when I commited this.
> 
> Thakns.

I sorry, I think I used the convention of the above function.

Thanks !