> -----Original Message-----
> From: Chia-Yu Chang (Nokia)
> Sent: Monday, May 5, 2025 12:14 AM
> To: Donald Hunter <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; Koen De Schepper (Nokia)
> <[email protected]>; g.white <[email protected]>;
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; vidhi_goel
> <[email protected]>
> Subject: RE: [PATCH v13 net-next 1/5] Documentation: netlink: specs: tc: Add
> DualPI2 specification
>
> > -----Original Message-----
> > From: Donald Hunter <[email protected]>
> > Sent: Tuesday, April 29, 2025 7:15 PM
> > To: Chia-Yu Chang (Nokia) <[email protected]>
> > Cc: [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > Koen De Schepper (Nokia) <[email protected]>;
> > g.white <[email protected]>; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; vidhi_goel <[email protected]>
> > Subject: Re: [PATCH v13 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.
> >
> >
> >
> > On Sat, 26 Apr 2025 at 18:20, <[email protected]> wrote:
> > >
> > > From: Chia-Yu Chang <[email protected]>
> > >
> > > 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).
> >
> > General comment is that this does not work. Please test it like this:
> >
> > sudo tc qdisc add dev eth0 handle 1: root dualpi2
> > ./tools/net/ynl/pyynl/cli.py \
> > --spec Documentation/netlink/specs/tc.yaml --dump getqdisc
> >
> > Consider moving this patch to the end of the series so that it can be
> > tested against the implementation patches.
>
> Hi Donald,
>
> Thanks for the tip, but I had an issue using the above script.
> It seems it expected either 4B or 8B payload, but several parameters
> used in dualpi2 are uint8, e.g., coupling.
> Or it is suggested to replace all u8 to u32, e.g., like what has been
> done in fq_codel?
>
> Error decoding 'coupling' from 'tc-dualpi2-attrs'
> Error decoding 'options' from 'tc-attrs'
> Traceback (most recent call last):
> File "/home/anc/net-next-github/net-next/./tools/net/ynl/pyynl/cli.py",
> line 160, in <module>
> main()
> File "/home/anc/net-next-github/net-next/./tools/net/ynl/pyynl/cli.py",
> line 141, in main
> reply = ynl.dump(args.dump, attrs)
> File "/home/anc/net-next-github/net-next/tools/net/ynl/pyynl/lib/ynl.py",
> line 1088, in dump
> return self._op(method, vals, dump=True)
> File "/home/anc/net-next-github/net-next/tools/net/ynl/pyynl/lib/ynl.py",
> line 1082, in _op
> return self._ops(ops)[0]
> File "/home/anc/net-next-github/net-next/tools/net/ynl/pyynl/lib/ynl.py",
> line 1069, in _ops
> rsp_msg = self._decode(decoded.raw_attrs, op.attr_set.name)
> File "/home/anc/net-next-github/net-next/tools/net/ynl/pyynl/lib/ynl.py",
> line 775, in _decode
> decoded = self._decode_sub_msg(attr, attr_spec, search_attrs)
> File "/home/anc/net-next-github/net-next/tools/net/ynl/pyynl/lib/ynl.py",
> line 726, in _decode_sub_msg
> subdict = self._decode(NlAttrs(attr.raw, offset), msg_format.attr_set)
> File "/home/anc/net-next-github/net-next/tools/net/ynl/pyynl/lib/ynl.py",
> line 759, in _decode
> decoded = attr.as_auto_scalar(attr_spec['type'], attr_spec.byte_order)
> File "/home/anc/net-next-github/net-next/tools/net/ynl/pyynl/lib/ynl.py",
> line 151, in as_auto_scalar
> raise Exception(f"Auto-scalar len payload be 4 or 8 bytes, got
> {len(self.raw)}")
> Exception: Auto-scalar len payload be 4 or 8 bytes, got 1
>
>
> Chia-Yu
>
I manage to resolve this issue by replacing uint into u32/u8, i.e., it reverts
the change suggested in v12.
And now the dualpi2 qdisc can be successfully produced.
{'family': 0,
'handle': 65536,
'hw-offload': 0,
'ifindex': 2,
'info': 17,
'kind': 'dualpi2',
'options': {'alpha': 40,
'beta': 818,
'c-protection': 10,
'coupling': 2,
'drop-early': 'drop-dequeue',
'drop-overload': 'drop',
'ecn-mask': 'l4s-ect',
'limit': 10000,
'memory-limit': 30280000,
'min-qlen-step': 0,
'split-gso': 'split-gso',
'step-packets': True,
'step-thresh': 10,
'target': 15000,
'tupdate': 16000},
'parent': 4294967295,
'stats': {'backlog': 0,
'bps': 0,
'bytes': 0,
'drops': 0,
'overlimits': 0,
'packets': 0,
'pps': 0,
'qlen': 0},
'stats2': {'app': {'credit': -121120,
'delay-c': 0,
'delay-l': 0,
'ecn-mark': 0,
'max-memory-used': 0,
'maxq': 0,
'memory-limit': 30280000,
'memory-used': 0,
'pkts-in-c': 0,
'pkts-in-l': 0,
'prob': 0,
'step-mark': 0},
'basic': {'bytes': 0, 'packets': 0},
'queue': {'backlog': 0,
'drops': 0,
'overlimits': 0,
'qlen': 0,
'requeues': 0}},
'xstats': {'credit': -121120,
'delay-c': 0,
'delay-l': 0,
'ecn-mark': 0,
'max-memory-used': 0,
'maxq': 0,
'memory-limit': 30280000,
'memory-used': 0,
'pkts-in-c': 0,
'pkts-in-l': 0,
'prob': 0,
'step-mark': 0}},
> > > Signed-off-by: Chia-Yu Chang <[email protected]>
> > > ---
> > > Documentation/netlink/specs/tc.yaml | 166
> > > ++++++++++++++++++++++++++++
> > > 1 file changed, 166 insertions(+)
> > >
> > > diff --git a/Documentation/netlink/specs/tc.yaml
> > > b/Documentation/netlink/specs/tc.yaml
> > > index aacccea5dfe4..9eaab15cc216 100644
> > > --- a/Documentation/netlink/specs/tc.yaml
> > > +++ b/Documentation/netlink/specs/tc.yaml
> > > @@ -51,6 +51,31 @@ definitions:
> > > - tundf
> > > - tunoam
> > > - tuncrit
> > > + -
> > > + name: tc-dualpi2-drop-overload-flags
> > > + type: flags
> > > + entries:
> > > + - drop
> > > + - overflow
> >
> > These enums need to be defined as part of the UAPI in pkt_sched.h and this
> > file needs to be in sync with those definitions.
> >
> > This enum seems to contradict the comment in sch_dualpi2.c:
> >
> > bool drop_overload; /* Drop (1) on overload, or overflow (0) */
> >
> > > + -
> > > + name: tc-dualpi2-drop-early-flags
> > > + type: flags
> > > + entries:
> > > + - drop-enqueue
> > > + - drop-dequeue
> >
> > Also contradicts comment in sch_dualpi2.c:
> >
> > bool drop_early; /* Drop at enqueue instead of dequeue if true */
> >
> > > + -
> > > + name: tc-dualpi2-ecn-mask-flags
> > > + type: flags
> > > + entries:
> > > + - l4s-ect
> > > + - any-ect
> > > + - none
> > > + -
> > > + name: tc-dualpi2-credit-queue-flags
> > > + type: flags
> > > + entries:
> > > + - C-queue
> > > + - L-queue
> > > -
> > > name: tc-stats
> > > type: struct
> > > @@ -816,6 +841,64 @@ definitions:
> > > -
> > > name: drop-overmemory
> > > type: u32
> > > + -
> > > + name: tc-dualpi2-xstats
> > > + type: struct
> > > + members:
> > > + -
> > > + name: prob
> > > + type: u32
> > > + doc: Current probability
> > > + -
> > > + name: delay-c
> > > + type: u32
> > > + doc: Current C-queue delay in microseconds
> > > + -
> > > + name: delay-l
> > > + type: u32
> > > + doc: Current L-queue delay in microseconds
> > > + -
> > > + name: pkts-in-c
> > > + type: u32
> > > + doc: Number of packets enqueued in the C-queue
> > > + -
> > > + name: pkts-in-l
> > > + type: u32
> > > + doc: Number of packets enqueued in the L-queue
> > > + -
> > > + name: maxq
> > > + type: u32
> > > + doc: Maximum number of packets seen by the DualPI2
> > > + -
> > > + name: ecn-mark
> > > + type: u32
> > > + doc: All packets marked with ecn
> > > + -
> > > + name: step-mark
> > > + type: u32
> > > + doc: Only packets marked with ecn due to L-queue step AQM
> > > + -
> > > + name: credit
> > > + type: s32
> > > + doc: Current credit value for WRR
> >
> > The credit member is declared in the wrong place here. The struct members
> > must match those from struct tc_dualpi2_xstats, in exactly the same order.
> >
> > > + -
> > > + name: credit-queue
> > > + type: u8
> > > + doc: Current credit queue
> > > + enum: tc-dualpi2-credit-queue-flags
> > > + enum-as-flags: true
> >
> > The credit-queue member does not exist in struct tc_dualpi2_xstats so
> > should be removed from here.
> >
> > > + -
> > > + name: memory-used
> > > + type: u32
> > > + doc: Memory used in bytes by the DualPI2
> > > + -
> > > + name: max-memory-used
> > > + type: u32
> > > + doc: Maximum memory used in bytes by the DualPI2
> > > + -
> > > + name: memory-limit
> > > + type: u32
> > > + doc: Memory limit in bytes
> > > -
> > > name: tc-fq-pie-xstats
> > > type: struct
> > > @@ -2299,6 +2382,83 @@ 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
> >
> > The convention used in YNL specs is to use the same naming as the enum
> > definition from the header, with the prefix stripped off. For this
> > attribute that would be TCA_DUALPI2_MEMORY_LIMIT -> memory-limit
> >
> > > + 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 (see also step-packets)
> > > + -
> > > + name: step-packets
> > > + type: flag
> > > + doc: L4S Step marking threshold unit in packets (otherwise is in
> > > microseconds)
> > > + -
> > > + 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
> >
> > The definition is TCA_DUALPI2_COUPLING so either this should be "coupling"
> > or the enum name should be expanded.
> >
> > > + type: uint
> > > + doc: Probability coupling factor between Classic and L4S (2 is
> > > recommended)
> > > + -
> > > + name: overload
> >
> > The definition is TCA_DUALPI2_DROP_OVERLOAD so this should be
> > "drop-overload"
> >
> > > + type: uint
> > > + doc: Control the overload strategy (drop to preserve latency or
> > > let the queue overflow)
> > > + enum: tc-dualpi2-drop-overload-flags
> > > + enum-as-flags: true
> > > + -
> > > + name: drop-early
> > > + type: uint
> > > + doc: Decide where the Classic packets are PI-based dropped or
> > > marked
> > > + enum: tc-dualpi2-drop-early-flags
> > > + enum-as-flags: true
> > > + -
> > > + name: classic-protection
> >
> > Also does not match the eum definition. Should the enum name be expanded?
> >
> > > + type: uint
> > > + doc: Classic WRR weight in percentage (from 0 to 100)
> > > + -
> > > + name: ecn-mask
> > > + type: uint
> > > + doc: Configure the L-queue ECN classifier
> > > + enum: tc-dualpi2-ecn-mask-flags
> > > + enum-as-flags: true
> > > + -
> > > + name: split-gso
> > > + type: flag
> > > + doc: Split aggregated skb or not
> > > + -
> > > + name: max-rtt
> >
> > Does not exist in the DUALPI2 enum so should be removed.
> >
> > > + type: uint
> > > + doc: The maximum expected RTT of the traffic that is controlled
> > > by DualPI2 in usec
> > > + -
> > > + name: typical-rtt
> >
> > Also does not exist in the DUALPI2 enum so should be removed.
> >
> > > + type: uint
> > > + doc: The typical base RTT of the traffic that is controlled
> > > + by DualPI2 in usec
> > > -
> > > name: tc-ematch-attrs
> > > attributes:
> > > @@ -3679,6 +3839,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 +4009,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
> > > --
> > > 2.34.1
> > >
