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

-----Original Message----- From: Donald Hunter donald.hunter@gmail.com Sent: Wednesday, April 23, 2025 1:29 PM To: Chia-Yu Chang (Nokia) chia-yu.chang@nokia-bell-labs.com Cc: xandfury@gmail.com; netdev@vger.kernel.org; dave.taht@gmail.com; pabeni@redhat.com; jhs@mojatatu.com; kuba@kernel.org; stephen@networkplumber.org; xiyou.wangcong@gmail.com; jiri@resnulli.us; davem@davemloft.net; edumazet@google.com; horms@kernel.org; andrew+netdev@lunn.ch; ast@fiberby.net; liuhangbin@gmail.com; shuah@kernel.org; linux-kselftest@vger.kernel.org; ij@kernel.org; ncardwell@google.com; Koen De Schepper (Nokia) koen.de_schepper@nokia-bell-labs.com; g.white g.white@cablelabs.com; ingemar.s.johansson@ericsson.com; mirja.kuehlewind@ericsson.com; cheshire@apple.com; rs.ietf@gmx.at; Jason_Livingood@comcast.com; vidhi_goel vidhi_goel@apple.com Subject: Re: [PATCH v12 net-next 1/5] Documentation: netlink: specs: tc: Add DualPI2 specification

CAUTION: This is an external email. Please be very careful when clicking links or opening attachments. See the URL nok.it/ext for additional information.

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']}

Hi Donald,

Thanks for the feedback, and I will take actions for below points as well as the corresponding iproute2-net fixes. One more question is I see "uint" type is not valid during validation - see below (but which was suggested in v11), shall I change it back to u32/u8?

Failed validating 'enum' in schema['properties']['definitions']['items']['properties']['members']['items']['properties']['type']: {'description': "The netlink attribute type. Members of type 'binary' " "or 'pad'\n" "must also have the 'len' property set.\n", 'enum': ['u8', 'u16', 'u32', 'u64', 's8', 's16', 's32', 's64', 'string', 'binary', 'pad']}

On instance['definitions'][42]['members'][12]['type']: 'uint'

Best regards, Chia-Yu

...

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