| 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:
<PAXPR07MB79840FF6B073C61136AC238CA3BA2@PAXPR07MB7984.eurprd07.prod.outlook.com>
Date: Wed, 23 Apr 2025 17:15:15 +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 v12 net-next 1/5] Documentation: netlink: specs: tc: Add
DualPI2 specification
> -----Original Message-----
> From: Donald Hunter <donald.hunter@...il.com>
> Sent: Wednesday, April 23, 2025 1:29 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 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@...ia-bell-labs.com writes:
>
> > 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).
> >
> > Signed-off-by: Chia-Yu Chang <chia-yu.chang@...ia-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
Powered by blists - more mailing lists