Re: [Cake] CAKE upstreaming - testers wanted, ACK filtering rescuers needed

[David Lang]

On Thu, 26 Apr 2018, Sebastian Moeller wrote:


On Apr 26, 2018, at 09:26, Jonathan Morton  wrote:


Genuine question:  I have a superpacket circa 64K, this is a lump of data in a 
tcp flow.  I have another small VOIP packet, it’s latency sensitive.  If I 
split the super packet into individual 1.5K packets as they would be on the 
wire, I can insert my VOIP packet at suitable place in time such that jitter 
targets are not exceeded.  If I don’t split the super packet, surely I have to 
wait till the end of the superpacket’s queue (for want of a better word) and 
possibly exceed my latency target.  That looks to me like ‘GSO/TSO’ is 
potentially bad for interflow latencies.



What don’t I understand here?


You have it exactly right.  For some reason, Eric is failing to consider the 
general case of flow-isolating queues at low link rates, and only considering 
high-rate FIFOs.

More to the point, at 64Kbps a maximal GSO packet can take 8 seconds to clear, 
while an MTU packet takes less than a quarter second!


	I venture a guess that it is not so much the concept but rather the 
terminology that irritates Eric? If at all Superpackets lighten the kernel's 
load and will in all likelihood make in-kernel processing faster which if at 
all decreases latency. The effect on flow fairness that we are concerned about 
might simply not register under "latency" for the kernel developers; I am sure 
they see the issue and this is really about which words to use to describe 
this... Also I point at the fact that Eric did not object to the feature per 
se, but rather the unconditionality and the rationale.


Keep in mind that he was also griping about the focus on <100M bandwidth limits 
and talking about >10G links. At those speeds, the large GSO packet is not a 
significant factor.


David Lang___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] CAKE upstreaming - testers wanted, ACK filtering rescuers needed

[Jonathan Morton]

> I really liked you initial idea to make the threshold when to segment a 
> superpacket based on the duration that packet would hogg the wire/shaper, as 
> that gives an intuitive feel for the worst case inter-flow latency induced. 
> Especially this would allow on many links intermediate sized superpackets to 
> survive fine while turning 64K "oil-tankers" into a fleet of speedboats ;). 
> This temporal threshold would also automatically solve the higher bandwdth 
> cases elegantly. What was the reason to rip that out again?

It probably had something to do with needing to scale that threshold with the 
number of active flows, which isn't necessarily known at enqueue time with 
sufficient accuracy to be relevant at dequeue, in order to be able to guarantee 
a given peak inter-flow induced latency.  Unconditional segmentation made the 
question easy and didn't seem to have very much effect on the CPU (at the link 
rates we were targeting).

It might still be reasonable to assume the number of active flows won't change 
*much* between enqueue and dequeue.  So then we could set the threshold at, 
say, half the Codel target divided by the number of active flows plus one.

 - Jonathan Morton

___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] CAKE upstreaming - testers wanted, ACK filtering rescuers needed

[Jonas Mårtensson]

I thought the discussion was only about GSO/TSO. Also, isn't GRO/LRO
incompatible with routing? Anyway, I was just supporting your
interpretation of what Eric potentially means.

/Jonas

On Thu, Apr 26, 2018 at 10:55 AM, Toke Høiland-Jørgensen 
wrote:

> Jonas Mårtensson  writes:
>
> > "I *think* that what Eric means is that the GSO logic should
> automatically
> > size the GSO superpackets so the latency cost is negligible for the
> actual
> > link rate."
> >
> > Something like this?
> >
> > https://lwn.net/Articles/564979/
> >
> > https://lwn.net/Articles/564978/
>
> Yeah, that's for TCP on the local host. I'm not sure how things work for
> GRO on a router (which is more relevant for the CAKE use case).
>
> -Toke
>
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] CAKE upstreaming - testers wanted, ACK filtering rescuers needed

[Toke Høiland-Jørgensen]

Jonas Mårtensson  writes:

> "I *think* that what Eric means is that the GSO logic should automatically
> size the GSO superpackets so the latency cost is negligible for the actual
> link rate."
>
> Something like this?
>
> https://lwn.net/Articles/564979/
>
> https://lwn.net/Articles/564978/

