net: introduce TX H/W shaping API

Documentation/netlink/specs/shaper.yaml new file +6 -0

+    entries:
+      -
+        name: bps
+        doc: Shaper operates on a bits per second basis
+      -
+        name: pps

Documentation/netlink/specs/shaper.yaml new file +35 -0

+      -
+        name: handle
+        type: nest
+        nested-attributes: handle
+        doc: Unique identifier for the given 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
+      -
+        name: bw-max
+        type: uint
+        doc: Shaping B/W for the given shaper or 0 when unlimited
+      -
+        name: burst
+        type: uint
+        doc: Maximum burst-size for bw-min and bw-max
+      -
+        name: priority
+        type: u32
+        doc: Scheduling priority for the given shaper
+      -
+        name: weight
+        type: u32
+        doc: |
+          Weighted round robin weight for given shaper.
+          The scheduling is applied to all the sibling
+          shapers with the same priority
+      -
+        name: scope

Documentation/netlink/specs/shaper.yaml new file +112 -0

+      -
+        name: shaper
+        type: nest
+        nested-attributes: ns-info
+        doc: |
+           Describes a single shaper for a @set operation
+  -
+    name: handle
+    subset-of: net-shaper
+    attributes:
+      -
+        name: scope
+      -
+        name: id
+  -
+    name: ns-info
+    subset-of: net-shaper
+    attributes:
+      -
+        name: handle
+      -
+        name: metric
+      -
+        name: bw-min
+      -
+        name: bw-max
+      -
+        name: burst
+      -
+        name: priority
+      -
+        name: weight
+  -
+    name: ns-output-info
+    subset-of: net-shaper
+    attributes:
+      -
+        name: parent
+      -
+        name: handle
+      -
+        name: metric
+      -
+        name: bw-min
+      -
+        name: bw-max
+      -
+        name: burst
+      -
+        name: priority
+      -
+        name: weight
+
+operations:
+  list:
+    -
+      name: get
+      doc: |
+        Get / Dump information about a/all the shaper for a given device
+      attribute-set: net-shaper
+
+      do:
+        request:
+          attributes:
+            - ifindex
+            - handle
+        reply:
+          attributes: &ns-attrs
+            - parent
+            - handle
+            - metric
+            - bw-min
+            - bw-max
+            - burst
+            - priority
+            - weight
+
+      dump:
+        request:
+          attributes:
+            - ifindex
+        reply:
+          attributes: *ns-attrs
+    -
+      name: set
+      doc: |
+        Create or configures the specified shaper.
+        On failures the extack is set accordingly.
+        Can't create @detached scope shaper, use
+        the @group operation instead.
+      attribute-set: net-shaper
+      flags: [ admin-perm ]
+
+      do:
+        request:
+          attributes:
+            - ifindex
+            - shaper
+
+    -
+      name: delete
+      doc: |
+        Clear (remove) the specified shaper. If after the removal
+        the parent shaper has no more children and the parent
+        shaper scope is @detached, even the parent is deleted,
+        recursively.
+        On failures the extack is set accordingly.
+      attribute-set: net-shaper
+      flags: [ admin-perm ]
+
+      do:
+        request:

Documentation/netlink/specs/shaper.yaml new file +9 -0

+      do:
+        request:
+          attributes:
+            - ifindex
+            - inputs
+            - output
+        reply:
+          attributes:
+            - handle

Documentation/netlink/specs/shaper.yaml

Quoted 5 lines · Paolo Abeni

Documentation/netlink/specs/shaper.yaml new file +0 -0

new file mode 100644
index 000000000000..7327f5596fdb

It's probably more user-friendly to use the same filename as the spec name, so net-shaper.yaml

Documentation/netlink/specs/shaper.yaml line 15

Quoted 4 lines

Documentation/netlink/specs/shaper.yaml new file +4 -0

+    render-max: true
+    entries:
+      - name: unspec
+        doc: The scope is not specified

What are the semantics of 'unspec' ? When can it be used?

Documentation/netlink/specs/shaper.yaml line 21

Quoted 6 lines

Documentation/netlink/specs/shaper.yaml new file +6 -0

+      -
+        name: port
+        doc: The root for the whole H/W
+      -
+        name: netdev
+        doc: The main shaper for the given network device.

What are the semantic differences between netdev and port?

Documentation/netlink/specs/shaper.yaml line 30

Quoted 9 lines

Documentation/netlink/specs/shaper.yaml new file +9 -0

+      -
+        name: queue
+        doc: The shaper is attached to the given device queue.
+      -
+        name: detached
+        doc: |
+             The shaper is not attached to any user-visible network
+             device component and allows nesting and grouping of
+             queues or others detached shapers.

I assume that shapers are always owned by the netdev regardless of attach status?

Documentation/netlink/specs/shaper.yaml line 110

Quoted 7 lines

+>> + -

Documentation/netlink/specs/shaper.yaml new file +6 -0

+        name: inputs
+        type: nest
+        multi-attr: true
+        nested-attributes: ns-info
+        doc: |
+           Describes a set of inputs shapers for a @group operation

The @group renders exactly as-is in the generated htmldocs. There may be a more .rst friendly markup you can use that will render better.

Documentation/netlink/specs/shaper.yaml line 241

Quoted 6 lines

Documentation/netlink/specs/shaper.yaml new file +6 -0

+    -
+      name: group
+      doc: |
+        Group the specified input shapers under the specified
+        output shaper, eventually creating the latter, if needed.
+        Input shapers scope must be either @queue or @detached.

It says above that you cannot create a detached shaper, so how do you create one to use as an input shaper here? Is this group op more like a multi-create op?

Documentation/netlink/specs/shaper.yaml line 252

Quoted 11 lines

Documentation/netlink/specs/shaper.yaml new file +11 -0

+        Output shaper scope must be either @detached or @netdev.
+        When using an output @detached scope shaper, if the
+        @handle @id is not specified, a new shaper of such scope
+        is created and, otherwise the specified output shaper
+        must be already existing.
+        The operation is atomic, on failures the extack is set
+        accordingly and no change is applied to the device
+        shaping configuration, otherwise the output shaper
+        handle is provided as reply.
+      attribute-set: net-shaper
+      flags: [ admin-perm ]

Does there need to be a reciprocal 'ungroup' operation? Without it, create / group / delete seems like they will have ambiguous semantics.

Quoted 10 lines · Donald Hunter

Documentation/netlink/specs/shaper.yaml

Quoted 5 lines · Paolo Abeni

Documentation/netlink/specs/shaper.yaml new file +0 -0

new file mode 100644
index 000000000000..7327f5596fdb

It's probably more user-friendly to use the same filename as the spec name, so net-shaper.yaml

No big objection on my side, but if we enforce 'Name:' to be $(basename $file .yaml), the 'Name' field becomes redundant.

[...]

Quoted 5 lines

Quoted 4 lines

Documentation/netlink/specs/shaper.yaml new file +4 -0

+    render-max: true
+    entries:
+      - name: unspec
+        doc: The scope is not specified

Quoted 1 line

What are the semantics of 'unspec' ? When can it be used?

I guess at this point it can be dropped. It was introduced in a previous incarnation to represent the port parent - the port does not have a parent, being the root of the hierarchy.

Quoted 8 lines

Documentation/netlink/specs/shaper.yaml line 21

Quoted 6 lines

Documentation/netlink/specs/shaper.yaml new file +6 -0

+      -
+        name: port
+        doc: The root for the whole H/W
+      -
+        name: netdev
+        doc: The main shaper for the given network device.

What are the semantic differences between netdev and port?

netdev == Linux network device
port == wire plug

Quoted 12 lines

Documentation/netlink/specs/shaper.yaml line 30

Quoted 9 lines

Documentation/netlink/specs/shaper.yaml new file +9 -0

+      -
+        name: queue
+        doc: The shaper is attached to the given device queue.
+      -
+        name: detached
+        doc: |
+             The shaper is not attached to any user-visible network
+             device component and allows nesting and grouping of
+             queues or others detached shapers.

I assume that shapers are always owned by the netdev regardless of attach status?

If you mean that it's up to the netdev clean them up on (netdev) removal, yes.

Quoted 8 lines · Donald Hunter

Quoted 6 lines · Paolo Abeni

Documentation/netlink/specs/shaper.yaml new file +6 -0

+        name: inputs
+        type: nest
+        multi-attr: true
+        nested-attributes: ns-info
+        doc: |
+           Describes a set of inputs shapers for a @group operation

Quoted 2 lines

The @group renders exactly as-is in the generated htmldocs. There may be a more .rst friendly markup you can use that will render better.

Uhm... AFAICS the problem is the target (e.g. 'group') is outside the htmldoc section itself, I can't find any existing markup to serve this purpose well. What about sticking to quotes '' everywhere?

FTR, I used @ following the kdoc style.

[...]

Quoted 9 lines

Documentation/netlink/specs/shaper.yaml line 241

Quoted 6 lines

Documentation/netlink/specs/shaper.yaml new file +6 -0

+    -
+      name: group
+      doc: |
+        Group the specified input shapers under the specified
+        output shaper, eventually creating the latter, if needed.
+        Input shapers scope must be either @queue or @detached.

It says above that you cannot create a detached shaper, so how do you create one to use as an input shaper here? Is this group op more like a multi-create op?

The group operation has the main goal of configuring a single WRR or SP scheduling group atomically. It can creates the needed shapers as needed, see below.

The need for such operation sparks from some H/W constraints:

https://lore.kernel.org/netdev/[email protected]/

Quoted 11 lines

Quoted 11 lines

Documentation/netlink/specs/shaper.yaml new file +11 -0

+        Output shaper scope must be either @detached or @netdev.
+        When using an output @detached scope shaper, if the
+        @handle @id is not specified, a new shaper of such scope
+        is created and, otherwise the specified output shaper
+        must be already existing.
+        The operation is atomic, on failures the extack is set
+        accordingly and no change is applied to the device
+        shaping configuration, otherwise the output shaper
+        handle is provided as reply.
+      attribute-set: net-shaper
+      flags: [ admin-perm ]

Quoted 2 lines

Does there need to be a reciprocal 'ungroup' operation? Without it, create / group / delete seems like they will have ambiguous semantics.

I guess we need a better description. Can you please tell where/how the current one is ambiguous?

Quoted 16 lines · Paolo Abeni

Quoted 10 lines · Donald Hunter

Documentation/netlink/specs/shaper.yaml line 110

Quoted 6 lines · Paolo Abeni

Documentation/netlink/specs/shaper.yaml new file +6 -0

+        name: inputs
+        type: nest
+        multi-attr: true
+        nested-attributes: ns-info
+        doc: |
+           Describes a set of inputs shapers for a @group operation

The @group renders exactly as-is in the generated htmldocs. There may be a more .rst friendly markup you can use that will render better.

Uhm... AFAICS the problem is the target (e.g. 'group') is outside the htmldoc section itself, I can't find any existing markup to serve this purpose well. What about sticking to quotes '' everywhere?

Quoted 1 line

FTR, I used @ following the kdoc style.

Yeah, I was just thinking of using .rst markup like ``code`` or italics, but the meaning of @ is pretty obvious when reading the spec. If you stick with @ then we could always teach ynl-to-rst to render it as ``code``.

Quoted 29 lines

[...]

Quoted 9 lines

Documentation/netlink/specs/shaper.yaml line 241

Quoted 6 lines

Documentation/netlink/specs/shaper.yaml new file +6 -0

+    -
+      name: group
+      doc: |
+        Group the specified input shapers under the specified
+        output shaper, eventually creating the latter, if needed.
+        Input shapers scope must be either @queue or @detached.

It says above that you cannot create a detached shaper, so how do you create one to use as an input shaper here? Is this group op more like a multi-create op?

The group operation has the main goal of configuring a single WRR or SP scheduling group atomically. It can creates the needed shapers as needed, see below.

The need for such operation sparks from some H/W constraints:

https://lore.kernel.org/netdev/[email protected]/

Quoted 11 lines

Quoted 11 lines

Documentation/netlink/specs/shaper.yaml new file +11 -0

+        Output shaper scope must be either @detached or @netdev.
+        When using an output @detached scope shaper, if the
+        @handle @id is not specified, a new shaper of such scope
+        is created and, otherwise the specified output shaper
+        must be already existing.
+        The operation is atomic, on failures the extack is set
+        accordingly and no change is applied to the device
+        shaping configuration, otherwise the output shaper
+        handle is provided as reply.
+      attribute-set: net-shaper
+      flags: [ admin-perm ]

Quoted 5 lines

Quoted 2 lines

Does there need to be a reciprocal 'ungroup' operation? Without it, create / group / delete seems like they will have ambiguous semantics.

I guess we need a better description. Can you please tell where/how the current one is ambiguous?

My expectation for 'group' would be to group existing things, with a reciprocal 'ungroup' operation. I think you intend 'group' to both be able to group existing shapers/groups and create a group of shapers.

Am I right in saying that delete lets you delete something from a group (with side-effect of deleting group if it becomes empty), or delete a whole group?

It feels a lot like each of 'set', 'group' and 'delete' are doing multiple things and the interaction between them all becomes challenging to describe, or to handle all the corner case > I think part of the problem is the mixed terminology of input, output for groups, handle, parent for shapers and using detached to differentiate from 'implicitly attached to a resource'.

Perhaps the API would be better if you had:

- shaper-new
- shaper-delete
- shaper-get/dump
- shaper-set
- group-new
- group-delete
- group-get/dump
- group-set

If you went with Jakub's suggestion to give every shaper n x inputs and an output, then you could recombine groups and shapers and just have 4 ops. And you could rename 'detached' to 'shaper' so that an attachment is one of port, netdev, queue or shaper.

Quoted 11 lines

Perhaps the API would be better if you had:

- shaper-new
- shaper-delete
- shaper-get/dump
- shaper-set
- group-new
- group-delete
- group-get/dump
- group-set

Quoted 4 lines

If you went with Jakub's suggestion to give every shaper n x inputs and an output, then you could recombine groups and shapers and just have 4 ops. And you could rename 'detached' to 'shaper' so that an attachment is one of port, netdev, queue or shaper.

I'm unsure I read the above correctly, and I'm unsure it's in the same direction of Jakub's suggestion. AFACS the above is basically the same interface we proposed in the past iteration and was explicitly nacked from Jakub,

Additionally, one of the constraint to be addressed here is allowing to setup/configures all the nodes in a 'group' with a single operation, to deal with H/W limitations. How would the above address such constraint?

Define the user-space visible interface to query, configure and delete network shapers via yaml definition.

Add dummy implementations for the relevant NL callbacks.

set() and delete() operations touch a single shaper creating/updating or deleting it.
The group() operation creates a shaper's group, nesting multiple input shapers under the specified output shaper.

Signed-off-by Paolo Abeni <[email protected]>

---
RFC v1 -> RFC v2:
- u64 -> uint
- net_shapers -> net-shapers
- documented all the attributes
- dropped [ admin-perm ] for get() op
- group op
- set/delete touch a single shaper
---

Documentation/netlink/specs/shaper.yaml | 262 ++++++++++++++++++++++++
MAINTAINERS                             |   1 +
include/uapi/linux/net_shaper.h         |  74 +++++++
net/Kconfig                             |   3 +
net/Makefile                            |   1 +
net/shaper/Makefile                     |   9 +
net/shaper/shaper.c                     |  34 +++
net/shaper/shaper_nl_gen.c              | 117 +++++++++++
net/shaper/shaper_nl_gen.h              |  27 +++
9 files changed, 528 insertions(+)

create mode 100644 Documentation/netlink/specs/shaper.yaml create mode 100644 include/uapi/linux/net_shaper.h create mode 100644 net/shaper/Makefile
create mode 100644 net/shaper/shaper.c
create mode 100644 net/shaper/shaper_nl_gen.c create mode 100644 net/shaper/shaper_nl_gen.h

Documentation/netlink/specs/shaper.yaml new file +8 -0

new file mode 100644
index 000000000000..7327f5596fdb
@@ -0,0 +1,262 @@
+# SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause)
+
+name: net-shaper
+
+doc: Network device HW Rate Limiting offload
+
+definitions:
+  -

Documentation/netlink/specs/shaper.yaml new file +12 -0

+  -
+    type: enum
+    name: metric
+    doc: different metric each shaper can support
+    entries:
+      -
+        name: bps
+        doc: Shaper operates on a bits per second basis
+      -
+        name: pps
+        doc: Shaper operates on a packets per second basis
+

Documentation/netlink/specs/shaper.yaml new file +65 -0

+
+operations:
+  list:
+    -
+      name: get
+      doc: |
+        Get / Dump information about a/all the shaper for a given device
+      attribute-set: net-shaper
+
+      do:
+        request:
+          attributes:
+            - ifindex
+            - handle
+        reply:
+          attributes: &ns-attrs
+            - parent
+            - handle
+            - metric
+            - bw-min
+            - bw-max
+            - burst
+            - priority
+            - weight
+
+      dump:
+        request:
+          attributes:
+            - ifindex
+        reply:
+          attributes: *ns-attrs
+    -
+      name: set
+      doc: |
+        Create or configures the specified shaper.
+        On failures the extack is set accordingly.
+        Can't create @detached scope shaper, use
+        the @group operation instead.
+      attribute-set: net-shaper
+      flags: [ admin-perm ]
+
+      do:
+        request:
+          attributes:
+            - ifindex
+            - shaper
+
+    -
+      name: delete
+      doc: |
+        Clear (remove) the specified shaper. If after the removal
+        the parent shaper has no more children and the parent
+        shaper scope is @detached, even the parent is deleted,
+        recursively.
+        On failures the extack is set accordingly.
+      attribute-set: net-shaper
+      flags: [ admin-perm ]
+
+      do:
+        request:
+          attributes:
+            - ifindex
+            - handle
+
+    -

Documentation/netlink/specs/shaper.yaml new file +5 -0

+        The operation is atomic, on failures the extack is set
+        accordingly and no change is applied to the device
+        shaping configuration, otherwise the output shaper
+        handle is provided as reply.
+      attribute-set: net-shaper

Documentation/netlink/specs/shaper.yaml line 30

Quoted 12 lines

Documentation/netlink/specs/shaper.yaml new file +12 -0

+      -
+        name: netdev
+        doc: The main shaper for the given network device.
+      -
+        name: queue
+        doc: The shaper is attached to the given device queue.
+      -
+        name: detached
+        doc: |
+             The shaper is not attached to any user-visible network
+             device component and allows nesting and grouping of
+             queues or others detached shapers.  

What is the purpose of the "detached" thing?

Documentation/netlink/specs/shaper.yaml line 18

Quoted 10 lines · Tue, Jul 30, 2024 at 10:39:45PM CEST, [email protected] wrote:

Documentation/netlink/specs/shaper.yaml new file +10 -0

+    type: enum
+    name: scope
+    doc: the different scopes where a shaper can be attached
+    render-max: true
+    entries:
+      - name: unspec
+        doc: The scope is not specified
+      -
+        name: port
+        doc: The root for the whole H/W

What is this "port"?

Documentation/netlink/specs/shaper.yaml line 30

Quoted 12 lines

Documentation/netlink/specs/shaper.yaml new file +12 -0

+      -
+        name: netdev
+        doc: The main shaper for the given network device.
+      -
+        name: queue
+        doc: The shaper is attached to the given device queue.
+      -
+        name: detached
+        doc: |
+             The shaper is not attached to any user-visible network
+             device component and allows nesting and grouping of
+             queues or others detached shapers.

What is the purpose of the "detached" thing?

Documentation/netlink/specs/shaper.yaml line 246

Quoted 11 lines

Documentation/netlink/specs/shaper.yaml new file +11 -0

+    -
+      name: group
+      doc: |
+        Group the specified input shapers under the specified
+        output shaper, eventually creating the latter, if needed.
+        Input shapers scope must be either @queue or @detached.
+        Output shaper scope must be either @detached or @netdev.
+        When using an output @detached scope shaper, if the
+        @handle @id is not specified, a new shaper of such scope
+        is created and, otherwise the specified output shaper
+        must be already existing.

I'm lost. Could this designt be described in details in the doc I asked in the cover letter? :/ Please.

Quoted 12 lines · Jiri Pirko

Quoted 10 lines · Tue, Jul 30, 2024 at 10:39:45PM CEST, [email protected] wrote:

Documentation/netlink/specs/shaper.yaml new file +10 -0

+    type: enum
+    name: scope
+    doc: the different scopes where a shaper can be attached
+    render-max: true
+    entries:
+      - name: unspec
+        doc: The scope is not specified
+      -
+        name: port
+        doc: The root for the whole H/W

Quoted 1 line

What is this "port"?

~ a wire plug.

Quoted 14 lines

Documentation/netlink/specs/shaper.yaml line 30

Quoted 13 lines

Documentation/netlink/specs/shaper.yaml new file +12 -0

+      -
+        name: netdev
+        doc: The main shaper for the given network device.
+      -
+        name: queue
+        doc: The shaper is attached to the given device queue.
+      -
+        name: detached
+        doc: |
+             The shaper is not attached to any user-visible network
+             device component and allows nesting and grouping of
+             queues or others detached shapers.

Quoted 1 line

What is the purpose of the "detached" thing?

I fear I can't escape reusing most of the wording above. 'detached' nodes goal is to create groups of other shapers. i.e. queue groups, allowing multiple levels nesting, i.e. to implement this kind of hierarchy:

q1 ----- \
q2 - \SP / RR ------

q3 - /    	    \
	q4 - \ SP -> (netdev)
	q5 - /	    /
                  /
	q6 - \ RR
	q7 - /

where q1..q7 are queue-level shapers and all the SP/RR are 'detached' one. The conf. does not necessary make any functional sense, just to describe the things.

Quoted 13 lines

Documentation/netlink/specs/shaper.yaml line 246

Quoted 12 lines

Documentation/netlink/specs/shaper.yaml new file +11 -0

+    -
+      name: group
+      doc: |
+        Group the specified input shapers under the specified
+        output shaper, eventually creating the latter, if needed.
+        Input shapers scope must be either @queue or @detached.
+        Output shaper scope must be either @detached or @netdev.
+        When using an output @detached scope shaper, if the
+        @handle @id is not specified, a new shaper of such scope
+        is created and, otherwise the specified output shaper
+        must be already existing.

Quoted 2 lines

I'm lost. Could this designt be described in details in the doc I asked in the cover letter? :/ Please.

I'm unsure if the context information here and in the previous replies helped somehow.

The group operation creates and configure a scheduling group, i.e. this

q1 ----- \
q2 - \SP / RR ------

q3 - /    	    \
	q4 - \ SP -> (netdev)
	q5 - /	    /
                  /
	q6 - \ RR
	q7 - /

can be create with:

group(inputs:[q6, q7], output:[detached,parent:netdev]) group(inputs:[q4, q5], output:[detached,parent:netdev]) group(inputs:[q1], output:[detached,parent:netdev]) group(inputs:[q2,q3], output:[detached,parent:<the detached shaper create above>])

I'm unsure if this the kind of info you are looking for?

Thanks,

Paolo

Quoted 13 lines · Thu, Aug 01, 2024 at 05:12:01PM CEST, [email protected] wrote:

Quoted 12 lines · Jiri Pirko

Quoted 10 lines · Tue, Jul 30, 2024 at 10:39:45PM CEST, [email protected] wrote:

Documentation/netlink/specs/shaper.yaml new file +10 -0

+    type: enum
+    name: scope
+    doc: the different scopes where a shaper can be attached
+    render-max: true
+    entries:
+      - name: unspec
+        doc: The scope is not specified
+      -
+        name: port
+        doc: The root for the whole H/W

Quoted 3 lines

Quoted 1 line

What is this "port"?

~ a wire plug.

What's "wire plug"? What of existing kernel objects this relates to? Is it a devlink port?

Quoted 28 lines

Quoted 13 lines

Documentation/netlink/specs/shaper.yaml line 30

Quoted 12 lines

Documentation/netlink/specs/shaper.yaml new file +12 -0

+      -
+        name: netdev
+        doc: The main shaper for the given network device.
+      -
+        name: queue
+        doc: The shaper is attached to the given device queue.
+      -
+        name: detached
+        doc: |
+             The shaper is not attached to any user-visible network
+             device component and allows nesting and grouping of
+             queues or others detached shapers.

Quoted 1 line

What is the purpose of the "detached" thing?

I fear I can't escape reusing most of the wording above. 'detached' nodes goal is to create groups of other shapers. i.e. queue groups, allowing multiple levels nesting, i.e. to implement this kind of hierarchy:

q1 ----- \
q2 - \SP / RR ------

q3 - /    	    \
	q4 - \ SP -> (netdev)
	q5 - /	    /
                   /
	q6 - \ RR
	q7 - /

Quoted 3 lines

where q1..q7 are queue-level shapers and all the SP/RR are 'detached' one. The conf. does not necessary make any functional sense, just to describe the things.

Can you "attach" the "detached" ones? They are "detached" from what?

Quoted 29 lines

Quoted 12 lines

Documentation/netlink/specs/shaper.yaml line 246

Quoted 11 lines

Documentation/netlink/specs/shaper.yaml new file +11 -0

+    -
+      name: group
+      doc: |
+        Group the specified input shapers under the specified
+        output shaper, eventually creating the latter, if needed.
+        Input shapers scope must be either @queue or @detached.
+        Output shaper scope must be either @detached or @netdev.
+        When using an output @detached scope shaper, if the
+        @handle @id is not specified, a new shaper of such scope
+        is created and, otherwise the specified output shaper
+        must be already existing.

Quoted 2 lines

I'm lost. Could this designt be described in details in the doc I asked in the cover letter? :/ Please.

I'm unsure if the context information here and in the previous replies helped somehow.

The group operation creates and configure a scheduling group, i.e. this

q1 ----- \
q2 - \SP / RR ------

q3 - /    	    \
	q4 - \ SP -> (netdev)
	q5 - /	    /
                   /
	q6 - \ RR
	q7 - /

Quoted 7 lines

can be create with:

group(inputs:[q6, q7], output:[detached,parent:netdev]) group(inputs:[q4, q5], output:[detached,parent:netdev]) group(inputs:[q1], output:[detached,parent:netdev]) group(inputs:[q2,q3], output:[detached,parent:<the detached shaper create above>])

So by "inputs" and "output" you are basically building a tree. In devlink rate, we have leaf and node, which is in sync with standard tree terminology.

If what you are building is tree, why don't you use the same terminology? If you are building tree, you just need to have the link to upper noded (output in your terminology). Why you have "inputs"? Isn't that redundant?

Hi all,

(same remark of my previous email). My replies this week will be delayed, please allow for some extra latency.

Quoted 14 lines · Jiri Pirko

Quoted 13 lines · Thu, Aug 01, 2024 at 05:12:01PM CEST, [email protected] wrote:

Quoted 12 lines · Jiri Pirko

Quoted 10 lines · Tue, Jul 30, 2024 at 10:39:45PM CEST, [email protected] wrote:

Documentation/netlink/specs/shaper.yaml new file +10 -0

+    type: enum
+    name: scope
+    doc: the different scopes where a shaper can be attached
+    render-max: true
+    entries:
+      - name: unspec
+        doc: The scope is not specified
+      -
+        name: port
+        doc: The root for the whole H/W

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

What is this "port"?

~ a wire plug.

What's "wire plug"? What of existing kernel objects this relates to? Is it a devlink port?

I'm sorry, my hasty translation of my native language was really inaccurate. Let me re-phrase from scratch: that is actually the root of the whole scheduling tree (yes, it's a tree) for a given network device.

One source of confusion is that in a previous iteration we intended to allow configuring even objects 'above' the network device level, but such feature has been dropped.

We could probably drop this scope entirely.

Quoted 33 lines

Quoted 32 lines

Quoted 14 lines

Documentation/netlink/specs/shaper.yaml line 30

Quoted 13 lines

Documentation/netlink/specs/shaper.yaml new file +12 -0

+      -
+        name: netdev
+        doc: The main shaper for the given network device.
+      -
+        name: queue
+        doc: The shaper is attached to the given device queue.
+      -
+        name: detached
+        doc: |
+             The shaper is not attached to any user-visible network
+             device component and allows nesting and grouping of
+             queues or others detached shapers.

Quoted 1 line

What is the purpose of the "detached" thing?

I fear I can't escape reusing most of the wording above. 'detached' nodes goal is to create groups of other shapers. i.e. queue groups, allowing multiple levels nesting, i.e. to implement this kind of hierarchy:

q1 ----- \
q2 - \SP / RR ------

q3 - /    	    \
	q4 - \ SP -> (netdev)
	q5 - /	    /
                   /
	q6 - \ RR
	q7 - /

where q1..q7 are queue-level shapers and all the SP/RR are 'detached' one. The conf. does not necessary make any functional sense, just to describe the things.

Quoted 1 line

Can you "attach" the "detached" ones? They are "detached" from what?

I see such name is very confusing. An alternative one could be 'group', but IIRC it was explicitly discarded while discussing a previous iteration.

The 'detached' name comes from the fact the such shapers are not a direct representation of some well-known kernel object (queues, devices),

Quoted 38 lines

Quoted 30 lines

Quoted 13 lines

Documentation/netlink/specs/shaper.yaml line 246

Quoted 12 lines

Documentation/netlink/specs/shaper.yaml new file +11 -0

+    -
+      name: group
+      doc: |
+        Group the specified input shapers under the specified
+        output shaper, eventually creating the latter, if needed.
+        Input shapers scope must be either @queue or @detached.
+        Output shaper scope must be either @detached or @netdev.
+        When using an output @detached scope shaper, if the
+        @handle @id is not specified, a new shaper of such scope
+        is created and, otherwise the specified output shaper
+        must be already existing.

Quoted 2 lines

I'm lost. Could this designt be described in details in the doc I asked in the cover letter? :/ Please.

I'm unsure if the context information here and in the previous replies helped somehow.

The group operation creates and configure a scheduling group, i.e. this

q1 ----- \
q2 - \SP / RR ------

q3 - /    	    \
	q4 - \ SP -> (netdev)
	q5 - /	    /
                   /
	q6 - \ RR
	q7 - /

Quoted 7 lines

can be create with:

group(inputs:[q6, q7], output:[detached,parent:netdev]) group(inputs:[q4, q5], output:[detached,parent:netdev]) group(inputs:[q1], output:[detached,parent:netdev]) group(inputs:[q2,q3], output:[detached,parent:<the detached shaper create above>])

Quoted 8 lines

So by "inputs" and "output" you are basically building a tree. In devlink rate, we have leaf and node, which is in sync with standard tree terminology.

If what you are building is tree, why don't you use the same terminology? If you are building tree, you just need to have the link to upper noded (output in your terminology). Why you have "inputs"? Isn't that redundant?

The idea behind the inputs/outputs naming is to represent the data flow towards the wire.
I'm fine with the parent/children naming, but IIRC Jakub was not happy with it. Is there any intermediate ground that could satisfy both of you?

Thanks,

Paolo

Quoted 32 lines · Mon, Aug 05, 2024 at 05:11:09PM CEST, [email protected] wrote:

Hi all,

(same remark of my previous email). My replies this week will be delayed, please allow for some extra latency.

Quoted 14 lines · Jiri Pirko

Quoted 13 lines · Thu, Aug 01, 2024 at 05:12:01PM CEST, [email protected] wrote:

Quoted 12 lines · Jiri Pirko

Quoted 10 lines · Tue, Jul 30, 2024 at 10:39:45PM CEST, [email protected] wrote:

Documentation/netlink/specs/shaper.yaml new file +10 -0

+    type: enum
+    name: scope
+    doc: the different scopes where a shaper can be attached
+    render-max: true
+    entries:
+      - name: unspec
+        doc: The scope is not specified
+      -
+        name: port
+        doc: The root for the whole H/W

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

What is this "port"?

~ a wire plug.

What's "wire plug"? What of existing kernel objects this relates to? Is it a devlink port?

I'm sorry, my hasty translation of my native language was really inaccurate. Let me re-phrase from scratch: that is actually the root of the whole scheduling tree (yes, it's a tree) for a given network device.

Quoted 5 lines

One source of confusion is that in a previous iteration we intended to allow configuring even objects 'above' the network device level, but such feature has been dropped.

We could probably drop this scope entirely.

Drop for now, correct? I agree that your patchset now only works on top of netdev. But all infra should be ready to work on top of something else, devlink seems like good candidate. I mean, for devlink port function rate, we will definitelly need something like that.

Quoted 33 lines

Quoted 33 lines

Quoted 32 lines

Quoted 14 lines

Documentation/netlink/specs/shaper.yaml line 30

Quoted 13 lines

Documentation/netlink/specs/shaper.yaml new file +12 -0

+      -
+        name: netdev
+        doc: The main shaper for the given network device.
+      -
+        name: queue
+        doc: The shaper is attached to the given device queue.
+      -
+        name: detached
+        doc: |
+             The shaper is not attached to any user-visible network
+             device component and allows nesting and grouping of
+             queues or others detached shapers.

Quoted 1 line

What is the purpose of the "detached" thing?

I fear I can't escape reusing most of the wording above. 'detached' nodes goal is to create groups of other shapers. i.e. queue groups, allowing multiple levels nesting, i.e. to implement this kind of hierarchy:

q1 ----- \
q2 - \SP / RR ------

q3 - /    	    \
	q4 - \ SP -> (netdev)
	q5 - /	    /
                    /
	q6 - \ RR
	q7 - /

where q1..q7 are queue-level shapers and all the SP/RR are 'detached' one. The conf. does not necessary make any functional sense, just to describe the things.

Quoted 7 lines

Quoted 1 line

Can you "attach" the "detached" ones? They are "detached" from what?

I see such name is very confusing. An alternative one could be 'group', but IIRC it was explicitly discarded while discussing a previous iteration.

The 'detached' name comes from the fact the such shapers are not a direct representation of some well-known kernel object (queues, devices),

Understand now. Maybe "node" would make more sense? Leaves are queues and root is the device? Aligns with the tree terminology...

Quoted 47 lines

Quoted 38 lines

Quoted 30 lines

Quoted 13 lines

Documentation/netlink/specs/shaper.yaml line 246

Quoted 12 lines

Documentation/netlink/specs/shaper.yaml new file +11 -0

+    -
+      name: group
+      doc: |
+        Group the specified input shapers under the specified
+        output shaper, eventually creating the latter, if needed.
+        Input shapers scope must be either @queue or @detached.
+        Output shaper scope must be either @detached or @netdev.
+        When using an output @detached scope shaper, if the
+        @handle @id is not specified, a new shaper of such scope
+        is created and, otherwise the specified output shaper
+        must be already existing.

Quoted 2 lines

I'm lost. Could this designt be described in details in the doc I asked in the cover letter? :/ Please.

I'm unsure if the context information here and in the previous replies helped somehow.

The group operation creates and configure a scheduling group, i.e. this

q1 ----- \
q2 - \SP / RR ------

q3 - /    	    \
	q4 - \ SP -> (netdev)
	q5 - /	    /
                    /
	q6 - \ RR
	q7 - /

Quoted 7 lines

can be create with:

group(inputs:[q6, q7], output:[detached,parent:netdev]) group(inputs:[q4, q5], output:[detached,parent:netdev]) group(inputs:[q1], output:[detached,parent:netdev]) group(inputs:[q2,q3], output:[detached,parent:<the detached shaper create above>])

Quoted 8 lines

So by "inputs" and "output" you are basically building a tree. In devlink rate, we have leaf and node, which is in sync with standard tree terminology.

If what you are building is tree, why don't you use the same terminology? If you are building tree, you just need to have the link to upper noded (output in your terminology). Why you have "inputs"? Isn't that redundant?

Quoted 4 lines

The idea behind the inputs/outputs naming is to represent the data flow towards the wire.
I'm fine with the parent/children naming, but IIRC Jakub was not happy with it. Is there any intermediate ground that could satisfy both of you?

It's a tree, so perhaps just stick with tree terminology, everyone is used to that. Makes sense? One way or another, this needs to be properly described in docs, all terminology. That would make things more clear, I believe.

Quoted 4 lines

It's a tree, so perhaps just stick with tree terminology, everyone is used to that. Makes sense? One way or another, this needs to be properly described in docs, all terminology. That would make things more clear, I believe.

@Jakub, would you be ok with:

'inputs' -> 'leaves'
'output' -> 'node'
?

Also while at it, I think renaming the 'group()' operation as 'node_set()' could be clearer (or at least less unclear), WDYT?

Note: I think it's would be more user-friendly to keep a single delete/get/dump operation for 'nodes' and leaves.

Quoted 5 lines · On Mon, 12 Aug 2024 16:58:33 +0200 Paolo Abeni wrote:

Quoted 4 lines

It's a tree, so perhaps just stick with tree terminology, everyone is used to that. Makes sense? One way or another, this needs to be properly described in docs, all terminology. That would make things more clear, I believe.

Quoted 5 lines

@Jakub, would you be ok with:

'inputs' -> 'leaves'
'output' -> 'node'
?

I think the confusion is primarily about the parent / child. input and output should be very clear, IMO.

Quoted 3 lines

Also while at it, I think renaming the 'group()' operation as 'node_set()' could be clearer (or at least less unclear), WDYT?

No idea how we arrived at node_set(), and how it can possibly

represent a grouping operation.
The operations is grouping inputs and creating a scheduler node.

Quoted 2 lines

Note: I think it's would be more user-friendly to keep a single delete/get/dump operation for 'nodes' and leaves.

Are you implying that nodes and leaves are different types of objects? Aren't leaves nodes without any inputs?

Quoted 2 lines · Mon, Aug 12, 2024 at 05:25:44PM CEST, [email protected] wrote:

I think the confusion is primarily about the parent / child. input and output should be very clear, IMO.

For me, "inputs" and "output" in this context sounds very odd. It should be children and parent, isn't it. Confused...

Quoted 4 lines

Quoted 2 lines

Also while at it, I think renaming the 'group()' operation as 'node_set()' could be clearer (or at least less unclear), WDYT?

No idea how we arrived at node_set(), and how it can possibly

subtree_set() ?

Quoted 8 lines

represent a grouping operation.
The operations is grouping inputs and creating a scheduler node.

Quoted 2 lines

Note: I think it's would be more user-friendly to keep a single delete/get/dump operation for 'nodes' and leaves.

Are you implying that nodes and leaves are different types of objects? Aren't leaves nodes without any inputs?

Agree. Same op would be nice for both.

Quoted 6 lines · On Mon, 12 Aug 2024 18:50:06 +0200 Jiri Pirko wrote:

Quoted 2 lines · Mon, Aug 12, 2024 at 05:25:44PM CEST, [email protected] wrote:

I think the confusion is primarily about the parent / child. input and output should be very clear, IMO.

For me, "inputs" and "output" in this context sounds very odd. It should be children and parent, isn't it. Confused...

Parent / child is completely confusing. Let's not.

User will classify traffic based on 'leaf' attributes. Therefore in my mind traffic enters the tree at the "leaves", and travels towards the root (whether or not that's how HW evaluates the hierarchy).

This is opposite to how trees as an data structure are normally traversed. Hence I find the tree analogy to be imperfect.

But yes, root and leaf are definitely better than parent / child.

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

Also while at it, I think renaming the 'group()' operation as 'node_set()' could be clearer (or at least less unclear), WDYT?

No idea how we arrived at node_set(), and how it can possibly

subtree_set() ?

The operation is grouping inputs and creating a scheduler node.

Quoted 11 lines

Quoted 9 lines

represent a grouping operation.
The operations is grouping inputs and creating a scheduler node.

Quoted 2 lines

Note: I think it's would be more user-friendly to keep a single delete/get/dump operation for 'nodes' and leaves.

Are you implying that nodes and leaves are different types of objects? Aren't leaves nodes without any inputs?

Agree. Same op would be nice for both.

Quoted 9 lines

Parent / child is completely confusing. Let's not.

User will classify traffic based on 'leaf' attributes. Therefore in my mind traffic enters the tree at the "leaves", and travels towards the root (whether or not that's how HW evaluates the hierarchy).

This is opposite to how trees as an data structure are normally traversed. Hence I find the tree analogy to be imperfect.

Normally?

Tree as a datastructure could be traversed freely, why it can't?

In this case, it is traversed from leaf to root. It's still a tree. Why the tree analogy is imperfect. From what I see, it fits 100%.

Quoted 1 line

But yes, root and leaf are definitely better than parent / child.

Node has 0-n children and 0-1 parents. In case it has 0 children, it's a leaf, in case it has 0 parents, it's a root.
This is the common tree terminology, isn't it?

Quoted 3 lines

Quoted 1 line

subtree_set() ?

The operation is grouping inputs and creating a scheduler node.

Creating a node inside a tree, isn't it? Therefore subtree.

But it could be unified to node_set() as Paolo suggested. That would work for any node, including leaf, tree, non-existent internal node.

To me using input / output is more intuitive, as it matches direction of traffic flow. I'm fine with root / leaf tho, as I said.

Quoted 4 lines

Quoted 3 lines

Quoted 1 line

subtree_set() ?

The operation is grouping inputs and creating a scheduler node.

Quoted 1 line

Creating a node inside a tree, isn't it? Therefore subtree.

All nodes are inside the tree.

Quoted 2 lines

But it could be unified to node_set() as Paolo suggested. That would work for any node, including leaf, tree, non-existent internal node.

A "set" operation which creates a node.

Quoted 7 lines

Quoted 1 line

Creating a node inside a tree, isn't it? Therefore subtree.

All nodes are inside the tree.

Quoted 2 lines

But it could be unified to node_set() as Paolo suggested. That would work for any node, including leaf, tree, non-existent internal node.

Quoted 1 line

A "set" operation which creates a node.

Here the outcome is unclear to me. My understanding is that group() does not fit Jiri nor Donald and and node_set() or subtree_set() do not fit Jakub.

Did I misread something? As a trade-off, what about, group_set()?

Quoted 13 lines · On Tue, 13 Aug 2024 16:47:34 +0200 Paolo Abeni wrote:

Quoted 8 lines

Quoted 1 line

Creating a node inside a tree, isn't it? Therefore subtree.

All nodes are inside the tree.

Quoted 2 lines

But it could be unified to node_set() as Paolo suggested. That would work for any node, including leaf, tree, non-existent internal node.

A "set" operation which creates a node.

Here the outcome is unclear to me. My understanding is that group() does not fit Jiri nor Donald and and node_set() or subtree_set() do not fit Jakub.

Quoted 1 line

Did I misread something? As a trade-off, what about, group_set()?

"set" is not a sensible verb for creating something. "group" in the original was the verb.
Why are both saying "set" and not "create"? What am I missing?

Quoted 3 lines

"set" is not a sensible verb for creating something. "group" in the original was the verb.
Why are both saying "set" and not "create"? What am I missing?

Please, don't read too much in my limited English skills! I'm fine with group_create() - or create_group()

Still WRT naming, I almost forgot about the much blamed 'detached' scope. Would 'node' or 'group' be a better name? (the latter only if we rename the homonymous operation)

Quoted 2 lines · Jakub Kicinski

To me using input / output is more intuitive, as it matches direction of traffic flow. I'm fine with root / leaf tho, as I said.

Can we converge on root / leaf ?

Quoted 5 lines

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

subtree_set() ?

The operation is grouping inputs and creating a scheduler node.

Creating a node inside a tree, isn't it? Therefore subtree.

Quoted 5 lines

All nodes are inside the tree.

Quoted 2 lines

But it could be unified to node_set() as Paolo suggested. That would work for any node, including leaf, tree, non-existent internal node.

A "set" operation which creates a node.

Here the outcome is unclear to me. My understanding is that group() does not fit Jiri nor Donald and and node_set() or subtree_set() do not fit Jakub.

Quoted 5 lines · On Mon, 12 Aug 2024 16:58:33 +0200 Paolo Abeni wrote:

Quoted 4 lines

It's a tree, so perhaps just stick with tree terminology, everyone is used to that. Makes sense? One way or another, this needs to be properly described in docs, all terminology. That would make things more clear, I believe.

Quoted 5 lines

@Jakub, would you be ok with:

'inputs' -> 'leaves'
'output' -> 'node'
?

I think the confusion is primarily about the parent / child. input and output should be very clear, IMO.

Quoted 12 lines · Jakub Kicinski

Quoted 10 lines · On Mon, 12 Aug 2024 16:58:33 +0200 Paolo Abeni wrote:

Quoted 4 lines

It's a tree, so perhaps just stick with tree terminology, everyone is used to that. Makes sense? One way or another, this needs to be properly described in docs, all terminology. That would make things more clear, I believe.

@Jakub, would you be ok with:

'inputs' -> 'leaves'
'output' -> 'node'
?

Quoted 2 lines

I think the confusion is primarily about th parent / child. input and output should be very clear, IMO.

input / output seems the most intuitive of the different terms that have been suggested.

Quoted 14 lines · Donald Hunter

Quoted 12 lines · Jakub Kicinski

Quoted 10 lines · On Mon, 12 Aug 2024 16:58:33 +0200 Paolo Abeni wrote:

Quoted 4 lines

It's a tree, so perhaps just stick with tree terminology, everyone is used to that. Makes sense? One way or another, this needs to be properly described in docs, all terminology. That would make things more clear, I believe.

@Jakub, would you be ok with:

'inputs' -> 'leaves'
'output' -> 'node'
?

Quoted 4 lines

Quoted 2 lines

I think the confusion is primarily about th parent / child. input and output should be very clear, IMO.

input / output seems the most intuitive of the different terms that have been suggested.

Since a sort of agreement was reached about using root / leaf instead, and (quoting someone smarter then me) a good compromise makes every party equally unhappy, would you consider such other option?

Documentation/netlink/specs/shaper.yaml new file +64 -0

+        doc: |
+           Describes a set of inputs shapers for a @group operation
+      -
+        name: output
+        type: nest
+        nested-attributes: ns-output-info
+        doc: |
+           Describes the output shaper for a @group operation
+           Differently from @inputs and @shaper allow specifying
+           the shaper parent handle, too.
+
+      -
+        name: shaper
+        type: nest
+        nested-attributes: ns-info
+        doc: |
+           Describes a single shaper for a @set operation
+  -
+    name: handle
+    subset-of: net-shaper
+    attributes:
+      -
+        name: scope
+      -
+        name: id
+  -
+    name: ns-info
+    subset-of: net-shaper
+    attributes:
+      -
+        name: handle
+      -
+        name: metric
+      -
+        name: bw-min
+      -
+        name: bw-max
+      -
+        name: burst
+      -
+        name: priority
+      -
+        name: weight
+  -
+    name: ns-output-info
+    subset-of: net-shaper
+    attributes:
+      -
+        name: parent
+      -
+        name: handle
+      -
+        name: metric
+      -
+        name: bw-min
+      -
+        name: bw-max
+      -
+        name: burst
+      -
+        name: priority
+      -
+        name: weight
+

net/shaper/shaper_nl_gen.c new file +9 -0

+const struct nla_policy net_shaper_ns_info_nl_policy[NET_SHAPER_A_WEIGHT + 1] = {
+	[NET_SHAPER_A_HANDLE] = NLA_POLICY_NESTED(net_shaper_handle_nl_policy),
+	[NET_SHAPER_A_METRIC] = NLA_POLICY_MAX(NLA_U32, 1),
+	[NET_SHAPER_A_BW_MIN] = { .type = NLA_UINT, },
+	[NET_SHAPER_A_BW_MAX] = { .type = NLA_UINT, },
+	[NET_SHAPER_A_BURST] = { .type = NLA_UINT, },
+	[NET_SHAPER_A_PRIORITY] = { .type = NLA_U32, },
+	[NET_SHAPER_A_WEIGHT] = { .type = NLA_U32, },
+};

+ name: handle

Introduce the basic infrastructure to implement the net-shaper core functionality. Each network devices carries a net-shaper cache, the NL get() operation fetches the data from such cache.

The cache is initially empty, will be fill by the set()/group() operation implemented later and is destroyed at device cleanup time.

Signed-off-by Paolo Abeni <[email protected]>

---
RFC v2 -> RFC v3:
- dev_put() -> netdev_put()
---

Documentation/networking/kapi.rst |   3 +
include/linux/netdevice.h         |  17 ++
include/net/net_shaper.h          | 158 +++++++++++++++++++
net/core/dev.c                    |   2 +
net/core/dev.h                    |   6 +
net/shaper/shaper.c               | 251 +++++++++++++++++++++++++++++-
6 files changed, 435 insertions(+), 2 deletions(-)

create mode 100644 include/net/net_shaper.h

Documentation/networking/kapi.rst +0 -0

index ea55f462cefa..98682b9a13ee 100644
@@ -104,6 +104,9 @@ Driver Support

.. kernel-doc:: include/linux/netdevice.h
:internal:

+.. kernel-doc:: include/net/net_shaper.h

Documentation/networking/kapi.rst +2 -0

+   :internal:
+

PHY Support
-----------

include/linux/netdevice.h +17 -0

index 607009150b5f..d3d952be711c 100644
@@ -81,6 +81,8 @@ struct xdp_frame;
 struct xdp_metadata_ops;
 struct xdp_md;
 struct ethtool_netdev_state;
+struct net_shaper_ops;
+struct net_shaper_data;
 
 typedef u32 xdp_features_t;
 

@@ -1598,6 +1600,14 @@ struct net_device_ops {
 	int			(*ndo_hwtstamp_set)(struct net_device *dev,
 						    struct kernel_hwtstamp_config *kernel_config,
 						    struct netlink_ext_ack *extack);
+
+#if IS_ENABLED(CONFIG_NET_SHAPER)
+	/**
+	 * @net_shaper_ops: Device shaping offload operations
+	 * see include/net/net_shapers.h
+	 */
+	const struct net_shaper_ops *net_shaper_ops;
+#endif
 };
 
 /**

@@ -2408,6 +2418,13 @@ struct net_device {
 	/** @irq_moder: dim parameters used if IS_ENABLED(CONFIG_DIMLIB). */
 	struct dim_irq_moder	*irq_moder;
 
+#if IS_ENABLED(CONFIG_NET_SHAPER)
+	/**
+	 * @net_shaper_data: data tracking the current shaper status
+	 *  see include/net/net_shapers.h
+	 */
+	struct net_shaper_data *net_shaper_data;
+#endif
 	u8			priv[] ____cacheline_aligned
 				       __counted_by(priv_len);
 } ____cacheline_aligned;

include/net/net_shaper.h new file +122 -0

new file mode 100644
index 000000000000..8cd65d727e52
@@ -0,0 +1,158 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#ifndef _NET_SHAPER_H_
+#define _NET_SHAPER_H_
+
+#include <linux/types.h>
+#include <linux/bits.h>
+#include <linux/bitfield.h>
+#include <linux/netdevice.h>
+#include <linux/netlink.h>
+
+#include <uapi/linux/net_shaper.h>
+
+/**
+ * struct net_shaper_info - represents a shaping node on the NIC H/W
+ * zeroed field are considered not set.
+ * @handle: Unique identifier for the shaper, see @net_shaper_make_handle
+ * @parent: Unique identifier for the shaper parent, usually implied. Only
+ *   NET_SHAPER_SCOPE_QUEUE, NET_SHAPER_SCOPE_NETDEV and NET_SHAPER_SCOPE_DETACHED
+ *   can have the parent handle explicitly set, placing such shaper under
+ *   the specified parent.
+ * @metric: Specify if the bw limits refers to PPS or BPS
+ * @bw_min: Minimum guaranteed rate for this shaper
+ * @bw_max: Maximum peak bw allowed for this shaper
+ * @burst: Maximum burst for the peek rate of this shaper
+ * @priority: Scheduling priority for this shaper
+ * @weight: Scheduling weight for this shaper
+ * @children: Number of nested shapers, accounted only for DETACHED scope
+ */
+struct net_shaper_info {
+	u32 handle;
+	u32 parent;
+	enum net_shaper_metric metric;
+	u64 bw_min;
+	u64 bw_max;
+	u64 burst;
+	u32 priority;
+	u32 weight;
+	u32 children;
+};
+
+/**
+ * define NET_SHAPER_SCOPE_VF - Shaper scope
+ *
+ * This shaper scope is not exposed to user-space; the shaper is attached to
+ * the given virtual function.
+ */
+#define NET_SHAPER_SCOPE_VF __NET_SHAPER_SCOPE_MAX
+
+/**
+ * struct net_shaper_ops - Operations on device H/W shapers
+ *
+ * The initial shaping configuration ad device initialization is empty/
+ * a no-op/does not constraint the b/w in any way.
+ * The network core keeps track of the applied user-configuration in
+ * per device storage.
+ *
+ * Each shaper is uniquely identified within the device with an 'handle',
+ * dependent on the shaper scope and other data, see @shaper_make_handle()
+ */
+struct net_shaper_ops {
+	/**
+	 * @group: create the specified shapers group
+	 *
+	 * Nest the specified @inputs shapers under the given @output shaper
+	 * on the network device @dev. The @input shaper array size is specified
+	 * by @nr_input.
+	 * Create either the @inputs and the @output shaper as needed,
+	 * otherwise move them as needed. Can't create @inputs shapers with
+	 * NET_SHAPER_SCOPE_DETACHED scope, a separate @group call with such
+	 * shaper as @output is needed.
+	 *
+	 * Returns 0 on group successfully created, otherwise an negative
+	 * error value and set @extack to describe the failure's reason.
+	 */
+	int (*group)(struct net_device *dev, int nr_input,
+		     const struct net_shaper_info *inputs,
+		     const struct net_shaper_info *output,
+		     struct netlink_ext_ack *extack);
+
+	/**
+	 * @set: Updates the specified shaper
+	 *
+	 * Updates or creates the specified @shaper on the given device
+	 * @dev. Can't create NET_SHAPER_SCOPE_DETACHED shapers, use @group
+	 * instead.
+	 *
+	 * Returns 0 on group successfully created, otherwise an negative
+	 * error value and set @extack to describe the failure's reason.
+	 */
+	int (*set)(struct net_device *dev,
+		   const struct net_shaper_info *shaper,
+		   struct netlink_ext_ack *extack);
+
+	/**
+	 * @delete: Removes the specified shaper from the NIC
+	 *
+	 * Removes the shaper configuration as identified by the given @handle
+	 * on the specified device @dev, restoring the default behavior.
+	 *
+	 * Returns 0 on group successfully created, otherwise an negative
+	 * error value and set @extack to describe the failure's reason.
+	 */
+	int (*delete)(struct net_device *dev, u32 handle,
+		      struct netlink_ext_ack *extack);
+};
+
+#define NET_SHAPER_SCOPE_SHIFT	26
+#define NET_SHAPER_ID_MASK	GENMASK(NET_SHAPER_SCOPE_SHIFT - 1, 0)
+#define NET_SHAPER_SCOPE_MASK	GENMASK(31, NET_SHAPER_SCOPE_SHIFT)
+
+#define NET_SHAPER_ID_UNSPEC NET_SHAPER_ID_MASK
+
+/**
+ * net_shaper_make_handle - creates an unique shaper identifier
+ * @scope: the shaper scope
+ * @id: the shaper id number
+ *
+ * Return: an unique identifier for the shaper
+ *
+ * Combines the specified arguments to create an unique identifier for
+ * the shaper. The @id argument semantic depends on the

include/net/net_shaper.h new file +26 -0

+}
+
+/**
+ * net_shaper_handle_scope - extract the scope from the given handle
+ * @handle: the shaper handle
+ *
+ * Return: the corresponding scope
+ */
+static inline enum net_shaper_scope net_shaper_handle_scope(u32 handle)
+{
+	return FIELD_GET(NET_SHAPER_SCOPE_MASK, handle);
+}
+
+/**
+ * net_shaper_handle_id - extract the id number from the given handle
+ * @handle: the shaper handle
+ *
+ * Return: the corresponding id number
+ */
+static inline int net_shaper_handle_id(u32 handle)
+{
+	return FIELD_GET(NET_SHAPER_ID_MASK, handle);
+}
+
+#endif
+

net/core/dev.c +2 -0

index 6ea1d20676fb..3dc1dd428eda 100644
@@ -11169,6 +11169,8 @@ void free_netdev(struct net_device *dev)
 	/* Flush device addresses */
 	dev_addr_flush(dev);
 
+	dev_shaper_flush(dev);
+
 	list_for_each_entry_safe(p, n, &dev->napi_list, dev_list)
 		netif_napi_del(p);
 

net/core/dev.h +6 -0

index 5654325c5b71..e376fc1c867b 100644
@@ -35,6 +35,12 @@ void dev_addr_flush(struct net_device *dev);
 int dev_addr_init(struct net_device *dev);
 void dev_addr_check(struct net_device *dev);
 
+#if IS_ENABLED(CONFIG_NET_SHAPER)
+void dev_shaper_flush(struct net_device *dev);
+#else
+static inline void dev_shaper_flush(struct net_device *dev) {}
+#endif
+
 /* sysctls not referred to from outside net/core/ */
 extern int		netdev_unregister_timeout_secs;
 extern int		weight_p;

net/shaper/shaper.c +1 -0

index 49de88c68e2f..5d1d6e600a6a 100644
@@ -1,19 +1,242 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 
 #include <linux/kernel.h>
+#include <linux/idr.h>
 #include <linux/skbuff.h>

net/shaper/shaper.c +141 -0

+};
+
+struct net_shaper_nl_ctx {
+	u32 start_handle;
+};
+
+static int fill_handle(struct sk_buff *msg, u32 handle, u32 type,
+		       const struct genl_info *info)
+{
+	struct nlattr *handle_attr;
+
+	if (!handle)
+		return 0;
+
+	handle_attr = nla_nest_start_noflag(msg, type);
+	if (!handle_attr)
+		return -EMSGSIZE;
+
+	if (nla_put_u32(msg, NET_SHAPER_A_SCOPE,
+			net_shaper_handle_scope(handle)) ||
+	    nla_put_u32(msg, NET_SHAPER_A_ID,
+			net_shaper_handle_id(handle)))
+		goto handle_nest_cancel;
+
+	nla_nest_end(msg, handle_attr);
+	return 0;
+
+handle_nest_cancel:
+	nla_nest_cancel(msg, handle_attr);
+	return -EMSGSIZE;
+}
+
+static int
+net_shaper_fill_one(struct sk_buff *msg, struct net_shaper_info *shaper,
+		    const struct genl_info *info)
+{
+	void *hdr;
+
+	hdr = genlmsg_iput(msg, info);
+	if (!hdr)
+		return -EMSGSIZE;
+
+	if (fill_handle(msg, shaper->parent, NET_SHAPER_A_PARENT, info) ||
+	    fill_handle(msg, shaper->handle, NET_SHAPER_A_HANDLE, info) ||
+	    nla_put_u32(msg, NET_SHAPER_A_METRIC, shaper->metric) ||
+	    nla_put_uint(msg, NET_SHAPER_A_BW_MIN, shaper->bw_min) ||
+	    nla_put_uint(msg, NET_SHAPER_A_BW_MAX, shaper->bw_max) ||
+	    nla_put_uint(msg, NET_SHAPER_A_BURST, shaper->burst) ||
+	    nla_put_u32(msg, NET_SHAPER_A_PRIORITY, shaper->priority) ||
+	    nla_put_u32(msg, NET_SHAPER_A_WEIGHT, shaper->weight))
+		goto nla_put_failure;
+
+	genlmsg_end(msg, hdr);
+
+	return 0;
+
+nla_put_failure:
+	genlmsg_cancel(msg, hdr);
+	return -EMSGSIZE;
+}
+
+/* On success sets pdev to the relevant device and acquires a reference
+ * to it
+ */
+static int fetch_dev(const struct genl_info *info, struct net_device **pdev)
+{
+	struct net *ns = genl_info_net(info);
+	struct net_device *dev;
+	int ifindex;
+
+	if (GENL_REQ_ATTR_CHECK(info, NET_SHAPER_A_IFINDEX))
+		return -EINVAL;
+
+	ifindex = nla_get_u32(info->attrs[NET_SHAPER_A_IFINDEX]);
+	dev = dev_get_by_index(ns, ifindex);
+	if (!dev) {
+		GENL_SET_ERR_MSG_FMT(info, "device %d not found", ifindex);
+		return -EINVAL;
+	}
+
+	if (!dev->netdev_ops->net_shaper_ops) {
+		GENL_SET_ERR_MSG_FMT(info, "device %s does not support H/W shaper",
+				     dev->name);
+		netdev_put(dev, NULL);
+		return -EOPNOTSUPP;
+	}
+
+	*pdev = dev;
+	return 0;
+}
+
+static struct xarray *__sc_container(struct net_device *dev)
+{
+	return dev->net_shaper_data ? &dev->net_shaper_data->shapers : NULL;
+}
+
+/* lookup the given shaper inside the cache */
+static struct net_shaper_info *sc_lookup(struct net_device *dev, u32 handle)
+{
+	struct xarray *xa = __sc_container(dev);
+
+	return xa ? xa_load(xa, handle) : NULL;
+}
+
+static int parse_handle(const struct nlattr *attr, const struct genl_info *info,
+			u32 *handle)
+{
+	struct nlattr *tb[NET_SHAPER_A_ID + 1];
+	struct nlattr *scope_attr, *id_attr;
+	enum net_shaper_scope scope;
+	u32 id = 0;
+	int ret;
+
+	ret = nla_parse_nested(tb, NET_SHAPER_A_ID, attr,
+			       net_shaper_handle_nl_policy, info->extack);
+	if (ret < 0)
+		return ret;
+
+	scope_attr = tb[NET_SHAPER_A_SCOPE];
+	if (!scope_attr) {
+		GENL_SET_ERR_MSG(info, "Missing 'scope' attribute for handle");
+		return -EINVAL;
+	}
+
+	scope = nla_get_u32(scope_attr);
+
+	/* the default id for detached scope shapers is an invalid one
+	 * to help the 'group' operation discriminate between new
+	 * detached shaper creation (ID_UNSPEC) and reuse of existing
+	 * shaper (any other value)
+	 */
+	id_attr = tb[NET_SHAPER_A_ID];
+	if (id_attr)
+		id =  nla_get_u32(id_attr);
+	else if (scope == NET_SHAPER_SCOPE_DETACHED)
+		id = NET_SHAPER_ID_UNSPEC;
+
+	*handle = net_shaper_make_handle(scope, id);
+	return 0;
+}
+

net/shaper/shaper.c +84 -1

+	if (ret)
+		return ret;
+
+	if (GENL_REQ_ATTR_CHECK(info, NET_SHAPER_A_HANDLE))
+		goto put;
+
+	ret = parse_handle(info->attrs[NET_SHAPER_A_HANDLE], info, &handle);
+	if (ret < 0)
+		goto put;
+
+	shaper = sc_lookup(dev, handle);
+	if (!shaper) {
+		GENL_SET_ERR_MSG_FMT(info, "Can't find shaper for handle %x", handle);
+		ret = -EINVAL;
+		goto put;
+	}
+
+	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	if (!msg) {
+		ret = -ENOMEM;
+		goto put;
+	}
+
+	ret = net_shaper_fill_one(msg, shaper, info);
+	if (ret)
+		goto free_msg;
+
+	ret =  genlmsg_reply(msg, info);
+	if (ret)
+		goto free_msg;
+
+put:
+	netdev_put(dev, NULL);
+	return ret;
+
+free_msg:
+	nlmsg_free(msg);
+	goto put;
 }
 
 int net_shaper_nl_get_dumpit(struct sk_buff *skb,
 			     struct netlink_callback *cb)
 {
-	return -EOPNOTSUPP;
+	struct net_shaper_nl_ctx *ctx = (struct net_shaper_nl_ctx *)cb->ctx;
+	const struct genl_info *info = genl_info_dump(cb);
+	struct net_shaper_info *shaper;
+	struct net_device *dev;
+	unsigned long handle;
+	int ret;
+
+	ret = fetch_dev(info, &dev);
+	if (ret)
+		return ret;
+
+	BUILD_BUG_ON(sizeof(struct net_shaper_nl_ctx) > sizeof(cb->ctx));
+
+	/* don't error out dumps performed before any set operation */
+	if (!dev->net_shaper_data) {
+		ret = 0;
+		goto put;
+	}
+
+	xa_for_each_range(&dev->net_shaper_data->shapers, handle, shaper,
+			  ctx->start_handle, U32_MAX) {
+		ret = net_shaper_fill_one(skb, shaper, info);
+		if (ret)
+			goto put;
+
+		ctx->start_handle = handle;
+	}
+
+put:
+	netdev_put(dev, NULL);
+	return ret;
 }
 
 int net_shaper_nl_set_doit(struct sk_buff *skb, struct genl_info *info)

@@ -26,6 +249,30 @@ int net_shaper_nl_delete_doit(struct sk_buff *skb, struct genl_info *info)
 	return -EOPNOTSUPP;
 }
 
+int net_shaper_nl_group_doit(struct sk_buff *skb, struct genl_info *info)
+{
+	return -EOPNOTSUPP;
+}
+
+void dev_shaper_flush(struct net_device *dev)
+{
+	struct xarray *xa = __sc_container(dev);
+	struct net_shaper_info *cur;
+	unsigned long index;
+
+	if (!xa)
+		return;
+
+	xa_lock(xa);

net/shaper/shaper.c +0 -0

 {
 	return genl_register_family(&net_shaper_nl_family);

Signature · 1 line

2.45.2

Quoted 15 lines · Tue, Jul 30, 2024 at 10:39:46PM CEST, [email protected] wrote:

include/net/net_shaper.h new file +15 -0

+/**
+ * net_shaper_make_handle - creates an unique shaper identifier
+ * @scope: the shaper scope
+ * @id: the shaper id number
+ *
+ * Return: an unique identifier for the shaper
+ *
+ * Combines the specified arguments to create an unique identifier for
+ * the shaper. The @id argument semantic depends on the
+ * specified scope.
+ * For @NET_SHAPER_SCOPE_QUEUE_GROUP, @id is the queue group id
+ * For @NET_SHAPER_SCOPE_QUEUE, @id is the queue number.
+ * For @NET_SHAPER_SCOPE_VF, @id is virtual function number.
+ */
+static inline u32 net_shaper_make_handle(enum net_shaper_scope scope,

include/net/net_shaper.h line 132

Quoted 4 lines

include/net/net_shaper.h new file +4 -0

+					 int id)
+{
+	return FIELD_PREP(NET_SHAPER_SCOPE_MASK, scope) |
+		FIELD_PREP(NET_SHAPER_ID_MASK, id);

Perhaps some scopes may find only part of u32 as limitting for id in the future? I find it elegant to have it in single u32 though. u64 may be nicer (I know, xarray) :)

Quoted 6 lines

int net_shaper_nl_get_doit(struct sk_buff *skb, struct genl_info *info) {

net/shaper/shaper.c +3 -1

-	return -EOPNOTSUPP;
+	struct net_shaper_info *shaper;
+	struct net_device *dev;
+	struct sk_buff *msg;

net/shaper/shaper.c line 165

Quoted 4 lines

net/shaper/shaper.c +4 -0

+	u32 handle;
+	int ret;
+
+	ret = fetch_dev(info, &dev);

This is quite net_device centric. Devlink rate shaper should be eventually visible throught this api as well, won't they? How do you imagine that?

Could we have various types of binding? Something like:

NET_SHAPER_A_BINDING nest
NET_SHAPER_A_BINDING_IFINDEX u32

or:
NET_SHAPER_A_BINDING nest
NET_SHAPER_A_BINDING_DEVLINK_PORT nest
DEVLINK_ATTR_BUS_NAME string
DEVLINK_ATTR_DEV_NAME string
DEVLINK_ATTR_PORT_INDEX u32

?

Quoted 2 lines

+
static int __init shaper_init(void)

fetch_dev
fill_handle
parse_handle
sc_lookup
__sc_container
dev_shaper_flush
shaper_init

Could you perhaps maintain net_shaper_ prefix for all of there?

Quoted 16 lines · Jiri Pirko

Quoted 15 lines · Tue, Jul 30, 2024 at 10:39:46PM CEST, [email protected] wrote:

include/net/net_shaper.h new file +15 -0

+/**
+ * net_shaper_make_handle - creates an unique shaper identifier
+ * @scope: the shaper scope
+ * @id: the shaper id number
+ *
+ * Return: an unique identifier for the shaper
+ *
+ * Combines the specified arguments to create an unique identifier for
+ * the shaper. The @id argument semantic depends on the
+ * specified scope.
+ * For @NET_SHAPER_SCOPE_QUEUE_GROUP, @id is the queue group id
+ * For @NET_SHAPER_SCOPE_QUEUE, @id is the queue number.
+ * For @NET_SHAPER_SCOPE_VF, @id is virtual function number.
+ */
+static inline u32 net_shaper_make_handle(enum net_shaper_scope scope,

Quoted 8 lines

include/net/net_shaper.h line 132

Quoted 4 lines

include/net/net_shaper.h new file +4 -0

+					 int id)
+{
+	return FIELD_PREP(NET_SHAPER_SCOPE_MASK, scope) |
+		FIELD_PREP(NET_SHAPER_ID_MASK, id);

Perhaps some scopes may find only part of u32 as limitting for id in the future? I find it elegant to have it in single u32 though. u64 may be nicer (I know, xarray) :)

With this code the id limit is 2^26 for each scope. The most capable H/W I'm aware of supports at most 64K shapers, overall. Are you aware of any specific constraint we need to address?

[...]

Quoted 6 lines

Quoted 6 lines

int net_shaper_nl_get_doit(struct sk_buff *skb, struct genl_info *info) {

net/shaper/shaper.c +3 -1

-	return -EOPNOTSUPP;
+	struct net_shaper_info *shaper;
+	struct net_device *dev;
+	struct sk_buff *msg;

Quoted 8 lines

net/shaper/shaper.c line 165

Quoted 4 lines

net/shaper/shaper.c +4 -0

+	u32 handle;
+	int ret;
+
+	ret = fetch_dev(info, &dev);

This is quite net_device centric. Devlink rate shaper should be eventually visible throught this api as well, won't they? How do you imagine that?

I'm unsure we are on the same page. Do you foresee this to replace and obsoleted the existing devlink rate API? It was not our so far.

Quoted 13 lines

Could we have various types of binding? Something like:

NET_SHAPER_A_BINDING nest
NET_SHAPER_A_BINDING_IFINDEX u32

or:
NET_SHAPER_A_BINDING nest
NET_SHAPER_A_BINDING_DEVLINK_PORT nest
DEVLINK_ATTR_BUS_NAME string
DEVLINK_ATTR_DEV_NAME string
DEVLINK_ATTR_PORT_INDEX u32

Quoted 1 line

?

Somewhat related, the current get()/dump() operations currently don't return the shaper ifindex. I guess we can include 'scope' and 'id' under NET_SHAPER_A_BINDING and replace the existing handle attribute with it.

It should cover eventual future devlink extensions and provide all the relevant info for get/dump sake.

Quoted 15 lines

Quoted 3 lines

+
static int __init shaper_init(void)

fetch_dev
fill_handle
parse_handle
sc_lookup
__sc_container
dev_shaper_flush
shaper_init

Quoted 1 line

Could you perhaps maintain net_shaper_ prefix for all of there?

Most of the helpers are static and should never be visible outside this compilation unit, so I did not bother with a prefix, I'll add it in the next revision.

Thanks,

Paolo

Quoted 17 lines · Tue, Aug 13, 2024 at 05:17:12PM CEST, [email protected] wrote:

On 8/1/24 15:42, Jiri Pirko wrote:
[...]

Quoted 14 lines

net/shaper/shaper.c line 165

Quoted 10 lines

int net_shaper_nl_get_doit(struct sk_buff *skb, struct genl_info *info) {

net/shaper/shaper.c +7 -1

-	return -EOPNOTSUPP;
+	struct net_shaper_info *shaper;
+	struct net_device *dev;
+	struct sk_buff *msg;
+	u32 handle;
+	int ret;
+
+	ret = fetch_dev(info, &dev);

This is quite net_device centric. Devlink rate shaper should be eventually visible throught this api as well, won't they? How do you imagine that?

Quoted 2 lines

I'm unsure we are on the same page. Do you foresee this to replace and obsoleted the existing devlink rate API? It was not our so far.

Driver-api-wise, yes. I believe that was the goal, to have drivers to implement one rate api.

Quoted 18 lines · Jiri Pirko

Quoted 17 lines · Tue, Aug 13, 2024 at 05:17:12PM CEST, [email protected] wrote:

On 8/1/24 15:42, Jiri Pirko wrote:
[...]

Quoted 14 lines

net/shaper/shaper.c line 165

Quoted 10 lines

int net_shaper_nl_get_doit(struct sk_buff *skb, struct genl_info *info) {

net/shaper/shaper.c +7 -1

-	return -EOPNOTSUPP;
+	struct net_shaper_info *shaper;
+	struct net_device *dev;
+	struct sk_buff *msg;
+	u32 handle;
+	int ret;
+
+	ret = fetch_dev(info, &dev);

This is quite net_device centric. Devlink rate shaper should be eventually visible throught this api as well, won't they? How do you imagine that?

Quoted 5 lines

Quoted 2 lines

I'm unsure we are on the same page. Do you foresee this to replace and obsoleted the existing devlink rate API? It was not our so far.

Driver-api-wise, yes. I believe that was the goal, to have drivers to implement one rate api.

I initially underlooked at this point, I'm sorry.

Re-reading this I think we are not on the same page.

The net_shaper_ops are per network device operations: they are aimed (also) at consolidating network device shaping related callbacks, but they can't operate on non-network device objects (devlink port).

Cheers,

Paolo

Quoted 27 lines · Fri, Aug 16, 2024 at 10:52:58AM CEST, [email protected] wrote:

Quoted 18 lines · Jiri Pirko

Quoted 17 lines · Tue, Aug 13, 2024 at 05:17:12PM CEST, [email protected] wrote:

On 8/1/24 15:42, Jiri Pirko wrote:
[...]

Quoted 14 lines

net/shaper/shaper.c line 165

Quoted 10 lines

int net_shaper_nl_get_doit(struct sk_buff *skb, struct genl_info *info) {

net/shaper/shaper.c +7 -1

-	return -EOPNOTSUPP;
+	struct net_shaper_info *shaper;
+	struct net_device *dev;
+	struct sk_buff *msg;
+	u32 handle;
+	int ret;
+
+	ret = fetch_dev(info, &dev);

This is quite net_device centric. Devlink rate shaper should be eventually visible throught this api as well, won't they? How do you imagine that?

Quoted 5 lines

Quoted 2 lines

I'm unsure we are on the same page. Do you foresee this to replace and obsoleted the existing devlink rate API? It was not our so far.

Driver-api-wise, yes. I believe that was the goal, to have drivers to implement one rate api.

I initially underlooked at this point, I'm sorry.

Quoted 5 lines

Re-reading this I think we are not on the same page.

The net_shaper_ops are per network device operations: they are aimed (also) at consolidating network device shaping related callbacks, but they can't operate on non-network device objects (devlink port).

Why not?

Quoted 30 lines · Jiri Pirko

Quoted 29 lines · Fri, Aug 16, 2024 at 10:52:58AM CEST, [email protected] wrote:

Quoted 18 lines · Jiri Pirko

Quoted 17 lines · Tue, Aug 13, 2024 at 05:17:12PM CEST, [email protected] wrote:

On 8/1/24 15:42, Jiri Pirko wrote:
[...]

Quoted 14 lines

Review

Quoted 10 lines

int net_shaper_nl_get_doit(struct sk_buff *skb, struct genl_info *info) {

(quoted diff) +7 -1

-	return -EOPNOTSUPP;
+	struct net_shaper_info *shaper;
+	struct net_device *dev;
+	struct sk_buff *msg;
+	u32 handle;
+	int ret;
+
+	ret = fetch_dev(info, &dev);

This is quite net_device centric. Devlink rate shaper should be eventually visible throught this api as well, won't they? How do you imagine that?

Quoted 5 lines

Quoted 2 lines

I'm unsure we are on the same page. Do you foresee this to replace and obsoleted the existing devlink rate API? It was not our so far.

Driver-api-wise, yes. I believe that was the goal, to have drivers to implement one rate api.

I initially underlooked at this point, I'm sorry.

Re-reading this I think we are not on the same page.

Quoted 5 lines

Quoted 3 lines

The net_shaper_ops are per network device operations: they are aimed (also) at consolidating network device shaping related callbacks, but they can't operate on non-network device objects (devlink port).

Why not?

Isn't the whole point of devlink to configure objects that are directly related to any network device? Would be somewhat awkward accessing devlink

port going through some net_device?

Side note: I experimented adding the 'binging' abstraction to this API and gives a quite significant uglification to the user syntax (due to the additional nesting required) and the code.

Still, if there is a very strong need for controlling devlink rate via this API _and_ we can assume that each net_device "relates" (/references/is connected to) at most a single devlink object (out of sheer ignorance on my side I'm unsure about this point, but skimming over the existing implementations it looks so), the current API definition would be IMHO sufficient and clean enough to reach for both devlink port rate objects and devlink node rate objects.

We could define additional scopes for each of such objects and use the id to discriminate the specific port or node within the relevant devlink.

I think such scopes definition should come with related implementation, e.g. not with this series.

Thanks,

Paolo

Quoted 3 lines · Mon, Aug 19, 2024 at 11:33:28AM CEST, [email protected] wrote:

Isn't the whole point of devlink to configure objects that are directly related to any network device? Would be somewhat awkward accessing devlink port going through some net_device?

I'm not sure why you are asking that. I didn't suggest anything like that. On contrary, as your API is netdev centric, I suggested to disconnect from netdev to the shapers could be used not only with them.

This is what I understood was a plan from very beginning.

Quoted 2 lines

We could define additional scopes for each of such objects and use the id to discriminate the specific port or node within the relevant devlink.

But you still want to use some netdevice as a handle IIUC, is that right?

Quoted 5 lines · Jiri Pirko

Quoted 3 lines · Mon, Aug 19, 2024 at 11:33:28AM CEST, [email protected] wrote:

Isn't the whole point of devlink to configure objects that are directly related to any network device? Would be somewhat awkward accessing devlink port going through some net_device?

Quoted 3 lines

I'm not sure why you are asking that. I didn't suggest anything like that. On contrary, as your API is netdev centric, I suggested to disconnect from netdev to the shapers could be used not only with them.

ndo_shaper_ops are basically net_device ndo. Any implementation of them will operate 'trough some net_device'.

I'm still not sure which one of the following you mean:

1) the shaper NL interface must be able to manage devlink (rate) objects. The core will lookup the devlink obj and use devlink_ops to do the change.

2) the shaper NL interface must be able to manage devlink (rate) objects, the core will use ndo_shaper_ops to do the actual change.

3) something else?

In my previous reply, I assumed you wanted option 2). If so, which kind of object should implement the ndo_shaper_ops callbacks? net_device? devlink? other?

Quoted 2 lines

This is what I understood was a plan from very beginning.

Originally the scope was much more limited than what defined here. Jakub asked to implement an interface capable to unify the network device shaping/rate related callbacks.

In a previous revision, I stretched that to cover objects "above" the network device level (e.g. PF/VF/SFs groups), but then I left them out because:

- it caused several inconsistencies (among other thing we can't use the 'shaper cache' there and Jakub wants the cache in place).
- we already have devlink for that.

Quoted 5 lines

Quoted 2 lines

We could define additional scopes for each of such objects and use the id to discriminate the specific port or node within the relevant devlink.

But you still want to use some netdevice as a handle IIUC, is that right?

The revision of the series that I hope to share soon still used net_device ndos. Shapers are identified within the network device by an handle.

Cheers,

Paolo

Quoted 5 lines

Quoted 1 line

This is what I understood was a plan from very beginning.

Originally the scope was much more limited than what defined here. Jakub asked to implement an interface capable to unify the network device shaping/rate related callbacks.

I'm not saying this is deal breaker for me. I just think that if the api is designed to be independent of the object shaper is bound to (netdev/devlink_port/etc), it would be much much easier to extend in the future. If you do everything netdev-centric from start, I'm sure no shaper consolidation will ever happen. And that I thought was one of the goals.

Perhaps Jakub has opinion.

Quoted 6 lines · On Thu, 22 Aug 2024 14:02:54 +0200 Jiri Pirko wrote:

Quoted 5 lines

Quoted 1 line

This is what I understood was a plan from very beginning.

Originally the scope was much more limited than what defined here. Jakub asked to implement an interface capable to unify the network device shaping/rate related callbacks.

Quoted 8 lines

I'm not saying this is deal breaker for me. I just think that if the api is designed to be independent of the object shaper is bound to (netdev/devlink_port/etc), it would be much much easier to extend in the future. If you do everything netdev-centric from start, I'm sure no shaper consolidation will ever happen. And that I thought was one of the goals.

Perhaps Jakub has opinion.

I think you and I are on the same page :) Other than the "reference object" (netdev / devlink port) the driver facing API should be identical. Making it possible for the same driver code to handle translating the parameters into HW config / FW requests, whether they shape at the device (devlink) or port (netdev) level.

Shaper NL for netdevs is separate from internal representation and driver API in my mind. My initial ask was to create the internal representation first, make sure it can express devlink and handful of exiting netdev APIs, and only once that's merged worry about exposing it via a new NL.

I'm not opposed to showing devlink shapers in netdev NL (RO as you say) but talking about it now strikes me as cart before the horse.

Quoted 15 lines

Quoted 8 lines

I'm not saying this is deal breaker for me. I just think that if the api is designed to be independent of the object shaper is bound to (netdev/devlink_port/etc), it would be much much easier to extend in the future. If you do everything netdev-centric from start, I'm sure no shaper consolidation will ever happen. And that I thought was one of the goals.

Perhaps Jakub has opinion.

I think you and I are on the same page :) Other than the "reference object" (netdev / devlink port) the driver facing API should be identical. Making it possible for the same driver code to handle translating the parameters into HW config / FW requests, whether they shape at the device (devlink) or port (netdev) level.

Quoted 8 lines

Shaper NL for netdevs is separate from internal representation and driver API in my mind. My initial ask was to create the internal representation first, make sure it can express devlink and handful of exiting netdev APIs, and only once that's merged worry about exposing it via a new NL.

I'm not opposed to showing devlink shapers in netdev NL (RO as you say) but talking about it now strikes me as cart before the horse.

FTR, I don't see both of you on the same page ?!?

I read the above as Jiri's preference is a single ndo set to control both devlink and device shapers, while I read Jakub's preference as for different sets of operations that will use the same arguments to specify the shaper informations.

which kind of object should implement the ndo_shaper_ops callbacks?

Or to phrase the above differently, Jiri is focusing on the shaper "binding" (how to locate/access it) while Jakub is focusing on the shaper "info" (content/definition/attributes). Please correct me If I misread something.

Still for the record, I interpret the current proposal as not clashing with Jakub's preference, and being tolerated from Jiri, again please correct me if I read too far.

Quoted 21 lines · On Thu, 22 Aug 2024 22:30:35 +0200 Paolo Abeni wrote:

Quoted 21 lines

Quoted 8 lines

I'm not saying this is deal breaker for me. I just think that if the api is designed to be independent of the object shaper is bound to (netdev/devlink_port/etc), it would be much much easier to extend in the future. If you do everything netdev-centric from start, I'm sure no shaper consolidation will ever happen. And that I thought was one of the goals.

Perhaps Jakub has opinion.

I think you and I are on the same page :) Other than the "reference object" (netdev / devlink port) the driver facing API should be identical. Making it possible for the same driver code to handle translating the parameters into HW config / FW requests, whether they shape at the device (devlink) or port (netdev) level.

Shaper NL for netdevs is separate from internal representation and driver API in my mind. My initial ask was to create the internal representation first, make sure it can express devlink and handful of exiting netdev APIs, and only once that's merged worry about exposing it via a new NL.

Quoted 6 lines

Quoted 2 lines

I'm not opposed to showing devlink shapers in netdev NL (RO as you say) but talking about it now strikes me as cart before the horse.

FTR, I don't see both of you on the same page ?!?

I read the above as Jiri's preference is a single ndo set to control

Quoted 3 lines

both devlink and device shapers, while I read Jakub's preference as for different sets of operations that will use the same arguments to specify the shaper informations.

Jiri replied:

Quoted 1 line

which kind of object should implement the ndo_shaper_ops callbacks?

Whoever implements the shaper in driver. If that is net_device tight shaper, driver should work with net_device. If that is devlink port related shaper, driver should work on top of devlink port based api.

I interpret this as having two almost identical versions of shaper ops, the only difference is that one takes netdev and the other devlink port.

We could simplify it slightly, and call the ndo for getting devlink port from netdev, and always pass devlink port in?

I _think_ (but I'm not 100% sure) that Jiri does _not_ mean that we would be able to render the internal shaper tree as ops for the existing devlink rate API. Because that may cause scope creep, inconsistencies and duplication.

Quoted 5 lines

Or to phrase the above differently, Jiri is focusing on the shaper "binding" (how to locate/access it) while Jakub is focusing on the shaper "info" (content/definition/attributes). Please correct me If I misread something.

Quoted 4 lines

Still for the record, I interpret the current proposal as not clashing with Jakub's preference, and being tolerated from Jiri, again please correct me if I read too far.

One more thing, Jiri said:

If you do everything netdev-centric from start, I'm sure no shaper consolidation will ever happen. And that I thought was one of the goals.

Consolidation was indeed one of the goals, and I share Jiri's concern :(

Quoted 25 lines · Fri, Aug 23, 2024 at 12:56:08AM CEST, [email protected] wrote:

Quoted 24 lines · On Thu, 22 Aug 2024 22:30:35 +0200 Paolo Abeni wrote:

Quoted 23 lines

Quoted 8 lines

I'm not saying this is deal breaker for me. I just think that if the api is designed to be independent of the object shaper is bound to (netdev/devlink_port/etc), it would be much much easier to extend in the future. If you do everything netdev-centric from start, I'm sure no shaper consolidation will ever happen. And that I thought was one of the goals.

Perhaps Jakub has opinion.

I think you and I are on the same page :) Other than the "reference object" (netdev / devlink port) the driver facing API should be identical. Making it possible for the same driver code to handle translating the parameters into HW config / FW requests, whether they shape at the device (devlink) or port (netdev) level.

Shaper NL for netdevs is separate from internal representation and driver API in my mind. My initial ask was to create the internal representation first, make sure it can express devlink and handful of exiting netdev APIs, and only once that's merged worry about exposing it via a new NL.

I'm not opposed to showing devlink shapers in netdev NL (RO as you say) but talking about it now strikes me as cart before the horse.

Quoted 3 lines

Quoted 3 lines

FTR, I don't see both of you on the same page ?!?

I read the above as Jiri's preference is a single ndo set to control

"Ndo" stands for netdev op and they are all tightly coupled with netdevices. So, "single ndo set to control both devlink and netdev shapers" sounds like nonsense to me.

Quoted 4 lines

Quoted 4 lines

Or to phrase the above differently, Jiri is focusing on the shaper "binding" (how to locate/access it) while Jakub is focusing on the shaper "info" (content/definition/attributes). Please correct me If I misread something.

Two(or more) similar ops structs looks odd to me. I think that the ops should should be shared and just the "binding point" should be somehow abstracted out. Code speaks, let me draft how it could be done:

enum net_shaper_binding_type {
NET_SHAPER_BINDING_TYPE_NETDEV,
NET_SHAPER_BINDING_TYPE_DEVLINK_PORT,
};

struct net_shaper_binding {
         enum net_shaper_binding_type type;
         union {
                 struct net_device *netdev;
                 struct devlink_port *devlink_port;
         };
};

struct net_shaper_ops {

(quoted diff) +48 -0

+       /**
+        * @group: create the specified shapers scheduling group
+        *
+        * Nest the @leaves shapers identified by @leaves_handles under the
+        * @root shaper identified by @root_handle. All the shapers belong
+        * to the network device @dev. The @leaves and @leaves_handles shaper
+        * arrays size is specified by @leaves_count.
+        * Create either the @leaves and the @root shaper; or if they already
+        * exists, links them together in the desired way.
+        * @leaves scope must be NET_SHAPER_SCOPE_QUEUE.
+        *
+        * Returns 0 on group successfully created, otherwise an negative
+        * error value and set @extack to describe the failure's reason.
+        */
+       int (*group)(const struct net_shaper_binding *binding, int leaves_count,
+                    const struct net_shaper_handle *leaves_handles,
+                    const struct net_shaper_info *leaves,
+                    const struct net_shaper_handle *root_handle,
+                    const struct net_shaper_info *root,
+                    struct netlink_ext_ack *extack);
+
+       /**
+        * @set: Updates the specified shaper
+        *
+        * Updates or creates the @shaper identified by the provided @handle
+        * on the given device @dev.
+        *
+        * Returns 0 on success, otherwise an negative
+        * error value and set @extack to describe the failure's reason.
+        */
+       int (*set)(const struct net_shaper_binding *binding,
+                  const struct net_shaper_handle *handle,
+                  const struct net_shaper_info *shaper,
+                  struct netlink_ext_ack *extack);
+
+       /**
+        * @delete: Removes the specified shaper from the NIC
+        *
+        * Removes the shaper configuration as identified by the given @handle
+        * on the specified device @dev, restoring the default behavior.
+        *
+        * Returns 0 on success, otherwise an negative
+        * error value and set @extack to describe the failure's reason.
+        */
+       int (*delete)(const struct net_shaper_binding *binding,
+                     const struct net_shaper_handle *handle,
+                     struct netlink_ext_ack *extack);
+};

static inline struct net_device * net_shaper_binding_netdev(struct net_shaper_binding *binding)

{
         WARN_ON(binding->type != NET_SHAPER_BINDING_TYPE_NETDEV)
         return binding->netdev;
}

static inline struct devlink_port * net_shaper_binding_devlink_port(struct net_shaper_binding *binding)

{
         WARN_ON(binding->type != NET_SHAPER_BINDING_TYPE_DEVLINK_PORT)
         return binding->devlink_port;
}

Then whoever calls the op fills-up the binding structure accordingly.

drivers can implement ops, for netdev-bound shaper like this:

static int driverx_shaper_set(const struct net_shaper_binding *binding,
const struct net_shaper_handle *handle, const struct net_shaper_info *shaper, struct netlink_ext_ack *extack);

{
         struct net_device *netdev = net_shaper_binding_netdev(binding);
         ......
}

struct net_shaper_ops driverx_shaper_ops {
         .set = driverx_shaper_set;
         ......
};

static const struct net_device_ops driverx_netdev_ops = {
.net_shaper_ops = &driverx_shaper_ops,
......
};

Quoted 26 lines · Jiri Pirko

Quoted 25 lines · Fri, Aug 23, 2024 at 12:56:08AM CEST, [email protected] wrote:

Quoted 24 lines · On Thu, 22 Aug 2024 22:30:35 +0200 Paolo Abeni wrote:

Quoted 23 lines

Quoted 8 lines

I'm not saying this is deal breaker for me. I just think that if the api is designed to be independent of the object shaper is bound to (netdev/devlink_port/etc), it would be much much easier to extend in the future. If you do everything netdev-centric from start, I'm sure no shaper consolidation will ever happen. And that I thought was one of the goals.

Perhaps Jakub has opinion.

I think you and I are on the same page :) Other than the "reference object" (netdev / devlink port) the driver facing API should be identical. Making it possible for the same driver code to handle translating the parameters into HW config / FW requests, whether they shape at the device (devlink) or port (netdev) level.

Shaper NL for netdevs is separate from internal representation and driver API in my mind. My initial ask was to create the internal representation first, make sure it can express devlink and handful of exiting netdev APIs, and only once that's merged worry about exposing it via a new NL.

I'm not opposed to showing devlink shapers in netdev NL (RO as you say) but talking about it now strikes me as cart before the horse.

Quoted 7 lines

Quoted 3 lines

Quoted 3 lines

FTR, I don't see both of you on the same page ?!?

I read the above as Jiri's preference is a single ndo set to control

"Ndo" stands for netdev op and they are all tightly coupled with netdevices. So, "single ndo set to control both devlink and netdev shapers" sounds like nonsense to me.

In this context, "NDOs" == set of function pointers operating on the same object.

Quoted 106 lines

Quoted 4 lines

Quoted 4 lines

Or to phrase the above differently, Jiri is focusing on the shaper "binding" (how to locate/access it) while Jakub is focusing on the shaper "info" (content/definition/attributes). Please correct me If I misread something.

Two(or more) similar ops structs looks odd to me. I think that the ops should should be shared and just the "binding point" should be somehow abstracted out. Code speaks, let me draft how it could be done:

enum net_shaper_binding_type {
NET_SHAPER_BINDING_TYPE_NETDEV,
NET_SHAPER_BINDING_TYPE_DEVLINK_PORT,
};

struct net_shaper_binding {
         enum net_shaper_binding_type type;
         union {
                 struct net_device *netdev;
                 struct devlink_port *devlink_port;
         };
};

struct net_shaper_ops {

(quoted diff) +48 -0

+       /**
+        * @group: create the specified shapers scheduling group
+        *
+        * Nest the @leaves shapers identified by @leaves_handles under the
+        * @root shaper identified by @root_handle. All the shapers belong
+        * to the network device @dev. The @leaves and @leaves_handles shaper
+        * arrays size is specified by @leaves_count.
+        * Create either the @leaves and the @root shaper; or if they already
+        * exists, links them together in the desired way.
+        * @leaves scope must be NET_SHAPER_SCOPE_QUEUE.
+        *
+        * Returns 0 on group successfully created, otherwise an negative
+        * error value and set @extack to describe the failure's reason.
+        */
+       int (*group)(const struct net_shaper_binding *binding, int leaves_count,
+                    const struct net_shaper_handle *leaves_handles,
+                    const struct net_shaper_info *leaves,
+                    const struct net_shaper_handle *root_handle,
+                    const struct net_shaper_info *root,
+                    struct netlink_ext_ack *extack);
+
+       /**
+        * @set: Updates the specified shaper
+        *
+        * Updates or creates the @shaper identified by the provided @handle
+        * on the given device @dev.
+        *
+        * Returns 0 on success, otherwise an negative
+        * error value and set @extack to describe the failure's reason.
+        */
+       int (*set)(const struct net_shaper_binding *binding,
+                  const struct net_shaper_handle *handle,
+                  const struct net_shaper_info *shaper,
+                  struct netlink_ext_ack *extack);
+
+       /**
+        * @delete: Removes the specified shaper from the NIC
+        *
+        * Removes the shaper configuration as identified by the given @handle
+        * on the specified device @dev, restoring the default behavior.
+        *
+        * Returns 0 on success, otherwise an negative
+        * error value and set @extack to describe the failure's reason.
+        */
+       int (*delete)(const struct net_shaper_binding *binding,
+                     const struct net_shaper_handle *handle,
+                     struct netlink_ext_ack *extack);
+};

static inline struct net_device * net_shaper_binding_netdev(struct net_shaper_binding *binding)

{
         WARN_ON(binding->type != NET_SHAPER_BINDING_TYPE_NETDEV)
         return binding->netdev;
}

static inline struct devlink_port * net_shaper_binding_devlink_port(struct net_shaper_binding *binding)

{
         WARN_ON(binding->type != NET_SHAPER_BINDING_TYPE_DEVLINK_PORT)
         return binding->devlink_port;
}

Then whoever calls the op fills-up the binding structure accordingly.

drivers can implement ops, for netdev-bound shaper like this:

static int driverx_shaper_set(const struct net_shaper_binding *binding,
const struct net_shaper_handle *handle, const struct net_shaper_info *shaper, struct netlink_ext_ack *extack);

{
         struct net_device *netdev = net_shaper_binding_netdev(binding);
         ......
}

struct net_shaper_ops driverx_shaper_ops {
         .set = driverx_shaper_set;
         ......
};

Quoted 4 lines

static const struct net_device_ops driverx_netdev_ops = {
.net_shaper_ops = &driverx_shaper_ops,
......
};

If I read correctly, the net_shaper_ops caller will have to discriminate between net_device and devlink, do the object-type-specific lookup to get the relevant net_device (or devlink) object and then pass to such net_device (or devlink) a "generic" binding. Did I misread something? If so I must admit I really dislike such interface.

I personally think it would be much cleaner to have 2 separate set of operations, with exactly the same semantic and argument list, except for the first argument (struct net_device or struct devlink).

The driver implementation could still de-duplicate a lot of code, as far as the shaper-related arguments are the same.

Side note, if the intention is to allow the user to touch/modify the queue-level and queue-group-level shapers via the devlink object? if that is the intention, we will need to drop the shaper cache and (re-)introduce a get() callback, as the same shaper could be reached via multiple binding/handle pairs and the core will not know all of such pairs for a given shaper.

Thanks,

Paolo

Quoted 3 lines · Fri, Aug 23, 2024 at 02:58:27PM CEST, [email protected] wrote:

I personally think it would be much cleaner to have 2 separate set of operations, with exactly the same semantic and argument list, except for the first argument (struct net_device or struct devlink).

I think it is totally subjective. You like something, I like something else. Both works. The amount of duplicity and need to change same things on multiple places in case of bugfixes and extensions is what I dislike on the 2 separate sets.

Plus, there might be another binding in
the future, will you copy the ops struct again then?

Quoted 9 lines

The driver implementation could still de-duplicate a lot of code, as far as the shaper-related arguments are the same.

Side note, if the intention is to allow the user to touch/modify the queue-level and queue-group-level shapers via the devlink object? if that is the intention, we will need to drop the shaper cache and (re-)introduce a get() callback, as the same shaper could be reached via multiple binding/handle pairs and the core will not know all of such pairs for a given shaper.

That is a good question, I don't know. But gut feeling is "no".

Quoted 9 lines · Jiri Pirko

Quoted 3 lines · Fri, Aug 23, 2024 at 02:58:27PM CEST, [email protected] wrote:

I personally think it would be much cleaner to have 2 separate set of operations, with exactly the same semantic and argument list, except for the first argument (struct net_device or struct devlink).

I think it is totally subjective. You like something, I like something else. Both works. The amount of duplicity and need to change same things on multiple places in case of bugfixes and extensions is what I dislike on the 2 separate sets.

My guestimate is that the amount of deltas caused by bugfixes and extensions will be much different in practice with the two approaches.

I guess that even with the net_shaper_ops between devlink and net_device, there will be different callbacks implementation for devlink and net_device, right?

If so, the differentiated operation list between devlink and net_device will trade a:

{
struct {net_device, netlink} =
net_shaper_binding_{netdevice_netlink}(binding);

preamble in every callback of every driver for a single additional operations set definition.

It will at least scale better with the number of driver implementing the interface.

Quoted 2 lines

Plus, there might be another binding in
the future, will you copy the ops struct again then?

Yes. Same reasons of the above.

Quoted 4 lines

Quoted 4 lines

The driver implementation could still de-duplicate a lot of code, as far as the shaper-related arguments are the same.

Quoted 8 lines

Quoted 6 lines

Side note, if the intention is to allow the user to touch/modify the queue-level and queue-group-level shapers via the devlink object? if that is the intention, we will need to drop the shaper cache and (re-)introduce a get() callback, as the same shaper could be reached via multiple binding/handle pairs and the core will not know all of such pairs for a given shaper.

That is a good question, I don't know. But gut feeling is "no".

Well, at least that is not in the direction of unlimited amount of additional time and pain ;)

Thanks,

Paolo

Quoted 21 lines · Fri, Aug 23, 2024 at 04:23:30PM CEST, [email protected] wrote:

Quoted 9 lines · Jiri Pirko

Quoted 3 lines · Fri, Aug 23, 2024 at 02:58:27PM CEST, [email protected] wrote:

I personally think it would be much cleaner to have 2 separate set of operations, with exactly the same semantic and argument list, except for the first argument (struct net_device or struct devlink).

I think it is totally subjective. You like something, I like something else. Both works. The amount of duplicity and need to change same things on multiple places in case of bugfixes and extensions is what I dislike on the 2 separate sets.

My guestimate is that the amount of deltas caused by bugfixes and extensions will be much different in practice with the two approaches.

I guess that even with the net_shaper_ops between devlink and net_device, there will be different callbacks implementation for devlink and net_device, right?

If so, the differentiated operation list between devlink and net_device will trade a:

Quoted 6 lines

{
struct {net_device, netlink} =
net_shaper_binding_{netdevice_netlink}(binding);

preamble in every callback of every driver for a single additional operations set definition.

So?

Quoted 6 lines

It will at least scale better with the number of driver implementing the interface.

Quoted 2 lines

Plus, there might be another binding in
the future, will you copy the ops struct again then?

Quoted 1 line

Yes. Same reasons of the above.

What's stopping anyone from diverging these 2-n sets? I mean, the whole purpose it unification and finding common ground. Once you have ops duplicated, sooner then later someone does change in A but ignore B. Having the "preamble" in every callback seems like very good tradeoff to prevent this scenario.

Quoted 5 lines

What's stopping anyone from diverging these 2-n sets? I mean, the whole purpose it unification and finding common ground. Once you have ops duplicated, sooner then later someone does change in A but ignore B. Having the "preamble" in every callback seems like very good tradeoff to prevent this scenario.

The main fact is that we do not agree on the above point - unify the shaper_ops between struct net_device and struct devlink.

I think a 3rd party opinion could help moving forward. @Jakub could you please share your view here?

Quoted 11 lines · On Tue, 27 Aug 2024 16:37:38 +0200 Paolo Abeni wrote:

Quoted 5 lines

What's stopping anyone from diverging these 2-n sets? I mean, the whole purpose it unification and finding common ground. Once you have ops duplicated, sooner then later someone does change in A but ignore B. Having the "preamble" in every callback seems like very good tradeoff to prevent this scenario.

The main fact is that we do not agree on the above point - unify the shaper_ops between struct net_device and struct devlink.

I think a 3rd party opinion could help moving forward. @Jakub could you please share your view here?

I don't mind Jiri's suggestion. Driver can declare its own helper:

static struct drv_port *
drv_shaper_binding_to_prot(const struct net_shaper_binding *binding)

{
      if (binding->type == NET_SHAPER_BINDING_TYPE_NETDEV)
              return /* netdev_priv() ? */;
      if (binding->type == NET_SHAPER_BINDING_TYPE_DEVLINK_PORT)
              return /* container_of() ? */;
      WARN_ONCE();
      return NULL;
}

And call that instead of netdev_priv()?

Quoted 19 lines

Quoted 5 lines

The main fact is that we do not agree on the above point - unify the shaper_ops between struct net_device and struct devlink.

I think a 3rd party opinion could help moving forward. @Jakub could you please share your view here?

I don't mind Jiri's suggestion. Driver can declare its own helper:

static struct drv_port *
drv_shaper_binding_to_port(const struct net_shaper_binding *binding)

{
      if (binding->type == NET_SHAPER_BINDING_TYPE_NETDEV)
              return /* netdev_priv() ? */;
      if (binding->type == NET_SHAPER_BINDING_TYPE_DEVLINK_PORT)
              return /* container_of() ? */;
      WARN_ONCE();
      return NULL;
}

Quoted 1 line

And call that instead of netdev_priv()?

As I wrote this does not look like something that would help de-deuplicate any code, but since you both seem to agree...

Double checking before I rewrote a significant amount of the core code:

In the NL API, I will replace ifindex with binding, the latter will include nested attributes ifindex, bus_name
and dev_name.

Note that 'struct net_shaper_binding' must include a ‘struct devlink *’ as opposed to a
‘struct devlink_port *’, as mentioned in the code snippet so far, or we will have to drop the
shaper cache and re-introduce a get() operation.

The ops will try to fetch the net_device or devlink according to the provided attributes.
At the moment the ops will error out with ENOTSUP for netlink object.

Most core helpers
will be re-factored to accept a 'binding *' argument in place of a 'struct net_device *'.

Full netlink support (for the brave that will implement it later) will likely require at least an
additional scope (devlink_port), include the net_shaper_ops and net_shaper_data inside
struct devlink, and likely code adaptation inside the core.

Literally the only change I would have expected based on this branch of the conversation is slight adjustment to the parameters to ops. Nothing else. No netlink changes. Only core change would be to wrap the ops.

Quoted 3 lines · Jakub Kicinski

Literally the only change I would have expected based on this branch of the conversation is slight adjustment to the parameters to ops. Nothing else. No netlink changes. Only core change would be to wrap the ops.

FTR, the above is quite the opposite of my understanding of the whole conversation so far. If the whole delta under discussion is just that, we could make such a change at any given time (and I would strongly support the idea to make it only when the devlink bits will be ready) and its presence (or absence) will not block the net_device/devlink shaper callback consolidation in any way.

I thought Jiri intended the whole core bits to be adapted to handle 'binding' instead of net_device, to allow a more seamless plug of devlink.
@Jiri: did I misread your words?

Thanks,

Paolo

Makes sense?

Quoted 1 line · Jiri Pirko

Makes sense?

Almost! Tacking aside the (very significant) differences between your proposition and Jakub’s, we can't use devlink port here, just devlink, or we will have to drop the cache too[1]. Specific devlink port shapers will be reached via different handles (scope/id).

Additionally, I think we don't need strictly the ‘binding’ nested attribute to extend the NL API with different binding objects (devlink), we could append the new attributes needed to support (identify) devlink at the end of the net shaper attributes list. I agree that would be likely less ‘nice’.

What about:
- Refactor the core and the driver api to support the ‘binding’ thing

- Update the NL definition to nest the ‘ifindex’ attribute under the ‘binding’ one. No mention/reference to devlink yet, so most of the documentation will be unchanged.

- devlink support will not be included, but there should be enough ground paved for it.