On 8/23/24 03:48, Jakub Kicinski wrote:

On Tue, 20 Aug 2024 17:12:23 +0200 Paolo Abeni wrote:

quoted

diff --git a/Documentation/netlink/specs/net_shaper.yaml b/Documentation/netlink/specs/net_shaper.yaml
new file mode 100644
index 000000000000..a2b7900646ae
--- /dev/null
+++ b/Documentation/netlink/specs/net_shaper.yaml

@@ -0,0 +1,289 @@
+# SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause)
+
+name: net-shaper
+
+doc: |
+  Network device HW rate limiting configuration.
+
+  This API allows configuring HR shapers available on the network

What's HR?

Type: HW

quoted

+  device at different levels (queues, network device) and allows
+  arbitrary manipulation of the scheduling tree of the involved
+  shapers.
+
+  Each @shaper is identified within the given device, by an @handle,
+  comprising both a @scope and an @id, and can be created via two
+  different modifiers: the @set operation, to create and update single

s/different modifiers/operations/

quoted

+  shaper, and the @group operation, to create and update a scheduling
+  group.
+
+  Existing shapers can be deleted via the @delete operation.

deleted -> deleted / reset

quoted

+  The user can query the running configuration via the @get operation.

The distinction between "scoped" nodes which can be "set"
and "detached" "node"s which can only be created via "group" (AFAIU)
needs clearer explanation.

How about re-phrasing the previous paragraph as:

   Each @shaper is identified within the given device, by an @handle,
   comprising both a @scope and an @id.

   Depending on the @scope value, the shapers are attached to specific
   HW objects (queues, devices) or, for @node scope, represent a
   scheduling group that can be placed in an arbitrary location of
   the scheduling tree.

   Shapers can be created with two different operations: the @set
   operation, to create and update single "attached" shaper, and
   the @group operation, to create and update a scheduling
   group. Only the @group operation can create @node scope shapers.

quoted

+definitions:
+  -
+    type: enum
+    name: scope
+    doc: The different scopes where a shaper can be attached.

Are they attached or are they the nodes themselves?
Maybe just say that scope defines the ID interpretation and that's it.

Will do.

quoted

+    render-max: true
+    entries:
+      - name: unspec
+        doc: The scope is not specified.
+      -
+        name: netdev
+        doc: The main shaper for the given network device.
+      -
+        name: queue
+        doc: The shaper is attached to the given device queue.
+      -
+        name: node
+        doc: |
+             The shaper allows grouping of queues or others
+             node shapers, is not attached to any user-visible

Saying it's not attached is confusing. Makes it sound like it exists
outside of the scope of a struct net_device.

What about:

   Can be placed in any arbitrary location of
   the scheduling tree, except leaves and root.

quoted

+             network device component, and can be nested to
+             either @netdev shapers or other @node shapers.

quoted

+attribute-sets:
+  -
+    name: net-shaper
+    attributes:
+      -
+        name: handle
+        type: nest
+        nested-attributes: handle
+        doc: Unique identifier for the given shaper inside the owning device.
+      -
+        name: info
+        type: nest
+        nested-attributes: info
+        doc: Fully describes the shaper.
+      -
+        name: metric
+        type: u32
+        enum: metric
+        doc: Metric used by the given shaper for bw-min, bw-max and burst.
+      -
+        name: bw-min
+        type: uint
+        doc: Minimum guaranteed B/W for the given shaper.

s/Minimum g/G/
Please spell out "bandwidth" in user-facing docs.

quoted

+      -
+        name: bw-max
+        type: uint
+        doc: Shaping B/W for the given shaper or 0 when unlimited.

s/Shaping/Maximum/

quoted

+      -
+        name: burst
+        type: uint
+        doc: Maximum burst-size for bw-min and bw-max.

How about:

s/bw-min and bw-max/shaping. Should not be interpreted as a quantum./

? >

quoted

+      -
+        name: priority
+        type: u32
+        doc: Scheduling priority for the given shaper.

Please clarify that that priority is only valid on children of
a scheduling node, and the priority values are only compared
between siblings.

quoted

+      -
+        name: weight
+        type: u32
+        doc: |
+          Weighted round robin weight for given shaper.

Relative weight of the input into a round robin node.

I would avoid mentioning 'input' unless we rolls back to the previous 
naming scheme.

?

quoted

+          The scheduling is applied to all the sibling
+          shapers with the same priority.
+      -
+        name: scope
+        type: u32
+        enum: scope
+        doc: The given shaper scope.

:)

quoted

+      -
+        name: id
+        type: u32
+        doc: |
+          The given shaper id.

"Numeric identifier of a shaper."

Do we ever use ID and scope directly in a nest with other attrs?
Or are they always wrapped in handle/parent ?
If they are always wrapped they can be a standalone attr set / space.

Will do in the next revision.

quoted

+      -
+        name: leaves
+        type: nest
+        multi-attr: true
+        nested-attributes: info
+        doc: |
+           Describes a set of leaves shapers for a @group operation.
+      -
+        name: root
+        type: nest
+        nested-attributes: root-info
+        doc: |
+           Describes the root shaper for a @group operation

missing full stop

quoted

+           Differently from @leaves and @shaper allow specifying
+           the shaper parent handle, too.

Maybe this attr is better called "node", after all.

Fine by me, but would that cause some confusion with the alias scope 
value?

quoted

+      -
+        name: shaper
+        type: nest
+        nested-attributes: info
+        doc: |
+           Describes a single shaper for a @set operation.

Hm. How is this different than "info"?

$ git grep SHAPER_A_INFO
include/uapi/linux/net_shaper.h:        NET_SHAPER_A_INFO,
$

is "info" supposed to be used?

left over from the previous revision, I will drop it.

quoted

+++ b/net/shaper/Makefile

@@ -0,0 +1,9 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Makefile for the Generic HANDSHAKE service
+#
+# Copyright (c) 2024, Red Hat, Inc.

Ironic that you added the copyright given the copy/paste
fail in the contents... ;)

Strictly speaking, since it was not intentional, it is more careless or 
stupid on my side.

Thanks,

Paolo