[PATCH] Disable TSO for non standard qdiscs

[PATCH] Disable TSO for non standard qdiscs

[Andi Kleen]

TSO interacts badly with many queueing disciplines because they rely on 
reordering packets from different streams and the large TSO packets can 
make this difficult. This patch disables TSO for sockets that send over 
devices with non standard queueing disciplines. That's anything but noop 
or pfifo_fast and pfifo right now.

Longer term other queueing disciplines could be checked if they
are also ok with TSO. If yes they can set the TCQ_F_GSO_OK flag too.

It is still enabled for the standard pfifo_fast because that will never
reorder packets with the same type-of-service. This means 99+% of all users
will still be able to use TSO just fine.

The status is only set up at socket creation so a shifted route
will not reenable TSO on a existing socket. I don't think that's a 
problem though.

Signed-off-by: Andi Kleen <ak@suse.de>

---
 include/net/sch_generic.h |    1 +
 net/core/sock.c           |    3 +++
 net/sched/sch_generic.c   |    5 +++--
 3 files changed, 7 insertions(+), 2 deletions(-)

Index: linux/include/net/sch_generic.h
===================================================================
--- linux.orig/include/net/sch_generic.h
+++ linux/include/net/sch_generic.h
@@ -31,6 +31,7 @@ struct Qdisc
 #define TCQ_F_BUILTIN	1
 #define TCQ_F_THROTTLED	2
 #define TCQ_F_INGRESS	4
+#define TCQ_F_GSO_OK	8
 	int			padded;
 	struct Qdisc_ops	*ops;
 	u32			handle;
Index: linux/net/sched/sch_generic.c
===================================================================
--- linux.orig/net/sched/sch_generic.c
+++ linux/net/sched/sch_generic.c
@@ -307,7 +307,7 @@ struct Qdisc_ops noop_qdisc_ops __read_m
 struct Qdisc noop_qdisc = {
 	.enqueue	=	noop_enqueue,
 	.dequeue	=	noop_dequeue,
-	.flags		=	TCQ_F_BUILTIN,
+	.flags		=	TCQ_F_BUILTIN | TCQ_F_GSO_OK,
 	.ops		=	&noop_qdisc_ops,
 	.list		=	LIST_HEAD_INIT(noop_qdisc.list),
 };
@@ -325,7 +325,7 @@ static struct Qdisc_ops noqueue_qdisc_op
 static struct Qdisc noqueue_qdisc = {
 	.enqueue	=	NULL,
 	.dequeue	=	noop_dequeue,
-	.flags		=	TCQ_F_BUILTIN,
+	.flags		=	TCQ_F_BUILTIN | TCQ_F_GSO_OK,
 	.ops		=	&noqueue_qdisc_ops,
 	.list		=	LIST_HEAD_INIT(noqueue_qdisc.list),
 };
@@ -538,6 +538,7 @@ void dev_activate(struct net_device *dev
 				printk(KERN_INFO "%s: activation failed\n", dev->name);
 				return;
 			}
