| 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: <CAD4GDZxPrgfMFLTG4Mc1VK0hM5ODP2tTFF771EhMzWnbC8BwaQ@mail.gmail.com>
Date: Tue, 29 Apr 2025 18:15:03 +0100
From: Donald Hunter <donald.hunter@...il.com>
To: 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@...ia-bell-labs.com,
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@...le.com
Subject: Re: [PATCH v13 net-next 1/5] Documentation: netlink: specs: tc: Add
DualPI2 specification
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.
> 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