Re: [PATCH v20 net-next 6/6] Documentation: netlink: specs: tc: Add DualPI2 specification

[Jakub Kicinski <kuba@kernel.org>]

On Sat, 21 Jun 2025 21:33:31 +0200 chia-yu.chang@nokia-bell-labs.com
wrote:
> From: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>
> 
> Introduce the specification of tc qdisc DualPI2 stats and attributes,
> which is the reference implementation of IETF RFC9332 DualQ Coupled AQM
> (https://datatracker.ietf.org/doc/html/rfc9332) providing two different
> queues: low latency queue (L-queue) and classic queue (C-queue).

> diff --git a/Documentation/netlink/specs/tc.yaml b/Documentation/netlink/specs/tc.yaml
> index 4cc1f6a45001..8a183799dceb 100644
> --- a/Documentation/netlink/specs/tc.yaml
> +++ b/Documentation/netlink/specs/tc.yaml
> @@ -56,6 +56,41 @@ definitions:
>        - tundf
>        - tunoam
>        - tuncrit
> +  -
> +    name: dualpi2-drop-overload
> +    name-prefix: tca-dualpi2-drop-overload-

As far as I understand tca / TCA stands for TC Attribute.
This is an enum for values, not for netlink attribute IDs,
tc-dualpi2-drop-overload- would be less confusing here

Which may in fact mean that you don't need name-prefix,
because I think that's what the default name would be.

same thing for other value enums

> +    type: enum
> +    entries:

FWIW if you don't document you can use the shorter notation:

	entries: [overflow, drop]

> +      - overflow
> +      - drop
> +  -
> +    name: dualpi2-drop-early
> +    name-prefix: tca-dualpi2-drop-early-
> +    type: enum
> +    entries:
> +      - drop-dequeue
> +      - drop-enqueue
> +  -
> +    name: dualpi2-ecn-mask
> +    name-prefix: tca-dualpi2-ecn-mask-
> +    type: enum

you can use

	value-start: 1
	entries: [l4s..

or just drop the values for entries other than first.
in enums the next value takes the previuos value + 1 by default

> +    entries:
> +      -
> +        name: l4s-ect
> +        value: 1
> +      -
> +        name: cla-ect
> +        value: 2
> +      -
> +        name: any-ect
> +        value: 3

> +  -
> +    name: tc-dualpi2-xstats
> +    type: struct
> +    members:
> +      -
> +        name: prob
> +        type: u32
> +        doc: Current probability

what's the probability of ?

> +      -
> +        name: ecn-mark
> +        type: u32
> +        doc: All packets marked with ecn

ECN

> +      -
> +        name: step-thresh
> +        type: u32
> +        doc: L4S step marking threshold (see also step-packets)
> +      -
> +        name: step-packets
> +        type: flag
> +        doc: L4S Step marking threshold unit in packets
> +             (otherwise is in microseconds)

Why mux two distinct values in one attr and carry another attr for the
unit? IMO it'd be more idiomatic for Netlink to define two attributes,
one for threshold in packets and one in time. And enforce that only one
can be sent in a message (probably using an explicit check in the code).
Conversely only report one in GET / DUMP.

> +      -
> +        name: min-qlen-step
> +        type: u32
> +        doc: Packets enqueued to the L-queue can apply the step threshold
> +             when the queue length of L-queue is larger than this value.
> +             (0 is recommended)
> +      -
> +        name: coupling
> +        type: u8
> +        doc: Probability coupling factor between Classic and L4S
> +             (2 is recommended)
> +      -
> +        name: drop-overload
> +        type: u8
> +        doc: Control the overload strategy (drop to preserve latency or
> +             let the queue overflow)
> +        enum: dualpi2-drop-overload
> +      -
> +        name: drop-early
> +        type: u8
> +        doc: Decide where the Classic packets are PI-based dropped or marked
> +        enum: dualpi2-drop-early
> +      -
> +        name: c-protection
> +        type: u8
> +        doc: Classic WRR weight in percentage (from 0 to 100)
> +      -
> +        name: ecn-mask
> +        type: u8
> +        doc: Configure the L-queue ECN classifier
> +        enum: dualpi2-ecn-mask
> +      -
> +        name: split-gso
> +        type: u8
> +        doc: Split aggregated skb or not
> +        enum: dualpi2-split-gso

Keep in mind that values in netlink are in 4B granularity, so all these
will be carried as 1B + 3B of padding. If there's any chance you may
need more bits in the future u32 is a safer default choice.