Yeah, that's for TCP on the local host. I'm not sure how things work for
GRO on a router (which is more relevant for the CAKE use case).

-Toke
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] CAKE upstreaming - testers wanted, ACK filtering rescuers needed

[Kevin Darbyshire-Bryant via Cake]

--- Begin Message ---


> On 26 Apr 2018, at 08:26, Jonathan Morton  wrote:
> 
>> Genuine question:  I have a superpacket circa 64K, this is a lump of data in 
>> a tcp flow.  I have another small VOIP packet, it’s latency sensitive.  If I 
>> split the super packet into individual 1.5K packets as they would be on the 
>> wire, I can insert my VOIP packet at suitable place in time such that jitter 
>> targets are not exceeded.  If I don’t split the super packet, surely I have 
>> to wait till the end of the superpacket’s queue (for want of a better word) 
>> and possibly exceed my latency target.  That looks to me like ‘GSO/TSO’ is 
>> potentially bad for interflow latencies.
> 
>> What don’t I understand here?
> 
> You have it exactly right.  For some reason, Eric is failing to consider the 
> general case of flow-isolating queues at low link rates, and only considering 
> high-rate FIFOs.

Thanks.  Well that’s a relief - Got something right!  The day can only go 
downhill from here :-)

Have to say, Sebastian’s analogy "turning 64K "oil-tankers" into a fleet of 
speedboats “ really made me smile.  Of course the cost is CPU, fortunately no 
risk of oil spillage :-)


signature.asc
Description: Message signed with OpenPGP
--- End Message ---
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] CAKE upstreaming - testers wanted, ACK filtering rescuers needed

[Toke Høiland-Jørgensen]

Kevin Darbyshire-Bryant  writes:

>> On 25 Apr 2018, at 21:45, Toke Høiland-Jørgensen  wrote:
>> 
>> For those who have not been following the discussion on the upstreaming
>> patches, here's an update:
>> 
>> 
>> 
>> So please do test the current git version (cobalt branch, still). I'm
>> planning to resubmit on Friday.
>
> The two routers running that latest code survived the night which is a
> good sign.

Awesome!

> I’ve sort of half been following the ‘discussion’, as ever the
> reaction from the kernel people makes it a place I never wish to
> look/contribute, ….. this from Eric has me disturbed "If you keep
> saying this old urban legend, I will NACK your patch.I am tired of
> people pretending GSO/TSO are bad for latencies.”

Heh, yeah, the tone on kernel lists can be a bit... abrasive... just
smile and wave and ignore the vitriol is my approach. But I can totally
understand why some people don't want to put up with it... :)

> Genuine question:  I have a superpacket circa 64K, this is a lump of
> data in a tcp flow.  I have another small VOIP packet, it’s latency
> sensitive.  If I split the super packet into individual 1.5K packets
> as they would be on the wire, I can insert my VOIP packet at suitable
> place in time such that jitter targets are not exceeded.  If I don’t
> split the super packet, surely I have to wait till the end of the
> superpacket’s queue (for want of a better word) and possibly exceed my
> latency target.  That looks to me like ‘GSO/TSO’ is potentially bad
> for interflow latencies.  What don’t I understand here?

You are right in principle, of course. I *think* that what Eric means is
that the GSO logic should automatically size the GSO superpackets so the
latency cost is negligible for the actual link rate. I was actually
thinking I would do some measurements at some point to test this at
various rates; since we have a nice piece of code that can adaptively
split GSO packets that should be pretty straight-forward :)

-Toke
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] CAKE upstreaming - testers wanted, ACK filtering rescuers needed

[Jonathan Morton]

> Genuine question:  I have a superpacket circa 64K, this is a lump of data in 
> a tcp flow.  I have another small VOIP packet, it’s latency sensitive.  If I 
> split the super packet into individual 1.5K packets as they would be on the 
> wire, I can insert my VOIP packet at suitable place in time such that jitter 
> targets are not exceeded.  If I don’t split the super packet, surely I have 
> to wait till the end of the superpacket’s queue (for want of a better word) 
> and possibly exceed my latency target.  That looks to me like ‘GSO/TSO’ is 
> potentially bad for interflow latencies.

