dtaht / sch_cake

Out of tree build for the new cake qdisc
100 stars 35 forks source link

RFC 8622 diffserv3, 4 & 8 LE PHB support #127

Closed ldir-EDB0 closed 4 years ago

ldir-EDB0 commented 4 years ago

It looks like the RFC 8622 patch didn't get sent upstream.

I also think it's incomplete and the 'wash' function doesn't obey

Re-marking to Other DSCPs/PHBs

"DSCP bleaching", i.e., setting the DSCP to '000000' (default PHB) is NOT RECOMMENDED for this PHB. This may cause effects that are in contrast to the original intent to protect BE traffic from LE traffic (the "no harm" property). In the case that a DS domain does not support the LE PHB, its nodes SHOULD treat LE-marked packets with the default PHB instead (by mapping the LE DSCP to the default PHB), but they SHOULD do so without re-marking to DSCP '000000'. This is because DS domains that are traversed later may then still have the opportunity to treat such packets according to the LE PHB.

moeller0 commented 4 years ago

Hi Kevin,

in spite of the text in the RFC and the gernal agreement that DSCP would be nice if they would be conserved end-to-end, I think that wash should keep doing a hard re-mapping to dscp 0. The point is, that DSCPs really are supposed to be variable and each diffserv domain is allowed to use what ever bit pattern for what ever PHB they support (which is simply accepting reality). PHB per-hop-behaviour can be considered to be conserved, if supported. Heck the rcommended LE dscp was selected purposefully such that it will most likely be treated like dscp 0 traffic to begin with. The cake/sm way here is simply to combine not-wash with besteffort (if the dscp is to be ignored but faithfully transmitted), or not-wash with diffservN if it is to be honored. Now, I realize that this is just my personal opinion, but I really think that empowering our users here means to keep wash simple and strict. Othewise, slippery slope all other DSCPs that want end-to-end conservation will come knocking, like the probably going to be dangerous NQB (which in the current draft aims to automatically map into wifi's second highest priority class AC_VI) and I am not convinced tjat all are as harmless as LE. Also given that Comcast already mismapps too much traffic into CS1 (effectively the old LE DSCP) (sure that probably comes from comcast not using CS1 for the LE PHB, but mismarkings like that will keep happening, and it would be great if cake would still maintain its usability).

Best Regards Sebastan

On Jan 18, 2020, at 16:13, Kevin Darbyshire-Bryant notifications@github.com wrote:

It looks like the RFC 8622 patch didn't get sent upstream.

I also think it's incomplete and the 'wash' function doesn't obey

Re-marking to Other DSCPs/PHBs

"DSCP bleaching", i.e., setting the DSCP to '000000' (default PHB) is NOT RECOMMENDED for this PHB. This may cause effects that are in contrast to the original intent to protect BE traffic from LE traffic (the "no harm" property). In the case that a DS domain does not support the LE PHB, its nodes SHOULD treat LE-marked packets with the default PHB instead (by mapping the LE DSCP to the default PHB), but they SHOULD do so without re-marking to DSCP '000000'. This is because DS domains that are traversed later may then still have the opportunity to treat such packets according to the LE PHB.

I think this is easily solved with:

` @@ -1630,7 +1630,7 @@ static u8 cake_handle_diffserv(struct sk_buff *skb, u16 wash) return 0;

        dscp = ipv4_get_dsfield(ip_hdr(skb)) >> 2;

• if (wash && dscp)

• if (wash && dscp && dscp != 1) ipv4_change_dsfield(ip_hdr(skb), INET_ECN_MASK, 0); return dscp;

@@ -1641,7 +1641,7 @@ static u8 cake_handle_diffserv(struct sk_buff *skb, u16 wash) return 0;

        dscp = ipv6_get_dsfield(ipv6_hdr(skb)) >> 2;

• if (wash && dscp)

• if (wash && dscp && dscp != 1) ipv6_change_dsfield(ipv6_hdr(skb), INET_ECN_MASK, 0); return dscp;

`

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

tohojo commented 4 years ago

moeller0 notifications@github.com writes:

Hi Kevin,

in spite of the text in the RFC and the gernal agreement that DSCP would be nice if they would be conserved end-to-end, I think that wash should keep doing a hard re-mapping to dscp 0.

I'd tend to agree that we shouldn't make exceptions for 'wash' behaviour. If the LE thing is really worth supporting specially (are there any examples of anyone actually setting it?), we could make it another keyword; wash_mask maybe? Or just tell people do to the washing themselves using BPF?

ldir-EDB0 commented 4 years ago

OK, so wash handling isn't desirable. Is the PHB update a candidate for upstream?

tohojo commented 4 years ago

Kevin Darbyshire-Bryant notifications@github.com writes:

OK, so wash handling isn't desirable. Is the PHB update a candidate for upstream?

You mean this one:

183b3201d610 ("RFC 8622 diffserv3, 4 & 8 LE PHB support") ?

Sure, don't see why not; however, didn't we have an unresolved issue with that? https://lists.bufferbloat.net/pipermail/cake/2019-December/005070.html

-Toke

dtaht commented 4 years ago

Toke Høiland-Jørgensen notifications@github.com writes:

Kevin Darbyshire-Bryant notifications@github.com writes:

OK, so wash handling isn't desirable. Is the PHB update a candidate for upstream?

You mean this one:

183b3201d610 ("RFC 8622 diffserv3, 4 & 8 LE PHB support") ?

Sure, don't see why not; however, didn't we have an unresolved issue with that? https://lists.bufferbloat.net/pipermail/cake/2019-December/005070.html

I agree that excepting LE from wash is not a good idea. (there was a lot of wishful thinking in that RFC, and let's not get started on NQB!)

I agree that adding LE to the background queue class is a good idea.

-Toke

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

ldir-EDB0 commented 4 years ago

Sure, don't see why not; however, didn't we have an unresolved issue with that? https://lists.bufferbloat.net/pipermail/cake/2019-December/005070.html

I had a look through that thread and part way through it says that Thibaut experienced his issue whether the PHB update was there or not. I had a chat via IRC in which he agreed with that summary. So I don't think there's a show stopper for the PHB LE change. He's going to rebuild with 'master' cake sometime this week to see if the SCE removal etc has helped. I guess wait till then.

tohojo commented 4 years ago

Kevin Darbyshire-Bryant notifications@github.com writes:

Sure, don't see why not; however, didn't we have an unresolved issue with that? https://lists.bufferbloat.net/pipermail/cake/2019-December/005070.html

I had a look through that thread and part way through it says that Thibaut experienced his issue whether the PHB update was there or not. I had a chat via IRC in which he agreed with that summary. So I don't think there's a show stopper for the PHB LE change. He's going to rebuild with 'master' cake sometime this week to see if the SCE removal etc has helped. I guess wait till then.

OK, sounds like a plan :)

dtaht commented 4 years ago

And the plan is?

dtaht commented 4 years ago

nag

ldir-EDB0 commented 4 years ago

the plan was to poke the reporter of the original 'PHB Breaks my cake' after they had tried latest cake again and proved it continues in it's broken/non broken state irrespective of 'PHB patch' - I have poked said person again.

tohojo commented 4 years ago

Right, so unless someone complains loudly I'm planning to include the RFC8622 patch in my next upstream submission (with the CE-marking and diffserv reading patches)...

tohojo commented 4 years ago

Now merged upstream (to net-next)