Skip to contentSkip to conversation

net: introduce TX H/W shaping API

PATCH Superseded Paolo Abeni 5 participants 34 messages Aug 20, 2024 – Aug 27, 2024

Subsystems (9)

v4 of 9 Superseded 1 of 12 applied Latest is v9 →

lore
Share & tools
lore

Paolo Abeni

author maintainer
#0 lore raw

v4 cover net: introduce TX H/W shaping API

We have a plurality of shaping-related drivers API, but none flexible enough to meet existing demand from vendors[1].

This series introduces new device APIs to configure in a flexible way TX H/W shaping. The new functionalities are exposed via a newly defined generic netlink interface and include introspection capabilities. Some self-tests are included, on top of a dummy netdevsim implementation, and a basic implementation for the iavf driver.

Some usage examples:

* Configure shaping on a given queue:

./tools/net/ynl/cli.py --spec Documentation/netlink/specs/shaper.yaml \
	--do set --json '{"ifindex":'$IFINDEX',
			"shaper": {"handle":
				{"scope": "queue", "id":'$QUEUEID' },
			"bw-max": 2000000 }}'

* Container B/W sharing

The orchestration infrastructure wants to group the container-related queues under a RR scheduling and limit the aggregate bandwidth:

./tools/net/ynl/cli.py --spec Documentation/netlink/specs/shaper.yaml \
	--do group --json '{"ifindex":'$IFINDEX', 
			"leaves": [ 
			  {"handle": {"scope": "queue", "id":'$QID1' },
			   "weight": '$W1'}, 
			  {"handle": {"scope": "queue", "id":'$QID2' },
			   "weight": '$W2'}], 
			  {"handle": {"scope": "queue", "id":'$QID3' },
			   "weight": '$W3'}], 
			"root": { "handle": {"scope":"node"},
			 	  "bw-max": 10000000}}'

{'ifindex': $IFINDEX, 'handle': {'scope': 'node', 'id': 0}}

Q1 \
    \
Q2 -- node 0 -------  netdev
    / (bw-max: 10M)

Q3 /

* Delegation

A containers wants to limit the aggregate B/W bandwidth of 2 of the 3 queues it owns - the starting configuration is the one from the previous point:

SPEC=Documentation/netlink/specs/net_shaper.yaml

./tools/net/ynl/cli.py --spec $SPEC \
	--do group --json '{"ifindex":'$IFINDEX', 
			"leaves": [ 
			  {"handle": {"scope": "queue", "id":'$QID1' },
			   "weight": '$W1'}, 
			  {"handle": {"scope": "queue", "id":'$QID2' },
			   "weight": '$W2'}], 
			"root": { "handle": {"scope": "node"},
				  "parent": {"scope": "node", "id": 0},
				  "bw-max": 5000000 }}'

{'ifindex': $IFINDEX, 'handle': {'scope': 'node', 'id': 1}}

Q1 -- node 1 --------\
    / (bw-max: 5M)    \
Q2 /                   node 0 -------  netdev
                      /  (bw-max: 10M)

Q3 ------------------

* Cleanup:

To delete a single queue shaper:

./tools/net/ynl/cli.py --spec $SPEC --do delete --json \
	'{"ifindex":'$IFINDEX', 
	  "handle": {"scope": "queue", "id":'$QID3' }}'
Q1 -- node 1 --------\
    / (bw-max: 5M)    \
Q2 /                   node 0 -------  netdev
                       (bw-max: 10M)

Deleting a node shaper relinks all its leaves to the node's parent:

./tools/net/ynl/cli.py --spec $SPEC --do delete --json \
	'{"ifindex":'$IFINDEX', 
	  "handle": {"scope": "node", "id":1 }}'
Q1 ------\
          \
          node 0 -------  netdev
         /  (bw-max: 10M)

Q2 -----

Deleting the last shaper under a node shaper deletes the node, too:

./tools/net/ynl/cli.py --spec $SPEC --do delete --json \
	'{"ifindex":'$IFINDEX', 
	  "handle": {"scope": "queue", "id":'$QID1' }}'
./tools/net/ynl/cli.py --spec $SPEC --do delete --json \
	'{"ifindex":'$IFINDEX', 
	  "handle": {"scope": "queue", "id":'$QID2' }}'
./tools/net/ynl/cli.py --spec $SPEC --do get --json '{"ifindex":'$IF', 
			  "handle": { "scope": "node", "id": 0}}'
Netlink error: No such file or directory
nl_len = 44 (28) nl_flags = 0x300 nl_type = 2
	error: -2
	extack: {'bad-attr': '.handle'}

---
Changes from V3:
- rename
- locking
- delete operates on node, too

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

Changes from RFC v2:
- added patch 1
- fixed deprecated API usage

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

Changes from RFC v1:
- set() and delete() ops operate on a single shaper
- added group() op to allow grouping and nesting
- split the NL implementation into multiple patches to help reviewing

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

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

Paolo Abeni (9):
tools: ynl: lift an assumption about spec file name
netlink: spec: add shaper YAML spec
net-shapers: implement NL get operation
net-shapers: implement NL set and delete operations
net-shapers: implement NL group operation
net-shapers: implement delete support for NODE scope shaper
netlink: spec: add shaper introspection support
net: shaper: implement introspection support
testing: net-drv: add basic shaper test

Sudheer Mogilappagari (1):
iavf: Add net_shaper_ops support

Wenjun Wu (2):
virtchnl: support queue rate limit and quanta size configuration
ice: Support VF queue rate limit and quanta size configuration

 Documentation/netlink/specs/net_shaper.yaml   |  373 +++++
 Documentation/networking/kapi.rst             |    3 +
 MAINTAINERS                                   |    1 +
 drivers/net/Kconfig                           |    1 +
 drivers/net/ethernet/intel/Kconfig            |    1 +
 drivers/net/ethernet/intel/iavf/iavf.h        |    3 +
 drivers/net/ethernet/intel/iavf/iavf_main.c   |  150 ++
 drivers/net/ethernet/intel/iavf/iavf_txrx.h   |    2 +
 .../net/ethernet/intel/iavf/iavf_virtchnl.c   |   65 +
 drivers/net/ethernet/intel/ice/ice.h          |    2 +
 drivers/net/ethernet/intel/ice/ice_base.c     |    2 +
 drivers/net/ethernet/intel/ice/ice_common.c   |   21 +
 .../net/ethernet/intel/ice/ice_hw_autogen.h   |    8 +
 drivers/net/ethernet/intel/ice/ice_txrx.h     |    1 +
 drivers/net/ethernet/intel/ice/ice_type.h     |    1 +
 drivers/net/ethernet/intel/ice/ice_vf_lib.h   |    8 +
 drivers/net/ethernet/intel/ice/ice_virtchnl.c |  333 +++++
 drivers/net/ethernet/intel/ice/ice_virtchnl.h |   11 +
 .../intel/ice/ice_virtchnl_allowlist.c        |    6 +
 drivers/net/netdevsim/netdev.c                |   41 +
 include/linux/avf/virtchnl.h                  |  119 ++
 include/linux/netdevice.h                     |   17 +
 include/net/net_shaper.h                      |  116 ++
 include/uapi/linux/net_shaper.h               |   90 ++
 net/Kconfig                                   |    3 +
 net/Makefile                                  |    1 +
 net/core/dev.c                                |    2 +
 net/core/dev.h                                |    6 +
 net/shaper/Makefile                           |    9 +
 net/shaper/shaper.c                           | 1202 +++++++++++++++++
 net/shaper/shaper_nl_gen.c                    |  152 +++
 net/shaper/shaper_nl_gen.h                    |   41 +
 tools/net/ynl/ynl-gen-c.py                    |    6 +-
 tools/testing/selftests/drivers/net/Makefile  |    1 +
 tools/testing/selftests/drivers/net/shaper.py |  236 ++++
 .../testing/selftests/net/lib/py/__init__.py  |    1 +
 tools/testing/selftests/net/lib/py/ynl.py     |    5 +
 37 files changed, 3038 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/netlink/specs/net_shaper.yaml
 create mode 100644 include/net/net_shaper.h
 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
 create mode 100755 tools/testing/selftests/drivers/net/shaper.py
Signature · 1 line
2.45.2
author maintainer
#1 lore raw

PATCH v4 1/12 tools: ynl: lift an assumption about spec file name

Currently the parsing code generator assumes that the yaml specification file name and the main 'name' attribute carried inside correspond, that is the field is the c-name representation of the file basename.

The above assumption held true within the current tree, but will be hopefully broken soon by the upcoming net shaper specification. Additionally, it makes the field 'name' itself useless.

Lift the assumption, always computing the generated include file name from the generated c file name.

---
 tools/net/ynl/ynl-gen-c.py | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
tools/net/ynl/ynl-gen-c.py +4 -2
index 51529fabd517..717530bc9c52 100755
@@ -2668,13 +2668,15 @@ def main():
         cw.p('#define ' + hdr_prot)
         cw.nl()
 
+    hdr_file=os.path.basename(args.out_file[:-2]) + ".h"
+
     if args.mode == 'kernel':
         cw.p('#include <net/netlink.h>')
         cw.p('#include <net/genetlink.h>')
         cw.nl()
         if not args.header:
             if args.out_file:
-                cw.p(f'#include "{os.path.basename(args.out_file[:-2])}.h"')
+                cw.p(f'#include "{hdr_file}"')
             cw.nl()
         headers = ['uapi/' + parsed.uapi_header]
         headers += parsed.kernel_family.get('headers', [])
@@ -2686,7 +2688,7 @@ def main():
             if family_contains_bitfield32(parsed):
                 cw.p('#include <linux/netlink.h>')
         else:
-            cw.p(f'#include "{parsed.name}-user.h"')
+            cw.p(f'#include "{hdr_file}"')
             cw.p('#include "ynl.h"')
         headers = [parsed.uapi_header]
     for definition in parsed['definitions']:
Signature · 1 line
2.45.2
author maintainer
#2 lore raw

PATCH v4 2/12 netlink: spec: add shaper YAML spec

In reply to Paolo Abeni

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.

---
v3 -> v4:
- spec file rename
- always use '@' for references
- detached scope -> node scope
- inputs/output -> leaves/root
- deduplicate leaves/root policy
- get/dump/group return ifindex, too
- added some general introduction to the doc
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/net_shaper.yaml | 289 ++++++++++++++++++++
 MAINTAINERS                                 |   1 +
 include/uapi/linux/net_shaper.h             |  73 +++++
 net/Kconfig                                 |   3 +
 net/Makefile                                |   1 +
 net/shaper/Makefile                         |   9 +
 net/shaper/shaper.c                         |  45 +++
 net/shaper/shaper_nl_gen.c                  | 125 +++++++++
 net/shaper/shaper_nl_gen.h                  |  33 +++
 9 files changed, 579 insertions(+)
 create mode 100644 Documentation/netlink/specs/net_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/net_shaper.yaml new file +289 -0
new file mode 100644
index 000000000000..a2b7900646ae
@@ -0,0 +1,289 @@
+# SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause)
+
+name: net-shaper
+
+doc: |
+  Network device HW rate limiting configuration.
+
+  This API allows configuring HR shapers available on the network
+  device at different levels (queues, network device) and allows
+  arbitrary manipulation of the scheduling tree of the involved
+  shapers.
+
+  Each @shaper is identified within the given device, by an @handle,
+  comprising both a @scope and an @id, and can be created via two
+  different modifiers: the @set operation, to create and update single
+  shaper, and the @group operation, to create and update a scheduling
+  group.
+
+  Existing shapers can be deleted via the @delete operation.
+
+  The user can query the running configuration via the @get operation.
+
+definitions:
+  -
+    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: netdev
+        doc: The main shaper for the given network device.
+      -
+        name: queue
+        doc: The shaper is attached to the given device queue.
+      -
+        name: node
+        doc: |
+             The shaper allows grouping of queues or others
+             node shapers, is not attached to any user-visible
+             network device component, and can be nested to
+             either @netdev shapers or other @node shapers.
+  -
+    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.
+
+attribute-sets:
+  -
+    name: net-shaper
+    attributes:
+      -
+        name: handle
+        type: nest
+        nested-attributes: handle
+        doc: Unique identifier for the given shaper inside the owning device.
+      -
+        name: info
+        type: nest
+        nested-attributes: info
+        doc: Fully describes the shaper.
+      -
+        name: metric
+        type: u32
+        enum: metric
+        doc: Metric used by the given shaper for bw-min, bw-max and burst.
+      -
+        name: bw-min
+        type: uint
+        doc: Minimum guaranteed B/W for the given shaper.
+      -
+        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
+        type: u32
+        enum: scope
+        doc: The given shaper scope.
+      -
+        name: id
+        type: u32
+        doc: |
+          The given shaper id. The id semantic depends on the actual
+          scope, e.g. for @queue scope it's the queue id, for
+          @node scope it's the node identifier.
+      -
+        name: ifindex
+        type: u32
+        doc: Interface index owning the specified shaper.
+      -
+        name: parent
+        type: nest
+        nested-attributes: handle
+        doc: |
+          Identifier for the parent of the affected shaper,
+          The parent is usually implied by the shaper handle itself,
+          with the only exception of the root shaper in the @group operation.
+      -
+        name: leaves
+        type: nest
+        multi-attr: true
+        nested-attributes: info
+        doc: |
+           Describes a set of leaves shapers for a @group operation.
+      -
+        name: root
+        type: nest
+        nested-attributes: root-info
+        doc: |
+           Describes the root shaper for a @group operation
+           Differently from @leaves and @shaper allow specifying
+           the shaper parent handle, too.
+      -
+        name: shaper
+        type: nest
+        nested-attributes: info
+        doc: |
+           Describes a single shaper for a @set operation.
+  -
+    name: handle
+    subset-of: net-shaper
+    attributes:
+      -
+        name: scope
+      -
+        name: id
+  -
+    name: info
+    subset-of: net-shaper
+    attributes:
+      -
+        name: handle
+      -
+        name: metric
+      -
+        name: bw-min
+      -
+        name: bw-max
+      -
+        name: burst
+      -
+        name: priority
+      -
+        name: weight
+  -
+    name: root-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:
+        pre: net-shaper-nl-pre-doit
+        post: net-shaper-nl-post-doit
+        request:
+          attributes: &ns-binding
+            - ifindex
+            - handle
+        reply:
+          attributes: &ns-attrs
+            - ifindex
+            - parent
+            - handle
+            - metric
+            - bw-min
+            - bw-max
+            - burst
+            - priority
+            - weight
+
+      dump:
+        request:
+          attributes:
+            - ifindex
+        reply:
+          attributes: *ns-attrs
+    -
+      name: set
+      doc: |
+        Create or updates the specified shaper.
+        On failure the extack is set accordingly.
+        Can't create @node scope shaper, use
+        the @group operation instead.
+      attribute-set: net-shaper
+      flags: [ admin-perm ]
+
+      do:
+        pre: net-shaper-nl-pre-doit
+        post: net-shaper-nl-post-doit
+        request:
+          attributes:
+            - ifindex
+            - shaper
+
+    -
+      name: delete
+      doc: |
+        Clear (remove) the specified shaper. When deleting
+        a @node shaper, relink all the node's leaves to the
+        deleted node parent.
+        If, after the removal, the parent shaper has no more
+        leaves and the parent shaper scope is @node, even
+        the parent node is deleted, recursively.
+        On failure the extack is set accordingly.
+      attribute-set: net-shaper
+      flags: [ admin-perm ]
+
+      do:
+        pre: net-shaper-nl-pre-doit
+        post: net-shaper-nl-post-doit
+        request:
+          attributes: *ns-binding
+
+    -
+      name: group
+      doc: |
+        Creates or updates a scheduling group, adding the specified
+        @leaves shapers under the specified @root, eventually creating
+        the latter, if needed.
+        The @leaves shapers scope must be @queue or @node scope and
+        the @root shaper scope must be either @node or @netdev.
+        When using a root @node scope shaper, if the
+        @handle @id is not specified, a new shaper of such scope
+        is created, otherwise the specified root shaper
+        must already exist.
+        The operation is atomic, on failure the extack is set
+        accordingly and no change is applied to the device
+        shaping configuration, otherwise the root shaper
+        binding (ifindex and handle) is provided as the reply.
+      attribute-set: net-shaper
+      flags: [ admin-perm ]
+
+      do:
+        pre: net-shaper-nl-pre-doit
+        post: net-shaper-nl-post-doit
+        request:
+          attributes:
+            - ifindex
+            - leaves
+            - root
+        reply:
+          attributes: *ns-binding
MAINTAINERS +1 -0
index 5dbf23cf11c8..553ca2635bcc 100644
@@ -15885,6 +15885,7 @@ F:	include/linux/inetdevice.h
 F:	include/linux/netdevice.h
 F:	include/uapi/linux/cn_proc.h
 F:	include/uapi/linux/if_*
+F:	include/uapi/linux/net_shaper.h
 F:	include/uapi/linux/netdevice.h
 X:	drivers/net/wireless/
 
include/uapi/linux/net_shaper.h new file +73 -0

73 lines not on this page · raw message

net/Kconfig +3 -0
index d27d0deac0bf..31fccfed04f7 100644
@@ -66,6 +66,9 @@ config SKB_DECRYPTED
 config SKB_EXTENSIONS
 	bool
 
+config NET_SHAPER
+	bool
+
 menu "Networking options"
 
 source "net/packet/Kconfig"
net/Makefile +1 -0
index 65bb8c72a35e..60ed5190eda8 100644
@@ -79,3 +79,4 @@ obj-$(CONFIG_XDP_SOCKETS)	+= xdp/
 obj-$(CONFIG_MPTCP)		+= mptcp/
 obj-$(CONFIG_MCTP)		+= mctp/
 obj-$(CONFIG_NET_HANDSHAKE)	+= handshake/
+obj-$(CONFIG_NET_SHAPER)	+= shaper/
net/shaper/Makefile new file +9 -0
new file mode 100644
index 000000000000..13375884d60e
@@ -0,0 +1,9 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Makefile for the Generic HANDSHAKE service
+#
+# Copyright (c) 2024, Red Hat, Inc.
+#
+
+obj-y += shaper.o shaper_nl_gen.o
+
net/shaper/shaper.c new file +45 -0

45 lines not on this page · raw message

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

125 lines not on this page · raw message