> What don’t I understand here?

You have it exactly right.  For some reason, Eric is failing to consider the 
general case of flow-isolating queues at low link rates, and only considering 
high-rate FIFOs.

More to the point, at 64Kbps a maximal GSO packet can take 8 seconds to clear, 
while an MTU packet takes less than a quarter second!

 - Jonathan Morton

___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] CAKE upstreaming - testers wanted, ACK filtering rescuers needed

[Toke Høiland-Jørgensen]

Ryan Mounce  writes:

>> On 26 Apr 2018, at 16:09, Toke Høiland-Jørgensen  wrote:
>>
>> Ryan Mounce  writes:
>>
>>> I'll investigate making the ACK filtering code safe, it is my mess after 
>>> all :)
>>>
>>> Eric obviously understands this stuff a lot better than me, it looks
>>> like there are two issues?
>>> - Lack of minimum length check for TCP header, should be fairly
>>> straight-forward to fix
>>> - The possibility of unsafely filtering part of a split GSO
>>> super-packet?
>>
>> The issue with the ACK filter in relation to GSO was just that a GSO
>> segment can't contain pure ACKs, so there's no reason to check after
>> splitting. I've already removed that part, so it's just the length
>> issue. I think it's just a matter of finding the length and calling
>> pskb_may_pull() (and aborting if that returns false).
>>
>> -Toke
>
> Yep I think I’ve got all of that together now and I’ve submitted a PR
> against the cobalt branch.

Awesome thanks! Only a small nit, which I commented on the PR.

> An optimisation to what I’ve submitted would be moving the separate
> TCP header length check to only the IPv4 case, and adding
> sizeof(tcphdr) to the initial IPv6 header length check. Not that my
> ACK filtering code is so efficient to begin with...

Yeah, might as well?

Another obvious optimisation would be to add a flag to the cb struct for
pure ACKs (which would be set at the beginning, when ack_check is called
on packet enqueue). That way the loop could skip over anything that is
not marked as a pure ACK and a lot of checks could be skipped for the
packets being iterated over in the queue...

-Toke
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] CAKE upstreaming - testers wanted, ACK filtering rescuers needed

[Ryan Mounce]

> On 26 Apr 2018, at 16:09, Toke Høiland-Jørgensen  wrote:
>
> Ryan Mounce  writes:
>
>> I'll investigate making the ACK filtering code safe, it is my mess after all 
>> :)
>>
>> Eric obviously understands this stuff a lot better than me, it looks
>> like there are two issues?
>> - Lack of minimum length check for TCP header, should be fairly
>> straight-forward to fix
>> - The possibility of unsafely filtering part of a split GSO
>> super-packet?
>
> The issue with the ACK filter in relation to GSO was just that a GSO
> segment can't contain pure ACKs, so there's no reason to check after
> splitting. I've already removed that part, so it's just the length
> issue. I think it's just a matter of finding the length and calling
> pskb_may_pull() (and aborting if that returns false).
>
> -Toke

Yep I think I’ve got all of that together now and I’ve submitted a PR
against the cobalt branch.

An optimisation to what I’ve submitted would be moving the separate
TCP header length check to only the IPv4 case, and adding
sizeof(tcphdr) to the initial IPv6 header length check. Not that my
ACK filtering code is so efficient to begin with...

Ryan
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] CAKE upstreaming - testers wanted, ACK filtering rescuers needed

[Toke Høiland-Jørgensen]

Ryan Mounce  writes:

> I'll investigate making the ACK filtering code safe, it is my mess after all 
> :)
>
> Eric obviously understands this stuff a lot better than me, it looks
> like there are two issues?
> - Lack of minimum length check for TCP header, should be fairly
> straight-forward to fix
> - The possibility of unsafely filtering part of a split GSO
> super-packet?

