| Message ID | 20220125060410.2691029-1-matt@codeconstruct.com.au (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] sch_cake: diffserv8 CS1 should be bulk | expand |
Matt Johnston <matt@codeconstruct.com.au> writes: > The CS1 priority (index 0x08) was changed from 0 to 1 when LE (index > 0x01) was added. This looks unintentional, it doesn't match the > docs and CS1 shouldn't be the same tin as AF1x Hmm, Kevin, any comments? -Toke
Mmmh, > On Jan 25, 2022, at 11:58, Toke Høiland-Jørgensen <toke@redhat.com> wrote: > > Matt Johnston <matt@codeconstruct.com.au> writes: > >> The CS1 priority (index 0x08) was changed from 0 to 1 when LE (index >> 0x01) was added. This looks unintentional, it doesn't match the >> docs and CS1 shouldn't be the same tin as AF1x > > Hmm, Kevin, any comments? I am clearly not Kevin, but.... https://elixir.bootlin.com/linux/v5.16.2/source/net/sched/sch_cake.c static const u8 diffserv8[] = { 2, 0, 1, 2, 4, 2, 2, 2, 1, 2, 1, 2, 1, 2, 1, 2, 5, 2, 4, 2, 4, 2, 4, 2, 3, 2, 3, 2, 3, 2, 3, 2, 6, 2, 3, 2, 3, 2, 3, 2, 6, 2, 2, 2, 6, 2, 6, 2, 7, 2, 2, 2, 2, 2, 2, 2, 7, 2, 2, 2, 2, 2, 2, 2, }; LE(1) is tin 0 the lowest CS1(8) is 1 slightly above LE CS0/BE(0) is 2 AF1x (10, 12, 14) are all in tin 1 as is CS1 AF2x (18, 20, 22) in tin4 AF3x (26, 28, 30) in tin3 AF4x (34, 36, 38) in tin3 Just as documented in the code: { /* Pruned list of traffic classes for typical applications: * * Network Control (CS6, CS7) * Minimum Latency (EF, VA, CS5, CS4) * Interactive Shell (CS2, TOS1) * Low Latency Transactions (AF2x, TOS4) * Video Streaming (AF4x, AF3x, CS3) * Bog Standard (CS0 etc.) * High Throughput (AF1x, TOS2) * Background Traffic (CS1, LE) * * Total 8 traffic classes. */ I note that this seems backwards, as I assumed the AFN to be in increasing order of priority, but at the same time I care very little for he 12 AF codepoints, they are not reliably end to end, and many important devices only allow 3 priority bits anyway, so I question whether they actually ever see much use at all, but that is tangential... BUT IMHO the main reason for introducing LE in the first place was/is that CS1 often is interpreted as higher priority than CS0 (e.g. by gear that looks at the 3 highest TOS bits), resulting in an priority inversion where BK packets end up with higher priority than BE in spite of the senders intention being the other way round. Having CS1 in the same tin/priority tier as CS0 seems harmless (priorities are not guaranteed e2e anyway, so CS1/LE will be routinely treated equally as CS0/BE already and senders will need to have made peace with that already). So I argue with the introduction of LE, CS1 should be treated equivalently to CS0 (giving it higher priority will actively do the wrong thing for senders still using CS1 for background). So I agree that there is potential for cahnge, but that change should IMHO be to move CS1 to tin2 in diffserv8... BUT to be really, really frank, none of this matters much, since DSCPs are not stable end to end, so local remapping seems required anyway, and then the re-mapper needs to look at the actual mapping scheme independent of the narratives given for different DSCPs/PHBs in IETF documents (affectionately called "DSCP-fan-fiction"). Regards Sebastian > > -Toke > > _______________________________________________ > Cake mailing list > Cake@lists.bufferbloat.net > https://lists.bufferbloat.net/listinfo/cake
On Tue, 2022-01-25 at 12:54 +0100, Sebastian Moeller wrote: > > LE(1) is tin 0 the lowest > CS1(8) is 1 slightly above LE > CS0/BE(0) is 2 > AF1x (10, 12, 14) are all in tin 1 as is CS1 ... > Just as documented in the code: > > * Bog Standard (CS0 etc.) > * High Throughput (AF1x, TOS2) > * Background Traffic (CS1, LE) The documentation doesn't match the code though. Almost, but it's off by one. I can submit a patch instead to change the docs, though it's not clear the divergence between code and docs was intended in the first place. (diffserv8 also needs a description in the cake manpage, I'll send a patch for that once the order is clarified) Cheers, Matt
> On 25 Jan 2022, at 10:58, Toke Høiland-Jørgensen <toke@redhat.com> wrote: > > Matt Johnston <matt@codeconstruct.com.au> writes: > >> The CS1 priority (index 0x08) was changed from 0 to 1 when LE (index >> 0x01) was added. This looks unintentional, it doesn't match the >> docs and CS1 shouldn't be the same tin as AF1x > > Hmm, Kevin, any comments? > > -Toke > I’ll have to find my thinking head & time machine :-) This would be a lot easier if we had ‘diffserv9’, LE could have simply been added as the ‘if you’ve really nothing better to do’ class that it is. And it’s why I’ve personally argued for a diffserv5: lowest;low;normal;high;highest moving on. I think I screwed up when LE was added to diffserv8 - Matt the CS1 change from 0 to 1 IS intentional and IIRC I tried to bump everything else up 1 to compensate.. I may have missed some things though. Cheers, Kevin D-B gpg: 012C ACB2 28C6 C53E 9775 9123 B3A2 389B 9DE2 334A
Hi Matt, > On Jan 27, 2022, at 04:14, Matt Johnston <matt@codeconstruct.com.au> wrote: > > On Tue, 2022-01-25 at 12:54 +0100, Sebastian Moeller wrote: >> >> LE(1) is tin 0 the lowest >> CS1(8) is 1 slightly above LE >> CS0/BE(0) is 2 >> AF1x (10, 12, 14) are all in tin 1 as is CS1 > ... >> Just as documented in the code: >> >> * Bog Standard (CS0 etc.) >> * High Throughput (AF1x, TOS2) >> * Background Traffic (CS1, LE) > > The documentation doesn't match the code though. Since I did not see your original mail, only Toke's response, which documentation is wrong here? > Almost, but it's off by one. I fully believe you, but could you spell it out by showing the line of code and the line of documentation that are off? > I can submit a patch instead to change the docs, though it's not clear the > divergence between code and docs was intended in the first place. > > (diffserv8 also needs a description in the cake manpage, I'll send a patch > for that once the order is clarified) Regards Sebastian > > Cheers, > Matt >
> On Jan 27, 2022, at 10:00, Kevin 'ldir' Darbyshire-Bryant via Cake <cake@lists.bufferbloat.net> wrote: > > > >> On 25 Jan 2022, at 10:58, Toke Høiland-Jørgensen <toke@redhat.com> wrote: >> >> Matt Johnston <matt@codeconstruct.com.au> writes: >> >>> The CS1 priority (index 0x08) was changed from 0 to 1 when LE (index >>> 0x01) was added. This looks unintentional, it doesn't match the >>> docs and CS1 shouldn't be the same tin as AF1x >> >> Hmm, Kevin, any comments? >> >> -Toke >> > > I’ll have to find my thinking head & time machine :-) > This would be a lot easier if we had ‘diffserv9’, LE could have simply > been added as the ‘if you’ve really nothing better to do’ class that it > is. And it’s why I’ve personally argued for a diffserv5: lowest;low;normal;high;highest > moving on. > > I think I screwed up when LE was added to diffserv8 - Matt the CS1 change from 0 to 1 IS intentional > and IIRC I tried to bump everything else up 1 to compensate.. I may have missed some things though. But that way, introduction of LE does not fix much... I really think with LE's existence ,CS1 should be put into the same tin as CS0/BE, for the simple reason that currently CS1 is already in use both for priority below and priority above CS0/BE, the only course forward avoiding priority inversions is to treat CS1 like CS0. As a case in point I remember that Dave Täht reported seeing oodles of packets marked CS1 in his ingress some time in the past, packets that should not be treated as bulk. Sure we can argue that anybody using DSCPs for priority steering needs to re-map anyways, but that is not the logic behind cake's default mapping. And that CS1 to BE mapping should apply to all diffserv modes, that offer a lower priority tier, no? Regards Sebastian > > > Cheers, > > Kevin D-B > > gpg: 012C ACB2 28C6 C53E 9775 9123 B3A2 389B 9DE2 334A > > _______________________________________________ > Cake mailing list > Cake@lists.bufferbloat.net > https://lists.bufferbloat.net/listinfo/cake
On Thu, 2022-01-27 at 17:00 +0100, Sebastian Moeller wrote: > > > > The documentation doesn't match the code though. > > Since I did not see your original mail, only Toke's response, which documentation is wrong here? Ah, I had missed that the docs were updated already on 6 Jan 2022. Sorry for the noise! 9ed6786c19ae ("sch_cake: diffserv8 CS1 should be bulk") Cheers, Matt
diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c index 857aaebd49f4..6ff2ddc5b812 100644 --- a/net/sched/sch_cake.c +++ b/net/sched/sch_cake.c @@ -313,7 +313,7 @@ static const u8 precedence[] = { static const u8 diffserv8[] = { 2, 0, 1, 2, 4, 2, 2, 2, - 1, 2, 1, 2, 1, 2, 1, 2, + 0, 2, 1, 2, 1, 2, 1, 2, 5, 2, 4, 2, 4, 2, 4, 2, 3, 2, 3, 2, 3, 2, 3, 2, 6, 2, 3, 2, 3, 2, 3, 2,
The CS1 priority (index 0x08) was changed from 0 to 1 when LE (index 0x01) was added. This looks unintentional, it doesn't match the docs and CS1 shouldn't be the same tin as AF1x Signed-off-by: Matt Johnston <matt@codeconstruct.com.au> Fixes: b8392808eb3f ("sch_cake: add RFC 8622 LE PHB support to CAKE diffserv handling") --- net/sched/sch_cake.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)