net: introduce TX H/W shaping API
Subsystems (8)
Paolo Abeni
maintainerv5 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. Finally a basic implementation for the iavf driver is provided.
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'}],
"node": {"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'}],
"node": {"handle": {"scope": "node"},
"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": '$IFINDEX',
"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'}[1] https://lore.kernel.org/netdev/[email protected]/
---
Changes from v4:
- ops operate on struct binding
- 'root' -> 'node' rename
- most core function/helpers operate on 'binding'
- use NL_SET_BAD_ATTR where possible
- some code deduplication
v4: https://lore.kernel.org/netdev/[email protected]/
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]/
Paolo Abeni (8):
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 (2):
iavf: Add net_shaper_ops support
iavf: add support to exchange qos capabilities
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 | 391 +++++ Documentation/networking/kapi.rst | 3 + MAINTAINERS | 1 + drivers/net/Kconfig | 1 + drivers/net/ethernet/intel/Kconfig | 1 + drivers/net/ethernet/intel/iavf/iavf.h | 13 + drivers/net/ethernet/intel/iavf/iavf_main.c | 190 ++- drivers/net/ethernet/intel/iavf/iavf_txrx.h | 2 + .../net/ethernet/intel/iavf/iavf_virtchnl.c | 157 +- 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 | 335 +++++ drivers/net/ethernet/intel/ice/ice_virtchnl.h | 11 + .../intel/ice/ice_virtchnl_allowlist.c | 6 + drivers/net/netdevsim/netdev.c | 43 + include/linux/avf/virtchnl.h | 119 ++ include/linux/netdevice.h | 15 + include/net/net_shaper.h | 121 ++ include/uapi/linux/net_shaper.h | 97 ++ net/Kconfig | 3 + net/Makefile | 1 + net/core/dev.c | 2 + net/core/dev.h | 6 + net/shaper/Makefile | 8 + net/shaper/shaper.c | 1337 +++++++++++++++++ net/shaper/shaper_nl_gen.c | 156 ++ net/shaper/shaper_nl_gen.h | 45 + tools/testing/selftests/drivers/net/Makefile | 1 + tools/testing/selftests/drivers/net/shaper.py | 290 ++++ .../testing/selftests/net/lib/py/__init__.py | 1 + tools/testing/selftests/net/lib/py/ynl.py | 5 + 36 files changed, 3402 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
Paolo Abeni
maintainerPATCH v5 1/12 netlink: spec: add shaper YAML spec
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.
---
v4 -> v5:
- moved ifindex under the binding attr
- moved id, scope to new attr set
- rename 'root' as 'node'
- deleted unused 'info' subset
- a lot of doc update and fixup
- removed empty black line at MAKEFILE eof
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 | 303 ++++++++++++++++++++ MAINTAINERS | 1 + include/uapi/linux/net_shaper.h | 80 ++++++ net/Kconfig | 3 + net/Makefile | 1 + net/shaper/Makefile | 8 + net/shaper/shaper.c | 55 ++++ net/shaper/shaper_nl_gen.c | 127 ++++++++ net/shaper/shaper_nl_gen.h | 35 +++ 9 files changed, 613 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 +303 -0
@@ -0,0 +1,303 @@ +# SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause) + +name: net-shaper + +doc: | + Networking HW rate limiting configuration. + + This API allows configuring HW shapers available on the network + devices 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. + + 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 a single "attached" shaper, and + the @group operation, to create and update a scheduling + group. Only the @group operation can create @node scope shapers + + Existing shapers can be deleted /reset via the @delete operation. + + The user can query the running configuration via the @get operation. + +definitions: + - + type: enum + name: scope + doc: Defines the shaper @id interpretation. + 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, + the @id represents the queue number. + - + name: node + doc: | + The shaper allows grouping of queues or other + node shapers; can be nested in either @netdev + shapers or other @node shapers, allowing placement + in any location of the scheduling tree, except + leaves and root. + - + type: enum + name: metric + doc: Different metric supported by the shaper. + 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: 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 bandwidth for the given shaper. + - + name: bw-max + type: uint + doc: Maximum bandwidth for the given shaper or 0 when unlimited. + - + name: burst + type: uint + doc: | + Maximum burst-size for shaping. Should not be interpreted + as a quantum. + - + name: priority + type: u32 + doc: | + Scheduling priority for the given shaper. The priority + scheduling is applied to sibling shapers. + - + name: weight + type: u32 + doc: | + Relative weight for round robin scheduling of the + given shaper. + The scheduling is applied to all sibling shapers + with the same priority. + - + 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. + Only needed for @group operation. + - + name: leaves + type: nest + multi-attr: true + nested-attributes: info + doc: | + Describes a set of leaves shapers for a @group operation. + - + name: node + type: nest + nested-attributes: node-info + doc: | + Describes the node 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 + attributes: + - + name: scope + type: u32 + enum: scope + doc: Defines the shaper @id interpretation. + - + name: id + type: u32 + doc: | + Numeric identifier of a shaper. The id semantic depends on + the scope. For @queue scope it's the queue id and for @node + scope it's the node identifier. + - + name: info + subset-of: net-shaper + attributes: + - + name: handle + - + name: metric + - + name: bw-min + - + name: bw-max + - + name: burst + - + name: priority + - + name: weight + - + name: node-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 information about a 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: + pre: net-shaper-nl-pre-dumpit + post: net-shaper-nl-post-dumpit + request: + attributes: + - ifindex + reply: + attributes: *ns-attrs + - + name: set + doc: | + Create or update the specified shaper. + The set operation can't be used to create a @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, reattach all the node's leaves to the + deleted node's parent. + If, after the removal, the parent shaper has no more + leaves and the parent shaper scope is @node,the parent + node is deleted, recursively. + When deleting a @queue shaper or a @netdev shaper, + the shaper disappears from the hierarchy, but the + queue/device can still send traffic: it has an implicit + node with infinite bandwidth. Queue's implicit node + feeds an implicit RR node at the root of the hierarchy. + 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: | + Create or update a scheduling group, attaching the specified + @leaves shapers under the specified @node, creating the latter, + if needed. + The @leaves shapers scope must be @queue and the @node shaper + scope must be either @node or @netdev. + When the @node shaper has @node scope, if the @handle @id is not + specified, a new shaper of such scope is created, otherwise the + specified @node must already exist. + The @parent handle for the @node shaper is optional in most cases. + For newly created @node scope shaper, the @parent is set by + default to the parent linked to the @leaves before the @group + operation. If, prior to the grouping operation, the @leaves + have different parents, the @node shaper parent must be explicitly + set. + The operation is atomic, on failure no change is applied to + the device shaping configuration, otherwise the @node shaper + full identifier, comprising @binding 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 + - node + reply: + attributes: *ns-binding
MAINTAINERS +1 -0
@@ -15892,6 +15892,7 @@ F: include/linux/platform_data/wiznet.h F: include/uapi/linux/cn_proc.h F: include/uapi/linux/ethtool_netlink.h F: include/uapi/linux/if_* +F: include/uapi/linux/net_shaper.h F: include/uapi/linux/netdev* F: tools/testing/selftests/drivers/net/ X: drivers/net/wireless/
include/uapi/linux/net_shaper.h new file +80 -0
80 lines not on this page · raw message
net/Kconfig +3 -0
@@ -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
@@ -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 +8 -0
@@ -0,0 +1,8 @@ +# SPDX-License-Identifier: GPL-2.0-only +# +# Makefile for the net shaper infrastructure. +# +# Copyright (c) 2024, Red Hat, Inc. +# + +obj-y += shaper.o shaper_nl_gen.o
net/shaper/shaper.c new file +55 -0
55 lines not on this page · raw message
net/shaper/shaper_nl_gen.c new file +127 -0
127 lines not on this page · raw message
net/shaper/shaper_nl_gen.h new file +35 -0
@@ -0,0 +1,35 @@ +/* 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_HANDLE_ID +1 ]; +extern const struct nla_policy net_shaper_info_nl_policy[NET_SHAPER_A_WEIGHT +1 ]; +extern const struct nla_policy net_shaper_node_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_pre_dumpit (struct netlink_callback *cb); +int net_shaper_nl_post_dumpit (struct netlink_callback *cb); + +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
Paolo Abeni
maintainerPATCH v5 2/12 net-shapers: implement NL get operation
In reply to Paolo AbeniIntroduce 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.
The net_shaper_ctx_init() and net_shaper_generic_pre() implementations handle generic index type attributes, despite the current caller always pass a constant value to avoid more noise in later patches using them.
---
v4 -> v5:
- ops operate on struct binding
- replace net_device * with binding* in most helpers
- include 'ifindex' in get/dump output
- use dev_tracker for real
- user pre/post for dump op, too
- use NL_SET_BAD_ATTR where applicable
- drop redundant/useless kdoc documentation
- add type arg to net_shaper_ctx_init() (moved from later patch)
- factor out generic pre/post helper for later usage in the series
- remove unneeded forward declaration from netdevice.h
- dropped 'inline' modifier in .c file
- dropped black line at net_shaper.h EoF
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 | 15 ++ include/net/net_shaper.h | 121 ++++++++++ net/core/dev.c | 2 + net/core/dev.h | 6 + net/shaper/shaper.c | 363 +++++++++++++++++++++++++++++- 6 files changed, 503 insertions(+), 7 deletions(-) create mode 100644 include/net/net_shaper.h
Documentation/networking/kapi.rst +3 -0
@@ -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 +15 -0
@@ -1603,6 +1603,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 };/**
@@ -2383,6 +2391,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 +121 -0
121 lines not on this page · raw message
net/core/dev.c +2 -0
@@ -11201,6 +11201,8 @@ void free_netdev(struct net_device *dev)/* Flush device addresses */ dev_addr_flush (dev); +net_shaper_flush_netdev (dev); +list_for_each_entry_safe (p, n, &dev->napi_list, dev_list)netif_napi_del (p);
net/core/dev.h +6 -0
@@ -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_netdev (struct net_device *dev); +#else +static inline void net_shaper_flush_netdev (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 +356 -7
400 lines not on this page · raw message
Signature · 1 line
2.45.2
Paolo Abeni
maintainerPATCH v5 3/12 net-shapers: implement NL set and delete operations
In reply to Paolo AbeniBoth 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.
---
v4 -> v5:
- replace net_device * with binding* in most helpers
- move check for scope NONE handle at parse time and leverage NL_SET_BAD_ATTR()
- move the default parent initialization to net_shaper_parse_info_nest()
v3 -> v4:
- add locking
- helper rename
RFC v2 -> RFC v3:
- dev_put() -> netdev_put()
--- net/shaper/shaper.c | 398 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 395 insertions(+), 3 deletions(-)
net/shaper/shaper.c +395 -3
@@ -23,6 +23,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 {
@@ -47,6 +51,27 @@ net_shaper_binding_data(struct net_shaper_binding *binding)return NULL ; } +static struct net_shaper_data * +net_shaper_binding_set_data (struct net_shaper_binding *binding, +struct net_shaper_data *data) +{ +if (binding->type ==NET_SHAPER_BINDING_TYPE_NETDEV ) +return cmpxchg (&binding->netdev->net_shaper_data,NULL , data); + +/* No devlink implementation yet.*/ +return NULL ; +} + +static const struct net_shaper_ops * +net_shaper_binding_ops (struct net_shaper_binding *binding) +{ +if (binding->type ==NET_SHAPER_BINDING_TYPE_NETDEV ) +return binding->netdev->netdev_ops->net_shaper_ops; + +/* No devlink implementation yet.*/ +return NULL ; +} +static int net_shaper_fill_binding (struct sk_buff *msg,const struct net_shaper_binding *binding,u32 type)
@@ -178,6 +203,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 +/* Lookup the given shaper inside the cache. */ static struct net_shaper_info * net_shaper_cache_lookup(struct net_shaper_binding *binding,
@@ -186,7 +231,132 @@ net_shaper_cache_lookup(struct net_shaper_binding *binding,struct net_shaper_data *data =net_shaper_binding_data (binding);u32 index =net_shaper_handle_to_index (handle); -return data ?xa_load (&data->shapers, index) :NULL ; +if (!data ||xa_get_mark (&data->shapers, index, +NET_SHAPER_CACHE_NOT_VALID )) +return NULL ; + +return xa_load (&data->shapers, index); +} + +/* Allocate on demand the per device shaper's cache. */ +static struct net_shaper_data * +net_shaper_cache_init (struct net_shaper_binding *binding, +struct netlink_ext_ack *extack) +{ +struct net_shaper_data *new, *data =net_shaper_binding_data (binding); + +if (!data) { + new =kmalloc (sizeof (*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 =net_shaper_binding_set_data (binding, new); +if (!data) + data = new; +else +kfree (new); + } +return data; +} + +/* Prepare the cache to actually insert the given shaper, doing +* in advance the needed allocations. +*/ +static int net_shaper_cache_pre_insert (struct net_shaper_binding *binding, +struct net_shaper_handle *handle, +struct netlink_ext_ack *extack) +{ +struct net_shaper_data *data =net_shaper_binding_data (binding); +struct net_shaper_info *prev, *cur; +bool id_allocated = false; +int ret, id, index; + +if (!data) +return -ENOMEM ; + + index =net_shaper_handle_to_index (handle); + cur =xa_load (&data->shapers, 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 (&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 (&data->shapers); + prev =__xa_store (&data->shapers, index, cur,GFP_KERNEL ); +__xa_set_mark (&data->shapers, index,NET_SHAPER_CACHE_NOT_VALID ); +xa_unlock (&data->shapers); +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 (&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_shaper_binding *binding, +int nr_shapers, +const struct net_shaper_handle *handle, +const struct net_shaper_info *shapers) +{ +struct net_shaper_data *data =net_shaper_binding_data (binding); +struct net_shaper_info *cur; +int index; +int i; + +xa_lock (&data->shapers); +for (i =0 ; i < nr_shapers; ++i) { + index =net_shaper_handle_to_index (&handle[i]); + + cur =xa_load (&data->shapers, index); +if (WARN_ON_ONCE (!cur)) +continue ; + +/* Successful update: drop the tentative mark +* and update the cache. +*/ +__xa_clear_mark (&data->shapers, index, +NET_SHAPER_CACHE_NOT_VALID ); + *cur = shapers[i]; + } +xa_unlock (&data->shapers); } static int net_shaper_parse_handle(const struct nlattr *attr,
@@ -227,6 +397,85 @@ static int net_shaper_parse_handle(const struct nlattr *attr,return 0 ; } +static int net_shaper_parse_info (struct net_shaper_binding *binding, +struct nlattr **tb, +const struct genl_info *info, +struct net_shaper_handle *handle, +struct net_shaper_info *shaper, +bool *cached) +{ +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; + +if (handle->scope ==NET_SHAPER_SCOPE_UNSPEC ) { +NL_SET_BAD_ATTR (info->extack, + info->attrs[NET_SHAPER_A_HANDLE ]); +return -EINVAL ; + } + +/* Fetch existing data, if any, so that user provide info will +* incrementally update the existing shaper configuration. +*/ + old =net_shaper_cache_lookup (binding, handle); +if (old) + *shaper = *old; + *cached = !!old; + +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_shaper_binding *binding, +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 ]; +bool cached; +int ret; + + ret =nla_parse_nested (tb,NET_SHAPER_A_WEIGHT , attr, + net_shaper_info_nl_policy, info->extack); +if (ret <0 ) +return ret; + + ret =net_shaper_parse_info (binding, tb, info, handle, shaper, &cached); +if (ret <0 ) +return ret; + +if (!cached) +net_shaper_default_parent (handle, &shaper->parent); +return 0 ; +} +static int net_shaper_generic_pre (struct genl_info *info,int type) {struct net_shaper_nl_ctx *ctx;
@@ -358,14 +607,153 @@ int net_shaper_nl_get_dumpit(struct sk_buff *skb,return 0 ; } +/* Update the H/W and on success update the local cache, too. */ +static int net_shaper_set (struct net_shaper_binding *binding, +const struct net_shaper_handle *h, +const struct net_shaper_info *shaper, +struct netlink_ext_ack *extack) +{ +struct net_shaper_data *data =net_shaper_cache_init (binding, extack); +const struct net_shaper_ops *ops =net_shaper_binding_ops (binding); +struct net_shaper_handle handle = *h; +int ret; + +if (!data) +return -ENOMEM ; + +/* Should never happen: binding lookup validates the ops presence */ +if (WARN_ON_ONCE (!ops)) +return -EOPNOTSUPP ; + +mutex_lock (&data->lock); +if (handle.scope ==NET_SHAPER_SCOPE_NODE && +net_shaper_cache_lookup (binding, &handle)) { + ret = -ENOENT ; + goto unlock; + } + + ret =net_shaper_cache_pre_insert (binding, &handle, extack); +if (ret) + goto unlock; + + ret = ops->set (binding, &handle, shaper, extack); +net_shaper_cache_commit (binding,1 , &handle, shaper); + +unlock: +mutex_unlock (&data->lock); +return ret; +} +int net_shaper_nl_set_doit (struct sk_buff *skb,struct genl_info *info) { -return -EOPNOTSUPP ; +struct net_shaper_binding *binding; +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 ; + + binding =net_shaper_binding_from_ctx (info->user_ptr[0 ]); + attr = info->attrs[NET_SHAPER_A_SHAPER ]; + ret =net_shaper_parse_info_nest (binding, attr, info, &handle, + &shaper); +if (ret) +return ret; + +return net_shaper_set (binding, &handle, &shaper, info->extack); +} + +static int __net_shaper_delete (struct net_shaper_binding *binding, +const struct net_shaper_handle *h, +struct net_shaper_info *shaper, +struct netlink_ext_ack *extack) +{ +const struct net_shaper_ops *ops =net_shaper_binding_ops (binding); +struct net_shaper_data *data =net_shaper_binding_data (binding); +struct net_shaper_handle parent_handle, handle = *h; +int ret; + +/* Should never happen: we are under the cache lock, the cache +* is already initialized. +*/ +if (WARN_ON_ONCE (!data || !ops)) +return -EINVAL ; + +again: + parent_handle = shaper->parent; + + ret = ops->delete (binding, &handle, extack); +if (ret <0 ) +return ret; + +xa_erase (&data->shapers,net_shaper_handle_to_index (&handle)); +if (handle.scope ==NET_SHAPER_SCOPE_NODE ) +idr_remove (&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 (binding, &parent_handle); +if (shaper && !--shaper->leaves) { + handle = parent_handle; + goto again; + } + } +return 0 ; +} + +static int net_shaper_delete (struct net_shaper_binding *binding, +const struct net_shaper_handle *handle, +struct netlink_ext_ack *extack) +{ +struct net_shaper_data *data =net_shaper_binding_data (binding); +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 (!data) +return -ENOENT ; + +mutex_lock (&data->lock); + shaper =net_shaper_cache_lookup (binding, 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 (binding, handle, shaper, extack); + +unlock: +mutex_unlock (&data->lock); +return ret; }int net_shaper_nl_delete_doit (struct sk_buff *skb,struct genl_info *info) { -return -EOPNOTSUPP ; +struct net_shaper_binding *binding; +struct net_shaper_handle handle; +int ret; + +if (GENL_REQ_ATTR_CHECK (info,NET_SHAPER_A_HANDLE )) +return -EINVAL ; + + binding =net_shaper_binding_from_ctx (info->user_ptr[0 ]); + ret =net_shaper_parse_handle (info->attrs[NET_SHAPER_A_HANDLE ], info, + &handle); +if (ret) +return ret; + +return net_shaper_delete (binding, &handle, info->extack); }static void net_shaper_flush (struct net_shaper_binding *binding)
@@ -377,12 +765,16 @@ static void net_shaper_flush(struct net_shaper_binding *binding)if (!data)return ; +mutex_lock (&data->lock);xa_lock (&data->shapers);xa_for_each (&data->shapers, index, cur) {__xa_erase (&data->shapers, index);kfree (cur); }xa_unlock (&data->shapers); +idr_destroy (&data->node_ids); +mutex_unlock (&data->lock); +kfree (data); }
Signature · 1 line
2.45.2
Paolo Abeni
maintainerPATCH v5 4/12 net-shapers: implement NL group operation
In reply to Paolo AbeniAllow 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.
---
v4 -> v5:
- replace net_device* with binding* in most helpers
- factor out net_shaper_fill_binding() helper for re-use in later patch
- move most sanity check at parse time and use NL_SET_BAD_ATTR
- reused net_shaper_fill_handle() in net_shaper_group_send_reply() instead of open-coding it.
v3 -> v4:
- 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 | 348 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 346 insertions(+), 2 deletions(-)
net/shaper/shaper.c +346 -2
@@ -72,6 +72,24 @@ net_shaper_binding_ops(struct net_shaper_binding *binding)return NULL ; } +/* 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_binding (struct sk_buff *msg,const struct net_shaper_binding *binding,u32 type)
@@ -359,6 +377,29 @@ static void net_shaper_cache_commit(struct net_shaper_binding *binding,xa_unlock (&data->shapers); } +/* Rollback all the tentative inserts from the shaper cache. */ +static void net_shaper_cache_rollback (struct net_shaper_binding *binding) +{ +struct net_shaper_data *data =net_shaper_binding_data (binding); +struct net_shaper_handle handle; +struct net_shaper_info *cur; +unsigned long index; + +if (!data) +return ; + +xa_lock (&data->shapers); +xa_for_each_marked (&data->shapers, index, cur, +NET_SHAPER_CACHE_NOT_VALID ) { +net_shaper_index_to_handle (index, &handle); +if (handle.scope ==NET_SHAPER_SCOPE_NODE ) +idr_remove (&data->node_ids, handle.id); +__xa_erase (&data->shapers, index); +kfree (cur); + } +xa_unlock (&data->shapers); +} +static int net_shaper_parse_handle (const struct nlattr *attr,const struct genl_info *info,struct net_shaper_handle *handle)
@@ -455,6 +496,7 @@ static int net_shaper_parse_info(struct net_shaper_binding *binding,static int net_shaper_parse_info_nest (struct net_shaper_binding *binding,const struct nlattr *attr,const struct genl_info *info, +enum net_shaper_scope expected_scope,struct net_shaper_handle *handle,struct net_shaper_info *shaper) {
@@ -471,11 +513,62 @@ static int net_shaper_parse_info_nest(struct net_shaper_binding *binding,if (ret <0 )return ret; +if (expected_scope !=NET_SHAPER_SCOPE_UNSPEC && + handle->scope != expected_scope) { +NL_SET_BAD_ATTR (info->extack, tb[NET_SHAPER_A_HANDLE ]); +return -EINVAL ; + } +if (!cached)net_shaper_default_parent (handle, &shaper->parent);return 0 ; } +/* Alike net_parse_shaper_info(), but additionally allow the user specifying +* the shaper's parent handle. +*/ +static int net_shaper_parse_node (struct net_shaper_binding *binding, +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 ]; +bool cached; +int ret; + + ret =nla_parse_nested (tb,NET_SHAPER_A_PARENT , attr, + net_shaper_node_info_nl_policy, + info->extack); +if (ret <0 ) +return ret; + + ret =net_shaper_parse_info (binding, tb, info, handle, shaper, + &cached); +if (ret) +return ret; + +if (handle->scope !=NET_SHAPER_SCOPE_NODE && + handle->scope !=NET_SHAPER_SCOPE_NETDEV ) { +NL_SET_BAD_ATTR (info->extack, tb[NET_SHAPER_A_HANDLE ]); +return -EINVAL ; + } + +if (tb[NET_SHAPER_A_PARENT ]) { + ret =net_shaper_parse_handle (tb[NET_SHAPER_A_PARENT ], info, + &shaper->parent); +if (ret) +return ret; + +if (shaper->parent.scope !=NET_SHAPER_SCOPE_NODE && + shaper->parent.scope !=NET_SHAPER_SCOPE_NETDEV ) { +NL_SET_BAD_ATTR (info->extack, tb[NET_SHAPER_A_PARENT ]); +return -EINVAL ; + } + } +return 0 ; +} +static int net_shaper_generic_pre (struct genl_info *info,int type) {struct net_shaper_nl_ctx *ctx;
@@ -657,8 +750,9 @@ int net_shaper_nl_set_doit(struct sk_buff *skb, struct genl_info *info) binding =net_shaper_binding_from_ctx (info->user_ptr[0 ]); attr = info->attrs[NET_SHAPER_A_SHAPER ]; - ret =net_shaper_parse_info_nest (binding, attr, info, &handle, - &shaper); + ret =net_shaper_parse_info_nest (binding, attr, info, +NET_SHAPER_SCOPE_UNSPEC , + &handle, &shaper);if (ret)return ret;
@@ -704,6 +798,100 @@ static int __net_shaper_delete(struct net_shaper_binding *binding,return 0 ; } +static int __net_shaper_group (struct net_shaper_binding *binding, +int leaves_count, +const struct net_shaper_handle *leaves_handles, +struct net_shaper_info *leaves, +struct net_shaper_handle *node_handle, +struct net_shaper_info *node, +struct netlink_ext_ack *extack) +{ +const struct net_shaper_ops *ops =net_shaper_binding_ops (binding); +struct net_shaper_info *parent =NULL ; +struct net_shaper_handle leaf_handle; +int i, ret; + +if (node_handle->scope ==NET_SHAPER_SCOPE_NODE ) { +if (node_handle->id !=NET_SHAPER_ID_UNSPEC && + !net_shaper_cache_lookup (binding, node_handle)) { +NL_SET_ERR_MSG_FMT (extack,"Node shaper %d:%d does not exists" , + node_handle->scope, node_handle->id); +return -ENOENT ; + } + +/* When unspecified, the node parent scope is inherited from +* the leaves. +*/ +if (node->parent.scope ==NET_SHAPER_SCOPE_UNSPEC ) { +for (i =1 ; i < leaves_count; ++i) { +if (leaves[i].parent.scope != + leaves[0 ].parent.scope || + leaves[i].parent.id != + leaves[0 ].parent.id) { +NL_SET_ERR_MSG_FMT (extack,"All the leaves shapers must have the same old parent" ); +return -EINVAL ; + } + } + +if (leaves_count >0 ) + node->parent = leaves[0 ].parent; + } + + }else { +net_shaper_default_parent (node_handle, &node->parent); + } + +if (node->parent.scope ==NET_SHAPER_SCOPE_NODE ) { + parent =net_shaper_cache_lookup (binding, &node->parent); +if (!parent) { +NL_SET_ERR_MSG_FMT (extack,"Node parent shaper %d:%d does not exists" , + node->parent.scope, node->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 (binding, node_handle, extack); +if (ret) +return ret; + +for (i =0 ; i < leaves_count; ++i) { + leaf_handle = leaves_handles[i]; + + ret =net_shaper_cache_pre_insert (binding, &leaf_handle, + extack); +if (ret) + goto rollback; + +if (leaves[i].parent.scope == node_handle->scope && + leaves[i].parent.id == node_handle->id) +continue ; + +/* The leaves shapers will be nested to the node, update the +* linking accordingly. +*/ + leaves[i].parent = *node_handle; + node->leaves++; + } + + ret = ops->group (binding, leaves_count, leaves_handles, leaves, + node_handle, node, extack); +if (ret <0 ) + goto rollback; + +if (parent) + parent->leaves++; +net_shaper_cache_commit (binding,1 , node_handle, node); +net_shaper_cache_commit (binding, leaves_count, leaves_handles, leaves); +return 0 ; + +rollback: +net_shaper_cache_rollback (binding); +return ret; +} +static int net_shaper_delete (struct net_shaper_binding *binding,const struct net_shaper_handle *handle,struct netlink_ext_ack *extack)
@@ -756,6 +944,162 @@ int net_shaper_nl_delete_doit(struct sk_buff *skb, struct genl_info *info)return net_shaper_delete (binding, &handle, info->extack); } +/* Update the H/W and on success update the local cache, too */ +static int net_shaper_group (struct net_shaper_binding *binding, +int leaves_count, +const struct net_shaper_handle *leaves_handles, +struct net_shaper_info *leaves, +struct net_shaper_handle *node_handle, +struct net_shaper_info *node, +struct netlink_ext_ack *extack) +{ +struct net_shaper_data *data =net_shaper_cache_init (binding, extack); +struct net_shaper_handle *old_nodes; +int i, ret, old_nodes_count =0 ; + +if (!data) +return -ENOMEM ; + + old_nodes =kcalloc (leaves_count,sizeof (struct net_shaper_handle ), +GFP_KERNEL ); +if (!old_nodes) +return -ENOMEM ; + +for (i =0 ; i < leaves_count; i++) +if (leaves[i].parent.scope ==NET_SHAPER_SCOPE_NODE && + (leaves[i].parent.scope != node_handle->scope || + leaves[i].parent.id != node_handle->id)) + old_nodes[old_nodes_count++] = leaves[i].parent; + +mutex_lock (&data->lock); + ret =__net_shaper_group (binding, leaves_count, leaves_handles, + leaves, node_handle, node, extack); + +/* Check if we need to delete any NODE left alone by the new leaves +* linkage. +*/ +for (i =0 ; i < old_nodes_count; ++i) { + node =net_shaper_cache_lookup (binding, &old_nodes[i]); +if (!node) +continue ; + +if (--node->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 (binding, &old_nodes[i], node, extack); + } + +mutex_unlock (&data->lock); + +kfree (old_nodes); +return ret; +} + +static int net_shaper_group_send_reply (struct genl_info *info, +struct net_shaper_handle *handle) +{ +struct net_shaper_binding *binding = info->user_ptr[0 ]; +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 (net_shaper_fill_binding (msg, binding,NET_SHAPER_A_IFINDEX )) + goto free_msg; + +if (net_shaper_fill_handle (msg, handle,NET_SHAPER_A_HANDLE )) + goto free_msg; + +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) +{ +struct net_shaper_handle *leaves_handles, node_handle; +struct net_shaper_info *leaves, node; +struct net_shaper_binding *binding; +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_NODE )) +return -EINVAL ; + + binding =net_shaper_binding_from_ctx (info->user_ptr[0 ]); + 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_node (binding, info->attrs[NET_SHAPER_A_NODE ], + info, &node_handle, &node); +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 (binding, attr, info, +NET_SHAPER_SCOPE_QUEUE , + &leaves_handles[i], + &leaves[i]); +if (ret) + goto free_shapers; + i++; + } + + ret =net_shaper_group (binding, leaves_count, leaves_handles, leaves, + &node_handle, &node, info->extack); +if (ret <0 ) + goto free_shapers; + + ret =net_shaper_group_send_reply (info, &node_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; +} +static void net_shaper_flush (struct net_shaper_binding *binding) {struct net_shaper_data *data =net_shaper_binding_data (binding);
Signature · 1 line
2.45.2
Paolo Abeni
maintainerPATCH v5 5/12 net-shapers: implement delete support for NODE scope shaper
In reply to Paolo AbeniLeverage 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.
---
v4 -> v5:
- replace net_device* with binding* in most helpers
--- net/shaper/shaper.c | 99 +++++++++++++++++++++++++++++++++++++++------ 1 file changed, 87 insertions(+), 12 deletions(-)
net/shaper/shaper.c +87 -12
@@ -799,7 +799,7 @@ static int __net_shaper_delete(struct net_shaper_binding *binding, } static int __net_shaper_group(struct net_shaper_binding *binding, - int leaves_count, + bool cache_node, int leaves_count, const struct net_shaper_handle *leaves_handles, struct net_shaper_info *leaves, struct net_shaper_handle *node_handle,
@@ -850,12 +850,15 @@ static int __net_shaper_group(struct net_shaper_binding *binding, } } -/* For newly created node scope shaper, the following will update -* the handle, due to id allocation. -*/ - ret =net_shaper_cache_pre_insert (binding, node_handle, extack); -if (ret) -return ret; +if (cache_node) { +/* For newly created node scope shaper, the following will +* update the handle, due to id allocation. +*/ + ret =net_shaper_cache_pre_insert (binding, node_handle, + extack); +if (ret) +return ret; + }for (i =0 ; i < leaves_count; ++i) { leaf_handle = leaves_handles[i];
@@ -883,7 +886,8 @@ static int __net_shaper_group(struct net_shaper_binding *binding,if (parent) parent->leaves++; -net_shaper_cache_commit (binding,1 , node_handle, node); +if (cache_node) +net_shaper_cache_commit (binding,1 , node_handle, node);net_shaper_cache_commit (binding, leaves_count, leaves_handles, leaves);return 0 ;
@@ -892,6 +896,76 @@ static int __net_shaper_group(struct net_shaper_binding *binding,return ret; } +static int __net_shaper_pre_del_node (struct net_shaper_binding *binding, +const struct net_shaper_handle *handle, +const struct net_shaper_info *shaper, +struct netlink_ext_ack *extack) +{ +struct net_shaper_data *data =net_shaper_binding_data (binding); +struct net_shaper_handle *leaves_handles, node_handle; +struct net_shaper_info *cur, *leaves, node = {}; +int ret, leaves_count =0 ; +unsigned long index; +bool cache_node; + +if (!shaper->leaves) +return 0 ; + +if (WARN_ON_ONCE (!data)) +return -EINVAL ; + +/* Fetch the new node information. */ + node_handle = shaper->parent; + cur =net_shaper_cache_lookup (binding, &node_handle); +if (cur) { + node = *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 (node_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 (&data->shapers, 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 node, would be surprising since the user is +* doing a delete operation. +*/ + cache_node = node_handle.scope !=NET_SHAPER_SCOPE_NETDEV ; + ret =__net_shaper_group (binding, cache_node, leaves_count, + leaves_handles, leaves, &node_handle, &node, + extack); + +free: +kfree (leaves); +return ret; +} +static int net_shaper_delete (struct net_shaper_binding *binding,const struct net_shaper_handle *handle,struct netlink_ext_ack *extack)
@@ -914,9 +988,10 @@ static int net_shaper_delete(struct net_shaper_binding *binding, }if (handle->scope ==NET_SHAPER_SCOPE_NODE ) { -/* TODO: implement support for scope NODE delete. */ - ret = -EINVAL ; - goto unlock; + ret =__net_shaper_pre_del_node (binding, handle, shaper, + extack); +if (ret) + goto unlock; } ret =__net_shaper_delete (binding, handle, shaper, extack);
@@ -972,7 +1047,7 @@ static int net_shaper_group(struct net_shaper_binding *binding, old_nodes[old_nodes_count++] = leaves[i].parent;mutex_lock (&data->lock); - ret =__net_shaper_group (binding, leaves_count, leaves_handles, + ret =__net_shaper_group (binding, true, leaves_count, leaves_handles, leaves, node_handle, node, extack);/* Check if we need to delete any NODE left alone by the new leaves
Signature · 1 line
2.45.2
Paolo Abeni
maintainerPATCH v5 6/12 netlink: spec: add shaper introspection support
In reply to Paolo AbeniAllow the user-space to fine-grain query the shaping features supported by the NIC on each domain.
---
v4 -> v5:
- added pre/post helpers for dump op
--- Documentation/netlink/specs/net_shaper.yaml | 88 +++++++++++++++++++++ include/uapi/linux/net_shaper.h | 17 ++++ net/shaper/shaper.c | 32 ++++++++ net/shaper/shaper_nl_gen.c | 29 +++++++ net/shaper/shaper_nl_gen.h | 10 +++ 5 files changed, 176 insertions(+)
Documentation/netlink/specs/net_shaper.yaml +88 -0
@@ -27,6 +27,11 @@ doc: | The user can query the running configuration via the @get operation. + Different devices can provide different feature sets, 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
@@ -191,6 +196,53 @@ attribute-sets: name: priority - name: weight + - + name: capabilities + attributes: + - + name: ifindex + type: u32 + doc: Interface index queried for shapers capabilities. + - + 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:
@@ -301,3 +353,39 @@ operations: - node reply: attributes: *ns-binding + + - + name: cap-get + doc: | + Get the shaper capabilities supported by the given device + for the specified scope. + 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: + pre: net-shaper-nl-cap-pre-dumpit + post: net-shaper-nl-cap-post-dumpit + request: + attributes: + - ifindex + reply: + attributes: *cap-attrs
include/uapi/linux/net_shaper.h +17 -0
@@ -67,11 +67,28 @@ enum {NET_SHAPER_A_HANDLE_MAX = (__NET_SHAPER_A_HANDLE_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 +32 -0
@@ -626,6 +626,27 @@ int net_shaper_nl_post_dumpit(struct netlink_callback *cb)return 0 ; } +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_cap_pre_dumpit (struct netlink_callback *cb) +{ +return -EOPNOTSUPP ; +} + +int net_shaper_nl_cap_post_dumpit (struct netlink_callback *cb) +{ +return -EOPNOTSUPP ; +} +int net_shaper_nl_get_doit (struct sk_buff *skb,struct genl_info *info) {struct net_shaper_binding *binding;
@@ -1175,6 +1196,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 ; +} +static void net_shaper_flush (struct net_shaper_binding *binding) {struct net_shaper_data *data =net_shaper_binding_data (binding);
net/shaper/shaper_nl_gen.c +29 -0
@@ -67,6 +67,17 @@ static const struct nla_policy net_shaper_group_nl_policy[NET_SHAPER_A_NODE + 1] [NET_SHAPER_A_NODE] = NLA_POLICY_NESTED(net_shaper_node_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[] = { {
@@ -114,6 +125,24 @@ static const struct genl_split_ops net_shaper_nl_ops[] = { .maxattr = NET_SHAPER_A_NODE, .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, + .start = net_shaper_nl_cap_pre_dumpit, + .dumpit = net_shaper_nl_cap_get_dumpit, + .done = net_shaper_nl_cap_post_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 +10 -0
@@ -18,17 +18,27 @@ extern const struct nla_policy net_shaper_node_info_nl_policy[NET_SHAPER_A_PARENint 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_pre_dumpit (struct netlink_callback *cb); +int net_shaper_nl_cap_pre_dumpit (struct netlink_callback *cb);int net_shaper_nl_post_dumpit (struct netlink_callback *cb); +int net_shaper_nl_cap_post_dumpit (struct netlink_callback *cb);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
Paolo Abeni
maintainerPATCH v5 7/12 net: shaper: implement introspection support
In reply to Paolo AbeniThe 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.
---
v4 -> v5:
- replace net_device* with binding* in most functions
- de-deplicate some code thanks to more generic helpers in previous patches
v3 -> v4:
- another dev_put() -> netdev_put() conversion, missed in previous iteration
RFC v2 -> v3:
- dev_put() -> netdev_put()
--- net/shaper/shaper.c | 96 +++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 93 insertions(+), 3 deletions(-)
net/shaper/shaper.c +93 -3
@@ -629,22 +629,29 @@ int net_shaper_nl_post_dumpit(struct netlink_callback *cb)int net_shaper_nl_cap_pre_doit (const struct genl_split_ops *ops,struct sk_buff *skb,struct genl_info *info) { -return -EOPNOTSUPP ; +return net_shaper_generic_pre (info,NET_SHAPER_A_CAPABILITIES_IFINDEX ); }void net_shaper_nl_cap_post_doit (const struct genl_split_ops *ops,struct sk_buff *skb,struct genl_info *info) { +net_shaper_generic_post (info); }int net_shaper_nl_cap_pre_dumpit (struct netlink_callback *cb) { -return -EOPNOTSUPP ; +struct net_shaper_nl_ctx *ctx = (struct net_shaper_nl_ctx *)cb->ctx; + +return net_shaper_ctx_init (genl_info_dump (cb), +NET_SHAPER_A_CAPABILITIES_IFINDEX , ctx); }int net_shaper_nl_cap_post_dumpit (struct netlink_callback *cb) { -return -EOPNOTSUPP ; +struct net_shaper_nl_ctx *ctx = (struct net_shaper_nl_ctx *)cb->ctx; + +net_shaper_ctx_cleanup (ctx); +return 0 ; }int net_shaper_nl_get_doit (struct sk_buff *skb,struct genl_info *info)
@@ -1196,14 +1203,97 @@ int net_shaper_nl_group_doit(struct sk_buff *skb, struct genl_info *info)return ret; } +static int +net_shaper_cap_fill_one (struct sk_buff *msg, +struct net_shaper_binding *binding, +enum net_shaper_scope scope,unsigned long flags, +const struct genl_info *info) +{ +unsigned long cur; +void *hdr; + + hdr =genlmsg_iput (msg, info); +if (!hdr) +return -EMSGSIZE ; + +if (net_shaper_fill_binding (msg, binding, +NET_SHAPER_A_CAPABILITIES_IFINDEX ) || +nla_put_u32 (msg,NET_SHAPER_A_CAPABILITIES_SCOPE , scope)) + goto nla_put_failure; + +for (cur =NET_SHAPER_A_CAPABILITIES_SUPPORT_METRIC_BPS ; + cur <=NET_SHAPER_A_CAPABILITIES_MAX ; ++cur) { +if (flags &BIT (cur) &&nla_put_flag (msg, cur)) + goto nla_put_failure; + } + +genlmsg_end (msg, hdr); + +return 0 ; + +nla_put_failure: +genlmsg_cancel (msg, hdr); +return -EMSGSIZE ; +} +int net_shaper_nl_cap_get_doit (struct sk_buff *skb,struct genl_info *info) { +struct net_shaper_binding *binding = info->user_ptr[0 ]; +const struct net_shaper_ops *ops; +enum net_shaper_scope scope; +struct sk_buff *msg; +unsigned long flags; +int ret; + +if (GENL_REQ_ATTR_CHECK (info,NET_SHAPER_A_CAPABILITIES_SCOPE )) +return -EINVAL ; + + scope =nla_get_u32 (info->attrs[NET_SHAPER_A_CAPABILITIES_SCOPE ]); + ops =net_shaper_binding_ops (binding); + ret = ops->capabilities (binding, scope, &flags); +if (ret) +return ret; + + msg =genlmsg_new (NLMSG_DEFAULT_SIZE ,GFP_KERNEL ); +if (!msg) +return -ENOMEM ; + + ret =net_shaper_cap_fill_one (msg, binding, scope, flags, info); +if (ret) + goto free_msg; + + ret =genlmsg_reply (msg, info); +if (ret) + goto free_msg;return 0 ; + +free_msg: +nlmsg_free (msg); +return ret; }int net_shaper_nl_cap_get_dumpit (struct sk_buff *skb,struct netlink_callback *cb) { +const struct genl_info *info =genl_info_dump (cb); +struct net_shaper_binding *binding; +const struct net_shaper_ops *ops; +enum net_shaper_scope scope; +unsigned long flags; +int ret; + + binding =net_shaper_binding_from_ctx (cb->ctx); + ops =net_shaper_binding_ops (binding); +for (scope =0 ; scope <=NET_SHAPER_SCOPE_MAX ; ++scope) { +if (ops->capabilities (binding, scope, &flags)) +continue ; + + ret =net_shaper_cap_fill_one (skb, binding, scope, flags, + info); +if (ret) +return ret; + } +return 0 ; }
Signature · 1 line
2.45.2
Paolo Abeni
maintainerPATCH v5 8/12 testing: net-drv: add basic shaper test
In reply to Paolo AbeniLeverage a basic/dummy netdevsim implementation to do functional coverage for NL interface.
---
v4 -> v5:
- updated to new driver API
- more consistent indentation
rfc v1 -> v2:
- added more test-cases WRT nesting and grouping
--- drivers/net/Kconfig | 1 + drivers/net/netdevsim/netdev.c | 43 +++ tools/testing/selftests/drivers/net/Makefile | 1 + tools/testing/selftests/drivers/net/shaper.py | 290 ++++++++++++++++++ .../testing/selftests/net/lib/py/__init__.py | 1 + tools/testing/selftests/net/lib/py/ynl.py | 5 + 6 files changed, 341 insertions(+) create mode 100755 tools/testing/selftests/drivers/net/shaper.py
drivers/net/Kconfig +1 -0
@@ -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 +43 -0
61 lines not on this page · raw message
tools/testing/selftests/drivers/net/Makefile +1 -0
@@ -9,6 +9,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 +290 -0
290 lines not on this page · raw message
tools/testing/selftests/net/lib/py/__init__.py +1 -0
@@ -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
@@ -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
Paolo Abeni
maintainerPATCH v5 9/12 virtchnl: support queue rate limit and quanta size configuration
In reply to Paolo AbeniFrom: 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
@@ -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
Paolo Abeni
maintainerPATCH v5 10/12 ice: Support VF queue rate limit and quanta size configuration
In reply to Paolo AbeniFrom: 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 | 335 ++++++++++++++++++ drivers/net/ethernet/intel/ice/ice_virtchnl.h | 11 + .../intel/ice/ice_virtchnl_allowlist.c | 6 + 10 files changed, 395 insertions(+)
drivers/net/ethernet/intel/ice/ice.h +2 -0
@@ -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
@@ -349,6 +349,8 @@ ice_setup_tx_ctx(struct ice_tx_ring *ring, struct ice_tlan_ctx *tlan_ctx, u16 pfbreak ; } + 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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 +335 -0
365 lines not on this page · raw message
drivers/net/ethernet/intel/ice/ice_virtchnl.h +11 -0
@@ -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
@@ -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
Paolo Abeni
maintainerPATCH v5 11/12 iavf: Add net_shaper_ops support
In reply to Paolo AbeniFrom: 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.
---
v4 -> v5:
- fix kdoc
--- drivers/net/ethernet/intel/Kconfig | 1 + drivers/net/ethernet/intel/iavf/iavf.h | 3 + drivers/net/ethernet/intel/iavf/iavf_main.c | 141 ++++++++++++++++++ drivers/net/ethernet/intel/iavf/iavf_txrx.h | 2 + .../net/ethernet/intel/iavf/iavf_virtchnl.c | 65 ++++++++ 5 files changed, 212 insertions(+)
drivers/net/ethernet/intel/Kconfig +1 -0
@@ -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
@@ -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 +141 -0
171 lines not on this page · raw message
drivers/net/ethernet/intel/iavf/iavf_txrx.h +2 -0
@@ -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
Paolo Abeni
maintainerPATCH v5 12/12 iavf: add support to exchange qos capabilities
In reply to Paolo AbeniFrom: Sudheer Mogilappagari <[email protected]>
During driver initialization VF determines QOS capability is allowed by PF and receives QOS parameters. After which quanta size for queues is configured which is not configurable and is set to 1KB currently.
--- drivers/net/ethernet/intel/iavf/iavf.h | 10 ++ drivers/net/ethernet/intel/iavf/iavf_main.c | 51 +++++++++- .../net/ethernet/intel/iavf/iavf_virtchnl.c | 92 ++++++++++++++++++- 3 files changed, 150 insertions(+), 3 deletions(-)
drivers/net/ethernet/intel/iavf/iavf.h +10 -0
@@ -251,6 +251,9 @@ struct iavf_cloud_filter {#define IAVF_RESET_WAIT_DETECTED_COUNT 500#define IAVF_RESET_WAIT_COMPLETE_COUNT 2000 +#define IAVF_MAX_QOS_TC_NUM 8 +#define IAVF_DEFAULT_QUANTA_SIZE 1024 +/* board specific private data structure */ struct iavf_adapter {struct workqueue_struct *wq;
@@ -338,6 +341,8 @@ struct iavf_adapter {#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) +#define IAVF_FLAG_AQ_CFG_QUEUES_QUANTA_SIZE BIT_ULL(40) +#define IAVF_FLAG_AQ_GET_QOS_CAPS BIT_ULL(41)/* flags for processing extended capability messages during * __IAVF_INIT_EXTENDED_CAPS. Each capability exchange requires
@@ -410,6 +415,8 @@ struct iavf_adapter { VIRTCHNL_VF_OFFLOAD_FDIR_PF)#define ADV_RSS_SUPPORT (_a) ((_a)->vf_res->vf_cap_flags & \ VIRTCHNL_VF_OFFLOAD_ADV_RSS_PF) +#define QOS_ALLOWED (_a) ((_a)->vf_res->vf_cap_flags & \ + VIRTCHNL_VF_OFFLOAD_QOS)struct virtchnl_vf_resource *vf_res;/* incl. all VSIs */ struct virtchnl_vsi_resource *vsi_res;/* our LAN VSI */ struct virtchnl_version_info pf_version;
@@ -418,6 +425,7 @@ struct iavf_adapter {struct virtchnl_vlan_caps vlan_v2_caps;u16 msg_enable;struct iavf_eth_stats current_stats; +struct virtchnl_qos_cap_list *qos_caps;struct iavf_vsi vsi;u32 aq_wait_count;/* RSS stuff */
@@ -584,6 +592,8 @@ 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_cfg_queues_quanta_size (struct iavf_adapter *adapter); +void iavf_get_qos_caps (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 +49 -2
93 lines not on this page · raw message
drivers/net/ethernet/intel/iavf/iavf_virtchnl.c +91 -1
116 lines not on this page · raw message
Signature · 1 line
2.45.2
Jakub Kicinski
maintainerRe: [PATCH v5 net-next 02/12] net-shapers: implement NL get operation
In reply to Paolo Abeninet/shaper/shaper.c line 355
Quoted 8 lines · On Thu, 29 Aug 2024 17:16:55 +0200 Paolo Abeni wrote:
net/shaper/shaper.c +8
-0+xa_for_each_range (&data->shapers, index, shaper, ctx->start_index, +U32_MAX ) { +net_shaper_index_to_handle (index, &handle); + ret =net_shaper_fill_one (skb, binding, &handle, shaper, info); +if (ret) +return ret; + + ctx->start_index = index;
75% sure this should say index + 1, or the xa_for_each.. should use start_index + 1. start position is inclusive.
XA iterators are hard to get right in netlink, I'd suggest using the same form of iteration as for_each_netdev_dump()..
Jakub Kicinski
maintainerRe: [PATCH v5 net-next 02/12] net-shapers: implement NL get operation
In reply to Paolo AbeniReview
Quoted 10 lines · On Thu, 29 Aug 2024 17:16:55 +0200 Paolo Abeni wrote:
(quoted diff) +10
-0+static int net_shaper_fill_handle(struct sk_buff *msg, + const struct net_shaper_handle *handle, + u32 type) +{ + struct nlattr *handle_attr; + + if (handle->scope == NET_SHAPER_SCOPE_UNSPEC) + return 0; + + handle_attr = nla_nest_start_noflag(msg, type);
_noflag() is deprecated
Quoted 21 lines
(quoted diff) +20
-0+ if (!handle_attr) + return -EMSGSIZE; + + if (nla_put_u32(msg, NET_SHAPER_A_HANDLE_SCOPE, handle->scope) || + (handle->scope >= NET_SHAPER_SCOPE_QUEUE && + nla_put_u32(msg, NET_SHAPER_A_HANDLE_ID, handle->id))) + goto handle_nest_cancel; + + nla_nest_end(msg, handle_attr); + return 0; + +handle_nest_cancel: + nla_nest_cancel(msg, handle_attr); + return -EMSGSIZE; +} +/* Initialize the context fetching the relevant device and + * acquiring a reference to it. + */ +static int net_shaper_ctx_init(const struct genl_info *info, int type, + struct net_shaper_nl_ctx *ctx)
Review
Quoted 10 lines
(quoted diff) +10
-0+{ + struct net *ns = genl_info_net(info); + struct net_device *dev; + int ifindex; + + memset(ctx, 0, sizeof(*ctx)); + if (GENL_REQ_ATTR_CHECK(info, type)) + return -EINVAL; + + ifindex = nla_get_u32(info->attrs[type]);
Let's limit the 'binding' thing to just driver call sites, we can redo the rest easily later. This line and next pretends to take "arbitrary" type but clearly wants a ifindex/netdev, right?
Quoted 12 lines
(quoted diff) +11
-0+ dev = netdev_get_by_index(ns, ifindex, &ctx->dev_tracker, GFP_KERNEL); + if (!dev) { + NL_SET_BAD_ATTR(info->extack, info->attrs[type]); + return -ENOENT; + } +static int net_shaper_parse_handle(const struct nlattr *attr, + const struct genl_info *info, + struct net_shaper_handle *handle) +{ + struct nlattr *tb[NET_SHAPER_A_HANDLE_MAX + 1]; + struct nlattr *scope_attr, *id_attr;
Review
Quoted 10 lines
(quoted diff) +10
-0+ u32 id = 0; + int ret; + + ret = nla_parse_nested(tb, NET_SHAPER_A_HANDLE_MAX, attr, + net_shaper_handle_nl_policy, info->extack); + if (ret < 0) + return ret; + + scope_attr = tb[NET_SHAPER_A_HANDLE_SCOPE]; + if (!scope_attr) {
NL_REQ_ATTR_CHECK()
Quoted 18 lines
(quoted diff) +18
-0+ NL_SET_BAD_ATTR(info->extack, + tb[NET_SHAPER_A_HANDLE_SCOPE]); + return -EINVAL; + } + + handle->scope = nla_get_u32(scope_attr); + + /* The default id for NODE scope shapers is an invalid one + * to help the 'group' operation discriminate between new + * NODE shaper creation (ID_UNSPEC) and reuse of existing + * shaper (any other value). + */ + id_attr = tb[NET_SHAPER_A_HANDLE_ID]; + if (id_attr) + id = nla_get_u32(id_attr); + else if (handle->scope == NET_SHAPER_SCOPE_NODE) + id = NET_SHAPER_ID_UNSPEC; +
Review
Quoted 10 lines
(quoted diff) +10
-0+ handle->id = id; + return 0; +} + +static int net_shaper_generic_pre(struct genl_info *info, int type) +{ + struct net_shaper_nl_ctx *ctx; + int ret; + + ctx = kmalloc(sizeof(*ctx), GFP_KERNEL);
Maybe send a patch like this, to avoid having to allocate this space, and special casing dump vs doit:
include/net/genetlink.h +6 -2
@@ -124,7 +124,8 @@ struct genl_family {* @genlhdr: generic netlink message header * @attrs: netlink attributes * @_net: network namespace -* @user_ptr: user pointers +* @ctx: storage space for the use by the family +* @user_ptr: user pointers (deprecated, use ctx instead) * @extack: extended ACK report struct */ struct genl_info {
@@ -135,7 +136,10 @@ struct genl_info {struct genlmsghdr * genlhdr;struct nlattr ** attrs;possible_net_t _net; -void * user_ptr[2 ]; +union { +u8 ctx[48 ]; +void * user_ptr[2 ]; + };struct netlink_ext_ack *extack; };
net/netlink/genetlink.c +2 -2
@@ -997,7 +997,7 @@ static int genl_start(struct netlink_callback *cb) info->info.attrs = attrs;genl_info_net_set (&info->info,sock_net (cb->skb->sk)); info->info.extack = cb->extack; -memset (&info->info.user_ptr,0 ,sizeof (info->info.user_ptr)); +memset (&info->info.ctx,0 ,sizeof (info->info.ctx)); cb->data = info;if (ops->start) {
@@ -1104,7 +1104,7 @@ static int genl_family_rcv_msg_doit(const struct genl_family *family, info.attrs = attrbuf; info.extack = extack;genl_info_net_set (&info, net); -memset (&info.user_ptr,0 ,sizeof (info.user_ptr)); +memset (&info.ctx,0 ,sizeof (info.ctx));if (ops->pre_doit) { err = ops->pre_doit (ops, skb, &info);
Quoted 16 lines
(quoted diff) +13
-1+ if (!ctx) + return -ENOMEM; + + ret = net_shaper_ctx_init(info, type, ctx); + if (ret) { + kfree(ctx); + return ret; + } + + info->user_ptr[0] = ctx; + return 0; +} + int net_shaper_nl_get_doit(struct sk_buff *skb, struct genl_info *info) { - return -EOPNOTSUPP;
Review
Quoted 10 lines
(quoted diff) +10
-0+ struct net_shaper_binding *binding; + struct net_shaper_handle handle; + struct net_shaper_info *shaper; + struct sk_buff *msg; + int ret; + + if (GENL_REQ_ATTR_CHECK(info, NET_SHAPER_A_HANDLE)) + return -EINVAL; + + binding = net_shaper_binding_from_ctx(info->user_ptr[0]);
This 'binding' has the same meaning as 'binding' in TCP ZC? :(
Review
Quoted 1 line
(quoted diff) +1
-0+ shaper = net_shaper_cache_lookup(binding, &handle);
Why call the stored info "cache"? It's the authoritative version of user configuration, isn't it?
Quoted 5 lines
(quoted diff) +5
-0+ if (!shaper) { + NL_SET_BAD_ATTR(info->extack, + info->attrs[NET_SHAPER_A_HANDLE]); + return -ENOENT; + }
Review
Quoted 10 lines
(quoted diff) +10
-0+ + msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL); + if (!msg) + return -ENOMEM; + + ret = net_shaper_fill_one(msg, binding, &handle, shaper, info); + if (ret) + goto free_msg; + + ret = genlmsg_reply(msg, info);
double space
Jakub Kicinski
maintainerRe: [PATCH v5 net-next 03/12] net-shapers: implement NL set and delete operations
In reply to Paolo AbeniQuoted 1 line · On Thu, 29 Aug 2024 17:16:56 +0200 Paolo Abeni wrote:
ithe next patch and will be implemented later in the series.
s/ithe/the/
net/shaper/shaper.c line 27
Quoted 10 lines
net/shaper/shaper.c +2
-0@@ -23,6 +23,10 @@struct net_shaper_data {struct xarray shapers; + +/* Serialize write ops and protects node_ids updates. */
By write ops you mean all driver ops but @capabilities? Maybe let's say all driver ops and avoid confusion?
net/shaper/shaper.c line 29
Quoted 2 lines
net/shaper/shaper.c +2
-0+struct mutex lock; +struct idr node_ids;
Do we need an IDR? can we not allocate the IDs using the xarray, starting at the offset of first NODE? Since type is on top bits?
net/shaper/shaper.c net_shaper_binding_data(struct net_shaper_binding *binding) · line 59
Quoted 13 lines
net/shaper/shaper.c +6
-0};struct net_shaper_nl_ctx {@@ -47,6 +51,27 @@ net_shaper_binding_data(struct net_shaper_binding *binding)return NULL ; } +static struct net_shaper_data * +net_shaper_binding_set_data (struct net_shaper_binding *binding, +struct net_shaper_data *data) +{ +if (binding->type ==NET_SHAPER_BINDING_TYPE_NETDEV ) +return cmpxchg (&binding->netdev->net_shaper_data,NULL , data);
Hmm. Do we need this because the lock is inside the struct we're allocating? I've been wondering if we shouldn't move this lock directly into net_device and combine it with the RSS lock. Create a "per-netdev" lock, instead of having multiple disparate mutexes which are hard to allocate?
Quoted 53 lines
net/shaper/shaper.c +38
-1+/* No devlink implementation yet.*/ +return NULL ; +} + +static const struct net_shaper_ops * +net_shaper_binding_ops (struct net_shaper_binding *binding) +{ +if (binding->type ==NET_SHAPER_BINDING_TYPE_NETDEV ) +return binding->netdev->netdev_ops->net_shaper_ops; + +/* No devlink implementation yet.*/ +return NULL ; +} +static int net_shaper_fill_binding (struct sk_buff *msg,const struct net_shaper_binding *binding,u32 type)@@ -178,6 +203,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 +/* Lookup the given shaper inside the cache. */ static struct net_shaper_info * net_shaper_cache_lookup(struct net_shaper_binding *binding,@@ -186,7 +231,132 @@ net_shaper_cache_lookup(struct net_shaper_binding *binding,struct net_shaper_data *data =net_shaper_binding_data (binding);u32 index =net_shaper_handle_to_index (handle); -return data ?xa_load (&data->shapers, index) :NULL ; +if (!data ||xa_get_mark (&data->shapers, index, +NET_SHAPER_CACHE_NOT_VALID )) +return NULL ; +
net/shaper/shaper.c net_shaper_cache_lookup(struct net_shaper_binding *binding, · line 247
Quoted 10 lines
net/shaper/shaper.c +10
-0+return xa_load (&data->shapers, index); +} + +/* Allocate on demand the per device shaper's cache. */ +static struct net_shaper_data * +net_shaper_cache_init (struct net_shaper_binding *binding, +struct netlink_ext_ack *extack) +{ +struct net_shaper_data *new, *data =net_shaper_binding_data (binding); +
Please don't call functions in variable init if you have to check what they returned later.
net/shaper/shaper.c net_shaper_cache_lookup(struct net_shaper_binding *binding, · line 248
Quoted 1 line
net/shaper/shaper.c +1
-0+if (!data) {
invert the condition and return early?
net/shaper/shaper.c net_shaper_cache_lookup(struct net_shaper_binding *binding, · line 251
Quoted 3 lines
net/shaper/shaper.c +3
-0+ new =kmalloc (sizeof (*data),GFP_KERNEL ); +if (!new) { +NL_SET_ERR_MSG (extack,"Can't allocate memory for shaper data" );
no error messages needed for GFP_KERNEL OOM (pls fix everywhere)
Quoted 32 lines
net/shaper/shaper.c +32
-0+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 =net_shaper_binding_set_data (binding, new); +if (!data) + data = new; +else +kfree (new); + } +return data; +} + +/* Prepare the cache to actually insert the given shaper, doing +* in advance the needed allocations. +*/ +static int net_shaper_cache_pre_insert (struct net_shaper_binding *binding, +struct net_shaper_handle *handle, +struct netlink_ext_ack *extack) +{ +struct net_shaper_data *data =net_shaper_binding_data (binding); +struct net_shaper_info *prev, *cur; +bool id_allocated = false; +int ret, id, index; + +if (!data) +return -ENOMEM ; +
net/shaper/shaper.c net_shaper_cache_lookup(struct net_shaper_binding *binding, · line 293
Quoted 10 lines
net/shaper/shaper.c +10
-0+ index =net_shaper_handle_to_index (handle); + cur =xa_load (&data->shapers, 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 (&data->node_ids,NULL , +0 ,NET_SHAPER_ID_UNSPEC ,GFP_ATOMIC );
How did we enter ATOMIC context?
net/shaper/shaper.c net_shaper_cache_lookup(struct net_shaper_binding *binding, · line 305
Quoted 12 lines
net/shaper/shaper.c +12
-0+ +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);
kzalloc() ?
net/shaper/shaper.c net_shaper_cache_lookup(struct net_shaper_binding *binding, · line 315
Quoted 10 lines
net/shaper/shaper.c +10
-0+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 (&data->shapers); + prev =__xa_store (&data->shapers, index, cur,GFP_KERNEL ); +__xa_set_mark (&data->shapers, index,NET_SHAPER_CACHE_NOT_VALID );
Maybe worth calling out if it's level to xa_set_mark on a non-inserted handle?
Quoted 149 lines
net/shaper/shaper.c +135
-0+xa_unlock (&data->shapers); +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 (&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_shaper_binding *binding, +int nr_shapers, +const struct net_shaper_handle *handle, +const struct net_shaper_info *shapers) +{ +struct net_shaper_data *data =net_shaper_binding_data (binding); +struct net_shaper_info *cur; +int index; +int i; + +xa_lock (&data->shapers); +for (i =0 ; i < nr_shapers; ++i) { + index =net_shaper_handle_to_index (&handle[i]); + + cur =xa_load (&data->shapers, index); +if (WARN_ON_ONCE (!cur)) +continue ; + +/* Successful update: drop the tentative mark +* and update the cache. +*/ +__xa_clear_mark (&data->shapers, index, +NET_SHAPER_CACHE_NOT_VALID ); + *cur = shapers[i]; + } +xa_unlock (&data->shapers); } static int net_shaper_parse_handle(const struct nlattr *attr,@@ -227,6 +397,85 @@ static int net_shaper_parse_handle(const struct nlattr *attr,return 0 ; } +static int net_shaper_parse_info (struct net_shaper_binding *binding, +struct nlattr **tb, +const struct genl_info *info, +struct net_shaper_handle *handle, +struct net_shaper_info *shaper, +bool *cached) +{ +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; + +if (handle->scope ==NET_SHAPER_SCOPE_UNSPEC ) { +NL_SET_BAD_ATTR (info->extack, + info->attrs[NET_SHAPER_A_HANDLE ]); +return -EINVAL ; + } + +/* Fetch existing data, if any, so that user provide info will +* incrementally update the existing shaper configuration. +*/ + old =net_shaper_cache_lookup (binding, handle); +if (old) + *shaper = *old; + *cached = !!old; + +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_shaper_binding *binding, +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 ]; +bool cached; +int ret; + + ret =nla_parse_nested (tb,NET_SHAPER_A_WEIGHT , attr, + net_shaper_info_nl_policy, info->extack); +if (ret <0 ) +return ret; + + ret =net_shaper_parse_info (binding, tb, info, handle, shaper, &cached); +if (ret <0 ) +return ret; + +if (!cached) +net_shaper_default_parent (handle, &shaper->parent); +return 0 ; +} +static int net_shaper_generic_pre (struct genl_info *info,int type) {struct net_shaper_nl_ctx *ctx;@@ -358,14 +607,153 @@ int net_shaper_nl_get_dumpit(struct sk_buff *skb,return 0 ; } +/* Update the H/W and on success update the local cache, too. */ +static int net_shaper_set (struct net_shaper_binding *binding, +const struct net_shaper_handle *h, +const struct net_shaper_info *shaper, +struct netlink_ext_ack *extack) +{ +struct net_shaper_data *data =net_shaper_cache_init (binding, extack); +const struct net_shaper_ops *ops =net_shaper_binding_ops (binding); +struct net_shaper_handle handle = *h; +int ret; + +if (!data)
net/shaper/shaper.c int net_shaper_nl_get_dumpit(struct sk_buff *skb, · line 631
Quoted 10 lines
net/shaper/shaper.c +10
-0+return -ENOMEM ; + +/* Should never happen: binding lookup validates the ops presence */ +if (WARN_ON_ONCE (!ops)) +return -EOPNOTSUPP ; + +mutex_lock (&data->lock); +if (handle.scope ==NET_SHAPER_SCOPE_NODE && +net_shaper_cache_lookup (binding, &handle)) { + ret = -ENOENT ;
EEXIST ? Presumably this is temporary as described in the commit message?
Jakub Kicinski
maintainerRe: [PATCH v5 net-next 04/12] net-shapers: implement NL group operation
In reply to Paolo AbeniQuoted 6 lines · On Thu, 29 Aug 2024 17:16:57 +0200 Paolo Abeni wrote:
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.
Need to s/root/parent/ the commit message?
Quoted 8 lines
net/shaper/shaper.c +8
-0+static int __net_shaper_group (struct net_shaper_binding *binding, +int leaves_count, +const struct net_shaper_handle *leaves_handles, +struct net_shaper_info *leaves, +struct net_shaper_handle *node_handle, +struct net_shaper_info *node, +struct netlink_ext_ack *extack) +{
net/shaper/shaper.c static int __net_shaper_delete(struct net_shaper_binding *binding, · line 818
Quoted 10 lines
net/shaper/shaper.c +10
-0+const struct net_shaper_ops *ops =net_shaper_binding_ops (binding); +struct net_shaper_info *parent =NULL ; +struct net_shaper_handle leaf_handle; +int i, ret; + +if (node_handle->scope ==NET_SHAPER_SCOPE_NODE ) { +if (node_handle->id !=NET_SHAPER_ID_UNSPEC && + !net_shaper_cache_lookup (binding, node_handle)) { +NL_SET_ERR_MSG_FMT (extack,"Node shaper %d:%d does not exists" , + node_handle->scope, node_handle->id);
BAD_ATTR would do?
net/shaper/shaper.c static int __net_shaper_delete(struct net_shaper_binding *binding, · line 830
Quoted 12 lines
net/shaper/shaper.c +12
-0+return -ENOENT ; + } + +/* When unspecified, the node parent scope is inherited from +* the leaves. +*/ +if (node->parent.scope ==NET_SHAPER_SCOPE_UNSPEC ) { +for (i =1 ; i < leaves_count; ++i) { +if (leaves[i].parent.scope != + leaves[0 ].parent.scope || + leaves[i].parent.id != + leaves[0 ].parent.id) {
memcmp() ? put a BUILD_BUG_ON(sizeof() != 8) to make sure we double check it if the struct grows?
net/shaper/shaper.c static int __net_shaper_delete(struct net_shaper_binding *binding, · line 832
Quoted 2 lines
net/shaper/shaper.c +2
-0+NL_SET_ERR_MSG_FMT (extack,"All the leaves shapers must have the same old parent" ); +return -EINVAL ;
5 indents is too many indents :( maybe make the for loop a helper?
net/shaper/shaper.c static int __net_shaper_delete(struct net_shaper_binding *binding, · line 836
Quoted 4 lines
net/shaper/shaper.c +4
-0+ } + } + +if (leaves_count >0 )
how can we get here and not have leaves? :o
Quoted 11 lines
net/shaper/shaper.c +10
-0+ node->parent = leaves[0 ].parent; + } + + }else { +net_shaper_default_parent (node_handle, &node->parent); + } +static int net_shaper_group_send_reply (struct genl_info *info, +struct net_shaper_handle *handle) +{ +struct net_shaper_binding *binding = info->user_ptr[0 ];
net/shaper/shaper.c int net_shaper_nl_delete_doit(struct sk_buff *skb, struct genl_info *info) · line 1015
Quoted 10 lines
net/shaper/shaper.c +10
-0+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;
return -ENOMEM;
net/shaper/shaper.c int net_shaper_nl_delete_doit(struct sk_buff *skb, struct genl_info *info) · line 1024
Quoted 9 lines
net/shaper/shaper.c +9
-0+ + hdr =genlmsg_iput (msg, info); +if (!hdr) + goto free_msg; + +if (net_shaper_fill_binding (msg, binding,NET_SHAPER_A_IFINDEX )) + goto free_msg; + +if (net_shaper_fill_handle (msg, handle,NET_SHAPER_A_HANDLE ))
you can combine the two fill ifs into one with ||
net/shaper/shaper.c int net_shaper_nl_delete_doit(struct sk_buff *skb, struct genl_info *info) · line 1031
Quoted 7 lines
net/shaper/shaper.c +7
-0+ goto free_msg; + +genlmsg_end (msg, hdr); + + ret =genlmsg_reply (msg, info); +if (ret) + goto free_msg;
reply always eats the skb, just:
return genlmsg_reply(msg, info);
net/shaper/shaper.c int net_shaper_nl_delete_doit(struct sk_buff *skb, struct genl_info *info) · line 1037
Quoted 6 lines
net/shaper/shaper.c +6
-0+ +return ret; + +free_msg: +nlmsg_free (msg); +return ret;
return -EMSGSIZE;
Quoted 30 lines
net/shaper/shaper.c +30
-0+} + +int net_shaper_nl_group_doit (struct sk_buff *skb,struct genl_info *info) +{ +struct net_shaper_handle *leaves_handles, node_handle; +struct net_shaper_info *leaves, node; +struct net_shaper_binding *binding; +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_NODE )) +return -EINVAL ; + + binding =net_shaper_binding_from_ctx (info->user_ptr[0 ]); + 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_node (binding, info->attrs[NET_SHAPER_A_NODE ], + info, &node_handle, &node); +if (ret) + goto free_shapers; +
net/shaper/shaper.c int net_shaper_nl_delete_doit(struct sk_buff *skb, struct genl_info *info) · line 1077
Quoted 10 lines
net/shaper/shaper.c +10
-0+ 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(binding, attr, info, + NET_SHAPER_SCOPE_QUEUE, + &leaves_handles[i],
Wouldn't it be convenient to store the handle in the "info" object? AFAIU the handle is forever for an info, so no risk of it being out of sync...
net/shaper/shaper.c int net_shaper_nl_delete_doit(struct sk_buff *skb, struct genl_info *info) · line 1085
Quoted 8 lines
net/shaper/shaper.c +8
-0+ &leaves[i]); +if (ret) + goto free_shapers; + i++; + } + + ret =net_shaper_group (binding, leaves_count, leaves_handles, leaves, + &node_handle, &node, info->extack);
...and it'd be nice if group had 5 rather than 7 params
net/shaper/shaper.c int net_shaper_nl_delete_doit(struct sk_buff *skb, struct genl_info *info) · line 1092
Quoted 7 lines
net/shaper/shaper.c +7
-0+if (ret <0 ) + goto free_shapers; + + ret =net_shaper_group_send_reply (info, &node_handle); +if (ret) { +/* Error on reply is not fatal to avoid rollback a successful +* configuration.
Slight issues with the grammar here, but I think it should be fatal. The sender will most likely block until they get a response. Not to mention that the caller will not know what the handle we allocated is.
Quoted 9 lines
net/shaper/shaper.c +9
-0+*/ +GENL_SET_ERR_MSG_FMT (info,"Can't send reply %d" , ret); + ret =0 ; + } + +free_shapers: +kfree (leaves); +return ret; +}
Jakub Kicinski
maintainerRe: [PATCH v5 net-next 11/12] iavf: Add net_shaper_ops support
In reply to Paolo Abenidrivers/net/ethernet/intel/iavf/iavf_main.c static netdev_features_t iavf_fix_features(struct net_device *netdev, · line 4935
Quoted 9 lines · On Thu, 29 Aug 2024 17:17:04 +0200 Paolo Abeni wrote:
drivers/net/ethernet/intel/iavf/iavf_main.c +9
-0+static int iavf_verify_handle (struct net_shaper_binding *binding, +const struct net_shaper_handle *handle, +struct netlink_ext_ack *extack) +{ +struct iavf_adapter *adapter =netdev_priv (binding->netdev); +enum net_shaper_scope scope = handle->scope; +int qid = handle->id; + +if (scope !=NET_SHAPER_SCOPE_QUEUE ) {
should be checked by the core. add "mask of supported scopes" to caps
drivers/net/ethernet/intel/iavf/iavf_main.c static netdev_features_t iavf_fix_features(struct net_device *netdev, · line 4941
Quoted 6 lines
drivers/net/ethernet/intel/iavf/iavf_main.c +6
-0+NL_SET_ERR_MSG_FMT (extack,"Invalid shaper handle, unsupported scope %d" , + scope); +return -EOPNOTSUPP ; + } + +if (qid >= adapter->num_active_queues) {
should be checked by the core..
we prolly want to trim the queue shapers on channel reconfig, then, too? :(
Quoted 6 lines
drivers/net/ethernet/intel/iavf/iavf_main.c +6
-0+NL_SET_ERR_MSG_FMT (extack,"Invalid shaper handle, queued id %d max %d" , + qid, adapter->num_active_queues); +return -EINVAL ; + } +return 0 ; +}
drivers/net/ethernet/intel/iavf/iavf_main.c static netdev_features_t iavf_fix_features(struct net_device *netdev, · line 5013
Quoted 10 lines
drivers/net/ethernet/intel/iavf/iavf_main.c +9
-0+static int iavf_shaper_group (struct net_shaper_binding *binding, +int leaves_count, +const struct net_shaper_handle *leaves_handles, +const struct net_shaper_info *leaves, +const struct net_shaper_handle *root_handle, +const struct net_shaper_info *root, +struct netlink_ext_ack *extack) +{ +return -EOPNOTSUPP ;
Core should check if op is defined.
Jakub Kicinski
maintainerRe: [PATCH v5 net-next 07/12] net: shaper: implement introspection support
In reply to Paolo AbeniQuoted 5 lines · On Thu, 29 Aug 2024 17:17:00 +0200 Paolo Abeni wrote:
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.
It's not just for introspection, it's also for the core to do error checking. Does something prevent user from installing a PPS shaper in iavf right now ?
Paolo Abeni
maintainerRe: [PATCH v5 net-next 02/12] net-shapers: implement NL get operation
In reply to Jakub KicinskiOn 8/30/24 03:20, Jakub Kicinski wrote:>> +/* Initialize the context fetching the relevant device and
Quoted 10 lines
Quoted 10 lines
+ * acquiring a reference to it.
+ */(quoted diff) +8
-0+static int net_shaper_ctx_init(const struct genl_info *info, int type, + struct net_shaper_nl_ctx *ctx) +{ + struct net *ns = genl_info_net(info); + struct net_device *dev; + int ifindex; + + memset(ctx, 0, sizeof(*ctx));
Quoted 8 lines
Review
Quoted 4 lines
(quoted diff) +4
-0+ if (GENL_REQ_ATTR_CHECK(info, type)) + return -EINVAL; + + ifindex = nla_get_u32(info->attrs[type]);Let's limit the 'binding' thing to just driver call sites, we can redo the rest easily later. This line and next pretends to take "arbitrary" type but clearly wants a ifindex/netdev, right?
There is a misunderstanding. This helper will be used in a following patch (7/12) with a different 'type' argument: NET_SHAPER_A_BINDING_IFINDEX. I've put a note in the commit message, but was unintentionally dropped in one of the recent refactors. I'll add that note back.
I hope you are ok with the struct net_shaper_binding * argument to most helpers? does not add complexity, will help to support devlink objects and swapping back and forth from/to struct net_device* can't be automated.
[...]
Quoted 18 lines
Maybe send a patch like this, to avoid having to allocate this space, and special casing dump vs doit:
include/net/genetlink.h +2
-1@@ -124,7 +124,8 @@ struct genl_family {* @genlhdr: generic netlink message header * @attrs: netlink attributes * @_net: network namespace -* @user_ptr: user pointers +* @ctx: storage space for the use by the family +* @user_ptr: user pointers (deprecated, use ctx instead) * @extack: extended ACK report struct */ struct genl_info {
include/net/genetlink.h struct genl_info { · line 144
Quoted 10 lines
include/net/genetlink.h +4
-1@@ -135,7 +136,10 @@ struct genl_info {struct genlmsghdr * genlhdr;struct nlattr ** attrs;possible_net_t _net; -void * user_ptr[2 ]; +union { +u8 ctx[48 ]; +void * user_ptr[2 ]; + };struct netlink_ext_ack *extack; };
Makes sense. Plus likely:
#define NETLINK_CTX_SIZE 48
and use such define above and in linux/netlink.h
Thanks,
Paolo
Paolo Abeni
maintainerRe: [PATCH v5 net-next 02/12] net-shapers: implement NL get operation
In reply to Jakub KicinskiHi,
Please allow me to put a few high level questions together, to both underline them as most critical, and keep the thread focused.
Quoted 1 line · Jakub Kicinski
This 'binding' has the same meaning as 'binding' in TCP ZC? :(
I hope we can agree that good naming is difficult. I thought we agreed on such naming in the past week’s discussion. The term 'binding' is already used in the networking stack in many places to identify different things (i.e. device tree, socket, netfilter.. ). The name prefix avoids any ambiguity and I think this a good name, but if you have any better suggestions, this change should be trivial.
[about per device shaper lock]
Quoted 4 lines
I've been wondering if we shouldn't move this lock directly into
net_deviceand combine it with the RSS lock. Create a "per-netdev" lock, instead of having multiple disparate mutexes which are hard to allocate?
The above looks like a quite unrelated refactor and one I think it will not be worthy. The complexity of locking code in this series is very limited, and self-encapsulated. Different locks for different things increases scalability. Possibly we will not see much contention on the same device, but some years ago we did not think there would be much contention on RTNL...
Additionally, if we use a per _network device_ lock, future expansion of the core to support devlink objects will be more difficult.
[about separate handle from shaper_info arguments]
Quoted 3 lines
Wouldn't it be convenient to store the handle in the "info" object? AFAIU the handle is forever for an info, so no risk of it being out of sync…
Was that way a couple of iterations ago. Jiri explicitly asked for the separation, I asked for confirmation and nobody objected.
Which if the 2 options is acceptable from both of you?
[about queue limit and channel reconf]
Quoted 2 lines
we probably want to trim the queue shapers on channel reconfig, then, too? :(
what about exposing to the drivers an helper alike:
net_shaper_notify_delete(binding, handle);
that tells the core the shaper at the given handle just went away in the H/W? The driver will call it in the queue deletion helper, and such helper could be later on used more generically, i.e. for vf/devlink port deletion.
[about capabilities support]
Quoted 2 lines
It's not just for introspection, it's also for the core to do error checking.
Actually, in the previous discussions it was never mentioned to use capabilities to fully centralize the error checking.
This really looks like another feature, and can easily be added in a second time (say, a follow-up series), with no functionality loss.
I (or anybody else) can’t keep adding new features at every iteration. At some point we need to draw a line, and we should agree that the scope of this activity has already expanded a lot in the past year. I would like to draw such a line here.
Thanks,
Paolo
Paolo Abeni
maintainerRe: [PATCH v5 net-next 04/12] net-shapers: implement NL group operation
In reply to Jakub KicinskiQuoted 12 lines · Jakub Kicinski
Quoted 12 lines
net/shaper/shaper.c +12
-0+static int __net_shaper_group (struct net_shaper_binding *binding, +int leaves_count, +const struct net_shaper_handle *leaves_handles, +struct net_shaper_info *leaves, +struct net_shaper_handle *node_handle, +struct net_shaper_info *node, +struct netlink_ext_ack *extack) +{ +const struct net_shaper_ops *ops =net_shaper_binding_ops (binding); +struct net_shaper_info *parent =NULL ; +struct net_shaper_handle leaf_handle; +int i, ret;
Quoted 8 lines
net/shaper/shaper.c static int __net_shaper_delete(struct net_shaper_binding *binding, · line 818
Quoted 6 lines
net/shaper/shaper.c +6
-0+ +if (node_handle->scope ==NET_SHAPER_SCOPE_NODE ) { +if (node_handle->id !=NET_SHAPER_ID_UNSPEC && + !net_shaper_cache_lookup (binding, node_handle)) { +NL_SET_ERR_MSG_FMT (extack,"Node shaper %d:%d does not exists" , + node_handle->scope, node_handle->id);
BAD_ATTRwould do?
We can reach here from the delete() op (next patch), there will be no paired attribute is such case. Even for the group() operation it will need to push here towards several callers additional context to identify the attribute, it should be quite ugly, can we keep with ERR_MSG_FMT here?
Quoted 4 lines
Quoted 4 lines
net/shaper/shaper.c +4
-0+if (ret <0 ) + goto free_shapers; + + ret =net_shaper_group_send_reply (info, &node_handle);
Quoted 8 lines
net/shaper/shaper.c int net_shaper_nl_delete_doit(struct sk_buff *skb, struct genl_info *info) · line 1092
Quoted 3 lines
net/shaper/shaper.c +3
-0+if (ret) { +/* Error on reply is not fatal to avoid rollback a successful +* configuration. Slight issues with the grammar here, but I think it should be fatal. The sender will most likely block until they get a response. Not to mention that the caller will not know what the handle we allocated is.
You mean we should return a negative error code, and _not_ that we should additionally attempt a rollback, right? The rollback will be very difficult at best: at this point destructive action have taken place.
Thanks,
Paolo
Jakub Kicinski
maintainerRe: [PATCH v5 net-next 02/12] net-shapers: implement NL get operation
In reply to Paolo AbeniQuoted 21 lines · On Fri, 30 Aug 2024 12:55:05 +0200 Paolo Abeni wrote:
On 8/30/24 03:20, Jakub Kicinski wrote:>> +/* Initialize the context fetching the relevant device and
Quoted 10 lines
Quoted 10 lines
+ * acquiring a reference to it.
+ */(quoted diff) +8
-0+static int net_shaper_ctx_init(const struct genl_info *info, int type, + struct net_shaper_nl_ctx *ctx) +{ + struct net *ns = genl_info_net(info); + struct net_device *dev; + int ifindex; + + memset(ctx, 0, sizeof(*ctx));Quoted 8 lines
Review
Quoted 4 lines
(quoted diff) +4
-0+ if (GENL_REQ_ATTR_CHECK(info, type)) + return -EINVAL; + + ifindex = nla_get_u32(info->attrs[type]);Let's limit the 'binding' thing to just driver call sites, we can redo the rest easily later. This line and next pretends to take "arbitrary" type but clearly wants a ifindex/netdev, right?
Quoted 5 lines
There is a misunderstanding. This helper will be used in a following patch (7/12) with a different 'type' argument:
NET_SHAPER_A_BINDING_IFINDEX. I've put a note in the commit message, but was unintentionally dropped in one of the recent refactors. I'll add that note back.
What I'm saying is that if you want to prep the ground for more "binding" types you should also add:
if (type != ...IFINDEX) {
/* other binding types are TBD */
return -EINVAL;
}Quoted 3 lines
I hope you are ok with the struct
net_shaper_binding* argument to most helpers? does not add complexity, will help to support devlink objects and swapping back and forth from/to struct net_device* can't be automated.
I am "okay" in the American sense of the word which AFAIU is "unhappy but won't complain unless asked".
Quoted 31 lines
Quoted 18 lines
Maybe send a patch like this, to avoid having to allocate this space, and special casing dump vs doit:
include/net/genetlink.h +2
-1@@ -124,7 +124,8 @@ struct genl_family {* @genlhdr: generic netlink message header * @attrs: netlink attributes * @_net: network namespace -* @user_ptr: user pointers +* @ctx: storage space for the use by the family +* @user_ptr: user pointers (deprecated, use ctx instead) * @extack: extended ACK report struct */ struct genl_info {include/net/genetlink.h struct genl_info { · line 144
Quoted 10 lines
include/net/genetlink.h +4
-1@@ -135,7 +136,10 @@ struct genl_info {struct genlmsghdr * genlhdr;struct nlattr ** attrs;possible_net_t _net; -void * user_ptr[2 ]; +union { +u8 ctx[48 ]; +void * user_ptr[2 ]; + };struct netlink_ext_ack *extack; };Makes sense. Plus likely:
Quoted 3 lines
#define
NETLINK_CTX_SIZE48and use such define above and in
linux/netlink.h
Aha, would be good to also have a checking macro. Maybe rename
NL_ASSERT_DUMP_CTX_FITS()
to apply more broadly? or add a new one? Weak preference for former.
Jakub Kicinski
maintainerRe: [PATCH v5 net-next 04/12] net-shapers: implement NL group operation
In reply to Paolo AbeniQuoted 13 lines · On Fri, 30 Aug 2024 18:48:41 +0200 Paolo Abeni wrote:
Quoted 4 lines
Quoted 4 lines
net/shaper/shaper.c +4
-0+const struct net_shaper_ops *ops =net_shaper_binding_ops (binding); +struct net_shaper_info *parent =NULL ; +struct net_shaper_handle leaf_handle; +int i, ret;Quoted 8 lines
net/shaper/shaper.c static int __net_shaper_delete(struct net_shaper_binding *binding, · line 818
Quoted 6 lines
net/shaper/shaper.c +6
-0+ +if (node_handle->scope ==NET_SHAPER_SCOPE_NODE ) { +if (node_handle->id !=NET_SHAPER_ID_UNSPEC && + !net_shaper_cache_lookup (binding, node_handle)) { +NL_SET_ERR_MSG_FMT (extack,"Node shaper %d:%d does not exists" , + node_handle->scope, node_handle->id);
BAD_ATTRwould do?
Quoted 4 lines
We can reach here from the
delete()op (next patch), there will be no paired attribute is such case. Even for thegroup()operation it will need to push here towards several callers additional context to identify the attribute, it should be quite ugly, can we keep withERR_MSG_FMThere?
Alright. But TBH I haven't grasped the semantics of how you use UNSPEC. So I reserve the right to complain again in v6 if I think of a better way ;)
Quoted 13 lines
Quoted 4 lines
Quoted 4 lines
net/shaper/shaper.c +4
-0+if (ret <0 ) + goto free_shapers; + + ret =net_shaper_group_send_reply (info, &node_handle);Quoted 8 lines
net/shaper/shaper.c int net_shaper_nl_delete_doit(struct sk_buff *skb, struct genl_info *info) · line 1092
Quoted 3 lines
net/shaper/shaper.c +3
-0+if (ret) { +/* Error on reply is not fatal to avoid rollback a successful +* configuration. Slight issues with the grammar here, but I think it should be fatal. The sender will most likely block until they get a response. Not to mention that the caller will not know what the handle we allocated is.
Quoted 3 lines
You mean we should return a negative error code, and _not_ that we should additionally attempt a rollback, right? The rollback will be very difficult at best: at this point destructive action have taken place.
net_shaper_group_send_reply() does a bit too much, TBH. Given the rollback complexity propagating the failure just from genlmsg_reply() is fine. I think the only case it fails is if the socket is congested, which is in senders control. But genlmsg_new() can be done before we start. And we should size the skb so that nla_puts sever fail (just pop a WARN_ON() on their error path, to make sure we catch it if they grow, I don't think they can fail with your current code).
Jakub Kicinski
maintainerRe: [PATCH v5 net-next 02/12] net-shapers: implement NL get operation
In reply to Paolo AbeniQuoted 4 lines · On Fri, 30 Aug 2024 17:43:08 +0200 Paolo Abeni wrote:
Please allow me to put a few high level questions together, to both underline them as most critical, and keep the thread focused.
On 8/30/24 03:20, Jakub Kicinski wrote:
Quoted 8 lines
Quoted 1 line
This 'binding' has the same meaning as 'binding' in TCP ZC? :(
I hope we can agree that good naming is difficult. I thought we agreed on such naming in the past week’s discussion. The term 'binding' is already used in the networking stack in many places to identify different things (i.e. device tree, socket, netfilter.. ). The name prefix avoids any ambiguity and I think this a good name, but if you have any better suggestions, this change should be trivial.
Ack. Maybe we can cut down the number of ambiguous nouns elsewhere:
maybe call net_shaper_info -> net_shaper ?
maybe net_shaper_data -> net_shaper_hierarchy ?
Quoted 11 lines
Quoted 4 lines
I've been wondering if we shouldn't move this lock directly into
net_deviceand combine it with the RSS lock. Create a "per-netdev" lock, instead of having multiple disparate mutexes which are hard to allocate?The above looks like a quite unrelated refactor and one I think it will not be worthy. The complexity of locking code in this series is very limited, and self-encapsulated. Different locks for different things increases scalability. Possibly we will not see much contention on the same device, but some years ago we did not think there would be much contention on RTNL...
We need to do this, anyway. Let me do it myself, then.
Quoted 2 lines
Additionally, if we use a per
_networkdevice_lock, future expansion of the core to support devlink objects will be more difficult.
You parse out the binding you can store a pointer to the right mutex.
Quoted 7 lines
[about separate handle from
shaper_infoarguments]Quoted 3 lines
Wouldn't it be convenient to store the handle in the "info" object? AFAIU the handle is forever for an info, so no risk of it being out of sync…
Was that way a couple of iterations ago. Jiri explicitly asked for the separation, I asked for confirmation and nobody objected.
Could you link to that? I must have not read it. You can keep it wrapped in a struct *_handle, that's fine. But it can live inside the shaper object.
Quoted 6 lines
Which if the 2 options is acceptable from both of you?
[about queue limit and channel reconf]
Quoted 2 lines
we probably want to trim the queue shapers on channel reconfig, then, too? :(
Quoted 8 lines
what about exposing to the drivers an helper alike:
net_shaper_notify_delete(binding, handle);that tells the core the shaper at the given handle just went away in the H/W? The driver will call it in the queue deletion helper, and such helper could be later on used more generically, i.e. for vf/devlink port deletion.
We can either prevent disabling queues which have shapers attached, or auto-removing the shapers. No preference on that. But put the callback in the core, please, netif_set_real_num_rx_queues() ? Why not?
Quoted 6 lines
Quoted 2 lines
It's not just for introspection, it's also for the core to do error checking.
Actually, in the previous discussions it was never mentioned to use capabilities to fully centralize the error checking.
Quoted 7 lines
This really looks like another feature, and can easily be added in a second time (say, a follow-up series), with no functionality loss.
I (or anybody else) can’t keep adding new features at every iteration. At some point we need to draw a line, and we should agree that the scope of this activity has already expanded a lot in the past year. I would like to draw such a line here.
I can help you. Just tell me which parts you want me to take care of.
Jakub Kicinski
maintainerRe: [PATCH v5 net-next 02/12] net-shapers: implement NL get operation
In reply to Jakub KicinskiQuoted 6 lines · On Fri, 30 Aug 2024 11:39:00 -0700 Jakub Kicinski wrote:
Quoted 5 lines
There is a misunderstanding. This helper will be used in a following patch (7/12) with a different 'type' argument:
NET_SHAPER_A_BINDING_IFINDEX. I've put a note in the commit message, but was unintentionally dropped in one of the recent refactors. I'll add that note back.
Quoted 7 lines
What I'm saying is that if you want to prep the ground for more "binding" types you should also add:
if (type != ...IFINDEX) { /* other binding types are TBD */ return -EINVAL; }
Ah, the part I missed is that there are two different types for ifindex:
NET_SHAPER_A_IFINDEXNET_SHAPER_A_CAPABILITIES_IFINDEX
Got it now.
Paolo Abeni
maintainerRe: [PATCH v5 net-next 02/12] net-shapers: implement NL get operation
In reply to Jakub KicinskiQuoted 14 lines · Jakub Kicinski
Quoted 4 lines · On Fri, 30 Aug 2024 17:43:08 +0200 Paolo Abeni wrote:
Please allow me to put a few high level questions together, to both underline them as most critical, and keep the thread focused.
On 8/30/24 03:20, Jakub Kicinski wrote:
Quoted 8 lines
Quoted 1 line
This 'binding' has the same meaning as 'binding' in TCP ZC? :(
I hope we can agree that good naming is difficult. I thought we agreed on such naming in the past week’s discussion. The term 'binding' is already used in the networking stack in many places to identify different things (i.e. device tree, socket, netfilter.. ). The name prefix avoids any ambiguity and I think this a good name, but if you have any better suggestions, this change should be trivial.
Quoted 5 lines
Ack. Maybe we can cut down the number of ambiguous nouns elsewhere:
maybe call
net_shaper_info->net_shaper?maybe
net_shaper_data->net_shaper_hierarchy?
Is everybody fine with the above?
Quoted 9 lines
Quoted 7 lines
[about separate handle from
shaper_infoarguments]Quoted 3 lines
Wouldn't it be convenient to store the handle in the "info" object? AFAIU the handle is forever for an info, so no risk of it being out of sync…
Was that way a couple of iterations ago. Jiri explicitly asked for the separation, I asked for confirmation and nobody objected.
Could you link to that? I must have not read it.
https://lore.kernel.org/netdev/[email protected]/
search for "I wonder if the handle should be part of this structure"
I must admit by wannabe reply on such point never left my outbox.
Quoted 2 lines
You can keep it wrapped in a struct
*_handle, that's fine. But it can live inside the shaper object.
That is basically the opposite of what Jiri asked. @Jiri would you be ok reverting to such layout?
Quoted 10 lines
Quoted 10 lines
Which if the 2 options is acceptable from both of you?
[about queue limit and channel reconf]
Quoted 2 lines
we probably want to trim the queue shapers on channel reconfig, then, too? :(
what about exposing to the drivers an helper alike:
net_shaper_notify_delete(binding, handle);
Quoted 7 lines
Quoted 4 lines
that tells the core the shaper at the given handle just went away in the H/W? The driver will call it in the queue deletion helper, and such helper could be later on used more generically, i.e. for vf/devlink port deletion.
We can either prevent disabling queues which have shapers attached, or auto-removing the shapers.
I think/fear that prevent disabling queues would lead to weird/unexpected results and more difficult administration, I prefer the callback option.
Quoted 3 lines
No preference on that. But put the
callback in the core, please,netif_set_real_num_rx_queues()? Why not?
It makes sense. I'll add a net_shaper_set_real_num_rx_queues() callback there.
/P
Paolo Abeni
maintainerRe: [PATCH v5 net-next 02/12] net-shapers: implement NL get operation
In reply to Jakub KicinskiQuoted 10 lines · Jakub Kicinski
Quoted 3 lines · On Fri, 30 Aug 2024 12:55:05 +0200 Paolo Abeni wrote:
#define
NETLINK_CTX_SIZE48and use such define above and in
linux/netlink.hAha, would be good to also have a checking macro. Maybe rename
NL_ASSERT_DUMP_CTX_FITS()to apply more broadly? or add a new one? Weak preference for former.
I will rename it to NL_ASSERT_CTX_FITS(), in the same patch.
/P
Jakub Kicinski
maintainerRe: [PATCH v5 net-next 02/12] net-shapers: implement NL get operation
In reply to Paolo AbeniQuoted 10 lines · On Mon, 2 Sep 2024 12:10:50 +0200 Paolo Abeni wrote:
Quoted 4 lines
Quoted 2 lines
Was that way a couple of iterations ago. Jiri explicitly asked for the separation, I asked for confirmation and nobody objected.
Could you link to that? I must have not read it.
https://lore.kernel.org/netdev/[email protected]/
search for "I wonder if the handle should be part of this structure"
I must admit by wannabe reply on such point never left my outbox.
"I wonder if .." does not sound like a strong preference. And the parent ID remained in the struct, so it still partially records its position in the hierarchy. Since there is no "move" op it's really not worth multiplying arguments to most functions 2x.