Re: [PATCH net-next V3 3/8] devlink: Extend devlink rate API with traffic classes bandwidth management

On 18/11/2024 10:37, Jiri Pirko wrote:

Sun, Nov 17, 2024 at 09:50:40PM CET, tariqt@nvidia.com wrote:

quoted

From: Carolina Jubran <redacted>

Introduce support for specifying bandwidth proportions between traffic
classes (TC) in the devlink-rate API. This new option allows users to
allocate bandwidth across multiple traffic classes in a single command.

This feature provides a more granular control over traffic management,
especially for scenarios requiring Enhanced Transmission Selection.

Users can now define a specific bandwidth share for each traffic class,
such as allocating 20% for TC0 (TCP/UDP) and 80% for TC5 (RoCE).

Example:
DEV=pci/0000:08:00.0

$ devlink port function rate add $DEV/vfs_group tx_share 10Gbit \
  tx_max 50Gbit tc-bw 0:20 1:0 2:0 3:0 4:0 5:80 6:0 7:0

$ devlink port function rate set $DEV/vfs_group \
  tc-bw 0:20 1:0 2:0 3:0 4:0 5:20 6:60 7:0

Signed-off-by: Carolina Jubran <redacted>
Reviewed-by: Cosmin Ratiu <redacted>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
Documentation/netlink/specs/devlink.yaml | 22 ++++++++
include/net/devlink.h                    |  7 +++
include/uapi/linux/devlink.h             |  3 +
net/devlink/netlink_gen.c                | 14 +++--
net/devlink/netlink_gen.h                |  1 +
net/devlink/rate.c                       | 71 +++++++++++++++++++++++-
6 files changed, 113 insertions(+), 5 deletions(-)

diff --git a/Documentation/netlink/specs/devlink.yaml b/Documentation/netlink/specs/devlink.yaml
index 09fbb4c03fc8..fece78ed60fe 100644
--- a/Documentation/netlink/specs/devlink.yaml
+++ b/Documentation/netlink/specs/devlink.yaml

@@ -820,6 +820,19 @@ attribute-sets:
       -
         name: region-direct
         type: flag
+      -
+        name: rate-tc-bw
+        type: u32
+        doc: |
+             Specifies the bandwidth allocation for the Traffic Class as a
+             percentage.
+        checks:
+          min: 0
+          max: 100
+      -
+        name: rate-tc-bw-values
+        type: nest
+        nested-attributes: dl-rate-tc-bw-values

Hmm, it's not a simple nest. It's an array. You probably need something
like type: indexed-array here. Please make sure you make this working
with ynl. Could you also please add examples of get and set commands
using ynl to the patch description?

It seems that type: indexed-array with sub-type: u32 would be the correct approach. However, I noticed that this support appears to be missing in the ynl-gen-c.py script in this series: https://lore.kernel.org/all/20240404063114.1221532-3-liuhangbin@gmail.com/ (local). If this is indeed the case, how should I specify the min and max values for the u32 entries in the indexed-array?

quoted

   -
     name: dl-dev-stats

@@ -1225,6 +1238,13 @@ attribute-sets:
       -
         name: flash
         type: flag
+  -
+    name: dl-rate-tc-bw-values
+    subset-of: devlink
+    attributes:
+      -
+        name: rate-tc-bw
+        type: u32

operations:
   enum-model: directional

@@ -2149,6 +2169,7 @@ operations:
             - rate-tx-priority
             - rate-tx-weight
             - rate-parent-node-name
+            - rate-tc-bw-values

     -
       name: rate-new

@@ -2169,6 +2190,7 @@ operations:
             - rate-tx-priority
             - rate-tx-weight
             - rate-parent-node-name
+            - rate-tc-bw-values

     -
       name: rate-del

diff --git a/include/net/devlink.h b/include/net/devlink.h
index fbb9a2668e24..277b826cdd60 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h

@@ -20,6 +20,7 @@

#include <uapi/linux/devlink.h>
#include <linux/xarray.h>
#include <linux/firmware.h>
+#include <linux/dcbnl.h>

struct devlink;
struct devlink_linecard;

@@ -117,6 +118,8 @@ struct devlink_rate {

	u32 tx_priority;
	u32 tx_weight;
+
+	u32 tc_bw[IEEE_8021QAZ_MAX_TCS];
};

