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

["Toke Høiland-Jørgensen" <toke@redhat.com>]

Eric Dumazet <eric.dumazet@gmail.com> writes:

> On 10/20/21 2:16 PM, Toke Høiland-Jørgensen wrote:
>> Eric Dumazet <eric.dumazet@gmail.com> writes:
>> 
>>> On 10/19/21 10:47 AM, Toke Høiland-Jørgensen wrote:
>>>> 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 <toke@redhat.com>
>>>> ---
>>>
>>> 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.
>
> 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.

Right, I'll do that next time. Thanks :)

-Toke