| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID:
<PAXPR07MB79842877B8262B919848AE85A38F2@PAXPR07MB7984.eurprd07.prod.outlook.com>
Date: Sun, 4 May 2025 22:14:03 +0000
From: "Chia-Yu Chang (Nokia)" <chia-yu.chang@...ia-bell-labs.com>
To: Donald Hunter <donald.hunter@...il.com>
CC: "xandfury@...il.com" <xandfury@...il.com>, "netdev@...r.kernel.org"
<netdev@...r.kernel.org>, "dave.taht@...il.com" <dave.taht@...il.com>,
"pabeni@...hat.com" <pabeni@...hat.com>, "jhs@...atatu.com"
<jhs@...atatu.com>, "kuba@...nel.org" <kuba@...nel.org>,
"stephen@...workplumber.org" <stephen@...workplumber.org>,
"xiyou.wangcong@...il.com" <xiyou.wangcong@...il.com>, "jiri@...nulli.us"
<jiri@...nulli.us>, "davem@...emloft.net" <davem@...emloft.net>,
"edumazet@...gle.com" <edumazet@...gle.com>, "horms@...nel.org"
<horms@...nel.org>, "andrew+netdev@...n.ch" <andrew+netdev@...n.ch>,
"ast@...erby.net" <ast@...erby.net>, "liuhangbin@...il.com"
<liuhangbin@...il.com>, "shuah@...nel.org" <shuah@...nel.org>,
"linux-kselftest@...r.kernel.org" <linux-kselftest@...r.kernel.org>,
"ij@...nel.org" <ij@...nel.org>, "ncardwell@...gle.com"
<ncardwell@...gle.com>, "Koen De Schepper (Nokia)"
<koen.de_schepper@...ia-bell-labs.com>, g.white <g.white@...lelabs.com>,
"ingemar.s.johansson@...csson.com" <ingemar.s.johansson@...csson.com>,
"mirja.kuehlewind@...csson.com" <mirja.kuehlewind@...csson.com>,
"cheshire@...le.com" <cheshire@...le.com>, "rs.ietf@....at" <rs.ietf@....at>,
"Jason_Livingood@...cast.com" <Jason_Livingood@...cast.com>, vidhi_goel
<vidhi_goel@...le.com>
Subject: RE: [PATCH v13 net-next 1/5] Documentation: netlink: specs: tc: Add
DualPI2 specification
> -----Original Message-----
> From: Donald Hunter <donald.hunter@...il.com>
> Sent: Tuesday, April 29, 2025 7:15 PM
> To: Chia-Yu Chang (Nokia) <chia-yu.chang@...ia-bell-labs.com>
> Cc: xandfury@...il.com; netdev@...r.kernel.org; dave.taht@...il.com; pabeni@...hat.com; jhs@...atatu.com; kuba@...nel.org; stephen@...workplumber.org; xiyou.wangcong@...il.com; jiri@...nulli.us; davem@...emloft.net; edumazet@...gle.com; horms@...nel.org; andrew+netdev@...n.ch; ast@...erby.net; liuhangbin@...il.com; shuah@...nel.org; linux-kselftest@...r.kernel.org; ij@...nel.org; ncardwell@...gle.com; Koen De Schepper (Nokia) <koen.de_schepper@...ia-bell-labs.com>; g.white <g.white@...lelabs.com>; ingemar.s.johansson@...csson.com; mirja.kuehlewind@...csson.com; cheshire@...le.com; rs.ietf@....at; Jason_Livingood@...cast.com; vidhi_goel <vidhi_goel@...le.com>
> 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, <chia-yu.chang@...ia-bell-labs.com> wrote:
> >
> > From: Chia-Yu Chang <chia-yu.chang@...ia-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).
>
> 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
> > Signed-off-by: Chia-Yu Chang <chia-yu.chang@...ia-bell-labs.com>
> > ---
> > 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
> >
Powered by blists - more mailing lists