Re: [PATCH net-next v2] fq_codel: generalise ce_threshold marking for subset of traffic

On 10/20/21 2:16 PM, Toke Høiland-Jørgensen wrote:

Eric Dumazet [off-list ref] writes:

quoted

On 10/19/21 10:47 AM, Toke Høiland-Jørgensen wrote:

quoted

The commit in the Fixes tag expanded the ce_threshold feature of FQ-CoDel
so it can be applied to a subset of the traffic, using the ECT(1) bit of
the ECN field as the classifier. However, hard-coding ECT(1) as the only
classifier for this feature seems limiting, so let's expand it to be more
general.

To this end, change the parameter from a ce_threshold_ect1 boolean, to a
one-byte selector/mask pair (ce_threshold_{selector,mask}) which is applied
to the whole diffserv/ECN field in the IP header. This makes it possible to
classify packets by any value in either the ECN field or the diffserv
field. In particular, setting a selector of INET_ECN_ECT_1 and a mask of
INET_ECN_MASK corresponds to the functionality before this patch, and a
mask of ~INET_ECN_MASK allows using the selector as a straight-forward
match against a diffserv code point:

 # apply ce_threshold to ECT(1) traffic
 tc qdisc replace dev eth0 root fq_codel ce_threshold 1ms ce_threshold_selector 0x1/0x3

 # apply ce_threshold to ECN-capable traffic marked as diffserv AF22
 tc qdisc replace dev eth0 root fq_codel ce_threshold 1ms ce_threshold_selector 0x50/0xfc

Regardless of the selector chosen, the normal rules for ECN-marking of
packets still apply, i.e., the flow must still declare itself ECN-capable
by setting one of the bits in the ECN field to get marked at all.

v2:
- Add tc usage examples to patch description

Fixes: e72aeb9ee0e3 ("fq_codel: implement L4S style ce_threshold_ect1 marking")
Signed-off-by: Toke Høiland-Jørgensen <redacted>
---

Reviewed-by: Eric Dumazet <edumazet@google.com>

Thanks!

quoted

BTW, the Fixes: tag seems not really needed, your patch is a
followup/generalization.

Yeah, I included it because I don't know of any other way to express
"this is a follow-up commit to this other one, and the two should be
kept together" - for e.g., backports. And I figured that since this
changes the UAPI from your initial patch, this was important to express
in case someone does backport that.

The patch targeted net-next, and was not stable material.

Also, this is pure opt-in behavior, so there was no risk
of breaking something.

Note that I have not provided yet an iproute2 patch, so only your
iproute2 change will possibly enable the new behavior.

Is there another way to express this that I'm not aware of?

Just mentioning the commit directly in the changelog is what I
do for followups.