+			qdisc->flags |= TCQ_F_GSO_OK;
 			list_add_tail(&qdisc->list, &dev->qdisc_list);
 		} else {
 			qdisc =  &noqueue_qdisc;
Index: linux/net/core/sock.c
===================================================================
--- linux.orig/net/core/sock.c
+++ linux/net/core/sock.c
@@ -112,6 +112,7 @@
 #include <linux/tcp.h>
 #include <linux/init.h>
 #include <linux/highmem.h>
+#include <net/sch_generic.h>
 
 #include <asm/uaccess.h>
 #include <asm/system.h>
@@ -1062,6 +1063,8 @@ void sk_setup_caps(struct sock *sk, stru
 {
 	__sk_dst_set(sk, dst);
 	sk->sk_route_caps = dst->dev->features;
+	if (!(dst->dev->qdisc->flags & TCQ_F_GSO_OK))
+		sk->sk_route_caps &= ~NETIF_F_GSO_MASK;
 	if (sk->sk_route_caps & NETIF_F_GSO)
 		sk->sk_route_caps |= NETIF_F_GSO_SOFTWARE;
 	if (sk_can_gso(sk)) {
Index: linux/net/sched/sch_fifo.c
===================================================================
--- linux.orig/net/sched/sch_fifo.c
+++ linux/net/sched/sch_fifo.c
@@ -62,6 +62,7 @@ static int fifo_init(struct Qdisc *sch, 
 
 		q->limit = ctl->limit;
 	}
+	sch->flags |= TCQ_F_GSO_OK;
 
 	return 0;
 }

Re: [PATCH] Disable TSO for non standard qdiscs

[Stephen Hemminger]

On Thu, 31 Jan 2008 13:46:32 +0100
Andi Kleen <andi@firstfloor.org> wrote:

> 
> TSO interacts badly with many queueing disciplines because they rely on 
> reordering packets from different streams and the large TSO packets can 
> make this difficult. This patch disables TSO for sockets that send over 
> devices with non standard queueing disciplines. That's anything but noop 
> or pfifo_fast and pfifo right now.
> 
> Longer term other queueing disciplines could be checked if they
> are also ok with TSO. If yes they can set the TCQ_F_GSO_OK flag too.
> 
> It is still enabled for the standard pfifo_fast because that will never
> reorder packets with the same type-of-service. This means 99+% of all users
> will still be able to use TSO just fine.
> 
> The status is only set up at socket creation so a shifted route
> will not reenable TSO on a existing socket. I don't think that's a 
> problem though.
> 
> Signed-off-by: Andi Kleen <ak@suse.de>
> 


Fix the broken qdisc instead.

-- 
Stephen Hemminger <stephen.hemminger@vyatta.com>

Re: [PATCH] Disable TSO for non standard qdiscs

[Patrick McHardy]

Andi Kleen wrote:
>> Fix the broken qdisc instead.
> 
> What do you mean? I don't think the qdiscs are broken.
> I cannot think of any way how e.g. TBF can do anything useful
> with large TSO packets.


Someone posted a patch some time ago to calculate the amount
of tokens needed in max_size portions and use that, but IMO
people should just configure TBF with the proper MTU for TSO.

Re: [PATCH] Disable TSO for non standard qdiscs

[Stephen Hemminger]

On Thu, 31 Jan 2008 19:37:35 +0100
Andi Kleen <andi@firstfloor.org> wrote:

> On Thu, Jan 31, 2008 at 07:01:00PM +0100, Patrick McHardy wrote:
> > Andi Kleen wrote:
> > >>Fix the broken qdisc instead.
> > >
> > >What do you mean? I don't think the qdiscs are broken.
> > >I cannot think of any way how e.g. TBF can do anything useful
> > >with large TSO packets.
> > 
> > 
> > Someone posted a patch some time ago to calculate the amount
> > of tokens needed in max_size portions and use that, but IMO
> > people should just configure TBF with the proper MTU for TSO.
> 
> TBF with 64k atomic units will always be chunky and uneven. I don't
> think that's a useful goal. 
> 
> -Andi

Then change TBF to use skb_gso_segment?  Be careful, the fact that
one skb ends up queueing multiple skb's would cause issues to parent
qdisc (ie work generating qdisc).

-- 
Stephen Hemminger <stephen.hemminger@vyatta.com>

Re: [PATCH] Disable TSO for non standard qdiscs

[Patrick McHardy]

Stephen Hemminger wrote:
> On Thu, 31 Jan 2008 19:37:35 +0100
> Andi Kleen <andi@firstfloor.org> wrote:
> 
>> On Thu, Jan 31, 2008 at 07:01:00PM +0100, Patrick McHardy wrote:
>>> Andi Kleen wrote:
>>>>> Fix the broken qdisc instead.
>>>> What do you mean? I don't think the qdiscs are broken.
>>>> I cannot think of any way how e.g. TBF can do anything useful
>>>> with large TSO packets.
>>>
>>> Someone posted a patch some time ago to calculate the amount
>>> of tokens needed in max_size portions and use that, but IMO
>>> people should just configure TBF with the proper MTU for TSO.
>> TBF with 64k atomic units will always be chunky and uneven. I don't
>> think that's a useful goal. 
>>
>> -Andi
> 
> Then change TBF to use skb_gso_segment?  Be careful, the fact that
> one skb ends up queueing multiple skb's would cause issues to parent
> qdisc (ie work generating qdisc).


How about keeping the TSO-capable flag on qdiscs, propagating
the non-capability up the tree and perform segmentation before
queueing to the root?

Re: [PATCH] Disable TSO for non standard qdiscs

[Patrick McHardy]

Andi Kleen wrote:
>> Then change TBF to use skb_gso_segment?  Be careful, the fact that
> 
> That doesn't help because it wants to interleave packets
> from different streams to get everything fair and smooth. The only 
> good way to handle that is to split it up and the simplest way to do 
> this is to just tell TCP to not do GSO in the first place.


Thats not correct, TBF keeps packets strictly ordered unless
an inner qdisc does reordering. But even then (let say you use
SFQ) packets of a single flow will stay ordered. Segmenting
TSO packets is no different than having them arrive independantly
for other reasons.

Re: [PATCH] Disable TSO for non standard qdiscs

[Rick Jones]

Andi Kleen wrote:
> TSO interacts badly with many queueing disciplines because they rely on 
> reordering packets from different streams and the large TSO packets can 
> make this difficult. This patch disables TSO for sockets that send over 
> devices with non standard queueing disciplines. That's anything but noop 
> or pfifo_fast and pfifo right now.

Does this also imply that JumboFrames interacts badly with these qdiscs? 
  Or IPoIB with its 65000ish byte MTU?

rick jones

Re: [PATCH] Disable TSO for non standard qdiscs

[Andi Kleen]

> Fix the broken qdisc instead.

What do you mean? I don't think the qdiscs are broken.
I cannot think of any way how e.g. TBF can do anything useful
with large TSO packets.

-Andi

Re: [PATCH] Disable TSO for non standard qdiscs

[Rick Jones]

Andi Kleen wrote:
> On Thu, Jan 31, 2008 at 10:26:19AM -0800, Rick Jones wrote:
> 
>>Andi Kleen wrote:
>>
>>>TSO interacts badly with many queueing disciplines because they rely on 
>>>reordering packets from different streams and the large TSO packets can 
>>>make this difficult. This patch disables TSO for sockets that send over 
>>>devices with non standard queueing disciplines. That's anything but noop 
>>>or pfifo_fast and pfifo right now.
>>
>>Does this also imply that JumboFrames interacts badly with these qdiscs? 
>> Or IPoIB with its 65000ish byte MTU?
> 
> 
> Correct. Of course it is always relative to the link speed. So if your
> link is 10x faster and your packets 10x bigger you can get similarly
> smooth shaping.

So, at what timescale do people using these qdiscs expect things to 
appear "smooth?"  64KB of data at GbE speeds is something just north of 
half a millisecond unless I've botched my units somewhere.

rick jones

Re: [PATCH] Disable TSO for non standard qdiscs

[Andi Kleen]

On Thu, Jan 31, 2008 at 07:01:00PM +0100, Patrick McHardy wrote:
> Andi Kleen wrote:
> >>Fix the broken qdisc instead.
> >
> >What do you mean? I don't think the qdiscs are broken.
> >I cannot think of any way how e.g. TBF can do anything useful
> >with large TSO packets.
> 
> 
> Someone posted a patch some time ago to calculate the amount
> of tokens needed in max_size portions and use that, but IMO
> people should just configure TBF with the proper MTU for TSO.

TBF with 64k atomic units will always be chunky and uneven. I don't
think that's a useful goal. 

-Andi

RE: [PATCH] Disable TSO for non standard qdiscs

[Waskiewicz Jr, Peter P]

> My point was that without TSO different submitters will 
> interleave their streams (because they compete about the 
> qdisc submission) and then you end up with a smooth rate over 
> time for all of them.
> 
> If you submit in large chunks only (as TSO does) it will 
> always be more bursty and that works against the TBF goal.
> 
> For a single submitter you would be correct.
> 
> -Andi

TSO by nature is bursty.  But disabling TSO without the option of having
it on or off to me seems to aggressive.  If someone is using a qdisc
that TSO is interfering with the effectiveness of the traffic shaping,
then they should turn off TSO via ethtool on the target device.  Some
people may want TSO with certain rate limiter settings.  That way (as
Stephen said) you can even allow the stack to GSO, then segment before
calling hard_start_xmit(), which still saves a number of cycles.

I'd rather not see this, but a documented recommendation why TSO could
be bad for some traffic shaping qdiscs.  Give the power to the user to
either shoot themselves in the foot or disable TSO when needed.

-PJ Waskiewicz
<peter.p.waskiewicz.jr@intel.com>

Re: [PATCH] Disable TSO for non standard qdiscs

[Patrick McHardy]

Andi Kleen wrote:
> On Thu, Jan 31, 2008 at 07:21:20PM +0100, Patrick McHardy wrote:
>> Andi Kleen wrote:
>>>> Then change TBF to use skb_gso_segment?  Be careful, the fact that
>>> That doesn't help because it wants to interleave packets
>> >from different streams to get everything fair and smooth. The only 
>>> good way to handle that is to split it up and the simplest way to do 
>>> this is to just tell TCP to not do GSO in the first place.
>>
>> Thats not correct, TBF keeps packets strictly ordered unless
> 
> My point was that without TSO different submitters will interleave
> their streams (because they compete about the qdisc submission) 
> and then you end up with a smooth rate over time for all of them.
> 
> If you submit in large chunks only (as TSO does) it will always 
> be more bursty and that works against the TBF goal.
> 
> For a single submitter you would be correct.


The TBF goal is not really fairness among different flows, but
I agree, avoiding TSO in the first place seems to make more sense.

Re: [PATCH] Disable TSO for non standard qdiscs

[Andi Kleen]

> Then change TBF to use skb_gso_segment?  Be careful, the fact that

That doesn't help because it wants to interleave packets
from different streams to get everything fair and smooth. The only 
good way to handle that is to split it up and the simplest way to do 
this is to just tell TCP to not do GSO in the first place.

-Andi

Re: [PATCH] Disable TSO for non standard qdiscs

[Andi Kleen]

On Thu, Jan 31, 2008 at 07:21:20PM +0100, Patrick McHardy wrote:
> Andi Kleen wrote:
> >>Then change TBF to use skb_gso_segment?  Be careful, the fact that
> >
> >That doesn't help because it wants to interleave packets
> >from different streams to get everything fair and smooth. The only 
> >good way to handle that is to split it up and the simplest way to do 
> >this is to just tell TCP to not do GSO in the first place.
> 
> 
> Thats not correct, TBF keeps packets strictly ordered unless

My point was that without TSO different submitters will interleave
their streams (because they compete about the qdisc submission) 
and then you end up with a smooth rate over time for all of them.

If you submit in large chunks only (as TSO does) it will always 
be more bursty and that works against the TBF goal.

For a single submitter you would be correct.

-Andi

Re: [PATCH] Disable TSO for non standard qdiscs

[Andi Kleen]

On Thu, Jan 31, 2008 at 10:26:19AM -0800, Rick Jones wrote:
> Andi Kleen wrote:
> >TSO interacts badly with many queueing disciplines because they rely on 
> >reordering packets from different streams and the large TSO packets can 
> >make this difficult. This patch disables TSO for sockets that send over 
> >devices with non standard queueing disciplines. That's anything but noop 
> >or pfifo_fast and pfifo right now.
> 
> Does this also imply that JumboFrames interacts badly with these qdiscs? 
>  Or IPoIB with its 65000ish byte MTU?

Correct. Of course it is always relative to the link speed. So if your
link is 10x faster and your packets 10x bigger you can get similarly
smooth shaping.

-Andi

Re: [PATCH] Disable TSO for non standard qdiscs

[Rick Jones]

Andi Kleen wrote:
>>So, at what timescale do people using these qdiscs expect things to 
>>appear "smooth?"  64KB of data at GbE speeds is something just north of 
>>half a millisecond unless I've botched my units somewhere.
> 
> 
> One typical use case for TBF is you talking to a DSL bridge that 
> is connected using a GBit Ethernet switch. For these DSL connections it gives
> much better behaviour to shape the traffic to slightly below
> your external link speed so that you can e.g. prioritize packets properly.

Sounds like the functionality needs to be in the DSL bridge :) (or the 
"router" in the same case) Particularly since it might be getting used 
by more than one host on the GbE switch.

> But the actual external link speed is much lower than GbE.
> A lot of GbE NICs enable TSO by default.

bluesky typing...

then the qdisc could/should place a cap on the size of a 'TSO' based on 
the bitrate (and perhaps input as to how much time any one "burst" of 
data should be allowed to consume on the network) and pass that up the 
stack?  right now you seem to be proposing what is effectively a cap of 
1 MSS.

rick jones

Re: [PATCH] Disable TSO for non standard qdiscs

[Andi Kleen]

> So, at what timescale do people using these qdiscs expect things to 
> appear "smooth?"  64KB of data at GbE speeds is something just north of 
> half a millisecond unless I've botched my units somewhere.

One typical use case for TBF is you talking to a DSL bridge that 
is connected using a GBit Ethernet switch. For these DSL connections it gives
much better behaviour to shape the traffic to slightly below
your external link speed so that you can e.g. prioritize packets properly.
But the actual external link speed is much lower than GbE.
A lot of GbE NICs enable TSO by default.

-Andi

Re: [PATCH] Disable TSO for non standard qdiscs

[Andi Kleen]

> TSO by nature is bursty.  But disabling TSO without the option of having
> it on or off to me seems to aggressive.  If someone is using a qdisc
> that TSO is interfering with the effectiveness of the traffic shaping,
> then they should turn off TSO via ethtool on the target device.  Some

The philosophical problem I have with this suggestion is that I expect
that the large majority of users will be more happy with disabled TSO
if they use non standard qdiscs and defaults that do not fit 
the majority use case are bad.

Basically you're suggesting that nearly everyone using tc should learn about
another obscure command.

-Andi

RE: [PATCH] Disable TSO for non standard qdiscs

[Waskiewicz Jr, Peter P]

> The philosophical problem I have with this suggestion is that 
> I expect that the large majority of users will be more happy 
> with disabled TSO if they use non standard qdiscs and 
> defaults that do not fit the majority use case are bad.
> 
> Basically you're suggesting that nearly everyone using tc 
> should learn about another obscure command.

If someone is using tc to load and configure a qdisc, I'd really hope
knowing or learning ethtool wouldn't be a stretch for them...  And I'm
not arguing the majority of people will want this or not, but taking
away the ability to use TSO at the kernel level here without allowing a
tuneable is making that decision for them, which is wrong IMO.

Cheers,

-PJ Waskiewicz
<peter.p.waskiewicz.jr@intel.com>

Re: [PATCH] Disable TSO for non standard qdiscs

[Jarek Poplawski]

Andi Kleen wrote, On 01/31/2008 08:34 PM:

>> TSO by nature is bursty.  But disabling TSO without the option of having
>> it on or off to me seems to aggressive.  If someone is using a qdisc
>> that TSO is interfering with the effectiveness of the traffic shaping,
>> then they should turn off TSO via ethtool on the target device.  Some
> 
> The philosophical problem I have with this suggestion is that I expect
> that the large majority of users will be more happy with disabled TSO
> if they use non standard qdiscs and defaults that do not fit 
> the majority use case are bad.


If you mean the large majority of the large minority of users, who use
non standard qdiscs - I agree - this is really the philosophical problem!

 
> Basically you're suggesting that nearly everyone using tc should learn about
> another obscure command.

...So, it sounds like tc is used by nearly everyone now...

It seems my distro really isn't up to date:

"Package: iproute
 ...
 Description: Professional tools to control the networking in Linux kernels
 This is `iproute', the professional set of tools to control the
 networking behavior in kernels 2.2.x and later."

And ethtool doesn't have to be learnt at all: "most friendly distros"
could use this in config or add some graphical wrapper.

Regards,
Jarek P.

Re: [PATCH] Disable TSO for non standard qdiscs

[Jarek Poplawski]

Jarek Poplawski wrote, On 01/31/2008 09:33 PM:

> Andi Kleen wrote, On 01/31/2008 08:34 PM

 ...

>> Basically you're suggesting that nearly everyone using tc should learn about
>> another obscure command

 
...On the other hand, with this DSL argument from the sub-thread you
could be quite right: if this "everyone" wants to use one NIC for
both high speed local network and such a DSL, then learning ethtool
could be not enough...

Jarek P.

Re: [PATCH] Disable TSO for non standard qdiscs

[Arnaldo Carvalho de Melo]

Em Thu, Jan 31, 2008 at 11:39:55AM -0800, Waskiewicz Jr, Peter P escreveu:
> > The philosophical problem I have with this suggestion is that 
> > I expect that the large majority of users will be more happy 
> > with disabled TSO if they use non standard qdiscs and 
> > defaults that do not fit the majority use case are bad.
> > 
> > Basically you're suggesting that nearly everyone using tc 
> > should learn about another obscure command.
> 
> If someone is using tc to load and configure a qdisc, I'd really hope
> knowing or learning ethtool wouldn't be a stretch for them...  And I'm
> not arguing the majority of people will want this or not, but taking
> away the ability to use TSO at the kernel level here without allowing a
> tuneable is making that decision for them, which is wrong IMO.

Well, it could be just that when using such qdiscs TSO would be
disabled, but the user could override this by using ethtool after
loading the qdiscs.

- Arnaldo

RE: [PATCH] Disable TSO for non standard qdiscs

[Waskiewicz Jr, Peter P]

> Well, it could be just that when using such qdiscs TSO would be
> disabled, but the user could override this by using ethtool after
> loading the qdiscs.

I still disagree with this.  The qdisc should not cause anything to happen to feature flags on the device. It's the scheduling layer and really shouldn't care about what features the device supports or not.  If someone has an issue with a feature hurting performance or causing odd behavior when using a qdisc, then they should disable the feature on the device using the appropriate tools provided.  If it's the qdisc causing issues, then either the qdisc needs to be fixed, or it should be documented what features are recommended to be on and off with the qdisc.  I don't agree that the scheduling layer should affect features on an underlying device.

Cheers,

-PJ Waskiewicz

Re: [PATCH] Disable TSO for non standard qdiscs

[Andy Furniss]

Rick Jones wrote:

> then the qdisc could/should place a cap on the size of a 'TSO' based on 
> the bitrate (and perhaps input as to how much time any one "burst" of 
> data should be allowed to consume on the network) and pass that up the 
> stack?  right now you seem to be proposing what is effectively a cap of 
> 1 MSS.

I don't have any gig nics to test, so this is not a rhetorical question.

How does tcp congestion control behave when a tc qdisc drops a big 
unsegmented TSO skb?

Andy.

Re: [PATCH] Disable TSO for non standard qdiscs

[Patrick McHardy]

Waskiewicz Jr, Peter P wrote:
>> Well, it could be just that when using such qdiscs TSO would be
>> disabled, but the user could override this by using ethtool after
>> loading the qdiscs.
> 
> I still disagree with this.  The qdisc should not cause anything to happen to feature flags on the device. It's the scheduling layer and really shouldn't care about what features the device supports or not.  If someone has an issue with a feature hurting performance or causing odd behavior when using a qdisc, then they should disable the feature on the device using the appropriate tools provided.  If it's the qdisc causing issues, then either the qdisc needs to be fixed, or it should be documented what features are recommended to be on and off with the qdisc.  I don't agree that the scheduling layer should affect features on an underlying device.

Andi's patch made the TSO capable flag a property of the
qdisc (not the ops), so it could still be explicitly
configured by the user.

Re: [PATCH] Disable TSO for non standard qdiscs

[Andi Kleen]

On Thu, Jan 31, 2008 at 11:14:34AM -0800, Rick Jones wrote:
> Sounds like the functionality needs to be in the DSL bridge :) (or the 
> "router" in the same case) Particularly since it might be getting used 
> by more than one host on the GbE switch.

Possible, but it is not usually in the real world. Setups like
WonderShaper which do this on the host side are pretty common.

> then the qdisc could/should place a cap on the size of a 'TSO' based on 
> the bitrate (and perhaps input as to how much time any one "burst" of 
> data should be allowed to consume on the network) and pass that up the 
> stack?  right now you seem to be proposing what is effectively a cap of 
> 1 MSS.

Hmm, that would probably be possible for TBF, but I'm not sure this can be
really done in a useful way for the more complicated qdiscs. Especially
since they would likely need to turn on/off GSO regularly when dynamic
circumstances change and there is not really a good way to affect a socket 
after it was created.

-Andi

Re: [PATCH] Disable TSO for non standard qdiscs

[Andi Kleen]

On Thu, Jan 31, 2008 at 03:42:54PM -0800, Waskiewicz Jr, Peter P wrote:
> > Well, it could be just that when using such qdiscs TSO would be
> > disabled, but the user could override this by using ethtool after
> > loading the qdiscs.
> 
> I still disagree with this.  The qdisc should not cause anything to happen to feature flags on the device. It's the scheduling layer and really shouldn't care about what features the device supports or not.  If someone has an issue with a feature hurting performance or causing odd behavior when using a qdisc, then they should disable the feature on the device using the appropriate tools provided.  If it's the qdisc causing issues, then either the qdisc needs to be fixed, or it should be documented what features are recommended to be on and off with the qdisc.  I don't agree that the scheduling layer should affect features on an underlying device.

You seem to only look at this from a high level theoretical standpoint.

But more down to earth: do you have a useful scenario where it makes
sense to do shaping or another qdisc on GSO packets? My take is that
when you decide to do any packet scheduling you really want to do 
it on wire packets, not some internal stack implementation implementation
detail units.

-Andi

Re: [PATCH] Disable TSO for non standard qdiscs

[Andi Kleen]

> Well, it could be just that when using such qdiscs TSO would be
> disabled, but the user could override this by using ethtool after
> loading the qdiscs.

If anything TC, not ethtool. Do you have an useful scenario where
GSO makes sense with TBF et.al.?

-Andi

Re: [PATCH] Disable TSO for non standard qdiscs

[Andi Kleen]

On Thu, Jan 31, 2008 at 09:33:44PM +0100, Jarek Poplawski wrote:
> Andi Kleen wrote, On 01/31/2008 08:34 PM:
> 
> >> TSO by nature is bursty.  But disabling TSO without the option of having
> >> it on or off to me seems to aggressive.  If someone is using a qdisc
> >> that TSO is interfering with the effectiveness of the traffic shaping,
> >> then they should turn off TSO via ethtool on the target device.  Some
> > 
> > The philosophical problem I have with this suggestion is that I expect
> > that the large majority of users will be more happy with disabled TSO
> > if they use non standard qdiscs and defaults that do not fit 
> > the majority use case are bad.
> 
> 
> If you mean the large majority of the large minority of users, who use
> non standard qdiscs - I agree - this is really the philosophical problem!

[....] Sorry if you had a point in this email I missed it.

-Andi

Re: [PATCH] Disable TSO for non standard qdiscs

[Glen Turner]

On Thu, 2008-01-31 at 20:34 +0100, Andi Kleen wrote:

> The philosophical problem I have with this suggestion is that I expect
> that the large majority of users will be more happy with disabled TSO
> if they use non standard qdiscs and defaults that do not fit 
> the majority use case are bad.

I wouldn't be so fast to assume that all users need an exact playout
rate, as people seem to do fine with the 8Kbps playout steps in Cisco
IOS.  A nerd-knob which expresses user's preference in the
accuracy/performance trade-off would be nice.

The problem with ethtool is that it's a non-obvious nerd knob.  At
the least the ethtool documentation should be updated to indicate that 
activating TSO effects tc accuracy.

Best wishes, Glen
[a network engineer]

Re: [PATCH] Disable TSO for non standard qdiscs

[Patrick McHardy]

Glen Turner wrote:
> On Thu, 2008-01-31 at 20:34 +0100, Andi Kleen wrote:
> 
>> The philosophical problem I have with this suggestion is that I expect
>> that the large majority of users will be more happy with disabled TSO
>> if they use non standard qdiscs and defaults that do not fit 
>> the majority use case are bad.
> 
> I wouldn't be so fast to assume that all users need an exact playout
> rate, as people seem to do fine with the 8Kbps playout steps in Cisco
> IOS.  A nerd-knob which expresses user's preference in the
> accuracy/performance trade-off would be nice.
> 
> The problem with ethtool is that it's a non-obvious nerd knob.  At
> the least the ethtool documentation should be updated to indicate that 
> activating TSO effects tc accuracy.


I agree with Andi, most user neither know nor care about TSO.
It should work properly by default and optimizations should
be explicitly configured. This is especially true if you
consider the common userbase of qdiscs - which is mostly
slow DSL lines, cablemodems etc.

Re: [PATCH] Disable TSO for non standard qdiscs

[Patrick McHardy]

Andi Kleen wrote:
>> The problem with ethtool is that it's a non-obvious nerd knob.  At
>> the least the ethtool documentation should be updated to indicate that 
>> activating TSO effects tc accuracy.
> 
> TSO tends to be activated by default in the driver; very few people who use it
> do even know that ethtool exist or what TSO is.


Indeed. As an example of an unknowing user, this discussion made me
check whether my cablemodem device (on which I'm using HFSC) uses
TSO :)

Re: [PATCH] Disable TSO for non standard qdiscs

[Jarek Poplawski]

On 01-02-2008 00:04, Jarek Poplawski wrote:
...
> ...On the other hand, with this DSL argument from the sub-thread you
> could be quite right: if this "everyone" wants to use one NIC for
> both high speed local network and such a DSL, then learning ethtool
> could be not enough...

...But, on the other hand, in this case the realization seems to be
wrong: probably still all locally created packets will be treated
the same - or I miss something?

Jarek P.

Re: [PATCH] Disable TSO for non standard qdiscs

[Andi Kleen]

> The problem with ethtool is that it's a non-obvious nerd knob.  At
> the least the ethtool documentation should be updated to indicate that 
> activating TSO effects tc accuracy.

TSO tends to be activated by default in the driver; very few people who use it
do even know that ethtool exist or what TSO is.

-Andi

RE: [PATCH] Disable TSO for non standard qdiscs

[Waskiewicz Jr, Peter P]

> ...But, on the other hand, in this case the realization seems to be
> wrong: probably still all locally created packets will be 
> treated the same - or I miss something?
> 
> Jarek P.

The TCP layer will generate TSO packets based on the kernel socket
features associated with the flow.  So if you have two devices, one
supporting TSO, the other not, then the flows associated with the
non-TSO device will not have their packets built for TSO.  This has no
bearing on the device supporting TSO, which its feature flags will
propogate into the kernel socket for that flow, and cause any TCP flows
to that device to be TSO packets.  So in a nutshell, disabling TSO is on
a per-device level, not a global switch.

-PJ Waskiewicz

RE: [PATCH] Disable TSO for non standard qdiscs

[Waskiewicz Jr, Peter P]

> Indeed. As an example of an unknowing user, this discussion 
> made me check whether my cablemodem device (on which I'm 
> using HFSC) uses TSO :)

The TSO defer logic is based on your congestion window and current
window size.  So the actual frame sizes hitting your NIC attached to
your DSL probably aren't anywhere near 64KB, but probably more in line
with whatever your window size is for DSL.

The bottom line is TSO saves CPU cycles.  If we want to make it go away
because of a traffic shaping qdisc interfering, then that's fine.  I
just don't think a TSO option should be added to the scheduler layer,
since it already exists in the ethtool layer.  Asking a user to type
'ethtool -k <devicename> tso off' is probably going to be much easier
than setting an option on your qdisc through tc to turn TSO back on.

I think we're having more of a disagreement of what is considered the
"normal case" user.  If you are on a slow link, such as a DSL/cable
line, your TCP window/congestion window aren't going to be big enough to
generate large TSO's, so what is the issue?  But disabling TSO, say on a
10 GbE link, can cut throughput by half (I have data on 8-core machines
with 10 GbE with/without TSO if you're interested).  Even on a
single-core machine with a 1GbE link can have bad performance hits.  So
this is why I'm so concerned about a proposal to turn off TSO outside of
the current established methods of using ethtool.  Rather than educating
the user about how to turn TSO back on using tc if they want it, educate
them why they may want to consider turning TSO off in certain
configurations.  And I don't consider any user effectively using a TBF
qdisc someone incapable of understanding how to use ethtool.

Cheers,

-PJ Waskiewicz

Re: [PATCH] Disable TSO for non standard qdiscs

[Patrick McHardy]

Waskiewicz Jr, Peter P wrote:
>> Indeed. As an example of an unknowing user, this discussion 
>> made me check whether my cablemodem device (on which I'm 
>> using HFSC) uses TSO :)
> 
> The TSO defer logic is based on your congestion window and current
> window size.  So the actual frame sizes hitting your NIC attached to
> your DSL probably aren't anywhere near 64KB, but probably more in line
> with whatever your window size is for DSL.
> 
> The bottom line is TSO saves CPU cycles.  If we want to make it go away
> because of a traffic shaping qdisc interfering, then that's fine.  I
> just don't think a TSO option should be added to the scheduler layer,
> since it already exists in the ethtool layer.  Asking a user to type
> 'ethtool -k <devicename> tso off' is probably going to be much easier
> than setting an option on your qdisc through tc to turn TSO back on.
> 
> I think we're having more of a disagreement of what is considered the
> "normal case" user.  If you are on a slow link, such as a DSL/cable
> line, your TCP window/congestion window aren't going to be big enough to
> generate large TSO's, so what is the issue?  But disabling TSO, say on a
> 10 GbE link, can cut throughput by half (I have data on 8-core machines
> with 10 GbE with/without TSO if you're interested).  Even on a
> single-core machine with a 1GbE link can have bad performance hits.  So
> this is why I'm so concerned about a proposal to turn off TSO outside of
> the current established methods of using ethtool.  Rather than educating
> the user about how to turn TSO back on using tc if they want it, educate
> them why they may want to consider turning TSO off in certain
> configurations.  And I don't consider any user effectively using a TBF
> qdisc someone incapable of understanding how to use ethtool.


We don't want to disable TSO for cases where it makes sense, but
who is using TBF on 10GbE? The point is that most users of qdiscs
which are incapable of dealing with TSO without hacks or special
configuration probably don't care, and 10GbE users know about
ethtool *and* don't use TBF or HTB (which are probably the only
qdiscs which actually have problems, maybe also CBQ).

Re: [PATCH] Disable TSO for non standard qdiscs

[jamal]

On Fri, 2008-01-02 at 10:56 +0100, Patrick McHardy wrote:

> We don't want to disable TSO for cases where it makes sense, but
> who is using TBF on 10GbE? The point is that most users of qdiscs
> which are incapable of dealing with TSO without hacks or special
> configuration probably don't care, and 10GbE users know about
> ethtool *and* don't use TBF or HTB (which are probably the only
> qdiscs which actually have problems, maybe also CBQ).

Right - Essentially it is a usability issue:
People who know how to use TSO (Peter for example) will be clueful
enough to turn it on. Which means the default should be to protect the
clueless and turn it off.
On Andis approach:
Turning TSO off at netdev registration time with a warning will be a
cleaner IMO. Or alternatively introducing a kernel-config "I know what
TSO is" option which is then used at netdev registration. From a
usability perspective it would make more sense to just keep ethtool as
the only way to configure TSO. 

[I recently spent a few days helping someone debug a problem with IFB
because he was redirecting packets from an TSO netdevice and occasionaly
some multi-packet will be missed in the calculation; my answer was "turn
off TSO"; so there are more use cases for this TSO issue]. 

cheers,
jamal

Re: [PATCH] Disable TSO for non standard qdiscs

[Andi Kleen]

> The TSO defer logic is based on your congestion window and current
> window size.  So the actual frame sizes hitting your NIC attached to
> your DSL probably aren't anywhere near 64KB, but probably more in line
> with whatever your window size is for DSL.

DSL windows can be quite large because a lot of DSL lines have a quite
long latency due to error correction. And with ADSL2 we have upto 16Mbit
now.

> I think we're having more of a disagreement of what is considered the
> "normal case" user.  If you are on a slow link, such as a DSL/cable
> line, your TCP window/congestion window aren't going to be big enough to
> generate large TSO's, so what is the issue?  But disabling TSO, say on a

64k TSOs are likely even with DSL. Anyways even with smaller TSOs the
change still makes sense because each increase makes packet scheduling
less smooth.

-Andi

Re: [PATCH] Disable TSO for non standard qdiscs

[Stephen Hemminger]

On Fri, 1 Feb 2008 15:34:21 +0100
Andi Kleen <andi@firstfloor.org> wrote:

> > The TSO defer logic is based on your congestion window and current
> > window size.  So the actual frame sizes hitting your NIC attached to
> > your DSL probably aren't anywhere near 64KB, but probably more in line
> > with whatever your window size is for DSL.
> 
> DSL windows can be quite large because a lot of DSL lines have a quite
> long latency due to error correction. And with ADSL2 we have upto 16Mbit
> now.
> 
> > I think we're having more of a disagreement of what is considered the
> > "normal case" user.  If you are on a slow link, such as a DSL/cable
> > line, your TCP window/congestion window aren't going to be big enough to
> > generate large TSO's, so what is the issue?  But disabling TSO, say on a
> 
> 64k TSOs are likely even with DSL. Anyways even with smaller TSOs the
> change still makes sense because each increase makes packet scheduling
> less smooth.
> 
> -Andi

I wish there was a per-device setting for the max-size of TSO burst.


-- 
Stephen Hemminger <stephen.hemminger@vyatta.com>

RE: [PATCH] Disable TSO for non standard qdiscs

[Waskiewicz Jr, Peter P]

> Right - Essentially it is a usability issue:
> People who know how to use TSO (Peter for example) will be 
> clueful enough to turn it on. Which means the default should 
> be to protect the clueless and turn it off.
> On Andis approach:
> Turning TSO off at netdev registration time with a warning 
> will be a cleaner IMO. Or alternatively introducing a 
> kernel-config "I know what TSO is" option which is then used 
> at netdev registration. From a usability perspective it would 
> make more sense to just keep ethtool as the only way to 
> configure TSO. 

To me this sounds like the most reasonable approach.  I've put out my
concerns, so I'll get out of the way now so we can move forward in some
direction.  :-)

Cheers,

-PJ Waskiewicz

Re: [PATCH] Disable TSO for non standard qdiscs

[Jarek Poplawski]

On Fri, Feb 01, 2008 at 01:28:15AM -0800, Waskiewicz Jr, Peter P wrote:
...
> The TCP layer will generate TSO packets based on the kernel socket
> features associated with the flow.  So if you have two devices, one
> supporting TSO, the other not, then the flows associated with the
> non-TSO device will not have their packets built for TSO.  This has no
> bearing on the device supporting TSO, which its feature flags will
> propogate into the kernel socket for that flow, and cause any TCP flows
> to that device to be TSO packets.  So in a nutshell, disabling TSO is on
> a per-device level, not a global switch.

Fine, but I was rather wondering if there could be something more in
the idea of this patch that can't be done with ethtool. And I don't
think qdisc code currently treats or should treat TCP special.

Jarek P.

Re: [PATCH] Disable TSO for non standard qdiscs

[Rick Jones]

>>Does this also imply that JumboFrames interacts badly with these qdiscs? 
>> Or IPoIB with its 65000ish byte MTU?
> 
> 
> Correct. Of course it is always relative to the link speed. So if your
> link is 10x faster and your packets 10x bigger you can get similarly
> smooth shaping.

If the later-in-thread mentioned person shaping for their DSL line 
happens to have enabled JumboFrames on their GbE network, will/should 
the qdisc negate that?  Or is the qdisc currently assuming that the 
remote end of the DSL will have asked for a smaller MSS?

rick jones

Re: [PATCH] Disable TSO for non standard qdiscs

[Jarek Poplawski]

jamal wrote, On 02/01/2008 01:06 PM:

> On Fri, 2008-01-02 at 10:56 +0100, Patrick McHardy wrote:
> 
>> We don't want to disable TSO for cases where it makes sense, but
>> who is using TBF on 10GbE? The point is that most users of qdiscs
>> which are incapable of dealing with TSO without hacks or special
>> configuration probably don't care, and 10GbE users know about
>> ethtool *and* don't use TBF or HTB (which are probably the only
>> qdiscs which actually have problems, maybe also CBQ).
> 
> Right - Essentially it is a usability issue:
> People who know how to use TSO (Peter for example) will be clueful
> enough to turn it on. Which means the default should be to protect the
> clueless and turn it off.
> On Andis approach:
> Turning TSO off at netdev registration time with a warning will be a
> cleaner IMO. Or alternatively introducing a kernel-config "I know what
> TSO is" option which is then used at netdev registration. From a
> usability perspective it would make more sense to just keep ethtool as
> the only way to configure TSO. 
> 
> [I recently spent a few days helping someone debug a problem with IFB
> because he was redirecting packets from an TSO netdevice and occasionaly
> some multi-packet will be missed in the calculation; my answer was "turn
> off TSO"; so there are more use cases for this TSO issue]. 

I totally disagree with these POVs:

- 10G cards should be treated by default as 10G cards - not DSL modems,
  and common users shouldn't have to read any warnings or configs to
  see this.
- tc with TBF or HTB are professional tools; there should be added some
  warnings to manuals. But trying to change the way they work because
  we think we know better what users want, and changing BTW some other
  things (making debugging this later a hell), is simply disrespectful
  for target users of these tools. There are some wrappers or "creators"
  invented for this. And, BTW, I think I've seen somewhere a system
  which does this this other way - with creators for professionals. So,
  you could be right with this too...

Cheers,
Jarek P.

RE: [PATCH] Disable TSO for non standard qdiscs

[Waskiewicz Jr, Peter P]

> I totally disagree with these POVs:
> 
> - 10G cards should be treated by default as 10G cards - not 
> DSL modems,
>   and common users shouldn't have to read any warnings or configs to
>   see this.
> - tc with TBF or HTB are professional tools; there should be 
> added some
>   warnings to manuals. But trying to change the way they work because
>   we think we know better what users want, and changing BTW some other
>   things (making debugging this later a hell), is simply disrespectful
>   for target users of these tools. There are some wrappers or 
> "creators"
>   invented for this. And, BTW, I think I've seen somewhere a system
>   which does this this other way - with creators for 
> professionals. So,
>   you could be right with this too...

Ok, maybe I'm not done quite yet.  Jarek is echo'ing my original point,
changing the behavior of the tool automatically (these qdiscs in
question) is not good for a normal end user.  It might be fine for
kernel developers, but not users of these tools, IMO.  A less disruptive
approach, such as a warning message printed when loading the qdisc if
TSO is enabled, and documenting recommended usage, I think is more
prudent here.

Cheers,
-PJ Waskiewicz

Re: [PATCH] Disable TSO for non standard qdiscs

[Andi Kleen]

On Fri, Feb 01, 2008 at 01:58:30PM -0800, Rick Jones wrote:
> >>Does this also imply that JumboFrames interacts badly with these qdiscs? 
> >>Or IPoIB with its 65000ish byte MTU?
> >
> >
> >Correct. Of course it is always relative to the link speed. So if your
> >link is 10x faster and your packets 10x bigger you can get similarly
> >smooth shaping.
> 
> If the later-in-thread mentioned person shaping for their DSL line 
> happens to have enabled JumboFrames on their GbE network, will/should 
> the qdisc negate that? 

I don't think so, mostly because jumbo frames are not enabled by
default.  I'm only concerned about usable defaults there -- if you
set non default options you should certainly know what you're doing.

There are other reasons to not use jumbo frames anyways; e.g. a lot 
of cards still do not support SG for them but only process 
them as a single continuous buffer in memory so you often run 
into memory fragmentation problems.

> Or is the qdisc currently assuming that the 
> remote end of the DSL will have asked for a smaller MSS?

First there are lots of different qdiscs that all do different things.
Take a look at net/sched/*. Then they usually don't strictly require particular
MTUs (or know anything about MSS), but tend to work better with smaller
MTUs because that allows more choices in packet scheduling.  Generally
the larger your packets the less they can be scheduled.

-Andi

Re: [PATCH] Disable TSO for non standard qdiscs

[Andi Kleen]

> Turning TSO off at netdev registration time with a warning will be a
> cleaner IMO. Or alternatively introducing a kernel-config "I know what

You mean the qdisc should force TSO off on the underlying device? 

> TSO is" option which is then used at netdev registration. From a
> usability perspective it would make more sense to just keep ethtool as
> the only way to configure TSO. 

Ok, reasonable point.

-Andi

Re: [PATCH] Disable TSO for non standard qdiscs

[Herbert Xu]

Andi Kleen <andi@firstfloor.org> wrote:
>> Then change TBF to use skb_gso_segment?  Be careful, the fact that
> 
> That doesn't help because it wants to interleave packets
> from different streams to get everything fair and smooth. The only 
> good way to handle that is to split it up and the simplest way to do 
> this is to just tell TCP to not do GSO in the first place.

Actually if we're going to do this I'd prefer you to call skb_gso_segment
instead because that lets us at least bypass netfilter which is one of
the key benefits of software GSO.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] Disable TSO for non standard qdiscs

[Herbert Xu]

Andi Kleen <andi@firstfloor.org> wrote:
> 
> Hmm, that would probably be possible for TBF, but I'm not sure this can be
> really done in a useful way for the more complicated qdiscs. Especially
> since they would likely need to turn on/off GSO regularly when dynamic
> circumstances change and there is not really a good way to affect a socket 
> after it was created.

You don't need to change the socket if you just call skb_gso_segment
when necessary.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] Disable TSO for non standard qdiscs

[Andi Kleen]

On Sun, Feb 03, 2008 at 09:57:10AM +1100, Herbert Xu wrote:
> Andi Kleen <andi@firstfloor.org> wrote:
> >> Then change TBF to use skb_gso_segment?  Be careful, the fact that
> > 
> > That doesn't help because it wants to interleave packets
> > from different streams to get everything fair and smooth. The only 
> > good way to handle that is to split it up and the simplest way to do 
> > this is to just tell TCP to not do GSO in the first place.
> 
> Actually if we're going to do this I'd prefer you to call skb_gso_segment
> instead because that lets us at least bypass netfilter which is one of
> the key benefits of software GSO.

Ok. The only problem I see right now is that skb_segment() seems to overuse
GFP_ATOMIC which would make it unreliable, but that is something that can be 
fixed.

-Andi