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

Eric Dumazet [off-list ref] writes:

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!

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.

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

-Toke