[Cake] Backporting 'tc class' support

[Cake] Backporting 'tc class' support

[Toke Høiland-Jørgensen]

Now that CAKE has been accepted upstream, I figured it was a good time
to backport the 'tc class' support. So I did, back to kernel v4.9.

This is in the master branch; anyone feel like testing? With this, the
version of CAKE in the master branch should be identical to the version
that will be in Linux 4.19 :)

-Toke

Re: [Cake] Backporting 'tc class' support

[Kevin Darbyshire-Bryant]

[-- Attachment #1.1: Type: text/plain, Size: 576 bytes --]



> On 14 Jul 2018, at 22:50, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> 
> Now that CAKE has been accepted upstream, I figured it was a good time
> to backport the 'tc class' support. So I did, back to kernel v4.9.
> 
> This is in the master branch; anyone feel like testing? With this, the
> version of CAKE in the master branch should be identical to the version
> that will be in Linux 4.19 :)

I need the attached patch to get it to build on openwrt - it looks like an include guard order thing.

Though it builds, not yet run in any way ;-)

KDB


[-- Attachment #1.2: 0001-Fix-build-on-Openwrt.patch --]
[-- Type: application/octet-stream, Size: 793 bytes --]

From ad2063e926d57f1a0f4cae8d1fdb3ba7ede6ec23 Mon Sep 17 00:00:00 2001
From: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
Date: Sun, 15 Jul 2018 01:17:18 +0100
Subject: [PATCH] Fix build on Openwrt

Signed-off-by: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
---
 sch_cake.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sch_cake.c b/sch_cake.c
index dfc967e..012af95 100644
--- a/sch_cake.c
+++ b/sch_cake.c
@@ -65,8 +65,8 @@
 #include <linux/reciprocal_div.h>
 #include <net/netlink.h>
 #include <linux/version.h>
-#include <linux/if_vlan.h>
 #include "pkt_sched.h"
+#include <linux/if_vlan.h>
 #include <net/pkt_cls.h>
 #include <net/tcp.h>
 #if LINUX_VERSION_CODE < KERNEL_VERSION(4, 2, 0)
-- 
2.15.2 (Apple Git-101.1)


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Cake] Backporting 'tc class' support

[Toke Høiland-Jørgensen]

Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:

>> On 14 Jul 2018, at 22:50, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>> 
>> Now that CAKE has been accepted upstream, I figured it was a good time
>> to backport the 'tc class' support. So I did, back to kernel v4.9.
>> 
>> This is in the master branch; anyone feel like testing? With this, the
>> version of CAKE in the master branch should be identical to the version
>> that will be in Linux 4.19 :)
>
> I need the attached patch to get it to build on openwrt - it looks
> like an include guard order thing.

Ah, right, thanks! Fixed in master :)

-Toke

Re: [Cake] Backporting 'tc class' support

[Kevin Darbyshire-Bryant]

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



> On 15 Jul 2018, at 10:48, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> 
> Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:
> 
>>> On 14 Jul 2018, at 22:50, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>>> 
>>> Now that CAKE has been accepted upstream, I figured it was a good time
>>> to backport the 'tc class' support. So I did, back to kernel v4.9.
>>> 
>>> This is in the master branch; anyone feel like testing? With this, the
>>> version of CAKE in the master branch should be identical to the version
>>> that will be in Linux 4.19 :)
>> 
>> I need the attached patch to get it to build on openwrt - it looks
>> like an include guard order thing.
> 
> Ah, right, thanks! Fixed in master :)

And now that I’ve run it, with Georgios’ help (I’ve never played with tc filters before!) I’ve fallen over a wrinkle:

So using sqm-scripts I have my standard cake instances on eth0 and ifb4eth0, both using diffserv3 <<— diffserv3 is important.  This creates according to tc -s qdisc Bulk, Best Effort & Voice tins. (where is he going with this?)

For ‘fun’ I wanted to classify stuff incoming to my bittorrent port as Bulk.  So you’d think that "tc filter add dev ifb4eth0 parent 8011: protocol ip u32 match ip dport 6981 0xffff action skbedit priority 8011:1” would do the trick.  8011:1 being the target tin.  Whilst syntactically correct you’d be disappointed by the result ‘cos…..diffserv3 & 4 put the bulk traffic in tin 2 although tc displays it as the first tin.

This is a bit horrible.  To fix we need to drop the ‘out of order’ tin dump in cake, and tweak the column heading orders in tc. e.g. from fprintf(f, "                   Bulk  Best Effort        Voice\n");
          to  fprintf(f, "            Best Effort         Bulk        Voice\n");

and break the ABI in the process?

I’ll go and fetch my coat.

KDB


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Cake] Backporting 'tc class' support

[Toke Høiland-Jørgensen]

Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:

>> On 15 Jul 2018, at 10:48, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>> 
>> Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:
>> 
>>>> On 14 Jul 2018, at 22:50, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>>>> 
>>>> Now that CAKE has been accepted upstream, I figured it was a good time
>>>> to backport the 'tc class' support. So I did, back to kernel v4.9.
>>>> 
>>>> This is in the master branch; anyone feel like testing? With this, the
>>>> version of CAKE in the master branch should be identical to the version
>>>> that will be in Linux 4.19 :)
>>> 
>>> I need the attached patch to get it to build on openwrt - it looks
>>> like an include guard order thing.
>> 
>> Ah, right, thanks! Fixed in master :)
>
> And now that I’ve run it, with Georgios’ help (I’ve never played with
> tc filters before!) I’ve fallen over a wrinkle:
>
> So using sqm-scripts I have my standard cake instances on eth0 and
> ifb4eth0, both using diffserv3 <<— diffserv3 is important. This
> creates according to tc -s qdisc Bulk, Best Effort & Voice tins.
> (where is he going with this?)
>
> For ‘fun’ I wanted to classify stuff incoming to my bittorrent port as
> Bulk.  So you’d think that "tc filter add dev ifb4eth0 parent 8011:
> protocol ip u32 match ip dport 6981 0xffff action skbedit priority
> 8011:1” would do the trick.  8011:1 being the target tin.  Whilst
> syntactically correct you’d be disappointed by the result
> ‘cos…..diffserv3 & 4 put the bulk traffic in tin 2 although tc
> displays it as the first tin.

Yeah, the tins are displayed in a different order than they are indexed.
See the bulk_order and normal_order definitions. Basically, the first
two are switched.

It's not actually obvious which is the right thing to do here? Use the
classifier output as the tin index, or modify it by the tin order... In
fact, I'm not quite sure what the purpose of the tin_order is in the
first place...

Jonathan, care to comment? :)

-Toke

Re: [Cake] Backporting 'tc class' support

[Kevin Darbyshire-Bryant]

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



> On 15 Jul 2018, at 21:23, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> 
> Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:
> 
>>> On 15 Jul 2018, at 10:48, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>>> 
>>> Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:
>>> 
>>>>> On 14 Jul 2018, at 22:50, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>>>>> 
>>>>> Now that CAKE has been accepted upstream, I figured it was a good time
>>>>> to backport the 'tc class' support. So I did, back to kernel v4.9.
>>>>> 
>>>>> This is in the master branch; anyone feel like testing? With this, the
>>>>> version of CAKE in the master branch should be identical to the version
>>>>> that will be in Linux 4.19 :)
>>>> 
>>>> I need the attached patch to get it to build on openwrt - it looks
>>>> like an include guard order thing.
>>> 
>>> Ah, right, thanks! Fixed in master :)
>> 
>> And now that I’ve run it, with Georgios’ help (I’ve never played with
>> tc filters before!) I’ve fallen over a wrinkle:
>> 
>> So using sqm-scripts I have my standard cake instances on eth0 and
>> ifb4eth0, both using diffserv3 <<— diffserv3 is important. This
>> creates according to tc -s qdisc Bulk, Best Effort & Voice tins.
>> (where is he going with this?)
>> 
>> For ‘fun’ I wanted to classify stuff incoming to my bittorrent port as
>> Bulk.  So you’d think that "tc filter add dev ifb4eth0 parent 8011:
>> protocol ip u32 match ip dport 6981 0xffff action skbedit priority
>> 8011:1” would do the trick.  8011:1 being the target tin.  Whilst
>> syntactically correct you’d be disappointed by the result
>> ‘cos…..diffserv3 & 4 put the bulk traffic in tin 2 although tc
>> displays it as the first tin.
> 
> Yeah, the tins are displayed in a different order than they are indexed.
> See the bulk_order and normal_order definitions. Basically, the first
> two are switched.
> 
> It's not actually obvious which is the right thing to do here? Use the
> classifier output as the tin index, or modify it by the tin order... In
> fact, I'm not quite sure what the purpose of the tin_order is in the
> first place…

IIRC in the old days tin 0 was always a 100% rate tin and bandwidth usage was charged to all lower tins, thus it would have been a bad idea to have bulk as being 100% rate, hence the swap for ‘bulk tin’ diffserv patterns.

That is no longer the case, see commits 423112e,  4f62bd1 and 008a276 at least :-)

AFAICT the tin order makes no difference whatsoever these days, indeed the dequeue mechanism picks up from the last tin and spins around rather than starting at either 0 or highest tin each time.

> 
> Jonathan, care to comment? :)
> 
> -Toke



[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Cake] Backporting 'tc class' support

[Jonathan Morton]

> On 16 Jul, 2018, at 1:41 am, Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> wrote:
> 
> AFAICT the tin order makes no difference whatsoever these days, indeed the dequeue mechanism picks up from the last tin and spins around rather than starting at either 0 or highest tin each time.

While it might not be entirely apparent from the code, the tin selector has two-phase behaviour, and this makes the tin ordering important.

If *any* tin contains traffic *and* meets its local schedule, the *highest* such tin is selected unconditionally.  Only if *no* such tin can be found does the search revert to finding the tin with traffic and with the most-nearly-satisfied schedule.  Both conditions are resolved on a single pass through the tin array.

In diffserv3/4, the Best Effort tin is set at the same rate as the global shaper.  Therefore, its local schedule is always met whenever the global schedule is.  Therefore, if it were placed higher in the tin list than Bulk, it would be able to strictly starve out Bulk traffic, which is not what I want.  Instead I give Bulk the next higher tin and a very restrictive schedule.

 - Jonathan Morton

Re: [Cake] Backporting 'tc class' support

[Dave Taht]

I am curious if the new tc class support would allow for recognizing
sebastian's desire to have the llc (?) dsl control packets moved to
the higher priority tin?

Similarly, there were other things in early-sqm (like deprioritizing
ping, prioritizing dns) that I liked, but disliked using iptables to
use.

lastly, the hopelessly overloaded skb->priority field is hopefully
still not recognized for anything that actually sets skb->priority
naively?

Re: [Cake] Backporting 'tc class' support

[Toke Høiland-Jørgensen]

Dave Taht <dave.taht@gmail.com> writes:

> I am curious if the new tc class support would allow for recognizing
> sebastian's desire to have the llc (?) dsl control packets moved to
> the higher priority tin?
>
> Similarly, there were other things in early-sqm (like deprioritizing
> ping, prioritizing dns) that I liked, but disliked using iptables to
> use.

You can write a tc filter in BPF, which allows you to do arbitrary
matching on packet data... So as long as the packets go through the
qdisc, the answer would be yes :)

> lastly, the hopelessly overloaded skb->priority field is hopefully
> still not recognized for anything that actually sets skb->priority
> naively?

CAKE will only react to the priority field if the upper 16 bits match
the major number of the qdisc; so anything that sets priority has to
know about the qdisc configuration for it to stick...

-Toke

Re: [Cake] Backporting 'tc class' support

[Toke Høiland-Jørgensen]

Jonathan Morton <chromatix99@gmail.com> writes:

>> On 16 Jul, 2018, at 1:41 am, Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> wrote:
>> 
>> AFAICT the tin order makes no difference whatsoever these days,
>> indeed the dequeue mechanism picks up from the last tin and spins
>> around rather than starting at either 0 or highest tin each time.
>
> While it might not be entirely apparent from the code, the tin
> selector has two-phase behaviour, and this makes the tin ordering
> important.
>
> If *any* tin contains traffic *and* meets its local schedule, the
> *highest* such tin is selected unconditionally. Only if *no* such tin
> can be found does the search revert to finding the tin with traffic
> and with the most-nearly-satisfied schedule. Both conditions are
> resolved on a single pass through the tin array.
>
> In diffserv3/4, the Best Effort tin is set at the same rate as the
> global shaper. Therefore, its local schedule is always met whenever
> the global schedule is. Therefore, if it were placed higher in the tin
> list than Bulk, it would be able to strictly starve out Bulk traffic,
> which is not what I want. Instead I give Bulk the next higher tin and
> a very restrictive schedule.

