> -----Original Message-----
> From: Donald Hunter <donald.hun...@gmail.com>
> Sent: Wednesday, April 23, 2025 1:29 PM
> To: Chia-Yu Chang (Nokia) <chia-yu.ch...@nokia-bell-labs.com>
> Cc: xandf...@gmail.com; net...@vger.kernel.org; dave.t...@gmail.com;
> pab...@redhat.com; j...@mojatatu.com; k...@kernel.org;
> step...@networkplumber.org; xiyou.wangc...@gmail.com; j...@resnulli.us;
> da...@davemloft.net; eduma...@google.com; ho...@kernel.org;
> andrew+net...@lunn.ch; a...@fiberby.net; liuhang...@gmail.com;
> sh...@kernel.org; linux-kselftest@vger.kernel.org; i...@kernel.org;
> ncardw...@google.com; Koen De Schepper (Nokia)
> <koen.de_schep...@nokia-bell-labs.com>; g.white <g.wh...@cablelabs.com>;
> ingemar.s.johans...@ericsson.com; mirja.kuehlew...@ericsson.com;
> chesh...@apple.com; rs.i...@gmx.at; jason_living...@comcast.com; vidhi_goel
> <vidhi_g...@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.ch...@nokia-bell-labs.com writes:
>
> > From: Chia-Yu Chang <chia-yu.ch...@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.ch...@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