struct devlink_port {

@@ -1469,6 +1472,8 @@ struct devlink_ops {

					 u32 tx_priority, struct netlink_ext_ack *extack);
	int (*rate_leaf_tx_weight_set)(struct devlink_rate *devlink_rate, void *priv,
				       u32 tx_weight, struct netlink_ext_ack *extack);
+	int (*rate_leaf_tc_bw_set)(struct devlink_rate *devlink_rate, void *priv,
+				   u32 *tc_bw, struct netlink_ext_ack *extack);
	int (*rate_node_tx_share_set)(struct devlink_rate *devlink_rate, void *priv,
				      u64 tx_share, struct netlink_ext_ack *extack);
	int (*rate_node_tx_max_set)(struct devlink_rate *devlink_rate, void *priv,

@@ -1477,6 +1482,8 @@ struct devlink_ops {

					 u32 tx_priority, struct netlink_ext_ack *extack);
	int (*rate_node_tx_weight_set)(struct devlink_rate *devlink_rate, void *priv,
				       u32 tx_weight, struct netlink_ext_ack *extack);
+	int (*rate_node_tc_bw_set)(struct devlink_rate *devlink_rate, void *priv,
+				   u32 *tc_bw, struct netlink_ext_ack *extack);
	int (*rate_node_new)(struct devlink_rate *rate_node, void **priv,
			     struct netlink_ext_ack *extack);
	int (*rate_node_del)(struct devlink_rate *rate_node, void *priv,

diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 9401aa343673..0940f8770319 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h

@@ -614,6 +614,9 @@ enum devlink_attr {

	DEVLINK_ATTR_REGION_DIRECT,		/* flag */

+	DEVLINK_ATTR_RATE_TC_BW,		/* u32 */
+	DEVLINK_ATTR_RATE_TC_BW_VALUES,		/* nested */

"values" sounds odd. When I look at the rest of the similar nested
attrs, we use either "S" or "_LIST" as suffix. Also, Please have the
nested attr first and the u32 as second (again, the rest of the attrs
have it like that). So something like:

	DEVLINK_ATTR_RATE_TC_BWS,		/* nested */
	DEVLINK_ATTR_RATE_TC_BW,		/* u32 */

Thanks for pointing this out. I'll go with DEVLINK_ATTR_RATE_TC_BWS.
 >> +

quoted

	/* Add new attributes above here, update the spec in
	 * Documentation/netlink/specs/devlink.yaml and re-generate
	 * net/devlink/netlink_gen.c.

diff --git a/net/devlink/netlink_gen.c b/net/devlink/netlink_gen.c
index f9786d51f68f..231c2752538f 100644
--- a/net/devlink/netlink_gen.c
+++ b/net/devlink/netlink_gen.c

@@ -18,6 +18,10 @@ const struct nla_policy devlink_dl_port_function_nl_policy[DEVLINK_PORT_FN_ATTR_

	[DEVLINK_PORT_FN_ATTR_CAPS] = NLA_POLICY_BITFIELD32(15),
};

+const struct nla_policy devlink_dl_rate_tc_bw_values_nl_policy[DEVLINK_ATTR_RATE_TC_BW + 1] = {
+	[DEVLINK_ATTR_RATE_TC_BW] = NLA_POLICY_RANGE(NLA_U32, 0, 100),
+};
+
const struct nla_policy devlink_dl_selftest_id_nl_policy[DEVLINK_ATTR_SELFTEST_ID_FLASH + 1] = {
	[DEVLINK_ATTR_SELFTEST_ID_FLASH] = { .type = NLA_FLAG, },
};

@@ -496,7 +500,7 @@ static const struct nla_policy devlink_rate_get_dump_nl_policy[DEVLINK_ATTR_DEV_

};

/* DEVLINK_CMD_RATE_SET - do */
-static const struct nla_policy devlink_rate_set_nl_policy[DEVLINK_ATTR_RATE_TX_WEIGHT + 1] = {
+static const struct nla_policy devlink_rate_set_nl_policy[DEVLINK_ATTR_RATE_TC_BW_VALUES + 1] = {
	[DEVLINK_ATTR_BUS_NAME] = { .type = NLA_NUL_STRING, },
	[DEVLINK_ATTR_DEV_NAME] = { .type = NLA_NUL_STRING, },
	[DEVLINK_ATTR_RATE_NODE_NAME] = { .type = NLA_NUL_STRING, },

@@ -505,10 +509,11 @@ static const struct nla_policy devlink_rate_set_nl_policy[DEVLINK_ATTR_RATE_TX_W

	[DEVLINK_ATTR_RATE_TX_PRIORITY] = { .type = NLA_U32, },
	[DEVLINK_ATTR_RATE_TX_WEIGHT] = { .type = NLA_U32, },
	[DEVLINK_ATTR_RATE_PARENT_NODE_NAME] = { .type = NLA_NUL_STRING, },
+	[DEVLINK_ATTR_RATE_TC_BW_VALUES] = NLA_POLICY_NESTED(devlink_dl_rate_tc_bw_values_nl_policy),
};

/* DEVLINK_CMD_RATE_NEW - do */
-static const struct nla_policy devlink_rate_new_nl_policy[DEVLINK_ATTR_RATE_TX_WEIGHT + 1] = {
+static const struct nla_policy devlink_rate_new_nl_policy[DEVLINK_ATTR_RATE_TC_BW_VALUES + 1] = {
	[DEVLINK_ATTR_BUS_NAME] = { .type = NLA_NUL_STRING, },
	[DEVLINK_ATTR_DEV_NAME] = { .type = NLA_NUL_STRING, },
	[DEVLINK_ATTR_RATE_NODE_NAME] = { .type = NLA_NUL_STRING, },

@@ -517,6 +522,7 @@ static const struct nla_policy devlink_rate_new_nl_policy[DEVLINK_ATTR_RATE_TX_W

	[DEVLINK_ATTR_RATE_TX_PRIORITY] = { .type = NLA_U32, },
	[DEVLINK_ATTR_RATE_TX_WEIGHT] = { .type = NLA_U32, },
	[DEVLINK_ATTR_RATE_PARENT_NODE_NAME] = { .type = NLA_NUL_STRING, },
+	[DEVLINK_ATTR_RATE_TC_BW_VALUES] = NLA_POLICY_NESTED(devlink_dl_rate_tc_bw_values_nl_policy),
};

/* DEVLINK_CMD_RATE_DEL - do */

@@ -1164,7 +1170,7 @@ const struct genl_split_ops devlink_nl_ops[74] = {

		.doit		= devlink_nl_rate_set_doit,
		.post_doit	= devlink_nl_post_doit,
		.policy		= devlink_rate_set_nl_policy,
-		.maxattr	= DEVLINK_ATTR_RATE_TX_WEIGHT,
+		.maxattr	= DEVLINK_ATTR_RATE_TC_BW_VALUES,
		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
	},
	{

@@ -1174,7 +1180,7 @@ const struct genl_split_ops devlink_nl_ops[74] = {

		.doit		= devlink_nl_rate_new_doit,
		.post_doit	= devlink_nl_post_doit,
		.policy		= devlink_rate_new_nl_policy,
-		.maxattr	= DEVLINK_ATTR_RATE_TX_WEIGHT,
+		.maxattr	= DEVLINK_ATTR_RATE_TC_BW_VALUES,
		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
	},
	{

diff --git a/net/devlink/netlink_gen.h b/net/devlink/netlink_gen.h
index 8f2bd50ddf5e..a8f0f20f6f0b 100644
--- a/net/devlink/netlink_gen.h
+++ b/net/devlink/netlink_gen.h

@@ -13,6 +13,7 @@

/* Common nested types */
extern const struct nla_policy devlink_dl_port_function_nl_policy[DEVLINK_PORT_FN_ATTR_CAPS + 1];
+extern const struct nla_policy devlink_dl_rate_tc_bw_values_nl_policy[DEVLINK_ATTR_RATE_TC_BW + 1];
extern const struct nla_policy devlink_dl_selftest_id_nl_policy[DEVLINK_ATTR_SELFTEST_ID_FLASH + 1];

/* Ops table for devlink */

diff --git a/net/devlink/rate.c b/net/devlink/rate.c
index 8828ffaf6cbc..4eb0598d40f9 100644
--- a/net/devlink/rate.c
+++ b/net/devlink/rate.c

@@ -86,7 +86,9 @@ static int devlink_nl_rate_fill(struct sk_buff *msg,

				int flags, struct netlink_ext_ack *extack)
{
	struct devlink *devlink = devlink_rate->devlink;
+	struct nlattr *nla_tc_bw;
	void *hdr;
+	int i;

	hdr = genlmsg_put(msg, portid, seq, &devlink_nl_family, flags, cmd);
	if (!hdr)

@@ -129,6 +131,19 @@ static int devlink_nl_rate_fill(struct sk_buff *msg,

				   devlink_rate->parent->name))
			goto nla_put_failure;

+	nla_tc_bw = nla_nest_start(msg, DEVLINK_ATTR_RATE_TC_BW_VALUES);
+	if (!nla_tc_bw)
+		goto nla_put_failure;
+
+	for (i = 0; i < IEEE_8021QAZ_MAX_TCS; i++) {
+		if (nla_put_u32(msg, DEVLINK_ATTR_RATE_TC_BW, devlink_rate->tc_bw[i])) {
+			nla_nest_cancel(msg, nla_tc_bw);
+			goto nla_put_failure;
+		}
+	}
+
+	nla_nest_end(msg, nla_tc_bw);
+
	genlmsg_end(msg, hdr);
	return 0;

@@ -316,11 +331,46 @@ devlink_nl_rate_parent_node_set(struct devlink_rate *devlink_rate,

	return 0;
}

+static int devlink_nl_rate_tc_bw_set(struct devlink_rate *devlink_rate,
+				     struct genl_info *info,
+				     struct nlattr *nla_tc_bw)
+{
+	struct devlink *devlink = devlink_rate->devlink;
+	const struct devlink_ops *ops = devlink->ops;
+	u32 tc_bw[IEEE_8021QAZ_MAX_TCS] = {0};

You don't need 0 between brackets.

I will drop this, thanks.

quoted

+	struct nlattr *nla_tc_entry;
+	int rem, err = 0, i = 0;
+
+	nla_for_each_nested(nla_tc_entry, nla_tc_bw, rem) {
+		if (i >= IEEE_8021QAZ_MAX_TCS || nla_type(nla_tc_entry) != DEVLINK_ATTR_RATE_TC_BW)

Fill up an extack message with proper reasoning.

quoted

+			return -EINVAL;
+
+		tc_bw[i++] = nla_get_u32(nla_tc_entry);
+	}
+
+	if (i != IEEE_8021QAZ_MAX_TCS)
+		return -EINVAL;
+
+	if (devlink_rate_is_leaf(devlink_rate))
+		err = ops->rate_leaf_tc_bw_set(devlink_rate, devlink_rate->priv, tc_bw,
+					       info->extack);
+	else if (devlink_rate_is_node(devlink_rate))
+		err = ops->rate_node_tc_bw_set(devlink_rate, devlink_rate->priv, tc_bw,
+					       info->extack);
+
+	if (err)
+		return err;
+
+	memcpy(devlink_rate->tc_bw, tc_bw, sizeof(tc_bw));
+
+	return 0;
+}
+
static int devlink_nl_rate_set(struct devlink_rate *devlink_rate,
			       const struct devlink_ops *ops,
			       struct genl_info *info)
{
-	struct nlattr *nla_parent, **attrs = info->attrs;
+	struct nlattr *nla_parent, *nla_tc_bw, **attrs = info->attrs;
	int err = -EOPNOTSUPP;
	u32 priority;
	u32 weight;

@@ -380,6 +430,13 @@ static int devlink_nl_rate_set(struct devlink_rate *devlink_rate,

		devlink_rate->tx_weight = weight;
	}

+	nla_tc_bw = attrs[DEVLINK_ATTR_RATE_TC_BW_VALUES];
+	if (nla_tc_bw) {
+		err = devlink_nl_rate_tc_bw_set(devlink_rate, info, nla_tc_bw);
+		if (err)
+			return err;
+	}
+
	nla_parent = attrs[DEVLINK_ATTR_RATE_PARENT_NODE_NAME];
	if (nla_parent) {
		err = devlink_nl_rate_parent_node_set(devlink_rate, info,

@@ -423,6 +480,12 @@ static bool devlink_rate_set_ops_supported(const struct devlink_ops *ops,

					    "TX weight set isn't supported for the leafs");
			return false;
		}
+		if (attrs[DEVLINK_ATTR_RATE_TC_BW_VALUES] && !ops->rate_leaf_tc_bw_set) {
+			NL_SET_ERR_MSG_ATTR(info->extack,
+					    attrs[DEVLINK_ATTR_RATE_TC_BW_VALUES],
+					    "TC bandwidth set isn't supported for the leafs");
+			return false;
+		}
	} else if (type == DEVLINK_RATE_TYPE_NODE) {
		if (attrs[DEVLINK_ATTR_RATE_TX_SHARE] && !ops->rate_node_tx_share_set) {
			NL_SET_ERR_MSG(info->extack, "TX share set isn't supported for the nodes");

@@ -449,6 +512,12 @@ static bool devlink_rate_set_ops_supported(const struct devlink_ops *ops,

					    "TX weight set isn't supported for the nodes");
			return false;
		}
+		if (attrs[DEVLINK_ATTR_RATE_TC_BW_VALUES] && !ops->rate_node_tc_bw_set) {
+			NL_SET_ERR_MSG_ATTR(info->extack,
+					    attrs[DEVLINK_ATTR_RATE_TC_BW_VALUES],
+					    "TC bandwidth set isn't supported for the nodes");
+			return false;
+		}
	} else {
		WARN(1, "Unknown type of rate object");
		return false;
-- 
2.44.0