net/shaper/shaper_nl_gen.h new file +33 -0
new file mode 100644
index 000000000000..9b0682c83a07
@@ -0,0 +1,33 @@
+/* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause) */
+/* Do not edit directly, auto-generated from: */
+/*	Documentation/netlink/specs/net_shaper.yaml */
+/* YNL-GEN kernel header */
+
+#ifndef _LINUX_NET_SHAPER_GEN_H
+#define _LINUX_NET_SHAPER_GEN_H
+
+#include <net/netlink.h>
+#include <net/genetlink.h>
+
+#include <uapi/linux/net_shaper.h>
+
+/* Common nested types */
+extern const struct nla_policy net_shaper_handle_nl_policy[NET_SHAPER_A_ID + 1];
+extern const struct nla_policy net_shaper_info_nl_policy[NET_SHAPER_A_WEIGHT + 1];
+extern const struct nla_policy net_shaper_root_info_nl_policy[NET_SHAPER_A_PARENT + 1];
+
+int net_shaper_nl_pre_doit(const struct genl_split_ops *ops,
+			   struct sk_buff *skb, struct genl_info *info);
+void
+net_shaper_nl_post_doit(const struct genl_split_ops *ops, struct sk_buff *skb,
+			struct genl_info *info);
+
+int net_shaper_nl_get_doit(struct sk_buff *skb, struct genl_info *info);
+int net_shaper_nl_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb);
+int net_shaper_nl_set_doit(struct sk_buff *skb, struct genl_info *info);
+int net_shaper_nl_delete_doit(struct sk_buff *skb, struct genl_info *info);
+int net_shaper_nl_group_doit(struct sk_buff *skb, struct genl_info *info);
+
+extern struct genl_family net_shaper_nl_family;
+
+#endif /* _LINUX_NET_SHAPER_GEN_H */
Signature · 1 line
2.45.2
maintainer
#3 lore raw

Re: [PATCH v4 net-next 02/12] netlink: spec: add shaper YAML spec

Quoted 6 lines · On Tue, 20 Aug 2024 17:12:23 +0200 Paolo Abeni wrote:
Documentation/netlink/specs/net_shaper.yaml new file +0 -0
new file mode 100644
index 000000000000..a2b7900646ae

Documentation/netlink/specs/net_shaper.yaml line 8

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

What's HR?

Documentation/netlink/specs/net_shaper.yaml line 15

Quoted 7 lines
Documentation/netlink/specs/net_shaper.yaml new file +7 -0
+  device at different levels (queues, network device) and allows
+  arbitrary manipulation of the scheduling tree of the involved
+  shapers.
+
+  Each @shaper is identified within the given device, by an @handle,
+  comprising both a @scope and an @id, and can be created via two
+  different modifiers: the @set operation, to create and update single

s/different modifiers/operations/

Documentation/netlink/specs/net_shaper.yaml line 19

Quoted 4 lines
Documentation/netlink/specs/net_shaper.yaml new file +4 -0
+  shaper, and the @group operation, to create and update a scheduling
+  group.
+
+  Existing shapers can be deleted via the @delete operation.

deleted -> deleted / reset

Documentation/netlink/specs/net_shaper.yaml line 21

Quoted 1 line
Documentation/netlink/specs/net_shaper.yaml new file +1 -0
+  The user can query the running configuration via the @get operation.

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

Documentation/netlink/specs/net_shaper.yaml line 27

Quoted 5 lines
Documentation/netlink/specs/net_shaper.yaml new file +5 -0
+definitions:
+  -
+    type: enum
+    name: scope
+    doc: The different scopes where a shaper can be attached.

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

Quoted 5 lines
Documentation/netlink/specs/net_shaper.yaml new file +5 -0
+    render-max: true
+    entries:
+      - name: unspec
+        doc: The scope is not specified.
+      -

Documentation/netlink/specs/net_shaper.yaml line 42

Quoted 10 lines
Documentation/netlink/specs/net_shaper.yaml new file +10 -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: node
+        doc: |
+             The shaper allows grouping of queues or others
+             node shapers, is not attached to any user-visible

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

Quoted 16 lines
Documentation/netlink/specs/net_shaper.yaml new file +15 -0
+             network device component, and can be nested to
+             either @netdev shapers or other @node shapers.
 
+attribute-sets:
+  -
+    name: net-shaper
+    attributes:
+      -
+        name: handle
+        type: nest
+        nested-attributes: handle
+        doc: Unique identifier for the given shaper inside the owning device.
+      -
+        name: info
+        type: nest
+        nested-attributes: info

Documentation/netlink/specs/net_shaper.yaml line 79

Quoted 10 lines
Documentation/netlink/specs/net_shaper.yaml new file +10 -0
+        doc: Fully describes the shaper.
+      -
+        name: metric
+        type: u32
+        enum: metric
+        doc: Metric used by the given shaper for bw-min, bw-max and burst.
+      -
+        name: bw-min
+        type: uint
+        doc: Minimum guaranteed B/W for the given shaper.

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

Documentation/netlink/specs/net_shaper.yaml line 83

Quoted 4 lines
Documentation/netlink/specs/net_shaper.yaml new file +4 -0
+      -
+        name: bw-max
+        type: uint
+        doc: Shaping B/W for the given shaper or 0 when unlimited.

s/Shaping/Maximum/

Documentation/netlink/specs/net_shaper.yaml line 87

Quoted 4 lines
Documentation/netlink/specs/net_shaper.yaml new file +4 -0
+      -
+        name: burst
+        type: uint
+        doc: Maximum burst-size for bw-min and bw-max.

How about:

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

?

Documentation/netlink/specs/net_shaper.yaml line 91

Quoted 4 lines
Documentation/netlink/specs/net_shaper.yaml new file +4 -0
+      -
+        name: priority
+        type: u32
+        doc: Scheduling priority for the given shaper.

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

Documentation/netlink/specs/net_shaper.yaml line 96

Quoted 5 lines
Documentation/netlink/specs/net_shaper.yaml new file +5 -0
+      -
+        name: weight
+        type: u32
+        doc: |
+          Weighted round robin weight for given shaper.

Relative weight of the input into a round robin node.

?

Documentation/netlink/specs/net_shaper.yaml line 103

Quoted 7 lines
Documentation/netlink/specs/net_shaper.yaml new file +7 -0
+          The scheduling is applied to all the sibling
+          shapers with the same priority.
+      -
+        name: scope
+        type: u32
+        enum: scope
+        doc: The given shaper scope.

:)

Documentation/netlink/specs/net_shaper.yaml

Quoted 5 lines
Documentation/netlink/specs/net_shaper.yaml new file +5 -0
+      -
+        name: id
+        type: u32
+        doc: |
+          The given shaper id. 

"Numeric identifier of a shaper."

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

Quoted 5 lines

The id semantic depends on the actual

Documentation/netlink/specs/net_shaper.yaml new file +4 -0
+          scope, e.g. for @queue scope it's the queue id, for
+          @node scope it's the node identifier.
+      -
+        name: ifindex

Documentation/netlink/specs/net_shaper.yaml line 122

Quoted 10 lines
Documentation/netlink/specs/net_shaper.yaml new file +10 -0
+        type: u32
+        doc: Interface index owning the specified shaper.
+      -
+        name: parent
+        type: nest
+        nested-attributes: handle
+        doc: |
+          Identifier for the parent of the affected shaper,
+          The parent is usually implied by the shaper handle itself,
+          with the only exception of the root shaper in the @group operation.

Maybe just say that specifying the parent is only needed for group operations? I think that's what you mean.

Documentation/netlink/specs/net_shaper.yaml line 135

Quoted 13 lines
Documentation/netlink/specs/net_shaper.yaml new file +13 -0
+      -
+        name: leaves
+        type: nest
+        multi-attr: true
+        nested-attributes: info
+        doc: |
+           Describes a set of leaves shapers for a @group operation.
+      -
+        name: root
+        type: nest
+        nested-attributes: root-info
+        doc: |
+           Describes the root shaper for a @group operation

missing full stop

Documentation/netlink/specs/net_shaper.yaml line 137

Quoted 2 lines
Documentation/netlink/specs/net_shaper.yaml new file +2 -0
+           Differently from @leaves and @shaper allow specifying
+           the shaper parent handle, too.

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

Documentation/netlink/specs/net_shaper.yaml line 143

Quoted 6 lines
Documentation/netlink/specs/net_shaper.yaml new file +6 -0
+      -
+        name: shaper
+        type: nest
+        nested-attributes: info
+        doc: |
+           Describes a single shaper for a @set operation.

Hm. How is this different than "info"?

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

is "info" supposed to be used?

Documentation/netlink/specs/net_shaper.yaml line 196

Quoted 6 lines
Documentation/netlink/specs/net_shaper.yaml new file +6 -0
+operations:
+  list:
+    -
+      name: get
+      doc: |
+        Get / Dump information about a/all the shaper for a given device.

There's no need to "/ dump" and "/all".

Documentation/netlink/specs/net_shaper.yaml line 227

Quoted 6 lines
Documentation/netlink/specs/net_shaper.yaml new file +5 -0
+      attribute-set: net-shaper
 
+    -
+      name: set
+      doc: |
+        Create or updates the specified shaper.

create or update

Quoted 1 line

+ On failure the extack is set accordingly.

it better be - no need to explain netlink basics

Documentation/netlink/specs/net_shaper.yaml line 230

Quoted 2 lines
Documentation/netlink/specs/net_shaper.yaml new file +2 -0
+        Can't create @node scope shaper, use
+        the @group operation instead.

"The set operation can't be used to create a @node scope shaper..."

Quoted 6 lines
Documentation/netlink/specs/net_shaper.yaml new file +6 -0
+      attribute-set: net-shaper
+      flags: [ admin-perm ]
+
+      do:
+        pre: net-shaper-nl-pre-doit
+        post: net-shaper-nl-post-doit

Documentation/netlink/specs/net_shaper.yaml line 246

Quoted 10 lines
Documentation/netlink/specs/net_shaper.yaml new file +10 -0
+        request:
+          attributes:
+            - ifindex
+            - shaper
+
+    -
+      name: delete
+      doc: |
+        Clear (remove) the specified shaper. When deleting
+        a @node shaper, relink all the node's leaves to the

relink -> reattach ?

Quoted 1 line

+ deleted node parent.

delete node's parent

Quoted 7 lines
Documentation/netlink/specs/net_shaper.yaml new file +7 -0
+        If, after the removal, the parent shaper has no more
+        leaves and the parent shaper scope is @node, even
+        the parent node is deleted, recursively.
+        On failure the extack is set accordingly.
+      attribute-set: net-shaper
+      flags: [ admin-perm ]
+

Documentation/netlink/specs/net_shaper.yaml line 264

Quoted 10 lines
Documentation/netlink/specs/net_shaper.yaml new file +10 -0
+      do:
+        pre: net-shaper-nl-pre-doit
+        post: net-shaper-nl-post-doit
+        request:
+          attributes: *ns-binding
+
+    -
+      name: group
+      doc: |
+        Creates or updates a scheduling group, adding the specified

Please use imperative mood, like in a commit message.

adding -> attach(ing)

Documentation/netlink/specs/net_shaper.yaml line 5

Quoted 7 lines

+++ b/net/shaper/Makefile

Documentation/netlink/specs/net_shaper.yaml new file +5 -0
@@ -0,0 +1,9 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Makefile for the Generic HANDSHAKE service
+#
+# Copyright (c) 2024, Red Hat, Inc.

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

author maintainer
#4 lore raw

Re: [PATCH v4 net-next 02/12] netlink: spec: add shaper YAML spec

Quoted 17 lines · Jakub Kicinski
Quoted 6 lines · On Tue, 20 Aug 2024 17:12:23 +0200 Paolo Abeni wrote:
Documentation/netlink/specs/net_shaper.yaml new file +0 -0
new file mode 100644
index 000000000000..a2b7900646ae

Documentation/netlink/specs/net_shaper.yaml line 8

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

What's HR?

Type: HW

Quoted 15 lines

Documentation/netlink/specs/net_shaper.yaml line 15

Quoted 7 lines
Documentation/netlink/specs/net_shaper.yaml new file +7 -0
+  device at different levels (queues, network device) and allows
+  arbitrary manipulation of the scheduling tree of the involved
+  shapers.
+
+  Each @shaper is identified within the given device, by an @handle,
+  comprising both a @scope and an @id, and can be created via two
+  different modifiers: the @set operation, to create and update single

s/different modifiers/operations/

Documentation/netlink/specs/net_shaper.yaml line 19

Quoted 4 lines
Documentation/netlink/specs/net_shaper.yaml new file +4 -0
+  shaper, and the @group operation, to create and update a scheduling
+  group.
+
+  Existing shapers can be deleted via the @delete operation.
Quoted 7 lines

deleted -> deleted / reset

Documentation/netlink/specs/net_shaper.yaml line 21

Quoted 1 line
Documentation/netlink/specs/net_shaper.yaml new file +1 -0
+  The user can query the running configuration via the @get operation.

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

How about re-phrasing the previous paragraph as:

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

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

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

Quoted 8 lines

Documentation/netlink/specs/net_shaper.yaml line 27

Quoted 5 lines
Documentation/netlink/specs/net_shaper.yaml new file +5 -0
+definitions:
+  -
+    type: enum
+    name: scope
+    doc: The different scopes where a shaper can be attached.

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

Will do.

Quoted 17 lines
Quoted 6 lines
Documentation/netlink/specs/net_shaper.yaml new file +5 -0
+    render-max: true
+    entries:
+      - name: unspec
+        doc: The scope is not specified.
+      -

Documentation/netlink/specs/net_shaper.yaml line 42

Quoted 10 lines
Documentation/netlink/specs/net_shaper.yaml new file +10 -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: node
+        doc: |
+             The shaper allows grouping of queues or others
+             node shapers, is not attached to any user-visible
Quoted 2 lines

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

What about:

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

Quoted 57 lines
Quoted 17 lines
Documentation/netlink/specs/net_shaper.yaml new file +15 -0
+             network device component, and can be nested to
+             either @netdev shapers or other @node shapers.
 
+attribute-sets:
+  -
+    name: net-shaper
+    attributes:
+      -
+        name: handle
+        type: nest
+        nested-attributes: handle
+        doc: Unique identifier for the given shaper inside the owning device.
+      -
+        name: info
+        type: nest
+        nested-attributes: info

Documentation/netlink/specs/net_shaper.yaml line 79

Quoted 10 lines
Documentation/netlink/specs/net_shaper.yaml new file +10 -0
+        doc: Fully describes the shaper.
+      -
+        name: metric
+        type: u32
+        enum: metric
+        doc: Metric used by the given shaper for bw-min, bw-max and burst.
+      -
+        name: bw-min
+        type: uint
+        doc: Minimum guaranteed B/W for the given shaper.

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

Documentation/netlink/specs/net_shaper.yaml line 83

Quoted 4 lines
Documentation/netlink/specs/net_shaper.yaml new file +4 -0
+      -
+        name: bw-max
+        type: uint
+        doc: Shaping B/W for the given shaper or 0 when unlimited.

s/Shaping/Maximum/

Documentation/netlink/specs/net_shaper.yaml line 87

Quoted 4 lines
Documentation/netlink/specs/net_shaper.yaml new file +4 -0
+      -
+        name: burst
+        type: uint
+        doc: Maximum burst-size for bw-min and bw-max.

How about:

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

? >

Documentation/netlink/specs/net_shaper.yaml line 91

Quoted 4 lines
Documentation/netlink/specs/net_shaper.yaml new file +4 -0
+      -
+        name: priority
+        type: u32
+        doc: Scheduling priority for the given shaper.

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

Quoted 7 lines

Documentation/netlink/specs/net_shaper.yaml line 96

Quoted 5 lines
Documentation/netlink/specs/net_shaper.yaml new file +5 -0
+      -
+        name: weight
+        type: u32
+        doc: |
+          Weighted round robin weight for given shaper.

Relative weight of the input into a round robin node.

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

Quoted 18 lines

?

Documentation/netlink/specs/net_shaper.yaml line 103

Quoted 7 lines
Documentation/netlink/specs/net_shaper.yaml new file +7 -0
+          The scheduling is applied to all the sibling
+          shapers with the same priority.
+      -
+        name: scope
+        type: u32
+        enum: scope
+        doc: The given shaper scope.

:)

Documentation/netlink/specs/net_shaper.yaml

Quoted 5 lines
Documentation/netlink/specs/net_shaper.yaml new file +5 -0
+      -
+        name: id
+        type: u32
+        doc: |
+          The given shaper id.
Quoted 5 lines

"Numeric identifier of a shaper."

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

Will do in the next revision.

Quoted 14 lines

Documentation/netlink/specs/net_shaper.yaml line 135

Quoted 13 lines
Documentation/netlink/specs/net_shaper.yaml new file +13 -0
+      -
+        name: leaves
+        type: nest
+        multi-attr: true
+        nested-attributes: info
+        doc: |
+           Describes a set of leaves shapers for a @group operation.
+      -
+        name: root
+        type: nest
+        nested-attributes: root-info
+        doc: |
+           Describes the root shaper for a @group operation
Quoted 6 lines

missing full stop

Documentation/netlink/specs/net_shaper.yaml line 137

Quoted 2 lines
Documentation/netlink/specs/net_shaper.yaml new file +2 -0
+           Differently from @leaves and @shaper allow specifying
+           the shaper parent handle, too.

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

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

Quoted 7 lines

Documentation/netlink/specs/net_shaper.yaml line 143

Quoted 6 lines
Documentation/netlink/specs/net_shaper.yaml new file +6 -0
+      -
+        name: shaper
+        type: nest
+        nested-attributes: info
+        doc: |
+           Describes a single shaper for a @set operation.
Quoted 7 lines

Hm. How is this different than "info"?

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

is "info" supposed to be used?

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

Quoted 10 lines

Documentation/netlink/specs/net_shaper.yaml line 5

Quoted 7 lines

+++ b/net/shaper/Makefile

Documentation/netlink/specs/net_shaper.yaml new file +5 -0
@@ -0,0 +1,9 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Makefile for the Generic HANDSHAKE service
+#
+# Copyright (c) 2024, Red Hat, Inc.

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

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

Thanks,

Paolo

author maintainer
#5 lore raw

Re: [PATCH v4 net-next 02/12] netlink: spec: add shaper YAML spec

Quoted 18 lines · Paolo Abeni
Quoted 17 lines · Jakub Kicinski
Quoted 15 lines · On Tue, 20 Aug 2024 17:12:23 +0200 Paolo Abeni wrote:
Documentation/netlink/specs/net_shaper.yaml new file +15 -0
+    render-max: true
+    entries:
+      - name: unspec
+        doc: The scope is not specified.
+      -
+        name: netdev
+        doc: The main shaper for the given network device.
+      -
+        name: queue
+        doc: The shaper is attached to the given device queue.
+      -
+        name: node
+        doc: |
+             The shaper allows grouping of queues or others
+             node shapers, is not attached to any user-visible
Quoted 7 lines
Quoted 2 lines

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

What about:

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

To rephrase the whole doc:

The shaper allows grouping of queues or others node shapers; can be nested to either @netdev shapers or other @node shapers, allowing placement in any arbitrary location of the scheduling tree, except leaves and root.

/P

maintainer
#6 lore raw

Re: [PATCH v4 net-next 02/12] netlink: spec: add shaper YAML spec

In reply to Paolo Abeni
Quoted 10 lines · On Fri, 23 Aug 2024 10:35:05 +0200 Paolo Abeni wrote:
Quoted 7 lines

deleted -> deleted / reset

Quoted 1 line

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

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

How about re-phrasing the previous paragraph as:

Quoted 7 lines

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

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

s/that can be placed in an arbitrary location of/which is an inner node/

So:

Depending on the @scope value, the shapers are attached to specific HW objects (queues, devices) or, for @node scope, represent a scheduling group which is an inner node the scheduling tree with multiple inputs.

Quoted 21 lines

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

Quoted 16 lines
Quoted 15 lines
Documentation/netlink/specs/net_shaper.yaml new file +15 -0
+    render-max: true
+    entries:
+      - name: unspec
+        doc: The scope is not specified.
+      -
+        name: netdev
+        doc: The main shaper for the given network device.
+      -
+        name: queue
+        doc: The shaper is attached to the given device queue.
+      -
+        name: node
+        doc: |
+             The shaper allows grouping of queues or others
+             node shapers, is not attached to any user-visible  
Quoted 7 lines
Quoted 2 lines

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

What about:

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

Oh, I was thinking along the same lines above.
Whether "except leaves or root" or "inner node" is clearer is up to you.

Quoted 10 lines
Quoted 7 lines

Documentation/netlink/specs/net_shaper.yaml line 96

Quoted 5 lines
Documentation/netlink/specs/net_shaper.yaml new file +5 -0
+      -
+        name: weight
+        type: u32
+        doc: |
+          Weighted round robin weight for given shaper.  

Relative weight of the input into a round robin node.

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

Okay, how about:

Relative weight used by a parent round robin node.

Quoted 7 lines
Quoted 4 lines

Documentation/netlink/specs/net_shaper.yaml line 137

Quoted 2 lines
Documentation/netlink/specs/net_shaper.yaml new file +2 -0
+           Differently from @leaves and @shaper allow specifying
+           the shaper parent handle, too.  

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

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

But to be clear, the "root" describes the node we're creating, right? Alternatively maybe we should remove the level of nesting and let the attributes live at the command level?

author maintainer
#7 lore raw

Re: [PATCH v4 net-next 02/12] netlink: spec: add shaper YAML spec

Quoted 9 lines · Jakub Kicinski
Quoted 8 lines · On Fri, 23 Aug 2024 10:35:05 +0200 Paolo Abeni wrote:
Quoted 7 lines

Documentation/netlink/specs/net_shaper.yaml line 42

Quoted 4 lines
Documentation/netlink/specs/net_shaper.yaml new file +4 -0
+        name: node
+        doc: |
+             The shaper allows grouping of queues or others
+             node shapers, is not attached to any user-visible

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

Quoted 7 lines
Quoted 4 lines

What about:

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

Oh, I was thinking along the same lines above.
Whether "except leaves or root" or "inner node" is clearer is up to you.

I agree "inner node" should be clear.

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
Documentation/netlink/specs/net_shaper.yaml new file +5 -0
+      -
+        name: weight
+        type: u32
+        doc: |
+          Weighted round robin weight for given shaper.
Quoted 8 lines
Quoted 4 lines
Quoted 1 line

Relative weight of the input into a round robin node.

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

Okay, how about:

Relative weight used by a parent round robin node.

Fine by me.

Quoted 9 lines
Quoted 7 lines
Quoted 4 lines

Documentation/netlink/specs/net_shaper.yaml line 137

Quoted 2 lines
Documentation/netlink/specs/net_shaper.yaml new file +2 -0
+           Differently from @leaves and @shaper allow specifying
+           the shaper parent handle, too.

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

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

But to be clear, the "root" describes the node we're creating, right?

Yes. I guess the possible confusion I mentioned will not be so confusing, after all. I'll go with this option.

Thanks,

Paolo

maintainer
#8 lore raw

Re: [PATCH v4 net-next 02/12] netlink: spec: add shaper YAML spec

In reply to Paolo Abeni

Documentation/netlink/specs/net_shaper.yaml line 247

Quoted 3 lines · On Tue, 20 Aug 2024 17:12:23 +0200 Paolo Abeni wrote:
Documentation/netlink/specs/net_shaper.yaml new file +3 -0
+        Clear (remove) the specified shaper. When deleting
+        a @node shaper, relink all the node's leaves to the
+        deleted node parent.

I think you should explain the other case too.
When deleting a queue shaper the shaper disappears from the hierarchy but the queue can still send traffic. It has an implicit node with infinite bandwidth and it feeds an implicit RR node at the root of the hierarchy.

author maintainer
#9 lore raw

PATCH v4 3/12 net-shapers: implement NL get operation

In reply to Paolo Abeni

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.

---
v3 -> v4:
- add scope prefix
- use forward declaration in the include
- move the handle out of shaper_info

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

---
 Documentation/networking/kapi.rst |   3 +
 include/linux/netdevice.h         |  17 ++
 include/net/net_shaper.h          | 105 +++++++++++
 net/core/dev.c                    |   2 +
 net/core/dev.h                    |   6 +
 net/shaper/shaper.c               | 297 +++++++++++++++++++++++++++++-
 6 files changed, 427 insertions(+), 3 deletions(-)
 create mode 100644 include/net/net_shaper.h
Documentation/networking/kapi.rst +3 -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
+   :internal:
+
 PHY Support
 -----------
 
include/linux/netdevice.h +17 -0
index 0ef3eaa23f4b..7b0b3b8fa927 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
 };
 
 /**
@@ -2376,6 +2386,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 +105 -0

105 lines not on this page · raw message

net/core/dev.c +2 -0
index e7260889d4cb..19e236870c32 100644
@@ -11178,6 +11178,8 @@ void free_netdev(struct net_device *dev)
 	/* Flush device addresses */
 	dev_addr_flush(dev);
 
+	net_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..8e43496ba53b 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 net_shaper_flush(struct net_device *dev);
+#else
+static inline void net_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 +294 -3

330 lines not on this page · raw message

Signature · 1 line
2.45.2
maintainer
#10 lore raw

Re: [PATCH v4 net-next 03/12] net-shapers: implement NL get operation

include/linux/netdevice.h struct xdp_frame; · line 85

Quoted 8 lines · On Tue, 20 Aug 2024 17:12:24 +0200 Paolo Abeni wrote:
include/linux/netdevice.h +2 -0
@@ -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;

no need, forward declarations are only needed for function declarations

include/linux/netdevice.h struct xdp_frame;

Quoted 9 lines
include/linux/netdevice.h +9 -0
+ * struct net_shaper_ops - Operations on device H/W shapers
+ *
+ * The initial shaping configuration at device initialization is empty:
+ * does not constraint the rate in any way.
+ * The network core keeps track of the applied user-configuration in
+ * the net_device structure.
+ * The operations are serialized via a per network device lock.
+ *
+ * Each shaper is uniquely identified within the device with an 'handle'

a handle

include/linux/netdevice.h

Quoted 13 lines

+ * comprising the shaper scope and a scope-specific id.
+ */