The issue with the ACK filter in relation to GSO was just that a GSO
segment can't contain pure ACKs, so there's no reason to check after
splitting. I've already removed that part, so it's just the length
issue. I think it's just a matter of finding the length and calling
pskb_may_pull() (and aborting if that returns false).

-Toke
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] CAKE upstreaming - testers wanted, ACK filtering rescuers needed

[Dave Taht]

On Wed, Apr 25, 2018 at 6:32 PM, Stephen Hemminger
 wrote:
> On Thu, 26 Apr 2018 10:40:29 +0930
> Ryan Mounce  wrote:
>
>> I'll investigate making the ACK filtering code safe, it is my mess after all 
>> :)
>>
>> Eric obviously understands this stuff a lot better than me, it looks
>> like there are two issues?
>> - Lack of minimum length check for TCP header, should be fairly
>> straight-forward to fix
>> - The possibility of unsafely filtering part of a split GSO super-packet?
>>
>> Regards,
>> Ryan Mounce
>>
>> r...@mounce.com.au
>> 0415 799 929
>
>
> It makes more sense to move it out of Cake altogether. Cake is already one
> of those projects that seems to have Golden Hammer disease.
>   https://en.wikipedia.org/wiki/Law_of_the_instrument
>
> Why not in netfilter or XDP? Then it could be used in other contexts like
> tunnels or selectively based on route etc.

'cause it needs a queue and state. netfilter doesn't have a queue.

 ___
> Cake mailing list
> Cake@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/cake



-- 

Dave Täht
CEO, TekLibre, LLC
http://www.teklibre.com
Tel: 1-669-226-2619
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] CAKE upstreaming - testers wanted, ACK filtering rescuers needed

[Stephen Hemminger]

On Thu, 26 Apr 2018 10:40:29 +0930
Ryan Mounce  wrote:

> I'll investigate making the ACK filtering code safe, it is my mess after all 
> :)
> 
> Eric obviously understands this stuff a lot better than me, it looks
> like there are two issues?
> - Lack of minimum length check for TCP header, should be fairly
> straight-forward to fix
> - The possibility of unsafely filtering part of a split GSO super-packet?
> 
> Regards,
> Ryan Mounce
> 
> r...@mounce.com.au
> 0415 799 929


It makes more sense to move it out of Cake altogether. Cake is already one
of those projects that seems to have Golden Hammer disease.
  https://en.wikipedia.org/wiki/Law_of_the_instrument

Why not in netfilter or XDP? Then it could be used in other contexts like
tunnels or selectively based on route etc.
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] CAKE upstreaming - testers wanted, ACK filtering rescuers needed

[Ryan Mounce]

I'll investigate making the ACK filtering code safe, it is my mess after all :)

Eric obviously understands this stuff a lot better than me, it looks
like there are two issues?
- Lack of minimum length check for TCP header, should be fairly
straight-forward to fix
- The possibility of unsafely filtering part of a split GSO super-packet?

Regards,
Ryan Mounce

r...@mounce.com.au
0415 799 929


On 26 April 2018 at 06:15, Toke Høiland-Jørgensen  wrote:
> For those who have not been following the discussion on the upstreaming
> patches, here's an update:
>
> - I've just pushed patches to only split GSO packets when shaping below
>   one gigabit; and hopefully made the overhead compensation code deal
>   gracefully with GSO packets if someone for some reason wants to use
>   the shaper at speeds higher than that and still use the overhead
>   compensation code.
>
> - It turns out that the ACK filtering code does not properly sanity
>   check the packet sizes, and so can potentially crash the box running
>   CAKE if it receives malformed packets. So if no one steps up to fix
>   that within the next few days, or I'll submit the next version without
>   it (I'm not going to open that particular can of worms)... This
>   doesn't mean it can't be added back later, of course, it just means it
>   won't go upstream this time around.
>
> - NAT mode is now enabled by default; doesn't seem to be a good reason
>   not to as the compile time dependency already makes the module depend
>   on conntrack.
>
>
> So please do test the current git version (cobalt branch, still). I'm
> planning to resubmit on Friday.
>
> -Toke
> ___
> Cake mailing list
> Cake@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/cake
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake