[Cake] Correct 'change' behaviour

[Cake] Correct 'change' behaviour

[Toke Høiland-Jørgensen]

So we've seen a couple of issues with reconfiguration being weird (i.e.
retaining previous values). Looking at the code in cake_change() and the
iproute2 code, this seems to stem from the fact that tc only sets the
parameters (in the netlink message) that are actually specified on the
command line, and the kernel only changes the parameters specified in
the netlink message.

This leads to the different behaviour: On init, not specifying a
parameter means "give me a sensible default", whereas on a reconfigure
it means "don't change this parameter from its current value". This is
obviously inconsistent. I'd argue that the right thing to do is the
former in all cases; is there any reason why that would not be the case?
If not, I'll go fix it :)

-Toke

Re: [Cake] Correct 'change' behaviour

[Kevin Darbyshire-Bryant]

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



On 03/11/15 12:59, Toke Høiland-Jørgensen wrote:
> So we've seen a couple of issues with reconfiguration being weird (i.e.
> retaining previous values). Looking at the code in cake_change() and the
> iproute2 code, this seems to stem from the fact that tc only sets the
> parameters (in the netlink message) that are actually specified on the
> command line, and the kernel only changes the parameters specified in
> the netlink message.
>
> This leads to the different behaviour: On init, not specifying a
> parameter means "give me a sensible default", whereas on a reconfigure
> it means "don't change this parameter from its current value". This is
> obviously inconsistent. I'd argue that the right thing to do is the
> former in all cases; is there any reason why that would not be the case?
> If not, I'll go fix it :)

Interestingly I think the opposite.  On a change parameters should be
left alone unless they're the thing I'm changing.  It's a 'change' to an
instantiation, *not* an instantiation if you get my drift.

Kevin



[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4816 bytes --]

Re: [Cake] Correct 'change' behaviour

[Toke Høiland-Jørgensen]

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

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

> Interestingly I think the opposite. On a change parameters should be
> left alone unless they're the thing I'm changing. It's a 'change' to
> an instantiation, *not* an instantiation if you get my drift.

Yes, that is indeed implied in the 'change' verb. However, the tc verb
is 'replace'. If we go with the "leave things alone" paradigm, there
will need to be some way to explicitly specify defaults...

-Toke

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 472 bytes --]

Re: [Cake] Correct 'change' behaviour

[Jonathan Morton]

> On 3 Nov, 2015, at 15:14, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> 
> Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:
> 
>> Interestingly I think the opposite. On a change parameters should be
>> left alone unless they're the thing I'm changing. It's a 'change' to
>> an instantiation, *not* an instantiation if you get my drift.
> 
> Yes, that is indeed implied in the 'change' verb. However, the tc verb
> is 'replace'. If we go with the "leave things alone" paradigm, there
> will need to be some way to explicitly specify defaults…

But tc has both “change” and “replace” verbs.  They should do different things; the latter implies removing an existing qdisc and inserting a new one.

 - Jonathan Morton

Re: [Cake] Correct 'change' behaviour

[Jonathan Morton]

> On 3 Nov, 2015, at 19:29, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> 
> Well, there's some evidence to suggest that this is not in fact the case:

I would consider that a tc bug.

 - Jonathan Morton

Re: [Cake] Correct 'change' behaviour

[Toke Høiland-Jørgensen]

Jonathan Morton <chromatix99@gmail.com> writes:

>> On 3 Nov, 2015, at 19:29, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>> 
>> Well, there's some evidence to suggest that this is not in fact the case:
>
> I would consider that a tc bug.

Well, tc does seem to be doing the right thing:

	if (matches(*argv, "change") == 0)
		return tc_qdisc_modify(RTM_NEWQDISC, 0, argc-1, argv+1);
	if (matches(*argv, "replace") == 0)
		return tc_qdisc_modify(RTM_NEWQDISC, NLM_F_CREATE|NLM_F_REPLACE, argc-1, argv+1);

However, this comment in sch_api.c in the kernel seems to indicate that
they are considered equivalent by the kernel:

				/* This magic test requires explanation.
				 *
				 *   We know, that some child q is already
				 *   attached to this parent and have choice:
				 *   either to change it or to create/graft new one.
				 *
				 *   1. We are allowed to create/graft only
				 *   if CREATE and REPLACE flags are set.
				 *
				 *   2. If EXCL is set, requestor wanted to say,
				 *   that qdisc tcm_handle is not expected
				 *   to exist, so that we choose create/graft too.
				 *
				 *   3. The last case is when no flags are set.
				 *   Alas, it is sort of hole in API, we
				 *   cannot decide what to do unambiguously.
				 *   For now we select create/graft, if
				 *   user gave KIND, which does not match existing.
				 */

but the comment doesn't seem to match the subsequent test:

				if ((n->nlmsg_flags & NLM_F_CREATE) &&
				    (n->nlmsg_flags & NLM_F_REPLACE) &&
				    ((n->nlmsg_flags & NLM_F_EXCL) ||
				     (tca[TCA_KIND] &&
				      nla_strcmp(tca[TCA_KIND], q->ops->id))))
					goto create_n_graft;


as best I can follow the logic in that function, it *should* actually
get recreated when those flags are set.

So why on earth are the values wrong? Memory initialisation problem?

-Toke

Re: [Cake] Correct 'change' behaviour

[Kevin Darbyshire-Bryant]

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

On 03/11/15 13:14, Toke Høiland-Jørgensen wrote:
> Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:
>
>> Interestingly I think the opposite. On a change parameters should be
>> left alone unless they're the thing I'm changing. It's a 'change' to
>> an instantiation, *not* an instantiation if you get my drift.
> Yes, that is indeed implied in the 'change' verb. However, the tc verb
> is 'replace'. If we go with the "leave things alone" paradigm, there
> will need to be some way to explicitly specify defaults...
>
> -Toke
Toke,

At the risk of bringing up a recent unfortunate bit of change
management, there was a lot of discussion/confusion over calculated
targets/intervals due to a change that ended up using uninitialised
variables in those calculations.  Certainly I remember 95mS targets
being a symptom of that amongst other things, and I also remember there
being a 'default' statement of 'that's a tc issue' when it was nothing
of the sort.

I'd like more evidence of there being an issue (steps to repro) and how
it manifests before stating 'bug in tc change/replace' whatever.  Call
me pedantic

Kevin (the pedantic)


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4816 bytes --]

Re: [Cake] Correct 'change' behaviour

[Toke Høiland-Jørgensen]

Toke Høiland-Jørgensen <toke@toke.dk> writes:

> as best I can follow the logic in that function, it *should* actually
> get recreated when those flags are set.
>
> So why on earth are the values wrong? Memory initialisation problem?

Well, instrumenting cake with some good old-fashioned printk debug
statements, it appears that a replace when it's already in place does in
fact only call cake_change, and not cake_destroy followed by cake_init.

Now *why* that is I'm not entirely sure...

-Toke

Re: [Cake] Correct 'change' behaviour

[Dave Taht]

in the general case, I recomend looking at the most current fq_codel
code to see how to do it as right as possible. There were several bugs
found and fixed in it as well....
Dave Täht
I just invested five years of my life to making wifi better. And,
now... the FCC wants to make my work, illegal for people to install.
https://www.gofundme.com/savewifi


On Tue, Nov 3, 2015 at 2:07 PM, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> Toke Høiland-Jørgensen <toke@toke.dk> writes:
>
>> as best I can follow the logic in that function, it *should* actually
>> get recreated when those flags are set.
>>
>> So why on earth are the values wrong? Memory initialisation problem?
>
> Well, instrumenting cake with some good old-fashioned printk debug
> statements, it appears that a replace when it's already in place does in
> fact only call cake_change, and not cake_destroy followed by cake_init.
>
> Now *why* that is I'm not entirely sure...
>
> -Toke
> _______________________________________________
> Cake mailing list
> Cake@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] Correct 'change' behaviour

[Toke Høiland-Jørgensen]

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

> in the general case, I recomend looking at the most current fq_codel
> code to see how to do it as right as possible. There were several bugs
> found and fixed in it as well....

Well, I did, and it doesn't seem to be doing anything different. So much
so that I tried the same experiment on fq_codel:

$ sudo tc qdisc del dev eno1 root
$ sudo tc qdisc replace dev eno1 root fq_codel target 100ms interval 200ms
$ sudo tc qdisc replace dev eno1 root fq_codel target 5ms                 
$ tc qdisc
qdisc fq_codel 8007: dev eno1 root refcnt 2 limit 10240p flows 1024 quantum 1514 target 5.0ms interval 200.0ms ecn 
$ sudo tc qdisc del dev eno1 root                                         
$ sudo tc qdisc replace dev eno1 root fq_codel target 5ms
$ tc qdisc                                          
qdisc fq_codel 8008: dev eno1 root refcnt 2 limit 10240p flows 1024 quantum 1514 target 5.0ms interval 100.0ms ecn 


I.e. fq_codel suffers from exactly the same problem.

Is this a bug or is it expected behaviour? I'd say bug?

-Toke

Re: [Cake] Correct 'change' behaviour

[Sebastian Moeller]

Hi Toke,

me too ;)

On Nov 4, 2015, at 12:09 , Toke Høiland-Jørgensen <toke@toke.dk> wrote:

> Dave Taht <dave.taht@gmail.com> writes:
> 
>> in the general case, I recomend looking at the most current fq_codel
>> code to see how to do it as right as possible. There were several bugs
>> found and fixed in it as well....
> 
> Well, I did, and it doesn't seem to be doing anything different. So much
> so that I tried the same experiment on fq_codel:
> 
> $ sudo tc qdisc del dev eno1 root
> $ sudo tc qdisc replace dev eno1 root fq_codel target 100ms interval 200ms
> $ sudo tc qdisc replace dev eno1 root fq_codel target 5ms                 
> $ tc qdisc
> qdisc fq_codel 8007: dev eno1 root refcnt 2 limit 10240p flows 1024 quantum 1514 target 5.0ms interval 200.0ms ecn 
> $ sudo tc qdisc del dev eno1 root                                         
> $ sudo tc qdisc replace dev eno1 root fq_codel target 5ms
> $ tc qdisc                                          
> qdisc fq_codel 8008: dev eno1 root refcnt 2 limit 10240p flows 1024 quantum 1514 target 5.0ms interval 100.0ms ecn 


$ sudo tc qdisc del dev eth0 root
$ sudo tc qdisc replace dev eth0 root fq_codel target 100ms interval 200ms
$ sudo tc qdisc replace dev eth0 root fq_codel target 5ms
$ sudo tc qdisc
qdisc fq_codel 800a: dev eth0 root refcnt 6 limit 10240p flows 1024 quantum 1514 target 5.0ms interval 200.0ms ecn 
$ sudo tc qdisc del dev eth0 root
$ sudo tc qdisc replace dev eth0 root fq_codel target 5ms
$ sudo tc qdisc
qdisc fq_codel 800b: dev eth0 root refcnt 6 limit 10240p flows 1024 quantum 1514 target 5.0ms interval 100.0ms ecn 

$ uname -a
Linux happy-horse 3.16.7-24-desktop #1 SMP PREEMPT Mon Aug 3 14:37:06 UTC 2015 (ec183cc) x86_64 x86_64 x86_64 GNU/Linux



$ sudo tc qdisc del dev eth0 root
$ sudo tc qdisc replace dev eth0 root fq_codel target 100ms interval 200ms
$ sudo tc qdisc replace dev eth0 root fq_codel target 5ms
$ sudo tc qdisc
qdisc fq_codel 8003: dev eth0 root refcnt 6 limit 10240p flows 1024 quantum 1514 target 5.0ms interval 200.0ms ecn 
$ sudo tc qdisc del dev eth0 root
$ sudo tc qdisc replace dev eth0 root fq_codel target 5ms
$ sudo tc qdisc
qdisc fq_codel 8004: dev eth0 root refcnt 6 limit 10240p flows 1024 quantum 1514 target 5.0ms interval 100.0ms ecn 
$ uname -a
Linux psychotb-01 3.19.0-31-generic #36~14.04.1-Ubuntu SMP Thu Oct 8 10:21:08 UTC 2015 x86_64 x86_64 x86_64 GNU/Linux
 
So at least the opensuse 3.16.7 and the ubuntu 3.19.0 kernels both suffer this issue. Maybe the kernel does this on purpose, or nobody ever tests this functionality…

Best Regards
	Sebastian


> 
> 
> I.e. fq_codel suffers from exactly the same problem.
> 
> Is this a bug or is it expected behaviour? I'd say bug?
> 
> -Toke
> _______________________________________________
> Cake mailing list
> Cake@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] Correct 'change' behaviour

[Kevin Darbyshire-Bryant]

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

On 04/11/15 11:09, Toke Høiland-Jørgensen wrote:
> Dave Taht <dave.taht@gmail.com> writes:
>
>> in the general case, I recomend looking at the most current fq_codel
>> code to see how to do it as right as possible. There were several bugs
>> found and fixed in it as well....
> Well, I did, and it doesn't seem to be doing anything different. So much
> so that I tried the same experiment on fq_codel:
>
> $ sudo tc qdisc del dev eno1 root
> $ sudo tc qdisc replace dev eno1 root fq_codel target 100ms interval 200ms
> $ sudo tc qdisc replace dev eno1 root fq_codel target 5ms                 
> $ tc qdisc
> qdisc fq_codel 8007: dev eno1 root refcnt 2 limit 10240p flows 1024 quantum 1514 target 5.0ms interval 200.0ms ecn 
> $ sudo tc qdisc del dev eno1 root                                         
> $ sudo tc qdisc replace dev eno1 root fq_codel target 5ms
> $ tc qdisc                                          
> qdisc fq_codel 8008: dev eno1 root refcnt 2 limit 10240p flows 1024 quantum 1514 target 5.0ms interval 100.0ms ecn 
>
>
> I.e. fq_codel suffers from exactly the same problem.
>
> Is this a bug or is it expected behaviour? I'd say bug?
>
> -Toke
> _______________________________________________
> Cake mailing list
> Cake@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/cake
Doesn't the comment in the code say in essence "if it's the same type of
qdisc then the operation is a change, else the operation is (an atomic)
remove/add" ?  Or did I mis-read it. 

Kevin


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4816 bytes --]

Re: [Cake] Correct 'change' behaviour

[Toke Høiland-Jørgensen]

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

> Doesn't the comment in the code say in essence "if it's the same type of
> qdisc then the operation is a change, else the operation is (an atomic)
> remove/add" ?  Or did I mis-read it.

Hmm, that's not how I understood the comment (nor the code). But it
certainly seems that that is what happens. Asked on netdev, let's see
what they say... :)

-Toke

Re: [Cake] Correct 'change' behaviour

[Sebastian Moeller]

Hi All,

On Nov 4, 2015, at 19:51 , Toke Høiland-Jørgensen <toke@toke.dk> wrote:

> Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:
> 
>> Doesn't the comment in the code say in essence "if it's the same type of
>> qdisc then the operation is a change, else the operation is (an atomic)
>> remove/add" ?  Or did I mis-read it.
> 
> Hmm, that's not how I understood the comment (nor the code). But it
> certainly seems that that is what happens. Asked on netdev, let's see
> what they say... :)

	That would be unfortunate, as I believe this would require another keyword for tc in addition to “change" and “replace" we would need “initialize”, “defaults” or “re-set” with ample opportunities for selecting that bike-shed’s color… If the replace degenerates to a change is on purpose we have no chance getting it changed, but even if it was accidental there might be people expecting that behavior now :(

Best Regards
	Sebastian

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