include/linux/netdevice.h +11 -0
+struct net_shaper_ops {
+	/**
+	 * @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.

Or SCOPE_NODE, no?

include/linux/netdevice.h

Quoted 2 lines
include/linux/netdevice.h +2 -0
+	 * Returns 0 on group successfully created, otherwise an negative
+	 * error value and set @extack to describe the failure's reason.

the return and extack lines are pretty obvious, you can drop

include/linux/netdevice.h

Quoted 10 lines
include/linux/netdevice.h +9 -0
+	 */
+	int (*group)(struct net_device *dev, 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);
 
+#endif
+

ooh, here's one of the trailing whitespace git was mentioning :)

include/linux/netdevice.h

Quoted 9 lines

#include <linux/kernel.h>

include/linux/netdevice.h +7 -0
+#include <linux/bits.h>
+#include <linux/bitfield.h>
+#include <linux/idr.h>
+#include <linux/netdevice.h>
+#include <linux/netlink.h>
 #include <linux/skbuff.h>
+#include <linux/xarray.h>
+#include <net/net_shaper.h>

kernel.h between idr.h and netdevice.h

include/linux/netdevice.h

Quoted 8 lines
include/linux/netdevice.h +8 -0
+static int net_shaper_fill_handle(struct sk_buff *msg,
+				  const struct net_shaper_handle *handle,
+				  u32 type, const struct genl_info *info)
+{
+	struct nlattr *handle_attr;
+
+	if (handle->scope == NET_SHAPER_SCOPE_UNSPEC)
+		return 0;

In what context can we try to fill handle with scope unspec?

include/linux/netdevice.h

Quoted 8 lines
include/linux/netdevice.h +8 -0
+	handle_attr = nla_nest_start_noflag(msg, type);
+	if (!handle_attr)
+		return -EMSGSIZE;
+
+	if (nla_put_u32(msg, NET_SHAPER_A_SCOPE, handle->scope) ||
+	    (handle->scope >= NET_SHAPER_SCOPE_QUEUE &&
+	     nla_put_u32(msg, NET_SHAPER_A_ID, handle->id)))
+		goto handle_nest_cancel;

So netdev root has no id and no scope?

Quoted 13 lines
include/linux/netdevice.h +12 -0
+	nla_nest_end(msg, handle_attr);
+	return 0;
+
+handle_nest_cancel:
+	nla_nest_cancel(msg, handle_attr);
+	return -EMSGSIZE;
+}
 
+/* On success sets pdev to the relevant device and acquires a reference
+ * to it.
+ */
+static int net_shaper_fetch_dev(const struct genl_info *info,
+				struct net_device **pdev)

include/linux/netdevice.h

Quoted 10 lines
include/linux/netdevice.h +10 -0
+{
+	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);

netdev_get_by_index()

include/linux/netdevice.h

Quoted 2 lines
include/linux/netdevice.h +2 -0
+	if (!dev) {
+		GENL_SET_ERR_MSG_FMT(info, "device %d not found", ifindex);

Point to the IFINDEX attribute, return -ENOENT. Please only use string errors when there's no way of expressing the error with machine readable attrs.

include/linux/netdevice.h

Quoted 6 lines
include/linux/netdevice.h +6 -0
+		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);

same as a above, point at device, -EOPNOTSUPP

include/linux/netdevice.h

Quoted 1 line
include/linux/netdevice.h +1 -0
+		netdev_put(dev, NULL);

I appears someone is coding to patchwork checks 🧐️

author maintainer
#11 lore raw

Re: [PATCH v4 net-next 03/12] net-shapers: implement NL get operation

Quoted 24 lines · Jakub Kicinski

include/linux/netdevice.h struct xdp_frame; · line 85

Quoted 8 lines · On Tue, 20 Aug 2024 17:12:24 +0200 Paolo Abeni wrote:
include/linux/netdevice.h +2 -0
@@ -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;

no need, forward declarations are only needed for function declarations

include/linux/netdevice.h struct xdp_frame;

Quoted 9 lines
include/linux/netdevice.h +9 -0
+ * struct net_shaper_ops - Operations on device H/W shapers
+ *
+ * The initial shaping configuration at device initialization is empty:
+ * does not constraint the rate in any way.
+ * The network core keeps track of the applied user-configuration in
+ * the net_device structure.
+ * The operations are serialized via a per network device lock.
+ *
+ * Each shaper is uniquely identified within the device with an 'handle'

a handle

Quoted 15 lines

include/linux/netdevice.h

Quoted 13 lines

+ * comprising the shaper scope and a scope-specific id.
+ */

include/linux/netdevice.h +11 -0
+struct net_shaper_ops {
+	/**
+	 * @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.

Or SCOPE_NODE, no?

I had a few back-and-forth between the two options, enforcing only QUEUE leaves or allowing even NODE.

I think the first option is general enough - can create arbitrary topologies with the same amount of operations - and leads to slightly simpler code, but no objections for allow both.

Quoted 31 lines

include/linux/netdevice.h

Quoted 3 lines
include/linux/netdevice.h +2 -0
+	 * Returns 0 on group successfully created, otherwise an negative
+	 * error value and set @extack to describe the failure's reason.

the return and extack lines are pretty obvious, you can drop

include/linux/netdevice.h

Quoted 10 lines
include/linux/netdevice.h +9 -0
+	 */
+	int (*group)(struct net_device *dev, 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);
 
+#endif
+

ooh, here's one of the trailing whitespace git was mentioning :)

include/linux/netdevice.h

Quoted 9 lines
  #include <linux/kernel.h>
include/linux/netdevice.h +7 -0
+#include <linux/bits.h>
+#include <linux/bitfield.h>
+#include <linux/idr.h>
+#include <linux/netdevice.h>
+#include <linux/netlink.h>
  #include <linux/skbuff.h>
+#include <linux/xarray.h>
+#include <net/net_shaper.h>

kernel.h between idr.h and netdevice.h

Quoted 10 lines

include/linux/netdevice.h

Quoted 8 lines
include/linux/netdevice.h +8 -0
+static int net_shaper_fill_handle(struct sk_buff *msg,
+				  const struct net_shaper_handle *handle,
+				  u32 type, const struct genl_info *info)
+{
+	struct nlattr *handle_attr;
+
+	if (handle->scope == NET_SHAPER_SCOPE_UNSPEC)
+		return 0;

In what context can we try to fill handle with scope unspec?

Uhmm... should happen only in buggy situation. What about adding adding WARN_ON_ONCE() ?

Quoted 10 lines

include/linux/netdevice.h

Quoted 8 lines
include/linux/netdevice.h +8 -0
+	handle_attr = nla_nest_start_noflag(msg, type);
+	if (!handle_attr)
+		return -EMSGSIZE;
+
+	if (nla_put_u32(msg, NET_SHAPER_A_SCOPE, handle->scope) ||
+	    (handle->scope >= NET_SHAPER_SCOPE_QUEUE &&
+	     nla_put_u32(msg, NET_SHAPER_A_ID, handle->id)))
+		goto handle_nest_cancel;

So netdev root has no id and no scope?

I don't understand the question.

The root handle has scope NETDEV and id 0, the id will not printed out as redundant: there is only a scope NETDEV shaper per struct net_device.

Thanks,

Paolo

maintainer
#12 lore raw

Re: [PATCH v4 net-next 03/12] net-shapers: implement NL get operation

Quoted 16 lines · On Fri, 23 Aug 2024 10:52:04 +0200 Paolo Abeni wrote:
Quoted 7 lines
Quoted 7 lines

+ * comprising the shaper scope and a scope-specific id.
+ */

include/net/net_shaper.h new file +5 -0
+struct net_shaper_ops {
+	/**
+	 * @group: create the specified shapers scheduling group
+	 *
+	 * Nest the @leaves shapers identified by @leaves_handles under the
Quoted 8 lines

include/net/net_shaper.h line 64

Quoted 6 lines
include/net/net_shaper.h new file +6 -0
+	 * @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.  

Or SCOPE_NODE, no?

Quoted 6 lines

I had a few back-and-forth between the two options, enforcing only QUEUE leaves or allowing even NODE.

I think the first option is general enough - can create arbitrary topologies with the same amount of operations - and leads to slightly simpler code, but no objections for allow both.

Ah, so we can only "grow the tree from the side of the leaves", so to speak? We can't create a group in the middle of the hierarchy? I have no strong use for groups in between, maybe just mention in a comment or cover letter.

Quoted 11 lines
Quoted 10 lines

net/shaper/shaper.c line 38

Quoted 8 lines
net/shaper/shaper.c +8 -0
+static int net_shaper_fill_handle(struct sk_buff *msg,
+				  const struct net_shaper_handle *handle,
+				  u32 type, const struct genl_info *info)
+{
+	struct nlattr *handle_attr;
+
+	if (handle->scope == NET_SHAPER_SCOPE_UNSPEC)
+		return 0;  

In what context can we try to fill handle with scope unspec?

Quoted 2 lines

Uhmm... should happen only in buggy situation. What about adding adding WARN_ON_ONCE() ?

That's better, at least it will express that it's not expected.

Quoted 11 lines
Quoted 10 lines

net/shaper/shaper.c line 47

Quoted 8 lines
net/shaper/shaper.c +8 -0
+	handle_attr = nla_nest_start_noflag(msg, type);
+	if (!handle_attr)
+		return -EMSGSIZE;
+
+	if (nla_put_u32(msg, NET_SHAPER_A_SCOPE, handle->scope) ||
+	    (handle->scope >= NET_SHAPER_SCOPE_QUEUE &&
+	     nla_put_u32(msg, NET_SHAPER_A_ID, handle->id)))
+		goto handle_nest_cancel;  

So netdev root has no id and no scope?

Quoted 4 lines

I don't understand the question.

The root handle has scope NETDEV and id 0, the id will not printed out as redundant: there is only a scope NETDEV shaper per struct net_device.

Misread, yes, no id but it does have scope. That's fine, sorry.

author maintainer
#13 lore raw

Re: [PATCH v4 net-next 03/12] net-shapers: implement NL get operation

Quoted 20 lines · Jakub Kicinski
Quoted 19 lines · On Fri, 23 Aug 2024 10:52:04 +0200 Paolo Abeni wrote:
Quoted 7 lines
Quoted 7 lines

+ * comprising the shaper scope and a scope-specific id.
+ */

include/net/net_shaper.h new file +5 -0
+struct net_shaper_ops {
+	/**
+	 * @group: create the specified shapers scheduling group
+	 *
+	 * Nest the @leaves shapers identified by @leaves_handles under the
Quoted 8 lines

include/net/net_shaper.h line 64

Quoted 6 lines
include/net/net_shaper.h new file +6 -0
+	 * @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.

Or SCOPE_NODE, no?

I had a few back-and-forth between the two options, enforcing only QUEUE leaves or allowing even NODE.

Quoted 6 lines
Quoted 3 lines

I think the first option is general enough - can create arbitrary topologies with the same amount of operations - and leads to slightly simpler code, but no objections for allow both.

Ah, so we can only "grow the tree from the side of the leaves", so to speak? We can't create a group in the middle of the hierarchy?

With the posted code, we can't. It can be implemented, but I think it will make the interface confusing.

Quoted 2 lines

I have no strong use for groups in between, maybe just mention in a comment or cover letter.

I'll do, thanks.

Quoted 12 lines
Quoted 12 lines
Quoted 11 lines

net/shaper/shaper.c line 38

Quoted 9 lines
net/shaper/shaper.c +8 -0
+static int net_shaper_fill_handle(struct sk_buff *msg,
+				  const struct net_shaper_handle *handle,
+				  u32 type, const struct genl_info *info)
+{
+	struct nlattr *handle_attr;
+
+	if (handle->scope == NET_SHAPER_SCOPE_UNSPEC)
+		return 0;

In what context can we try to fill handle with scope unspec?

Quoted 4 lines
Quoted 2 lines

Uhmm... should happen only in buggy situation. What about adding adding WARN_ON_ONCE() ?

That's better, at least it will express that it's not expected.

I added the WARN in my local build, and that reminded me the tree root (netdev) has UNSPEC parent. So I think we are better off with simply a comment there.

Thanks,

Paolo

author maintainer
#14 lore raw

PATCH v4 4/12 net-shapers: implement NL set and delete operations

In reply to Paolo Abeni

Both NL operations directly map on the homonymous device shaper callbacks, update accordingly the shapers cache and are serialized via a per device lock.
Implement the cache modification helpers to additionally deal with NODE scope shaper. That will be needed by the group() operation implemented in the next patch.
The delete implementation is partial: does not handle NODE scope shaper yet. Such support will require infrastructure from ithe next patch and will be implemented later in the series.

---
v3 -> v3:
- add locking
- helper rename

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

---
 net/shaper/shaper.c | 363 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 359 insertions(+), 4 deletions(-)
net/shaper/shaper.c +359 -4
index 723f0c5ec479..055fda39176b 100644
@@ -22,6 +22,10 @@
 
 struct net_shaper_data {
 	struct xarray shapers;
+
+	/* Serialize write ops and protects node_ids updates. */
+	struct mutex lock;
+	struct idr node_ids;
 };
 
 struct net_shaper_nl_ctx {
@@ -137,6 +141,26 @@ static void net_shaper_index_to_handle(u32 index,
 	handle->id = FIELD_GET(NET_SHAPER_ID_MASK, index);
 }
 
+static void net_shaper_default_parent(const struct net_shaper_handle *handle,
+				      struct net_shaper_handle *parent)
+{
+	switch (handle->scope) {
+	case NET_SHAPER_SCOPE_UNSPEC:
+	case NET_SHAPER_SCOPE_NETDEV:
+	case __NET_SHAPER_SCOPE_MAX:
+		parent->scope = NET_SHAPER_SCOPE_UNSPEC;
+		break;
+
+	case NET_SHAPER_SCOPE_QUEUE:
+	case NET_SHAPER_SCOPE_NODE:
+		parent->scope = NET_SHAPER_SCOPE_NETDEV;
+		break;
+	}
+	parent->id = 0;
+}
+
+#define NET_SHAPER_CACHE_NOT_VALID XA_MARK_0
+
 static struct xarray *net_shaper_cache_container(struct net_device *dev)
 {
 	/* The barrier pairs with cmpxchg on init. */
@@ -145,6 +169,11 @@ static struct xarray *net_shaper_cache_container(struct net_device *dev)
 	return data ? &data->shapers : NULL;
 }
 
+static struct mutex *net_shaper_cache_lock(struct net_device *dev)
+{
+	return dev->net_shaper_data ? &dev->net_shaper_data->lock : NULL;
+}
+
 /* Lookup the given shaper inside the cache. */
 static struct net_shaper_info *
 net_shaper_cache_lookup(struct net_device *dev,
@@ -153,7 +182,128 @@ net_shaper_cache_lookup(struct net_device *dev,
 	struct xarray *xa = net_shaper_cache_container(dev);
 	u32 index = net_shaper_handle_to_index(handle);
 
-	return xa ? xa_load(xa, index) : NULL;
+	if (!xa || xa_get_mark(xa, index, NET_SHAPER_CACHE_NOT_VALID))
+		return NULL;
+
+	return xa_load(xa, index);
+}
+
+/* Allocate on demand the per device shaper's cache. */
+static struct mutex *net_shaper_cache_init(struct net_device *dev,
+					   struct netlink_ext_ack *extack)
+{
+	struct net_shaper_data *new, *data = READ_ONCE(dev->net_shaper_data);
+
+	if (!data) {
+		new = kmalloc(sizeof(*dev->net_shaper_data), GFP_KERNEL);
+		if (!new) {
+			NL_SET_ERR_MSG(extack, "Can't allocate memory for shaper data");
+			return NULL;
+		}
+
+		mutex_init(&new->lock);
+		xa_init(&new->shapers);
+		idr_init(&new->node_ids);
+
+		/* No lock acquired yet, we can race with other operations. */
+		data = cmpxchg(&dev->net_shaper_data, NULL, new);
+		if (!data)
+			data = new;
+		else
+			kfree(new);
+	}
+	return &data->lock;
+}
+
+/* Prepare the cache to actually insert the given shaper, doing
+ * in advance the needed allocations.
+ */
+static int net_shaper_cache_pre_insert(struct net_device *dev,
+				       struct net_shaper_handle *handle,
+				       struct netlink_ext_ack *extack)
+{
+	struct xarray *xa = net_shaper_cache_container(dev);
+	struct net_shaper_info *prev, *cur;
+	bool id_allocated = false;
+	int ret, id, index;
+
+	if (!xa)
+		return -ENOMEM;
+
+	index = net_shaper_handle_to_index(handle);
+	cur = xa_load(xa, index);
+	if (cur)
+		return 0;
+
+	/* Allocated a new id, if needed. */
+	if (handle->scope == NET_SHAPER_SCOPE_NODE &&
+	    handle->id == NET_SHAPER_ID_UNSPEC) {
+		id = idr_alloc(&dev->net_shaper_data->node_ids, NULL,
+			       0, NET_SHAPER_ID_UNSPEC, GFP_ATOMIC);
+
+		if (id < 0) {
+			NL_SET_ERR_MSG(extack, "Can't allocate new id for NODE shaper");
+			return id;
+		}
+
+		handle->id = id;
+		index = net_shaper_handle_to_index(handle);
+		id_allocated = true;
+	}
+
+	cur = kmalloc(sizeof(*cur), GFP_KERNEL | __GFP_ZERO);
+	if (!cur) {
+		NL_SET_ERR_MSG(extack, "Can't allocate memory for cached shaper");
+		ret = -ENOMEM;
+		goto free_id;
+	}
+
+	/* Mark 'tentative' shaper inside the cache. */
+	xa_lock(xa);
+	prev = __xa_store(xa, index, cur, GFP_KERNEL);
+	__xa_set_mark(xa, index, NET_SHAPER_CACHE_NOT_VALID);
+	xa_unlock(xa);
+	if (xa_err(prev)) {
+		NL_SET_ERR_MSG(extack, "Can't insert shaper into cache");
+		kfree(cur);
+		ret = xa_err(prev);
+		goto free_id;
+	}
+	return 0;
+
+free_id:
+	if (id_allocated)
+		idr_remove(&dev->net_shaper_data->node_ids, handle->id);
+	return ret;
+}
+
+/* Commit the tentative insert with the actual values.
+ * Must be called only after a successful net_shaper_pre_insert().
+ */
+static void net_shaper_cache_commit(struct net_device *dev, int nr_shapers,
+				    const struct net_shaper_handle *handle,
+				    const struct net_shaper_info *shapers)
+{
+	struct xarray *xa = net_shaper_cache_container(dev);
+	struct net_shaper_info *cur;
+	int index;
+	int i;
+
+	xa_lock(xa);
+	for (i = 0; i < nr_shapers; ++i) {
+		index = net_shaper_handle_to_index(&handle[i]);
+
+		cur = xa_load(xa, index);
+		if (WARN_ON_ONCE(!cur))
+			continue;
+
+		/* Successful update: drop the tentative mark
+		 * and update the cache.
+		 */
+		__xa_clear_mark(xa, index, NET_SHAPER_CACHE_NOT_VALID);
+		*cur = shapers[i];
+	}
+	xa_unlock(xa);
 }
 
 static int net_shaper_parse_handle(const struct nlattr *attr,
@@ -193,6 +343,71 @@ static int net_shaper_parse_handle(const struct nlattr *attr,
 	return 0;
 }
 
+static int net_shaper_parse_info(struct net_device *dev, struct nlattr **tb,
+				 const struct genl_info *info,
+				 struct net_shaper_handle *handle,
+				 struct net_shaper_info *shaper)
+{
+	struct net_shaper_info *old;
+	int ret;
+
+	/* The shaper handle is the only mandatory attribute. */
+	if (NL_REQ_ATTR_CHECK(info->extack, NULL, tb, NET_SHAPER_A_HANDLE))
+		return -EINVAL;
+
+	ret = net_shaper_parse_handle(tb[NET_SHAPER_A_HANDLE], info, handle);
+	if (ret)
+		return ret;
+
+	/* Fetch existing data, if any, so that user provide info will
+	 * incrementally update the existing shaper configuration.
+	 */
+	old = net_shaper_cache_lookup(dev, handle);
+	if (old)
+		*shaper = *old;
+	else
+		net_shaper_default_parent(handle, &shaper->parent);
+
+	if (tb[NET_SHAPER_A_METRIC])
+		shaper->metric = nla_get_u32(tb[NET_SHAPER_A_METRIC]);
+
+	if (tb[NET_SHAPER_A_BW_MIN])
+		shaper->bw_min = nla_get_uint(tb[NET_SHAPER_A_BW_MIN]);
+
+	if (tb[NET_SHAPER_A_BW_MAX])
+		shaper->bw_max = nla_get_uint(tb[NET_SHAPER_A_BW_MAX]);
+
+	if (tb[NET_SHAPER_A_BURST])
+		shaper->burst = nla_get_uint(tb[NET_SHAPER_A_BURST]);
+
+	if (tb[NET_SHAPER_A_PRIORITY])
+		shaper->priority = nla_get_u32(tb[NET_SHAPER_A_PRIORITY]);
+
+	if (tb[NET_SHAPER_A_WEIGHT])
+		shaper->weight = nla_get_u32(tb[NET_SHAPER_A_WEIGHT]);
+	return 0;
+}
+
+/* Fetch the cached shaper info and update them with the user-provided
+ * attributes.
+ */
+static int net_shaper_parse_info_nest(struct net_device *dev,
+				      const struct nlattr *attr,
+				      const struct genl_info *info,
+				      struct net_shaper_handle *handle,
+				      struct net_shaper_info *shaper)
+{
+	struct nlattr *tb[NET_SHAPER_A_WEIGHT + 1];
+	int ret;
+
+	ret = nla_parse_nested(tb, NET_SHAPER_A_WEIGHT, attr,
+			       net_shaper_info_nl_policy, info->extack);
+	if (ret < 0)
+		return ret;
+
+	return net_shaper_parse_info(dev, tb, info, handle, shaper);
+}
+
 int net_shaper_nl_pre_doit(const struct genl_split_ops *ops,
 			   struct sk_buff *skb, struct genl_info *info)
 {
@@ -295,14 +510,149 @@ int net_shaper_nl_get_dumpit(struct sk_buff *skb,
 	return ret;
 }
 
+/* Update the H/W and on success update the local cache, too. */
+static int net_shaper_set(struct net_device *dev,
+			  const struct net_shaper_handle *h,
+			  const struct net_shaper_info *shaper,
+			  struct netlink_ext_ack *extack)
+{
+	struct mutex *lock = net_shaper_cache_init(dev, extack);
+	struct net_shaper_handle handle = *h;
+	int ret;
+
+	if (!lock)
+		return -ENOMEM;
+
+	if (handle.scope == NET_SHAPER_SCOPE_UNSPEC) {
+		NL_SET_ERR_MSG_FMT(extack, "Can't set shaper with unspec scope");
+		return -EINVAL;
+	}
+
+	mutex_lock(lock);
+	if (handle.scope == NET_SHAPER_SCOPE_NODE &&
+	    net_shaper_cache_lookup(dev, &handle)) {
+		ret = -ENOENT;
+		goto unlock;
+	}
+
+	ret = net_shaper_cache_pre_insert(dev, &handle, extack);
+	if (ret)
+		goto unlock;
+
+	ret = dev->netdev_ops->net_shaper_ops->set(dev, &handle, shaper, extack);
+	net_shaper_cache_commit(dev, 1, &handle, shaper);
+
+unlock:
+	mutex_unlock(lock);
+	return ret;
+}
+
 int net_shaper_nl_set_doit(struct sk_buff *skb, struct genl_info *info)
 {
-	return -EOPNOTSUPP;
+	struct net_device *dev = info->user_ptr[0];
+	struct net_shaper_handle handle;
+	struct net_shaper_info shaper;
+	struct nlattr *attr;
+	int ret;
+
+	if (GENL_REQ_ATTR_CHECK(info, NET_SHAPER_A_SHAPER))
+		return -EINVAL;
+
+	attr = info->attrs[NET_SHAPER_A_SHAPER];
+	ret = net_shaper_parse_info_nest(dev, attr, info, &handle, &shaper);
+	if (ret)
+		return ret;
+
+	return net_shaper_set(dev, &handle, &shaper, info->extack);
+}
+
+static int __net_shaper_delete(struct net_device *dev,
+			       const struct net_shaper_handle *h,
+			       struct net_shaper_info *shaper,
+			       struct netlink_ext_ack *extack)
+{
+	struct net_shaper_handle parent_handle, handle = *h;
+	struct xarray *xa = net_shaper_cache_container(dev);
+	int ret;
+
+	/* Should never happen: we are under the cache lock, the cache
+	 * is already initialized.
+	 */
+	if (WARN_ON_ONCE(!xa))
+		return -EINVAL;
+
+again:
+	parent_handle = shaper->parent;
+
+	ret = dev->netdev_ops->net_shaper_ops->delete(dev, &handle, extack);
+	if (ret < 0)
+		return ret;
+
+	xa_erase(xa, net_shaper_handle_to_index(&handle));
+	if (handle.scope == NET_SHAPER_SCOPE_NODE)
+		idr_remove(&dev->net_shaper_data->node_ids, handle.id);
+	kfree(shaper);
+
+	/* Eventually delete the parent, if it is left over with no leaves. */
+	if (parent_handle.scope == NET_SHAPER_SCOPE_NODE) {
+		shaper = net_shaper_cache_lookup(dev, &parent_handle);
+		if (shaper && !--shaper->leaves) {
+			handle = parent_handle;
+			goto again;
+		}
+	}
+	return 0;
+}
+
+static int net_shaper_delete(struct net_device *dev,
+			     const struct net_shaper_handle *handle,
+			     struct netlink_ext_ack *extack)
+{
+	struct mutex *lock = net_shaper_cache_lock(dev);
+	struct net_shaper_info *shaper;
+	int ret;
+
+	/* The lock is null when the cache is not initialized, and thus
+	 * no shaper has been created yet.
+	 */
+	if (!lock)
+		return -ENOENT;
+
+	mutex_lock(lock);
+	shaper = net_shaper_cache_lookup(dev, handle);
+	if (!shaper) {
+		ret = -ENOENT;
+		goto unlock;
+	}
+
+	if (handle->scope == NET_SHAPER_SCOPE_NODE) {
+		/* TODO: implement support for scope NODE delete. */
+		ret = -EINVAL;
+		goto unlock;
+	}
+
+	ret = __net_shaper_delete(dev, handle, shaper, extack);
+
+unlock:
+	mutex_unlock(lock);
+	return ret;
 }
 
 int net_shaper_nl_delete_doit(struct sk_buff *skb, struct genl_info *info)
 {
-	return -EOPNOTSUPP;
+	struct net_device *dev = info->user_ptr[0];
+	struct net_shaper_handle handle;
+	int ret;
+
+	if (GENL_REQ_ATTR_CHECK(info, NET_SHAPER_A_HANDLE))
+		return -EINVAL;
+
+	ret = net_shaper_parse_handle(info->attrs[NET_SHAPER_A_HANDLE], info,
+				      &handle);
+	if (ret)
+		return ret;
+
+	return net_shaper_delete(dev, &handle, info->extack);
 }
 
 int net_shaper_nl_group_doit(struct sk_buff *skb, struct genl_info *info)
@@ -313,18 +663,23 @@ int net_shaper_nl_group_doit(struct sk_buff *skb, struct genl_info *info)
 void net_shaper_flush(struct net_device *dev)
 {
 	struct xarray *xa = net_shaper_cache_container(dev);
+	struct mutex *lock = net_shaper_cache_lock(dev);
 	struct net_shaper_info *cur;
 	unsigned long index;
 
-	if (!xa)
+	if (!xa || !lock)
 		return;
 
+	mutex_lock(lock);
 	xa_lock(xa);
 	xa_for_each(xa, index, cur) {
 		__xa_erase(xa, index);
 		kfree(cur);
 	}
 	xa_unlock(xa);
+	idr_destroy(&dev->net_shaper_data->node_ids);
+	mutex_unlock(lock);
+
 	kfree(dev->net_shaper_data);
 }
 
Signature · 1 line
2.45.2
author maintainer
#15 lore raw

PATCH v4 5/12 net-shapers: implement NL group operation

In reply to Paolo Abeni

Allow grouping multiple leaves shaper under the given root. The root and the leaves shapers are created, if needed, otherwise the existing shapers are re-linked as requested.

Try hard to pre-allocated the needed resources, to avoid non trivial H/W configuration rollbacks in case of any failure.

---
v3 -> v3:
- cleanup left-over scope node shaper after re-link, as needed
- add locking
- separate arguments for shaper handle

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

---
 net/shaper/shaper.c | 320 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 319 insertions(+), 1 deletion(-)
net/shaper/shaper.c +319 -1
index 055fda39176b..c4228f98b416 100644
@@ -32,6 +32,24 @@ struct net_shaper_nl_ctx {
 	u32 start_index;
 };
 
+/* Count the number of [multi] attributes of the given type. */
+static int net_shaper_list_len(struct genl_info *info, int type)
+{
+	struct nlattr *attr;
+	int rem, cnt = 0;
+
+	nla_for_each_attr_type(attr, type, genlmsg_data(info->genlhdr),
+			       genlmsg_len(info->genlhdr), rem)
+		cnt++;
+	return cnt;
+}
+
+static int net_shaper_handle_size(void)
+{
+	return nla_total_size(nla_total_size(sizeof(u32)) +
+			      nla_total_size(sizeof(u32)));
+}
+
 static int net_shaper_fill_handle(struct sk_buff *msg,
 				  const struct net_shaper_handle *handle,
 				  u32 type, const struct genl_info *info)
@@ -306,6 +324,28 @@ static void net_shaper_cache_commit(struct net_device *dev, int nr_shapers,
 	xa_unlock(xa);
 }
 
+/* Rollback all the tentative inserts from the shaper cache. */
+static void net_shaper_cache_rollback(struct net_device *dev)
+{
+	struct xarray *xa = net_shaper_cache_container(dev);
+	struct net_shaper_handle handle;
+	struct net_shaper_info *cur;
+	unsigned long index;
+
+	if (!xa)
+		return;
+
+	xa_lock(xa);
+	xa_for_each_marked(xa, index, cur, NET_SHAPER_CACHE_NOT_VALID) {
+		net_shaper_index_to_handle(index, &handle);
+		if (handle.scope == NET_SHAPER_SCOPE_NODE)
+			idr_remove(&dev->net_shaper_data->node_ids, handle.id);
+		__xa_erase(xa, index);
+		kfree(cur);
+	}
+	xa_unlock(xa);
+}
+
 static int net_shaper_parse_handle(const struct nlattr *attr,
 				   const struct genl_info *info,
 				   struct net_shaper_handle *handle)
@@ -408,6 +448,37 @@ static int net_shaper_parse_info_nest(struct net_device *dev,
 	return net_shaper_parse_info(dev, tb, info, handle, shaper);
 }
 
+/* Alike net_parse_shaper_info(), but additionally allow the user specifying
+ * the shaper's parent handle.
+ */
+static int net_shaper_parse_root(struct net_device *dev,
+				 const struct nlattr *attr,
+				 const struct genl_info *info,
+				 struct net_shaper_handle *handle,
+				 struct net_shaper_info *shaper)
+{
+	struct nlattr *tb[NET_SHAPER_A_PARENT + 1];
+	int ret;
+
+	ret = nla_parse_nested(tb, NET_SHAPER_A_PARENT, attr,
+			       net_shaper_root_info_nl_policy,
+			       info->extack);
+	if (ret < 0)
+		return ret;
+
+	ret = net_shaper_parse_info(dev, tb, info, handle, shaper);
+	if (ret)
+		return ret;
+
+	if (tb[NET_SHAPER_A_PARENT]) {
+		ret = net_shaper_parse_handle(tb[NET_SHAPER_A_PARENT], info,
+					      &shaper->parent);
+		if (ret)
+			return ret;
+	}
+	return 0;
+}
+
 int net_shaper_nl_pre_doit(const struct genl_split_ops *ops,
 			   struct sk_buff *skb, struct genl_info *info)
 {
@@ -604,6 +675,89 @@ static int __net_shaper_delete(struct net_device *dev,
 	return 0;
 }
 
+static int __net_shaper_group(struct net_device *dev, int leaves_count,
+			      const struct net_shaper_handle *leaves_handles,
+			      struct net_shaper_info *leaves,
+			      struct net_shaper_handle *root_handle,
+			      struct net_shaper_info *root,
+			      struct netlink_ext_ack *extack)
+{
+	struct net_shaper_info *parent = NULL;
+	struct net_shaper_handle leaf_handle;
+	int i, ret;
+
+	if (root_handle->scope == NET_SHAPER_SCOPE_NODE) {
+		if (root_handle->id != NET_SHAPER_ID_UNSPEC &&
+		    !net_shaper_cache_lookup(dev, root_handle)) {
+			NL_SET_ERR_MSG_FMT(extack, "Root shaper %d:%d does not exists",
+					   root_handle->scope, root_handle->id);
+			return -ENOENT;
+		}
+		if (root->parent.scope != NET_SHAPER_SCOPE_NODE &&
+		    root->parent.scope != NET_SHAPER_SCOPE_NETDEV) {
+			NL_SET_ERR_MSG_FMT(extack, "Invalid scope %d for root parent shaper",
+					   root->parent.scope);
+			return -EINVAL;
+		}
+	}
+
+	if (root->parent.scope == NET_SHAPER_SCOPE_NODE) {
+		parent = net_shaper_cache_lookup(dev, &root->parent);
+		if (!parent) {
+			NL_SET_ERR_MSG_FMT(extack, "Root parent shaper %d:%d does not exists",
+					   root->parent.scope, root->parent.id);
+			return -ENOENT;
+		}
+	}
+
+	/* For newly created node scope shaper, the following will update
+	 * the handle, due to id allocation.
+	 */
+	ret = net_shaper_cache_pre_insert(dev, root_handle, extack);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < leaves_count; ++i) {
+		leaf_handle = leaves_handles[i];
+		if (leaf_handle.scope != NET_SHAPER_SCOPE_QUEUE) {
+			ret = -EINVAL;
+			NL_SET_ERR_MSG_FMT(extack, "Invalid scope %d for leaf shaper %d",
+					   leaf_handle.scope, i);
+			goto rollback;
+		}
+
+		ret = net_shaper_cache_pre_insert(dev, &leaf_handle, extack);
+		if (ret)
+			goto rollback;
+
+		if (leaves[i].parent.scope == root_handle->scope &&
+		    leaves[i].parent.id == root_handle->id)
+			continue;
+
+		/* The leaves shapers will be nested to the root, update the
+		 * linking accordingly.
+		 */
+		leaves[i].parent = *root_handle;
+		root->leaves++;
+	}
+
+	ret = dev->netdev_ops->net_shaper_ops->group(dev, leaves_count,
+						     leaves_handles, leaves,
+						     root_handle, root,
+						     extack);
+	if (ret < 0)
+		goto rollback;
+
+	if (parent)
+		parent->leaves++;
+	net_shaper_cache_commit(dev, 1, root_handle, root);
+	net_shaper_cache_commit(dev, leaves_count, leaves_handles, leaves);
+	return 0;
+
+rollback:
+	net_shaper_cache_rollback(dev);
+	return ret;
+}
 static int net_shaper_delete(struct net_device *dev,
 			     const struct net_shaper_handle *handle,
 			     struct netlink_ext_ack *extack)
@@ -655,9 +809,173 @@ int net_shaper_nl_delete_doit(struct sk_buff *skb, struct genl_info *info)
 	return net_shaper_delete(dev, &handle, info->extack);
 }
 
+/* Update the H/W and on success update the local cache, too */
+static int net_shaper_group(struct net_device *dev, int leaves_count,
+			    const struct net_shaper_handle *leaves_handles,
+			    struct net_shaper_info *leaves,
+			    struct net_shaper_handle *root_handle,
+			    struct net_shaper_info *root,
+			    struct netlink_ext_ack *extack)
+{
+	struct mutex *lock = net_shaper_cache_init(dev, extack);
+	struct net_shaper_handle *old_roots;
+	int i, ret, old_roots_count = 0;
+
+	if (!lock)
+		return -ENOMEM;
+
+	if (root_handle->scope != NET_SHAPER_SCOPE_NODE &&
+	    root_handle->scope != NET_SHAPER_SCOPE_NETDEV) {
+		NL_SET_ERR_MSG_FMT(extack, "Invalid scope %d for root shaper",
+				   root_handle->scope);
+		return -EINVAL;
+	}
+
+	old_roots = kcalloc(leaves_count, sizeof(struct net_shaper_handle),
+			    GFP_KERNEL);
+	if (!old_roots)
+		return -ENOMEM;
+
+	for (i = 0; i < leaves_count; i++)
+		if (leaves[i].parent.scope == NET_SHAPER_SCOPE_NODE &&
+		    (leaves[i].parent.scope != root_handle->scope ||
+		     leaves[i].parent.id != root_handle->id))
+			old_roots[old_roots_count++] = leaves[i].parent;
+
+	mutex_lock(lock);
+	ret = __net_shaper_group(dev, leaves_count, leaves_handles,
+				 leaves, root_handle, root, extack);
+
+	/* Check if we need to delete any NODE left alone by the new leaves
+	 * linkage.
+	 */
+	for (i = 0; i < old_roots_count; ++i) {
+		root = net_shaper_cache_lookup(dev, &old_roots[i]);
+		if (!root)
+			continue;
+
+		if (--root->leaves > 0)
+			continue;
+
+		/* Errors here are not fatal: the grouping operation is
+		 * completed, and user-space can still explicitly clean-up
+		 * left-over nodes.
+		 */
+		__net_shaper_delete(dev, &old_roots[i], root, extack);
+	}
+
+	mutex_unlock(lock);
+
+	kfree(old_roots);
+	return ret;
+}
+
+static int net_shaper_group_send_reply(struct genl_info *info,
+				       struct net_shaper_handle *handle)
+{
+	struct net_device *dev = info->user_ptr[0];
+	struct nlattr *handle_attr;
+	struct sk_buff *msg;
+	int ret = -EMSGSIZE;
+	void *hdr;
+
+	/* Prepare the msg reply in advance, to avoid device operation
+	 * rollback.
+	 */
+	msg = genlmsg_new(net_shaper_handle_size(), GFP_KERNEL);
+	if (!msg)
+		return ret;
+
+	hdr = genlmsg_iput(msg, info);
+	if (!hdr)
+		goto free_msg;
+
+	if (nla_put_u32(msg, NET_SHAPER_A_IFINDEX, dev->ifindex))
+		goto free_msg;
+
+	handle_attr = nla_nest_start(msg, NET_SHAPER_A_HANDLE);
+	if (!handle_attr)
+		goto free_msg;
+
+	if (nla_put_u32(msg, NET_SHAPER_A_SCOPE, handle->scope))
+		goto free_msg;
+
+	if (nla_put_u32(msg, NET_SHAPER_A_ID, handle->id))
+		goto free_msg;
+
+	nla_nest_end(msg, handle_attr);
+	genlmsg_end(msg, hdr);
+
+	ret = genlmsg_reply(msg, info);
+	if (ret)
+		goto free_msg;
+
+	return ret;
+
+free_msg:
+	nlmsg_free(msg);
+	return ret;
+}
+
 int net_shaper_nl_group_doit(struct sk_buff *skb, struct genl_info *info)
 {
-	return -EOPNOTSUPP;
+	struct net_shaper_handle *leaves_handles, root_handle;
+	struct net_device *dev = info->user_ptr[0];
+	struct net_shaper_info *leaves, root;
+	int i, ret, rem, leaves_count;
+	struct nlattr *attr;
+
+	if (GENL_REQ_ATTR_CHECK(info, NET_SHAPER_A_LEAVES) ||
+	    GENL_REQ_ATTR_CHECK(info, NET_SHAPER_A_ROOT))
+		return -EINVAL;
+
+	leaves_count = net_shaper_list_len(info, NET_SHAPER_A_LEAVES);
+	leaves = kcalloc(leaves_count, sizeof(struct net_shaper_info) +
+			 sizeof(struct net_shaper_handle), GFP_KERNEL);
+	if (!leaves) {
+		GENL_SET_ERR_MSG_FMT(info, "Can't allocate memory for %d leaves shapers",
+				     leaves_count);
+		return -ENOMEM;
+	}
+	leaves_handles = (struct net_shaper_handle *)&leaves[leaves_count];
+
+	ret = net_shaper_parse_root(dev, info->attrs[NET_SHAPER_A_ROOT],
+				    info, &root_handle, &root);
+	if (ret)
+		goto free_shapers;
+
+	i = 0;
+	nla_for_each_attr_type(attr, NET_SHAPER_A_LEAVES,
+			       genlmsg_data(info->genlhdr),
+			       genlmsg_len(info->genlhdr), rem) {
+		if (WARN_ON_ONCE(i >= leaves_count))
+			goto free_shapers;
+
+		ret = net_shaper_parse_info_nest(dev, attr, info,
+						 &leaves_handles[i],
+						 &leaves[i]);
+		if (ret)
+			goto free_shapers;
+		i++;
+	}
+
+	ret = net_shaper_group(dev, leaves_count, leaves_handles, leaves,
+			       &root_handle, &root, info->extack);
+	if (ret < 0)
+		goto free_shapers;
+
+	ret = net_shaper_group_send_reply(info, &root_handle);
+	if (ret) {
+		/* Error on reply is not fatal to avoid rollback a successful
+		 * configuration.
+		 */
+		GENL_SET_ERR_MSG_FMT(info, "Can't send reply %d", ret);
+		ret = 0;
+	}
+
+free_shapers:
+	kfree(leaves);
+	return ret;
 }
 
 void net_shaper_flush(struct net_device *dev)
Signature · 1 line
2.45.2
author maintainer
#16 lore raw

PATCH v4 6/12 net-shapers: implement delete support for NODE scope shaper

In reply to Paolo Abeni

Leverage the previously introduced group operation to implement the removal of NODE scope shaper, re-linking its leaves under the the parent node before actually deleting the specified NODE scope shaper.

---
 net/shaper/shaper.c | 98 +++++++++++++++++++++++++++++++++++++++------
 1 file changed, 86 insertions(+), 12 deletions(-)
net/shaper/shaper.c +86 -12
index c4228f98b416..e5282c5bebe1 100644
@@ -675,7 +675,8 @@ static int __net_shaper_delete(struct net_device *dev,
 	return 0;
 }
 
-static int __net_shaper_group(struct net_device *dev, int leaves_count,
+static int __net_shaper_group(struct net_device *dev,
+			      bool cache_root, int leaves_count,
 			      const struct net_shaper_handle *leaves_handles,
 			      struct net_shaper_info *leaves,
 			      struct net_shaper_handle *root_handle,
@@ -710,12 +711,14 @@ static int __net_shaper_group(struct net_device *dev, int leaves_count,
 		}
 	}
 
-	/* For newly created node scope shaper, the following will update
-	 * the handle, due to id allocation.
-	 */
-	ret = net_shaper_cache_pre_insert(dev, root_handle, extack);
-	if (ret)
-		return ret;
+	if (cache_root) {
+		/* For newly created node scope shaper, the following will
+		 * update the handle, due to id allocation.
+		 */
+		ret = net_shaper_cache_pre_insert(dev, root_handle, extack);
+		if (ret)
+			return ret;
+	}
 
 	for (i = 0; i < leaves_count; ++i) {
 		leaf_handle = leaves_handles[i];
@@ -750,7 +753,8 @@ static int __net_shaper_group(struct net_device *dev, int leaves_count,
 
 	if (parent)
 		parent->leaves++;
-	net_shaper_cache_commit(dev, 1, root_handle, root);
+	if (cache_root)
+		net_shaper_cache_commit(dev, 1, root_handle, root);
 	net_shaper_cache_commit(dev, leaves_count, leaves_handles, leaves);
 	return 0;
 
@@ -758,6 +762,76 @@ static int __net_shaper_group(struct net_device *dev, int leaves_count,
 	net_shaper_cache_rollback(dev);
 	return ret;
 }
+
+static int __net_shaper_pre_del_node(struct net_device *dev,
+				     const struct net_shaper_handle *handle,
+				     const struct net_shaper_info *shaper,
+				     struct netlink_ext_ack *extack)
+{
+	struct net_shaper_handle *leaves_handles, root_handle;
+	struct xarray *xa = net_shaper_cache_container(dev);
+	struct net_shaper_info *cur, *leaves, root = {};
+	int ret, leaves_count = 0;
+	unsigned long index;
+	bool cache_root;
+
+	if (!shaper->leaves)
+		return 0;
+
+	if (WARN_ON_ONCE(!xa))
+		return -EINVAL;
+
+	/* Fetch the new root information. */
+	root_handle = shaper->parent;
+	cur = net_shaper_cache_lookup(dev, &root_handle);
+	if (cur) {
+		root = *cur;
+	} else {
+		/* A scope NODE shaper can be nested only to the NETDEV scope
+		 * shaper without creating the latter, this check may fail only
+		 * if the cache is in inconsistent status.
+		 */
+		if (WARN_ON_ONCE(root_handle.scope != NET_SHAPER_SCOPE_NETDEV))
+			return -EINVAL;
+	}
+
+	leaves = kcalloc(shaper->leaves,
+			 sizeof(struct net_shaper_info) +
+			 sizeof(struct net_shaper_handle), GFP_KERNEL);
+	if (!leaves)
+		return -ENOMEM;
+
+	leaves_handles = (struct net_shaper_handle *)&leaves[shaper->leaves];
+
+	/* Build the leaves arrays. */
+	xa_for_each(xa, index, cur) {
+		if (cur->parent.scope != handle->scope ||
+		    cur->parent.id != handle->id)
+			continue;
+
+		if (WARN_ON_ONCE(leaves_count == shaper->leaves)) {
+			ret = -EINVAL;
+			goto free;
+		}
+
+		net_shaper_index_to_handle(index,
+					   &leaves_handles[leaves_count]);
+		leaves[leaves_count++] = *cur;
+	}
+
+	/* When re-linking to the netdev shaper, avoid the eventual, implicit,
+	 * creation of the new root, would be surprising since the user is
+	 * doing a delete operation.
+	 */
+	cache_root = root_handle.scope != NET_SHAPER_SCOPE_NETDEV;
+	ret = __net_shaper_group(dev, cache_root, leaves_count, leaves_handles,
+				 leaves, &root_handle, &root, extack);
+
+free:
+	kfree(leaves);
+	return ret;
+}
+
 static int net_shaper_delete(struct net_device *dev,
 			     const struct net_shaper_handle *handle,
 			     struct netlink_ext_ack *extack)
@@ -780,9 +854,9 @@ static int net_shaper_delete(struct net_device *dev,
 	}
 
 	if (handle->scope == NET_SHAPER_SCOPE_NODE) {
-		/* TODO: implement support for scope NODE delete. */
-		ret = -EINVAL;
-		goto unlock;
+		ret = __net_shaper_pre_del_node(dev, handle, shaper, extack);
+		if (ret)
+			goto unlock;
 	}
 
 	ret = __net_shaper_delete(dev, handle, shaper, extack);
@@ -843,7 +917,7 @@ static int net_shaper_group(struct net_device *dev, int leaves_count,
 			old_roots[old_roots_count++] = leaves[i].parent;
 
 	mutex_lock(lock);
-	ret = __net_shaper_group(dev, leaves_count, leaves_handles,
+	ret = __net_shaper_group(dev, true, leaves_count, leaves_handles,
 				 leaves, root_handle, root, extack);
 
 	/* Check if we need to delete any NODE left alone by the new leaves
Signature · 1 line
2.45.2
author maintainer
#17 lore raw

PATCH v4 7/12 netlink: spec: add shaper introspection support

In reply to Paolo Abeni

Allow the user-space to fine-grain query the shaping features supported by the NIC on each domain.

---
 Documentation/netlink/specs/net_shaper.yaml | 84 +++++++++++++++++++++
 include/uapi/linux/net_shaper.h             | 17 +++++
 net/shaper/shaper.c                         | 22 ++++++
 net/shaper/shaper_nl_gen.c                  | 27 +++++++
 net/shaper/shaper_nl_gen.h                  |  8 ++
 5 files changed, 158 insertions(+)
Documentation/netlink/specs/net_shaper.yaml +84 -0
index a2b7900646ae..64d827176a61 100644
@@ -20,6 +20,11 @@ doc: |
 
   The user can query the running configuration via the @get operation.
 
+  Different devices can provide different feature set, e.g. with no
+  support for complex scheduling hierarchy, or for some shaping
+  parameters. The user can introspect the HW capabilities via the
+  @cap-get operation.
+
 definitions:
   -
     type: enum
@@ -187,6 +192,52 @@ attribute-sets:
         name: priority
       -
         name: weight
+  -
+    name: capabilities
+    attributes:
+      -
+        name: ifindex
+        type: u32
+      -
+        name: scope
+        type: u32
+        enum: scope
+        doc: The scope to which the queried capabilities apply.
+      -
+        name: support-metric-bps
+        type: flag
+        doc: The device accepts 'bps' metric for bw-min, bw-max and burst.
+      -
+        name: support-metric-pps
+        type: flag
+        doc: The device accepts 'pps' metric for bw-min, bw-max and burst.
+      -
+        name: support-nesting
+        type: flag
+        doc: |
+          The device supports nesting shaper belonging to this scope
+          below 'node' scoped shapers. Only 'queue' and 'node'
+          scope can have flag 'support-nesting'.
+      -
+        name: support-bw-min
+        type: flag
+        doc: The device supports a minimum guaranteed B/W.
+      -
+        name: support-bw-max
+        type: flag
+        doc: The device supports maximum B/W shaping.
+      -
+        name: support-burst
+        type: flag
+        doc: The device supports a maximum burst size.
+      -
+        name: support-priority
+        type: flag
+        doc: The device supports priority scheduling.
+      -
+        name: support-weight
+        type: flag
+        doc: The device supports weighted round robin scheduling.
 
 operations:
   list:
@@ -287,3 +338,36 @@ operations:
             - root
         reply:
           attributes: *ns-binding
+
+    -
+      name: cap-get
+      doc: |
+        Get / Dump the shaper capabilities supported by the given device.
+      attribute-set: capabilities
+
+      do:
+        pre: net-shaper-nl-cap-pre-doit
+        post: net-shaper-nl-cap-post-doit
+        request:
+          attributes:
+            - ifindex
+            - scope
+        reply:
+          attributes: &cap-attrs
+            - ifindex
+            - scope
+            - support-metric-bps
+            - support-metric-pps
+            - support-nesting
+            - support-bw-min
+            - support-bw-max
+            - support-burst
+            - support-priority
+            - support-weight
+
+      dump:
+        request:
+          attributes:
+            - ifindex
+        reply:
+          attributes: *cap-attrs
include/uapi/linux/net_shaper.h +17 -0
index 05917f10b021..70d9639f9345 100644
@@ -60,11 +60,28 @@ enum {
 	NET_SHAPER_A_MAX = (__NET_SHAPER_A_MAX - 1)
 };
 
+enum {
+	NET_SHAPER_A_CAPABILITIES_IFINDEX = 1,
+	NET_SHAPER_A_CAPABILITIES_SCOPE,
+	NET_SHAPER_A_CAPABILITIES_SUPPORT_METRIC_BPS,
+	NET_SHAPER_A_CAPABILITIES_SUPPORT_METRIC_PPS,
+	NET_SHAPER_A_CAPABILITIES_SUPPORT_NESTING,
+	NET_SHAPER_A_CAPABILITIES_SUPPORT_BW_MIN,
+	NET_SHAPER_A_CAPABILITIES_SUPPORT_BW_MAX,
+	NET_SHAPER_A_CAPABILITIES_SUPPORT_BURST,
+	NET_SHAPER_A_CAPABILITIES_SUPPORT_PRIORITY,
+	NET_SHAPER_A_CAPABILITIES_SUPPORT_WEIGHT,
+
+	__NET_SHAPER_A_CAPABILITIES_MAX,
+	NET_SHAPER_A_CAPABILITIES_MAX = (__NET_SHAPER_A_CAPABILITIES_MAX - 1)
+};
+
 enum {
 	NET_SHAPER_CMD_GET = 1,
 	NET_SHAPER_CMD_SET,
 	NET_SHAPER_CMD_DELETE,
 	NET_SHAPER_CMD_GROUP,
+	NET_SHAPER_CMD_CAP_GET,
 
 	__NET_SHAPER_CMD_MAX,
 	NET_SHAPER_CMD_MAX = (__NET_SHAPER_CMD_MAX - 1)
net/shaper/shaper.c +22 -0
index e5282c5bebe1..d3bb0ee1a18a 100644
@@ -501,6 +501,17 @@ void net_shaper_nl_post_doit(const struct genl_split_ops *ops,
 	netdev_put(dev, NULL);
 }
 
+int net_shaper_nl_cap_pre_doit(const struct genl_split_ops *ops,
+			       struct sk_buff *skb, struct genl_info *info)
+{
+	return -EOPNOTSUPP;
+}
+
+void net_shaper_nl_cap_post_doit(const struct genl_split_ops *ops,
+				 struct sk_buff *skb, struct genl_info *info)
+{
+}
+
 int net_shaper_nl_get_doit(struct sk_buff *skb, struct genl_info *info)
 {
 	struct net_device *dev = info->user_ptr[0];
@@ -1052,6 +1063,17 @@ int net_shaper_nl_group_doit(struct sk_buff *skb, struct genl_info *info)
 	return ret;
 }
 
+int net_shaper_nl_cap_get_doit(struct sk_buff *skb, struct genl_info *info)
+{
+	return 0;
+}
+
+int net_shaper_nl_cap_get_dumpit(struct sk_buff *skb,
+				 struct netlink_callback *cb)
+{
+	return 0;
+}
+
 void net_shaper_flush(struct net_device *dev)
 {
 	struct xarray *xa = net_shaper_cache_container(dev);
net/shaper/shaper_nl_gen.c +27 -0
index b0a4bdf1f00a..04f9cbf3aee0 100644
@@ -67,6 +67,17 @@ static const struct nla_policy net_shaper_group_nl_policy[NET_SHAPER_A_ROOT + 1]
 	[NET_SHAPER_A_ROOT] = NLA_POLICY_NESTED(net_shaper_root_info_nl_policy),
 };
 
+/* NET_SHAPER_CMD_CAP_GET - do */
+static const struct nla_policy net_shaper_cap_get_do_nl_policy[NET_SHAPER_A_CAPABILITIES_SCOPE + 1] = {
+	[NET_SHAPER_A_CAPABILITIES_IFINDEX] = { .type = NLA_U32, },
+	[NET_SHAPER_A_CAPABILITIES_SCOPE] = NLA_POLICY_MAX(NLA_U32, 3),
+};
+
+/* NET_SHAPER_CMD_CAP_GET - dump */
+static const struct nla_policy net_shaper_cap_get_dump_nl_policy[NET_SHAPER_A_CAPABILITIES_IFINDEX + 1] = {
+	[NET_SHAPER_A_CAPABILITIES_IFINDEX] = { .type = NLA_U32, },
+};
+
 /* Ops table for net_shaper */
 static const struct genl_split_ops net_shaper_nl_ops[] = {
 	{
@@ -112,6 +123,22 @@ static const struct genl_split_ops net_shaper_nl_ops[] = {
 		.maxattr	= NET_SHAPER_A_ROOT,
 		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
 	},
+	{
+		.cmd		= NET_SHAPER_CMD_CAP_GET,
+		.pre_doit	= net_shaper_nl_cap_pre_doit,
+		.doit		= net_shaper_nl_cap_get_doit,
+		.post_doit	= net_shaper_nl_cap_post_doit,
+		.policy		= net_shaper_cap_get_do_nl_policy,
+		.maxattr	= NET_SHAPER_A_CAPABILITIES_SCOPE,
+		.flags		= GENL_CMD_CAP_DO,
+	},
+	{
+		.cmd		= NET_SHAPER_CMD_CAP_GET,
+		.dumpit		= net_shaper_nl_cap_get_dumpit,
+		.policy		= net_shaper_cap_get_dump_nl_policy,
+		.maxattr	= NET_SHAPER_A_CAPABILITIES_IFINDEX,
+		.flags		= GENL_CMD_CAP_DUMP,
+	},
 };
 
 struct genl_family net_shaper_nl_family __ro_after_init = {
net/shaper/shaper_nl_gen.h +8 -0
index 9b0682c83a07..6cec8903f25b 100644
@@ -18,15 +18,23 @@ extern const struct nla_policy net_shaper_root_info_nl_policy[NET_SHAPER_A_PAREN
 
 int net_shaper_nl_pre_doit(const struct genl_split_ops *ops,
 			   struct sk_buff *skb, struct genl_info *info);
+int net_shaper_nl_cap_pre_doit(const struct genl_split_ops *ops,
+			       struct sk_buff *skb, struct genl_info *info);
 void
 net_shaper_nl_post_doit(const struct genl_split_ops *ops, struct sk_buff *skb,
 			struct genl_info *info);
+void
+net_shaper_nl_cap_post_doit(const struct genl_split_ops *ops,
+			    struct sk_buff *skb, struct genl_info *info);
 
 int net_shaper_nl_get_doit(struct sk_buff *skb, struct genl_info *info);
 int net_shaper_nl_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb);
 int net_shaper_nl_set_doit(struct sk_buff *skb, struct genl_info *info);
 int net_shaper_nl_delete_doit(struct sk_buff *skb, struct genl_info *info);
 int net_shaper_nl_group_doit(struct sk_buff *skb, struct genl_info *info);
+int net_shaper_nl_cap_get_doit(struct sk_buff *skb, struct genl_info *info);
+int net_shaper_nl_cap_get_dumpit(struct sk_buff *skb,
+				 struct netlink_callback *cb);
 
 extern struct genl_family net_shaper_nl_family;
 
Signature · 1 line
2.45.2
author maintainer
#18 lore raw

PATCH v4 8/12 net: shaper: implement introspection support

In reply to Paolo Abeni

The netlink op is a simple wrapper around the device callback.

Extend the existing fetch_dev() helper adding an attribute argument for the requested device. Reuse such helper in the newly implemented operation.

---
v3 -> v4:
- another dev_put() -> netdev_put() conversion, missed in previous iteration

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

---
 include/net/net_shaper.h |  11 ++++
 net/shaper/shaper.c      | 111 ++++++++++++++++++++++++++++++++++++---
 2 files changed, 115 insertions(+), 7 deletions(-)
include/net/net_shaper.h +11 -0
index bfb521d31c78..1610bc33d476 100644
@@ -99,6 +99,17 @@ struct net_shaper_ops {
 	int (*delete)(struct net_device *dev,
 		      const struct net_shaper_handle *handle,
 		      struct netlink_ext_ack *extack);
+
+	/**
+	 * @capabilities: get the shaper features supported by the NIC
+	 *
+	 * Fills the bitmask @cap with the supported cababilites for the
+	 * specified @scope and device @dev.
+	 *
+	 * Returns 0 on success or a negative error value otherwise.
+	 */
+	int (*capabilities)(struct net_device *dev,
+			    enum net_shaper_scope scope, unsigned long *cap);
 };
 
 #endif
net/shaper/shaper.c +104 -7

162 lines not on this page · raw message

Signature · 1 line
2.45.2
author maintainer
#19 lore raw

PATCH v4 9/12 testing: net-drv: add basic shaper test

In reply to Paolo Abeni

Leverage a basic/dummy netdevsim implementation to do functional coverage for NL interface.

---
rfc v1 -> v2:
- added more test-cases WRT nesting and grouping

---
 drivers/net/Kconfig                           |   1 +
 drivers/net/netdevsim/netdev.c                |  41 +++
 tools/testing/selftests/drivers/net/Makefile  |   1 +
 tools/testing/selftests/drivers/net/shaper.py | 236 ++++++++++++++++++
 .../testing/selftests/net/lib/py/__init__.py  |   1 +
 tools/testing/selftests/net/lib/py/ynl.py     |   5 +
 6 files changed, 285 insertions(+)
 create mode 100755 tools/testing/selftests/drivers/net/shaper.py
drivers/net/Kconfig +1 -0
index 9920b3a68ed1..1fd5acdc73c6 100644
@@ -641,6 +641,7 @@ config NETDEVSIM
 	depends on PTP_1588_CLOCK_MOCK || PTP_1588_CLOCK_MOCK=n
 	select NET_DEVLINK
 	select PAGE_POOL
+	select NET_SHAPER
 	help
 	  This driver is a developer testing tool and software model that can
 	  be used to test various control path networking APIs, especially
drivers/net/netdevsim/netdev.c +41 -0

59 lines not on this page · raw message

tools/testing/selftests/drivers/net/Makefile +1 -0
index e54f382bcb02..432306f11664 100644
@@ -6,6 +6,7 @@ TEST_PROGS := \
 	ping.py \
 	queues.py \
 	stats.py \
+	shaper.py
 # end of TEST_PROGS
 
 include ../../lib.mk
tools/testing/selftests/drivers/net/shaper.py new file +236 -0

236 lines not on this page · raw message

tools/testing/selftests/net/lib/py/__init__.py +1 -0
index b6d498d125fe..54d8f5eba810 100644
@@ -6,3 +6,4 @@ from .netns import NetNS
 from .nsim import *
 from .utils import *
 from .ynl import NlError, YnlFamily, EthtoolFamily, NetdevFamily, RtnlFamily
+from .ynl import NetshaperFamily
tools/testing/selftests/net/lib/py/ynl.py +5 -0
index 1ace58370c06..a0d689d58c57 100644
@@ -47,3 +47,8 @@ class NetdevFamily(YnlFamily):
     def __init__(self):
         super().__init__((SPEC_PATH / Path('netdev.yaml')).as_posix(),
                          schema='')
+
+class NetshaperFamily(YnlFamily):
+    def __init__(self):
+        super().__init__((SPEC_PATH / Path('net_shaper.yaml')).as_posix(),
+                         schema='')
Signature · 1 line
2.45.2
#20 lore raw

Re: [PATCH v4 net-next 09/12] testing: net-drv: add basic shaper test

kernel test robot noticed the following build warnings:

Hi Paolo,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Paolo-Abeni/tools-ynl-lift-an-assumption-about-spec-file-name/20240820-231626
base:   net-next/main
patch link:    https://lore.kernel.org/r/4cf74f285fa5f07be546cb83ef96775f86aa0dbf.1724165948.git.pabeni%40redhat.com
patch subject: [PATCH v4 net-next 09/12] testing: net-drv: add basic shaper test
config: hexagon-randconfig-r112-20240821 (https://download.01.org/0day-ci/archive/20240822/[email protected]/config)
compiler: clang version 16.0.6 (https://github.com/llvm/llvm-project 7cbf1a2591520c2491aa35339f227775f4d3adf6)
reproduce: (https://download.01.org/0day-ci/archive/20240822/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

sparse warnings: (new ones prefixed by >>)

>> net/shaper/shaper.c:227:24: sparse: sparse: Using plain integer as NULL pointer

vim +227 net/shaper/shaper.c

d8a22f9bfdaade Paolo Abeni 2024-08-20  208  
d8a22f9bfdaade Paolo Abeni 2024-08-20  209  /* Allocate on demand the per device shaper's cache. */
d8a22f9bfdaade Paolo Abeni 2024-08-20  210  static struct mutex *net_shaper_cache_init(struct net_device *dev,
d8a22f9bfdaade Paolo Abeni 2024-08-20  211  					   struct netlink_ext_ack *extack)
d8a22f9bfdaade Paolo Abeni 2024-08-20  212  {
d8a22f9bfdaade Paolo Abeni 2024-08-20  213  	struct net_shaper_data *new, *data = READ_ONCE(dev->net_shaper_data);
d8a22f9bfdaade Paolo Abeni 2024-08-20  214  
d8a22f9bfdaade Paolo Abeni 2024-08-20  215  	if (!data) {
d8a22f9bfdaade Paolo Abeni 2024-08-20  216  		new = kmalloc(sizeof(*dev->net_shaper_data), GFP_KERNEL);
d8a22f9bfdaade Paolo Abeni 2024-08-20  217  		if (!new) {
d8a22f9bfdaade Paolo Abeni 2024-08-20  218  			NL_SET_ERR_MSG(extack, "Can't allocate memory for shaper data");
d8a22f9bfdaade Paolo Abeni 2024-08-20  219  			return NULL;
d8a22f9bfdaade Paolo Abeni 2024-08-20  220  		}
d8a22f9bfdaade Paolo Abeni 2024-08-20  221  
d8a22f9bfdaade Paolo Abeni 2024-08-20  222  		mutex_init(&new->lock);
d8a22f9bfdaade Paolo Abeni 2024-08-20  223  		xa_init(&new->shapers);
d8a22f9bfdaade Paolo Abeni 2024-08-20  224  		idr_init(&new->node_ids);
d8a22f9bfdaade Paolo Abeni 2024-08-20  225  
d8a22f9bfdaade Paolo Abeni 2024-08-20  226  		/* No lock acquired yet, we can race with other operations. */
d8a22f9bfdaade Paolo Abeni 2024-08-20 @227  		data = cmpxchg(&dev->net_shaper_data, NULL, new);
d8a22f9bfdaade Paolo Abeni 2024-08-20  228  		if (!data)
d8a22f9bfdaade Paolo Abeni 2024-08-20  229  			data = new;
d8a22f9bfdaade Paolo Abeni 2024-08-20  230  		else
d8a22f9bfdaade Paolo Abeni 2024-08-20  231  			kfree(new);
d8a22f9bfdaade Paolo Abeni 2024-08-20  232  	}
d8a22f9bfdaade Paolo Abeni 2024-08-20  233  	return &data->lock;
d8a22f9bfdaade Paolo Abeni 2024-08-20  234  }

d8a22f9bfdaade Paolo Abeni 2024-08-20 235

Signature · 2 lines
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
author maintainer
#21 lore raw

Re: [PATCH v4 net-next 09/12] testing: net-drv: add basic shaper test

Quoted 14 lines · kernel test robot

Hi Paolo,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Paolo-Abeni/tools-ynl-lift-an-assumption-about-spec-file-name/20240820-231626
base:   net-next/main
patch link:    https://lore.kernel.org/r/4cf74f285fa5f07be546cb83ef96775f86aa0dbf.1724165948.git.pabeni%40redhat.com
patch subject: [PATCH v4 net-next 09/12] testing: net-drv: add basic shaper test
config: hexagon-randconfig-r112-20240821 (https://download.01.org/0day-ci/archive/20240822/[email protected]/config)
compiler: clang version 16.0.6 (https://github.com/llvm/llvm-project 7cbf1a2591520c2491aa35339f227775f4d3adf6)
reproduce: (https://download.01.org/0day-ci/archive/20240822/[email protected]/reproduce)
Quoted 7 lines

If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

sparse warnings: (new ones prefixed by >>)

Quoted 1 line
Quoted 1 line

net/shaper/shaper.c:227:24: sparse: sparse: Using plain integer as NULL pointer

AFAICS this warning comes directly from/is due to the hexgon cmpxchg implementation:

#define arch_cmpxchg(ptr, old, new)                             \
({                                                              \
        __typeof__(ptr) __ptr = (ptr);                          \
        __typeof__(*(ptr)) __old = (old);                       \
        __typeof__(*(ptr)) __new = (new);                       \
        __typeof__(*(ptr)) __oldval = 0;                        \
				^^^^^^^ here.

Cheers,

Paolo

reviewer
#22 lore raw

Re: [PATCH v4 net-next 09/12] testing: net-drv: add basic shaper test

Cc Brian Cain, linux-hexagon
Quoted 28 lines · Paolo Abeni
Quoted 14 lines · kernel test robot

Hi Paolo,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Paolo-Abeni/tools-ynl-lift-an-assumption-about-spec-file-name/20240820-231626
base:   net-next/main
patch link:    https://lore.kernel.org/r/4cf74f285fa5f07be546cb83ef96775f86aa0dbf.1724165948.git.pabeni%40redhat.com
patch subject: [PATCH v4 net-next 09/12] testing: net-drv: add basic shaper test
config: hexagon-randconfig-r112-20240821 (https://download.01.org/0day-ci/archive/20240822/[email protected]/config)
compiler: clang version 16.0.6 (https://github.com/llvm/llvm-project 7cbf1a2591520c2491aa35339f227775f4d3adf6)
reproduce: (https://download.01.org/0day-ci/archive/20240822/[email protected]/reproduce)
Quoted 7 lines

If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

sparse warnings: (new ones prefixed by >>)

Quoted 1 line
Quoted 1 line

net/shaper/shaper.c:227:24: sparse: sparse: Using plain integer as NULL pointer

AFAICS this warning comes directly from/is due to the hexgon cmpxchg implementation:

Quoted 7 lines
#define arch_cmpxchg(ptr, old, new)                             \
({                                                              \
        __typeof__(ptr) __ptr = (ptr);                          \
        __typeof__(*(ptr)) __old = (old);                       \
        __typeof__(*(ptr)) __new = (new);                       \
        __typeof__(*(ptr)) __oldval = 0;                        \
				^^^^^^^ here.

FWIIW, I agree.

It seems that arch_cmpxchg, as implemented above, expects ptr to be an integer. And indeed it is used in that way from include/linux/atomic/atomic-arch-fallback.h:raw_atomic_cmpxchg_acquire().

https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/include/linux/atomic/atomic-arch-fallback.h?id=f8fdda9e4f988c210b1e4519a28ddbf7d29b0038#n2055

As a hack, I allowed the function to handle either int or any type of pointer. With this in place Sparse no longer flags the problem described above in shaper.c.

Perhaps someone has a suggestion of how to fix this properly.

arch/hexagon/include/asm/cmpxchg.h +7 -1
index bf6cf5579cf4..d8decb8fb456 100644
@@ -51,12 +51,18 @@ __arch_xchg(unsigned long x, volatile void *ptr, int size)
  *  variable casting.
  */
 
+#define arch_cmpxchg_zero(ptr)					\
+	(__typeof__(*(ptr)))					\
+		_Generic(*(ptr),				\
+			 int:		0,			\
+			 default:	NULL)
+
 #define arch_cmpxchg(ptr, old, new)				\
 ({								\
 	__typeof__(ptr) __ptr = (ptr);				\
 	__typeof__(*(ptr)) __old = (old);			\
 	__typeof__(*(ptr)) __new = (new);			\
-	__typeof__(*(ptr)) __oldval = 0;			\
+	__typeof__(*(ptr)) __oldval = arch_cmpxchg_zero(ptr);	\
 								\
 	asm volatile(						\
 		"1:	%0 = memw_locked(%1);\n"		\
author maintainer
#23 lore raw

PATCH v4 10/12 virtchnl: support queue rate limit and quanta size configuration

In reply to Paolo Abeni

From: Wenjun Wu <[email protected]>

This patch adds new virtchnl opcodes and structures for rate limit and quanta size configuration, which include:
1. VIRTCHNL_OP_CONFIG_QUEUE_BW, to configure max bandwidth for each VF per queue.
2. VIRTCHNL_OP_CONFIG_QUANTA, to configure quanta size per queue.
3. VIRTCHNL_OP_GET_QOS_CAPS, VF queries current QoS configuration, such as enabled TCs, arbiter type, up2tc and bandwidth of VSI node. The configuration is previously set by DCB and PF, and now is the potential QoS capability of VF. VF can take it as reference to configure queue TC mapping.

---
 include/linux/avf/virtchnl.h | 119 +++++++++++++++++++++++++++++++++++
 1 file changed, 119 insertions(+)
include/linux/avf/virtchnl.h +119 -0
index f41395264dca..223e433c39fe 100644
@@ -89,6 +89,9 @@ enum virtchnl_rx_hsplit {
 	VIRTCHNL_RX_HSPLIT_SPLIT_SCTP    = 8,
 };
 
+enum virtchnl_bw_limit_type {
+	VIRTCHNL_BW_SHAPER = 0,
+};
 /* END GENERIC DEFINES */
 
 /* Opcodes for VF-PF communication. These are placed in the v_opcode field
@@ -151,6 +154,11 @@ enum virtchnl_ops {
 	VIRTCHNL_OP_DISABLE_VLAN_STRIPPING_V2 = 55,
 	VIRTCHNL_OP_ENABLE_VLAN_INSERTION_V2 = 56,
 	VIRTCHNL_OP_DISABLE_VLAN_INSERTION_V2 = 57,
+	/* opcode 57 - 65 are reserved */
+	VIRTCHNL_OP_GET_QOS_CAPS = 66,
+	/* opcode 68 through 111 are reserved */
+	VIRTCHNL_OP_CONFIG_QUEUE_BW = 112,
+	VIRTCHNL_OP_CONFIG_QUANTA = 113,
 	VIRTCHNL_OP_MAX,
 };
 
@@ -261,6 +269,7 @@ VIRTCHNL_CHECK_STRUCT_LEN(16, virtchnl_vsi_resource);
 #define VIRTCHNL_VF_OFFLOAD_RX_FLEX_DESC	BIT(26)
 #define VIRTCHNL_VF_OFFLOAD_ADV_RSS_PF		BIT(27)
 #define VIRTCHNL_VF_OFFLOAD_FDIR_PF		BIT(28)
+#define VIRTCHNL_VF_OFFLOAD_QOS			BIT(29)
 
 #define VF_BASE_MODE_OFFLOADS (VIRTCHNL_VF_OFFLOAD_L2 | \
 			       VIRTCHNL_VF_OFFLOAD_VLAN | \
@@ -1416,6 +1425,85 @@ struct virtchnl_fdir_del {
 
 VIRTCHNL_CHECK_STRUCT_LEN(12, virtchnl_fdir_del);
 
+struct virtchnl_shaper_bw {
+	/* Unit is Kbps */
+	u32 committed;
+	u32 peak;
+};
+
+VIRTCHNL_CHECK_STRUCT_LEN(8, virtchnl_shaper_bw);
+
+/* VIRTCHNL_OP_GET_QOS_CAPS
+ * VF sends this message to get its QoS Caps, such as
+ * TC number, Arbiter and Bandwidth.
+ */
+struct virtchnl_qos_cap_elem {
+	u8 tc_num;
+	u8 tc_prio;
+#define VIRTCHNL_ABITER_STRICT      0
+#define VIRTCHNL_ABITER_ETS         2
+	u8 arbiter;
+#define VIRTCHNL_STRICT_WEIGHT      1
+	u8 weight;
+	enum virtchnl_bw_limit_type type;
+	union {
+		struct virtchnl_shaper_bw shaper;
+		u8 pad2[32];
+	};
+};
+
+VIRTCHNL_CHECK_STRUCT_LEN(40, virtchnl_qos_cap_elem);
+
+struct virtchnl_qos_cap_list {
+	u16 vsi_id;
+	u16 num_elem;
+	struct virtchnl_qos_cap_elem cap[];
+};
+
+VIRTCHNL_CHECK_STRUCT_LEN(4, virtchnl_qos_cap_list);
+#define virtchnl_qos_cap_list_LEGACY_SIZEOF	44
+
+/* VIRTCHNL_OP_CONFIG_QUEUE_BW */
+struct virtchnl_queue_bw {
+	u16 queue_id;
+	u8 tc;
+	u8 pad;
+	struct virtchnl_shaper_bw shaper;
+};
+
+VIRTCHNL_CHECK_STRUCT_LEN(12, virtchnl_queue_bw);
+
+struct virtchnl_queues_bw_cfg {
+	u16 vsi_id;
+	u16 num_queues;
+	struct virtchnl_queue_bw cfg[];
+};
+
+VIRTCHNL_CHECK_STRUCT_LEN(4, virtchnl_queues_bw_cfg);
+#define virtchnl_queues_bw_cfg_LEGACY_SIZEOF	16
+
+enum virtchnl_queue_type {
+	VIRTCHNL_QUEUE_TYPE_TX			= 0,
+	VIRTCHNL_QUEUE_TYPE_RX			= 1,
+};
+
+/* structure to specify a chunk of contiguous queues */
+struct virtchnl_queue_chunk {
+	/* see enum virtchnl_queue_type */
+	s32 type;
+	u16 start_queue_id;
+	u16 num_queues;
+};
+
+VIRTCHNL_CHECK_STRUCT_LEN(8, virtchnl_queue_chunk);
+
+struct virtchnl_quanta_cfg {
+	u16 quanta_size;
+	struct virtchnl_queue_chunk queue_select;
+};
+
+VIRTCHNL_CHECK_STRUCT_LEN(12, virtchnl_quanta_cfg);
+
 #define __vss_byone(p, member, count, old)				      \
 	(struct_size(p, member, count) + (old - 1 - struct_size(p, member, 0)))
 
@@ -1438,6 +1526,8 @@ VIRTCHNL_CHECK_STRUCT_LEN(12, virtchnl_fdir_del);
 		 __vss(virtchnl_vlan_filter_list_v2, __vss_byelem, p, m, c),  \
 		 __vss(virtchnl_tc_info, __vss_byelem, p, m, c),	      \
 		 __vss(virtchnl_rdma_qvlist_info, __vss_byelem, p, m, c),     \
+		 __vss(virtchnl_qos_cap_list, __vss_byelem, p, m, c),	      \
+		 __vss(virtchnl_queues_bw_cfg, __vss_byelem, p, m, c),	      \
 		 __vss(virtchnl_rss_key, __vss_byone, p, m, c),		      \
 		 __vss(virtchnl_rss_lut, __vss_byone, p, m, c))
 
@@ -1637,6 +1727,35 @@ virtchnl_vc_validate_vf_msg(struct virtchnl_version_info *ver, u32 v_opcode,
 	case VIRTCHNL_OP_DISABLE_VLAN_INSERTION_V2:
 		valid_len = sizeof(struct virtchnl_vlan_setting);
 		break;
+	case VIRTCHNL_OP_GET_QOS_CAPS:
+		break;
+	case VIRTCHNL_OP_CONFIG_QUEUE_BW:
+		valid_len = virtchnl_queues_bw_cfg_LEGACY_SIZEOF;
+		if (msglen >= valid_len) {
+			struct virtchnl_queues_bw_cfg *q_bw =
+				(struct virtchnl_queues_bw_cfg *)msg;
+
+			valid_len = virtchnl_struct_size(q_bw, cfg,
+							 q_bw->num_queues);
+			if (q_bw->num_queues == 0) {
+				err_msg_format = true;
+				break;
+			}
+		}
+		break;
+	case VIRTCHNL_OP_CONFIG_QUANTA:
+		valid_len = sizeof(struct virtchnl_quanta_cfg);
+		if (msglen >= valid_len) {
+			struct virtchnl_quanta_cfg *q_quanta =
+				(struct virtchnl_quanta_cfg *)msg;
+
+			if (q_quanta->quanta_size == 0 ||
+			    q_quanta->queue_select.num_queues == 0) {
+				err_msg_format = true;
+				break;
+			}
+		}
+		break;
 	/* These are always errors coming from the VF. */
 	case VIRTCHNL_OP_EVENT:
 	case VIRTCHNL_OP_UNKNOWN:
Signature · 1 line
2.45.2
author maintainer
#24 lore raw

PATCH v4 11/12 ice: Support VF queue rate limit and quanta size configuration

In reply to Paolo Abeni

From: Wenjun Wu <[email protected]>

Add support to configure VF queue rate limit and quanta size.

For quanta size configuration, the quanta profiles are divided evenly by PF numbers. For each port, the first quanta profile is reserved for default. When VF is asked to set queue quanta size, PF will search for an available profile, change the fields and assigned this profile to the queue.

---
 drivers/net/ethernet/intel/ice/ice.h          |   2 +
 drivers/net/ethernet/intel/ice/ice_base.c     |   2 +
 drivers/net/ethernet/intel/ice/ice_common.c   |  21 ++
 .../net/ethernet/intel/ice/ice_hw_autogen.h   |   8 +
 drivers/net/ethernet/intel/ice/ice_txrx.h     |   1 +
 drivers/net/ethernet/intel/ice/ice_type.h     |   1 +
 drivers/net/ethernet/intel/ice/ice_vf_lib.h   |   8 +
 drivers/net/ethernet/intel/ice/ice_virtchnl.c | 333 ++++++++++++++++++
 drivers/net/ethernet/intel/ice/ice_virtchnl.h |  11 +
 .../intel/ice/ice_virtchnl_allowlist.c        |   6 +
 10 files changed, 393 insertions(+)
drivers/net/ethernet/intel/ice/ice.h +2 -0
index caaa10157909..35ace1907a62 100644
@@ -659,6 +659,8 @@ struct ice_pf {
 	struct ice_agg_node vf_agg_node[ICE_MAX_VF_AGG_NODES];
 	struct ice_dplls dplls;
 	struct device *hwmon_dev;
+
+	u8 num_quanta_prof_used;
 };
 
 extern struct workqueue_struct *ice_lag_wq;
drivers/net/ethernet/intel/ice/ice_base.c +2 -0
index 1facf179a96f..100d4644cfd4 100644
@@ -349,6 +349,8 @@ ice_setup_tx_ctx(struct ice_tx_ring *ring, struct ice_tlan_ctx *tlan_ctx, u16 pf
 		break;
 	}
 
+	tlan_ctx->quanta_prof_idx = ring->quanta_prof_id;
+
 	tlan_ctx->tso_ena = ICE_TX_LEGACY;
 	tlan_ctx->tso_qnum = pf_q;
 
drivers/net/ethernet/intel/ice/ice_common.c +21 -0
index 009716a12a26..b22e71dc59d4 100644
@@ -2436,6 +2436,25 @@ ice_parse_func_caps(struct ice_hw *hw, struct ice_hw_func_caps *func_p,
 	ice_recalc_port_limited_caps(hw, &func_p->common_cap);
 }
 
+/**
+ * ice_func_id_to_logical_id - map from function id to logical pf id
+ * @active_function_bitmap: active function bitmap
+ * @pf_id: function number of device
+ *
+ * Return: logical PF ID.
+ */
+static int ice_func_id_to_logical_id(u32 active_function_bitmap, u8 pf_id)
+{
+	u8 logical_id = 0;
+	u8 i;
+
+	for (i = 0; i < pf_id; i++)
+		if (active_function_bitmap & BIT(i))
+			logical_id++;
+
+	return logical_id;
+}
+
 /**
  * ice_parse_valid_functions_cap - Parse ICE_AQC_CAPS_VALID_FUNCTIONS caps
  * @hw: pointer to the HW struct
@@ -2453,6 +2472,8 @@ ice_parse_valid_functions_cap(struct ice_hw *hw, struct ice_hw_dev_caps *dev_p,
 	dev_p->num_funcs = hweight32(number);
 	ice_debug(hw, ICE_DBG_INIT, "dev caps: num_funcs = %d\n",
 		  dev_p->num_funcs);
+
+	hw->logical_pf_id = ice_func_id_to_logical_id(number, hw->pf_id);
 }
 
 /**
drivers/net/ethernet/intel/ice/ice_hw_autogen.h +8 -0
index 91cbae1eec89..af9302f0e376 100644
@@ -6,6 +6,14 @@
 #ifndef _ICE_HW_AUTOGEN_H_
 #define _ICE_HW_AUTOGEN_H_
 
+#define GLCOMM_QUANTA_PROF(_i)			(0x002D2D68 + ((_i) * 4))
+#define GLCOMM_QUANTA_PROF_MAX_INDEX		15
+#define GLCOMM_QUANTA_PROF_QUANTA_SIZE_S	0
+#define GLCOMM_QUANTA_PROF_QUANTA_SIZE_M	ICE_M(0x3FFF, 0)
+#define GLCOMM_QUANTA_PROF_MAX_CMD_S		16
+#define GLCOMM_QUANTA_PROF_MAX_CMD_M		ICE_M(0xFF, 16)
+#define GLCOMM_QUANTA_PROF_MAX_DESC_S		24
+#define GLCOMM_QUANTA_PROF_MAX_DESC_M		ICE_M(0x3F, 24)
 #define QTX_COMM_DBELL(_DBQM)			(0x002C0000 + ((_DBQM) * 4))
 #define QTX_COMM_HEAD(_DBQM)			(0x000E0000 + ((_DBQM) * 4))
 #define QTX_COMM_HEAD_HEAD_S			0
drivers/net/ethernet/intel/ice/ice_txrx.h +1 -0
index feba314a3fe4..ea2fae9035b5 100644
@@ -406,6 +406,7 @@ struct ice_tx_ring {
 #define ICE_TX_FLAGS_RING_VLAN_L2TAG2	BIT(2)
 	u8 flags;
 	u8 dcb_tc;			/* Traffic class of ring */
+	u16 quanta_prof_id;
 } ____cacheline_internodealigned_in_smp;
 
 static inline bool ice_ring_uses_build_skb(struct ice_rx_ring *ring)
drivers/net/ethernet/intel/ice/ice_type.h +1 -0
index b9e443232335..953576003425 100644
@@ -904,6 +904,7 @@ struct ice_hw {
 	u8 revision_id;
 
 	u8 pf_id;		/* device profile info */
+	u8 logical_pf_id;
 
 	u16 max_burst_size;	/* driver sets this value */
 
drivers/net/ethernet/intel/ice/ice_vf_lib.h +8 -0
index be4266899690..4261fe1c2bcd 100644
@@ -59,6 +59,13 @@ struct ice_fdir_prof_info {
 	u64 fdir_active_cnt;
 };
 
+struct ice_vf_qs_bw {
+	u32 committed;
+	u32 peak;
+	u16 queue_id;
+	u8 tc;
+};
+
 /* VF operations */
 struct ice_vf_ops {
 	enum ice_disq_rst_src reset_type;
@@ -140,6 +147,7 @@ struct ice_vf {
 	struct devlink_port devlink_port;
 
 	u16 num_msix;			/* num of MSI-X configured on this VF */
+	struct ice_vf_qs_bw qs_bw[ICE_MAX_RSS_QS_PER_VF];
 };
 
 /* Flags for controlling behavior of ice_reset_vf */
drivers/net/ethernet/intel/ice/ice_virtchnl.c +333 -0

363 lines not on this page · raw message

drivers/net/ethernet/intel/ice/ice_virtchnl.h +11 -0
index 3a4115869153..0c629aef9baf 100644
@@ -13,6 +13,13 @@
 /* Restrict number of MAC Addr and VLAN that non-trusted VF can programmed */
 #define ICE_MAX_VLAN_PER_VF		8
 
+#define ICE_DFLT_QUANTA 1024
+#define ICE_MAX_QUANTA_SIZE 4096
+#define ICE_MIN_QUANTA_SIZE 256
+
+#define calc_quanta_desc(x)	\
+	max_t(u16, 12, min_t(u16, 63, (((x) + 66) / 132) * 2 + 4))
+
 /* MAC filters: 1 is reserved for the VF's default/perm_addr/LAA MAC, 1 for
  * broadcast, and 16 for additional unicast/multicast filters
  */
@@ -61,6 +68,10 @@ struct ice_virtchnl_ops {
 	int (*dis_vlan_stripping_v2_msg)(struct ice_vf *vf, u8 *msg);
 	int (*ena_vlan_insertion_v2_msg)(struct ice_vf *vf, u8 *msg);
 	int (*dis_vlan_insertion_v2_msg)(struct ice_vf *vf, u8 *msg);
+	int (*get_qos_caps)(struct ice_vf *vf);
+	int (*cfg_q_tc_map)(struct ice_vf *vf, u8 *msg);
+	int (*cfg_q_bw)(struct ice_vf *vf, u8 *msg);
+	int (*cfg_q_quanta)(struct ice_vf *vf, u8 *msg);
 };
 
 #ifdef CONFIG_PCI_IOV
drivers/net/ethernet/intel/ice/ice_virtchnl_allowlist.c +6 -0
index d796dbd2a440..c105a82ee136 100644
@@ -84,6 +84,11 @@ static const u32 fdir_pf_allowlist_opcodes[] = {
 	VIRTCHNL_OP_ADD_FDIR_FILTER, VIRTCHNL_OP_DEL_FDIR_FILTER,
 };
 
+static const u32 tc_allowlist_opcodes[] = {
+	VIRTCHNL_OP_GET_QOS_CAPS, VIRTCHNL_OP_CONFIG_QUEUE_BW,
+	VIRTCHNL_OP_CONFIG_QUANTA,
+};
+
 struct allowlist_opcode_info {
 	const u32 *opcodes;
 	size_t size;
@@ -104,6 +109,7 @@ static const struct allowlist_opcode_info allowlist_opcodes[] = {
 	ALLOW_ITEM(VIRTCHNL_VF_OFFLOAD_ADV_RSS_PF, adv_rss_pf_allowlist_opcodes),
 	ALLOW_ITEM(VIRTCHNL_VF_OFFLOAD_FDIR_PF, fdir_pf_allowlist_opcodes),
 	ALLOW_ITEM(VIRTCHNL_VF_OFFLOAD_VLAN_V2, vlan_v2_allowlist_opcodes),
+	ALLOW_ITEM(VIRTCHNL_VF_OFFLOAD_QOS, tc_allowlist_opcodes),
 };
 
 /**
Signature · 1 line
2.45.2
author maintainer
#25 lore raw

PATCH v4 12/12 iavf: Add net_shaper_ops support

In reply to Paolo Abeni

From: Sudheer Mogilappagari <[email protected]>

Implement net_shaper_ops support for IAVF. This enables configuration of rate limiting on per queue basis. Customer intends to enforce bandwidth limit on Tx traffic steered to the queue by configuring rate limits on the queue.

To set rate limiting for a queue, update shaper object of given queues in driver and send VIRTCHNL_OP_CONFIG_QUEUE_BW to PF to update HW configuration.

Deleting shaper configured for queue is nothing but configuring shaper with bw_max 0. The PF restores the default rate limiting config when bw_max is zero.

---
 drivers/net/ethernet/intel/Kconfig            |   1 +
 drivers/net/ethernet/intel/iavf/iavf.h        |   3 +
 drivers/net/ethernet/intel/iavf/iavf_main.c   | 150 ++++++++++++++++++
 drivers/net/ethernet/intel/iavf/iavf_txrx.h   |   2 +
 .../net/ethernet/intel/iavf/iavf_virtchnl.c   |  65 ++++++++
 5 files changed, 221 insertions(+)
drivers/net/ethernet/intel/Kconfig +1 -0
index 0375c7448a57..20bc40eec487 100644
@@ -258,6 +258,7 @@ config I40E_DCB
 config IAVF
 	tristate
 	select LIBIE
+	select NET_SHAPER
 
 config I40EVF
 	tristate "Intel(R) Ethernet Adaptive Virtual Function support"
drivers/net/ethernet/intel/iavf/iavf.h +3 -0
index 48cd1d06761c..a84bdbfbb0f7 100644
@@ -34,6 +34,7 @@
 #include <net/tc_act/tc_gact.h>
 #include <net/tc_act/tc_mirred.h>
 #include <net/tc_act/tc_skbedit.h>
+#include <net/net_shaper.h>
 
 #include "iavf_type.h"
 #include <linux/avf/virtchnl.h>
@@ -336,6 +337,7 @@ struct iavf_adapter {
 #define IAVF_FLAG_AQ_DISABLE_CTAG_VLAN_INSERTION	BIT_ULL(36)
 #define IAVF_FLAG_AQ_ENABLE_STAG_VLAN_INSERTION		BIT_ULL(37)
 #define IAVF_FLAG_AQ_DISABLE_STAG_VLAN_INSERTION	BIT_ULL(38)
+#define IAVF_FLAG_AQ_CONFIGURE_QUEUES_BW		BIT_ULL(39)
 
 	/* flags for processing extended capability messages during
 	 * __IAVF_INIT_EXTENDED_CAPS. Each capability exchange requires
@@ -581,6 +583,7 @@ void iavf_virtchnl_completion(struct iavf_adapter *adapter,
 int iavf_config_rss(struct iavf_adapter *adapter);
 int iavf_lan_add_device(struct iavf_adapter *adapter);
 int iavf_lan_del_device(struct iavf_adapter *adapter);
+void iavf_cfg_queues_bw(struct iavf_adapter *adapter);
 void iavf_enable_channels(struct iavf_adapter *adapter);
 void iavf_disable_channels(struct iavf_adapter *adapter);
 void iavf_add_cloud_filter(struct iavf_adapter *adapter);
drivers/net/ethernet/intel/iavf/iavf_main.c +150 -0

180 lines not on this page · raw message

drivers/net/ethernet/intel/iavf/iavf_txrx.h +2 -0
index d7b5587aeb8e..dd503ee50b7f 100644
@@ -296,6 +296,8 @@ struct iavf_ring {
 					 */
 
 	u32 rx_buf_len;
+	struct net_shaper_info q_shaper;
+	bool q_shaper_update;
 } ____cacheline_internodealigned_in_smp;
 
 #define IAVF_ITR_ADAPTIVE_MIN_INC	0x0002
drivers/net/ethernet/intel/iavf/iavf_virtchnl.c +65 -0

83 lines not on this page · raw message

Signature · 1 line
2.45.2
#26 lore raw

Re: [PATCH v4 net-next 12/12] iavf: Add net_shaper_ops support

kernel test robot noticed the following build warnings:

Hi Paolo,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Paolo-Abeni/tools-ynl-lift-an-assumption-about-spec-file-name/20240820-231626
base:   net-next/main
patch link:    https://lore.kernel.org/r/08cd87e754552c5f413ead220abdaf1ccfadf21c.1724165948.git.pabeni%40redhat.com
patch subject: [PATCH v4 net-next 12/12] iavf: Add net_shaper_ops support
config: loongarch-allmodconfig (https://download.01.org/0day-ci/archive/20240821/[email protected]/config)
compiler: loongarch64-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240821/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All warnings (new ones prefixed by >>):

>> drivers/net/ethernet/intel/iavf/iavf_main.c:4965: warning: Function parameter or struct member 'handle' not described in 'iavf_shaper_set'

vim +4965 drivers/net/ethernet/intel/iavf/iavf_main.c

4948
4949 /**
4950 * iavf_shaper_set - check that shaper info received
4951 * @dev: pointer to netdev
4952 * @shaper: configuration of shaper.
4953 * @extack: Netlink extended ACK for reporting errors
4954 *
4955 * Returns:
4956 * * %0 - Success
4957 * * %-EOPNOTSUPP - Driver doesn't support this scope.
4958 * * %-EINVAL - Invalid queue number in input
4959 **/
4960 static int
4961 iavf_shaper_set(struct net_device *dev,
4962 const struct net_shaper_handle *handle,
4963 const struct net_shaper_info *shaper,
4964 struct netlink_ext_ack *extack)

> 4965	{

4966 struct iavf_adapter *adapter = netdev_priv(dev);
4967 bool need_cfg_update = false;
4968 int ret = 0;
4969
4970 ret = iavf_verify_handle(dev, handle, extack);
4971 if (ret)
4972 return ret;
4973
4974 if (handle->scope == NET_SHAPER_SCOPE_QUEUE) {
4975 struct iavf_ring *tx_ring = &adapter->tx_rings[handle->id];
4976
4977 tx_ring->q_shaper.bw_min = div_u64(shaper->bw_min, 1000);
4978 tx_ring->q_shaper.bw_max = div_u64(shaper->bw_max, 1000);
4979 tx_ring->q_shaper_update = true;
4980 need_cfg_update = true;
4981 }
4982
4983 if (need_cfg_update)
4984 adapter->aq_required |= IAVF_FLAG_AQ_CONFIGURE_QUEUES_BW;
4985
4986 return 0;
4987 }
4988

Signature · 2 lines
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
maintainer
#27 lore raw
In reply to Paolo Abeni
Quoted 1 line · On Tue, 20 Aug 2024 17:12:21 +0200 Paolo Abeni wrote:

tools: ynl: lift an assumption about spec file name

I'll pop this one in, no point carrying it around.

Double check other patches for trailing whitespace. There were some warnings when I was pulling the series in.

#28 lore raw
In reply to Paolo Abeni
This series was applied to netdev/net-next.git (main) by Jakub Kicinski <[email protected]>:

Hello:

This series was applied to netdev/net-next.git (main) by Jakub Kicinski <[email protected]>:

Quoted 11 lines · On Tue, 20 Aug 2024 17:12:21 +0200 you wrote:

We have a plurality of shaping-related drivers API, but none flexible enough to meet existing demand from vendors[1].

This series introduces new device APIs to configure in a flexible way TX H/W shaping. The new functionalities are exposed via a newly defined generic netlink interface and include introspection capabilities. Some self-tests are included, on top of a dummy netdevsim implementation, and a basic implementation for the iavf driver.

[...]

Here is the summary with links:
- [v4,net-next,01/12] tools: ynl: lift an assumption about spec file name

    https://git.kernel.org/netdev/net-next/c/f32c821ae019

- [v4,net-next,02/12] netlink: spec: add shaper YAML spec
(no matching commit)
- [v4,net-next,03/12] net-shapers: implement NL get operation
(no matching commit)
- [v4,net-next,04/12] net-shapers: implement NL set and delete operations (no matching commit)
- [v4,net-next,05/12] net-shapers: implement NL group operation
(no matching commit)
- [v4,net-next,06/12] net-shapers: implement delete support for NODE scope shaper (no matching commit)
- [v4,net-next,07/12] netlink: spec: add shaper introspection support (no matching commit)
- [v4,net-next,08/12] net: shaper: implement introspection support
(no matching commit)
- [v4,net-next,09/12] testing: net-drv: add basic shaper test
(no matching commit)
- [v4,net-next,10/12] virtchnl: support queue rate limit and quanta size configuration (no matching commit)
- [v4,net-next,11/12] ice: Support VF queue rate limit and quanta size configuration (no matching commit)
- [v4,net-next,12/12] iavf: Add net_shaper_ops support
(no matching commit)

You are awesome, thank you!

Signature · 2 lines
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
maintainer
#29 lore raw
In reply to Paolo Abeni
Quoted 7 lines · On Tue, 20 Aug 2024 17:12:21 +0200 Paolo Abeni wrote:

* Delegation

A containers wants to limit the aggregate B/W bandwidth of 2 of the 3 queues it owns - the starting configuration is the one from the previous point:

SPEC=Documentation/netlink/specs/net_shaper.yaml

Quoted 9 lines
./tools/net/ynl/cli.py --spec $SPEC \
	--do group --json '{"ifindex":'$IFINDEX', 
			"leaves": [ 
			  {"handle": {"scope": "queue", "id":'$QID1' },
			   "weight": '$W1'}, 
			  {"handle": {"scope": "queue", "id":'$QID2' },
			   "weight": '$W2'}], 
			"root": { "handle": {"scope": "node"},
				  "parent": {"scope": "node", "id": 0},

In the delegation use case I was hoping "parent" would be automatic.

author maintainer
#30 lore raw
Quoted 18 lines · Jakub Kicinski
Quoted 7 lines · On Tue, 20 Aug 2024 17:12:21 +0200 Paolo Abeni wrote:

* Delegation

A containers wants to limit the aggregate B/W bandwidth of 2 of the 3 queues it owns - the starting configuration is the one from the previous point:

SPEC=Documentation/netlink/specs/net_shaper.yaml

Quoted 9 lines
./tools/net/ynl/cli.py --spec $SPEC \
	--do group --json '{"ifindex":'$IFINDEX',
			"leaves": [
			  {"handle": {"scope": "queue", "id":'$QID1' },
			   "weight": '$W1'},
			  {"handle": {"scope": "queue", "id":'$QID2' },
			   "weight": '$W2'}],
			"root": { "handle": {"scope": "node"},
				  "parent": {"scope": "node", "id": 0},
Quoted 1 line

In the delegation use case I was hoping "parent" would be automatic.

Currently the parent is automatic/implicit when creating a node directly nested to the the netdev shaper.

I now see we can use as default parent the current leaves' parent, when that is the same for all the to-be-grouped leaves.

Actually, if we restrict the group operation to operate only on set of leaves respecting the above, I *guess* we will not lose generality and we could simplify a bit the spec. WDYT?

Thanks,

Paolo

maintainer
#31 lore raw
Quoted 24 lines · On Fri, 23 Aug 2024 09:51:24 +0200 Paolo Abeni wrote:
Quoted 18 lines · Jakub Kicinski
Quoted 7 lines · On Tue, 20 Aug 2024 17:12:21 +0200 Paolo Abeni wrote:

* Delegation

A containers wants to limit the aggregate B/W bandwidth of 2 of the 3 queues it owns - the starting configuration is the one from the previous point:

SPEC=Documentation/netlink/specs/net_shaper.yaml

Quoted 9 lines
./tools/net/ynl/cli.py --spec $SPEC \
	--do group --json '{"ifindex":'$IFINDEX',
			"leaves": [
			  {"handle": {"scope": "queue", "id":'$QID1' },
			   "weight": '$W1'},
			  {"handle": {"scope": "queue", "id":'$QID2' },
			   "weight": '$W2'}],
			"root": { "handle": {"scope": "node"},
				  "parent": {"scope": "node", "id": 0},  
Quoted 1 line

In the delegation use case I was hoping "parent" would be automatic.

Currently the parent is automatic/implicit when creating a node directly nested to the the netdev shaper.

Quoted 6 lines

I now see we can use as default parent the current leaves' parent, when that is the same for all the to-be-grouped leaves.

Actually, if we restrict the group operation to operate only on set of leaves respecting the above, I *guess* we will not lose generality and we could simplify a bit the spec. WDYT?

I remember having a use case in mind where specifying parent would be very useful. I think it may have been related to atomic changes. I'm not sure if what I describe below is exactly that case...

Imagine:

Qx -{hierarchy}---\
\{hierarchy}-- netdev
Q0-------P0\ SP----/
Q1--\ RR-P1/
Q2--/

Let's say we own queues 0,1,2 and want to remove the SP layer. It's convenient to do:

	$node = get($SP-node)
	group(leaves: [Q0, Q1, Q2], parent=$node.parent)

And have the kernel "garbage collect" the old RR node and the old SP node (since they will now have no children). We want to avoid the situations where user space has to do complex transitions thru states which device may not support (make sure Q1, Q2 have right prios, delete old RR, now we have SP w/ 3 inputs, delete the SP, create a new group).

For the case above we could technically identify the correct parent by skipping the nodes which will be garbage collected later. But imagine that instead of deleting the hierarchy we wanted to move Q1 from P1 to P0:

	group(leaves: [Q0, Q1], parent=SP, prio=P0)

does the job.

I admit this are somewhat contrived, and I agree that we won't lose generality, but I think it will narrow the range of hierarchies we can transition between atomically.

author maintainer
#32 lore raw
Quoted 50 lines · Jakub Kicinski
Quoted 24 lines · On Fri, 23 Aug 2024 09:51:24 +0200 Paolo Abeni wrote:
Quoted 18 lines · Jakub Kicinski
Quoted 7 lines · On Tue, 20 Aug 2024 17:12:21 +0200 Paolo Abeni wrote:

* Delegation

A containers wants to limit the aggregate B/W bandwidth of 2 of the 3 queues it owns - the starting configuration is the one from the previous point:

SPEC=Documentation/netlink/specs/net_shaper.yaml

Quoted 9 lines
./tools/net/ynl/cli.py --spec $SPEC \
	--do group --json '{"ifindex":'$IFINDEX',
			"leaves": [
			  {"handle": {"scope": "queue", "id":'$QID1' },
			   "weight": '$W1'},
			  {"handle": {"scope": "queue", "id":'$QID2' },
			   "weight": '$W2'}],
			"root": { "handle": {"scope": "node"},
				  "parent": {"scope": "node", "id": 0},
Quoted 1 line

In the delegation use case I was hoping "parent" would be automatic.

Currently the parent is automatic/implicit when creating a node directly nested to the the netdev shaper.

Quoted 6 lines

I now see we can use as default parent the current leaves' parent, when that is the same for all the to-be-grouped leaves.

Actually, if we restrict the group operation to operate only on set of leaves respecting the above, I *guess* we will not lose generality and we could simplify a bit the spec. WDYT?

I remember having a use case in mind where specifying parent would be very useful. I think it may have been related to atomic changes. I'm not sure if what I describe below is exactly that case...

Imagine:

Qx -{hierarchy}---\
\{hierarchy}-- netdev
Q0-------P0\ SP----/
Q1--\ RR-P1/
Q2--/

Let's say we own queues 0,1,2 and want to remove the SP layer. It's convenient to do:

	$node = get($SP-node)
	group(leaves: [Q0, Q1, Q2], parent=$node.parent)
Quoted 6 lines

And have the kernel "garbage collect" the old RR node and the old SP node (since they will now have no children). We want to avoid the situations where user space has to do complex transitions thru states which device may not support (make sure Q1, Q2 have right prios, delete old RR, now we have SP w/ 3 inputs, delete the SP, create a new group).

FTR, while updating the group() implementation to infer the root's parent handle in most cases, I stumbled upon a similar scenario.

Quoted 2 lines

For the case above we could technically identify the correct parent by skipping the nodes which will be garbage collected later.

I think that implementation would be quite non trivial/error prone, and I think making the new root's parent explicit would be more clear from user-space perspective.

What I have now in my local tree is a group() implementation the inherits the newly created root's parent handle from the leaves, if all of them have the same parent prior to the group() invocation. Otherwise it requires the user to specify the root's parent handle. In any case, the user-specified root's parent handle value overrides the 'inherited'/guessed one.

It will cover the above and will not require an explicit parent in most case. Would that be good enough?

Thanks,

Paolo

maintainer
#33 lore raw
Quoted 14 lines · On Tue, 27 Aug 2024 09:54:52 +0200 Paolo Abeni wrote:
Quoted 2 lines

For the case above we could technically identify the correct parent by skipping the nodes which will be garbage collected later.

I think that implementation would be quite non trivial/error prone, and I think making the new root's parent explicit would be more clear from user-space perspective.

What I have now in my local tree is a group() implementation the inherits the newly created root's parent handle from the leaves, if all of them have the same parent prior to the group() invocation. Otherwise it requires the user to specify the root's parent handle. In any case, the user-specified root's parent handle value overrides the 'inherited'/guessed one.

Quoted 2 lines

It will cover the above and will not require an explicit parent in most case. Would that be good enough?

Yes, that's great.