Re: [PATCH v12 net-next 1/5] Documentation: netlink: specs: tc: Add DualPI2 specification

[Donald Hunter <donald.hunter@gmail.com>]

chia-yu.chang@nokia-bell-labs.com writes:

> 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).
>
> Signed-off-by: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>
> ---
>  Documentation/netlink/specs/tc.yaml | 144 ++++++++++++++++++++++++++++
>  1 file changed, 144 insertions(+)

The syntax is not valid so this doesn't pass the schema check and
presumably hasn't been tested. Please validate YNL .yaml additions e.g.

./tools/net/ynl/pyynl/cli.py \
    --spec Documentation/netlink/specs/tc.yaml \
    --list-ops

...
jsonschema.exceptions.ValidationError: Additional properties are not allowed ('entries' was unexpected)
...
On instance['attribute-sets'][30]['attributes'][14]:
    {'name': 'gso_split',
     'type': 'flags',
     'doc': 'Split aggregated skb or not',
     'entries': ['split_gso', 'no_split_gso']}


>
> diff --git a/Documentation/netlink/specs/tc.yaml b/Documentation/netlink/specs/tc.yaml
> index aacccea5dfe4..08255bba81c4 100644
> --- a/Documentation/netlink/specs/tc.yaml
> +++ b/Documentation/netlink/specs/tc.yaml
> @@ -816,6 +816,58 @@ definitions:
>        -
>          name: drop-overmemory
>          type: u32
> +  -
> +    name: tc-dualpi2-xstats
> +    type: struct
> +    members:
> +      -
> +        name: prob
> +        type: uint
> +        doc: Current probability
> +      -
> +        name: delay_c

Please use dashes in member names, e.g. "delay-c", to follow YNL
conventions. Same for all member and attribute names below.

> +        type: uint
> +        doc: Current C-queue delay in microseconds
> +      -
> +        name: delay_l
> +        type: uint
> +        doc: Current L-queue delay in microseconds
> +      -
> +        name: pkts_in_c
> +        type: uint
> +        doc: Number of packets enqueued in the C-queue
> +      -
> +        name: pkts_in_l
> +        type: uint
> +        doc: Number of packets enqueued in the L-queue
> +      -
> +        name: maxq
> +        type: uint
> +        doc: Maximum number of packets seen by the DualPI2
> +      -
> +        name: ecn_mark
> +        type: uint
> +        doc: All packets marked with ecn
> +      -
> +        name: step_mark
> +        type: uint
> +        doc: Only packets marked with ecn due to L-queue step AQM
> +      -
> +        name: credit
> +        type: int
> +        doc: Current credit value for WRR
> +      -
> +        name: memory_used
> +        type: uint
> +        doc: Memory used in bytes by the DualPI2
> +      -
> +        name: max_memory_used
> +        type: uint
> +        doc: Maximum memory used in bytes by the DualPI2
> +      -
> +        name: memory_limit
> +        type: uint
> +        doc: Memory limit in bytes
>    -
>      name: tc-fq-pie-xstats
>      type: struct
> @@ -2299,6 +2351,92 @@ attribute-sets:
>        -
>          name: quantum
>          type: u32
> +  -
> +    name: tc-dualpi2-attrs
> +    attributes:
> +      -
> +        name: limit
> +        type: uint
> +        doc: Limit of total number of packets in queue
> +      -
> +        name: memlimit
> +        type: uint
> +        doc: Memory limit of total number of packets in queue
> +      -
> +        name: target
> +        type: uint
> +        doc: Classic target delay in microseconds
> +      -
> +        name: tupdate
> +        type: uint
> +        doc: Drop probability update interval time in microseconds
> +      -
> +        name: alpha
> +        type: uint
> +        doc: Integral gain factor in Hz for PI controller
> +      -
> +        name: beta
> +        type: uint
> +        doc: Proportional gain factor in Hz for PI controller
> +      -
> +        name: step_thresh
> +        type: uint
> +        doc: L4S step marking threshold in microseconds or in packet (see step_packets)
> +      -
> +        name: step_packets
> +        type: flags
> +        doc: L4S Step marking threshold unit
> +        entries:
> +        - microseconds
> +        - packets

This is not valid syntax. Enumerations and sets of flags need to be
defined separately. For example, look at the definition of tc-cls-flags
and its usage.

BUT step_packets is defined as a boolean in the implementation so could
be implemented as a boolean flag in the API. If it needs to be
extensible in future then it should be declared as an enum in uAPI and
defined in this spec as an enum. Either way, the parsing and policy in
patch 2 should be made more robust.

> +      -
> +        name: min_qlen_step
> +        type: uint
> +        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_factor
> +        type: uint
> +        doc: Probability coupling factor between Classic and L4S (2 is recommended)
> +      -
> +        name: drop_overload
> +        type: flags
> +        doc: Control the overload strategy (drop to preserve latency or let the queue overflow)
> +        entries:
> +        - drop_on_overload
> +        - overflow

Not valid syntax. Use a boolean flag or define an enum.

> +      -
> +        name: drop_early
> +        type: flags
> +        doc: Decide where the Classic packets are PI-based dropped or marked
> +        entries:
> +        - drop_enqueue
> +        - drop_dequeue

Not valid syntax. Use a boolean flag or define an enum.

> +      -
> +        name: classic_protection
> +        type: uint
> +        doc: Classic WRR weight in percentage (from 0 to 100)
> +      -
> +        name: ecn_mask
> +        type: flags
> +        doc: Configure the L-queue ECN classifier
> +        entries:
> +        - l4s_ect
> +        - any_ect

Not valid syntax. Type should probably match implementation, unless you
want to enumerate the valid values by definining an enum.

> +      -
> +        name: gso_split
> +        type: flags
> +        doc: Split aggregated skb or not
> +        entries:
> +        - split_gso
> +        - no_split_gso

Not valid syntax. Use a boolean flag or define an enum.

> +      -
> +        name: max_rtt
> +        type: uint
> +        doc: The maximum expected RTT of the traffic that is controlled by DualPI2 in usec
> +      -
> +        name: typical_rtt
> +        type: uint
> +        doc: The typical base RTT of the traffic that is controlled by DualPI2 in usec
>    -
>      name: tc-ematch-attrs
>      attributes:
> @@ -3679,6 +3817,9 @@ sub-messages:
>        -
>          value: drr
>          attribute-set: tc-drr-attrs
> +      -
> +        value: dualpi2
> +        attribute-set: tc-dualpi2-attrs
>        -
>          value: etf
>          attribute-set: tc-etf-attrs
> @@ -3846,6 +3987,9 @@ sub-messages:
>        -
>          value: codel
>          fixed-header: tc-codel-xstats
> +      -
> +        value: dualpi2
> +        fixed-header: tc-dualpi2-xstats
>        -
>          value: fq
>          fixed-header: tc-fq-qd-stats