Right, this makes sense.

Still doesn't give an obvious answer to what the semantics for a filter
should be, though. On the one hand, everywhere else we expose tins to
userspace, we obey tin_order. On the other hand, the "first tin is the
default (best effort)" semantic is used for the internal matching, such
as diffserv.

I'm leaning towards making tc filter classification obey tin_order;
opinions? :)

-Toke

Re: [Cake] Backporting 'tc class' support

[Jonathan Morton]

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

On 12:30pm, Mon, 16 Jul 2018 Toke Høiland-Jørgensen, <toke@toke.dk> wrote:

> I'm leaning towards making tc filter classification obey tin_order;
> opinions? :)

Sounds fine to me.

- Jonathan Morton

[-- Attachment #2: Type: text/html, Size: 333 bytes --]

Re: [Cake] Backporting 'tc class' support

[Toke Høiland-Jørgensen]

Jonathan Morton <chromatix99@gmail.com> writes:

> On 12:30pm, Mon, 16 Jul 2018 Toke Høiland-Jørgensen, <toke@toke.dk> wrote:
>
>> I'm leaning towards making tc filter classification obey tin_order;
>> opinions? :)
>
> Sounds fine to me.

Right, pushed an update to this effect. Will submit it upstream as well
once someone has verified that it does what it's supposed to :)

-Toke

Re: [Cake] Backporting 'tc class' support

[Georgios Amanakis]

On Mon, 2018-07-16 at 12:18 +0200, Toke Høiland-Jørgensen wrote:
> Right, pushed an update to this effect. Will submit it upstream as
> well
> once someone has verified that it does what it's supposed to :)

I just verified it works as it should on diffserv3.

# tc qdisc add dev ens4 root handle 8011 cake bandwidth 18000kbit dual-
dsthost docsis ingress
# tc filter add dev ens4 parent 8011: protocol all u32 match u32 0 0
action skbedit priority 8011:1

In the second command: 
skbedit priority 8011:1 --> Bulk
skbedit priority 8011:2 --> Best Effort
skbedit priority 8011:3 --> Voice

George

Re: [Cake] Backporting 'tc class' support

[Toke Høiland-Jørgensen]

Georgios Amanakis <gamanakis@gmail.com> writes:

> On Mon, 2018-07-16 at 12:18 +0200, Toke Høiland-Jørgensen wrote:
>> Right, pushed an update to this effect. Will submit it upstream as
>> well
>> once someone has verified that it does what it's supposed to :)
>
> I just verified it works as it should on diffserv3.
>
> # tc qdisc add dev ens4 root handle 8011 cake bandwidth 18000kbit dual-
> dsthost docsis ingress
> # tc filter add dev ens4 parent 8011: protocol all u32 match u32 0 0
> action skbedit priority 8011:1
>
> In the second command: 
> skbedit priority 8011:1 --> Bulk
> skbedit priority 8011:2 --> Best Effort
> skbedit priority 8011:3 --> Voice

Great, thanks for testing! :)

-Toke

Re: [Cake] Backporting 'tc class' support

[Kevin Darbyshire-Bryant]

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



> On 16 Jul 2018, at 13:28, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> 
> Georgios Amanakis <gamanakis@gmail.com> writes:
> 
>> On Mon, 2018-07-16 at 12:18 +0200, Toke Høiland-Jørgensen wrote:
>>> Right, pushed an update to this effect. Will submit it upstream as
>>> well
>>> once someone has verified that it does what it's supposed to :)
>> 
>> I just verified it works as it should on diffserv3.
>> 
>> # tc qdisc add dev ens4 root handle 8011 cake bandwidth 18000kbit dual-
>> dsthost docsis ingress
>> # tc filter add dev ens4 parent 8011: protocol all u32 match u32 0 0
>> action skbedit priority 8011:1
>> 
>> In the second command:
>> skbedit priority 8011:1 --> Bulk
>> skbedit priority 8011:2 --> Best Effort
>> skbedit priority 8011:3 --> Voice
> 
> Great, thanks for testing! :)

And I can confirm that my bittorrent redirect scenario that started this 15 hours ago is solved too :-)

Thanks Toke.

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


Cheers,

Kevin D-B

012C ACB2 28C6 C53E 9775  9123 B3A2 389B 9DE2 334A


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]