[PATCH iproute2-next 0/5] netshaper: Extend netshaper support

[PATCH iproute2-next 0/5] netshaper: Extend netshaper support

[Mohsin Bashir]

From: Mohsin Bashir <hmohsin@meta.com>

This series extends the netshaper CLI with missing parameter support
and adds the group command for building scheduling hierarchies.

The existing netshaper tool only supports setting bw-max on individual
shapers. This series adds the remaining shaper attributes (bw-min,
weight, priority) needed for TX scheduling, and introduces the
group command which ties leaf shapers to a parent node in a single
operation.

Mohsin Bashir (5):
  netshaper: Extract parse_scope() and parse_rate() helpers
  netshaper: Add bw-min and weight parameter support
  netshaper: Extend show output with parent, bw-min and weight
  netshaper: Make handle id optional for node scope
  netshaper: Add group command for creating scheduling hierarchies

 netshaper/netshaper.c | 398 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 324 insertions(+), 74 deletions(-)

-- 
2.52.0

[PATCH iproute2-next 1/5] netshaper: Extract parse_scope() and parse_rate() helpers

[Mohsin Bashir]

Add parse_scope() and parse_rate() helpers in preparation for
adding the group command which will need scope and rate parsing
at various places in the code. This patch will help to avoid
code duplication.

Verify that the tool works as before:
./netshaper set dev eth2 handle scope queue id 0 bw-max 100kbit
./netshaper delete dev eth2 handle scope queue id 0 bw-max 100kbit

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Mohsin Bashir <mohsin.bashr@gmail.com>
---
 netshaper/netshaper.c | 65 +++++++++++++++++++++++--------------------
 1 file changed, 35 insertions(+), 30 deletions(-)

diff --git a/netshaper/netshaper.c b/netshaper/netshaper.c
index 47fb805e..1603e6e5 100644
--- a/netshaper/netshaper.c
+++ b/netshaper/netshaper.c
@@ -47,6 +47,28 @@ static const char *net_shaper_scope_names[NET_SHAPER_SCOPE_MAX + 1] = {
 	"node"
 };
 
+static int parse_scope(const char *str)
+{
+	if (strcmp(str, "netdev") == 0)
+		return NET_SHAPER_SCOPE_NETDEV;
+	if (strcmp(str, "queue") == 0)
+		return NET_SHAPER_SCOPE_QUEUE;
+	if (strcmp(str, "node") == 0)
+		return NET_SHAPER_SCOPE_NODE;
+	return -1;
+}
+
+static int parse_rate(const char *str, __u64 *rate_bps)
+{
+	if (get_rate64(rate_bps, str)) {
+		fprintf(stderr, "Invalid rate value \"%s\"\n", str);
+		return -1;
+	}
+	/* get_rate64 returns bytes/sec, convert to bits/sec */
+	*rate_bps *= 8;
+	return 0;
+}
+
 static void print_netshaper_attrs(struct nlmsghdr *answer)
 {
 	struct genlmsghdr *ghdr = NLMSG_DATA(answer);
@@ -117,12 +139,8 @@ static int do_cmd(int argc, char **argv, int cmd)
 			ifindex = ll_name_to_index(*argv);
 		} else if (strcmp(*argv, "bw-max") == 0) {
 			NEXT_ARG();
-			if (get_rate64(&bw_max_bps, *argv)) {
-				fprintf(stderr, "Invalid bw-max value\n");
+			if (parse_rate(*argv, &bw_max_bps))
 				return -1;
-			}
-			/* Convert Bps to bps */
-			bw_max_bps *= 8;
 		} else if (strcmp(*argv, "handle") == 0) {
 			handle_present = true;
 			NEXT_ARG();
@@ -134,34 +152,24 @@ static int do_cmd(int argc, char **argv, int cmd)
 			}
 			NEXT_ARG();
 
-			if (strcmp(*argv, "netdev") == 0) {
-				handle_scope = NET_SHAPER_SCOPE_NETDEV;
-				/* For netdev scope, id is optional - check if next arg is "id" */
+			handle_scope = parse_scope(*argv);
+			if (handle_scope < 0) {
+				fprintf(stderr, "Invalid scope \"%s\"\n", *argv);
+				return -1;
+			}
+
+			if (handle_scope == NET_SHAPER_SCOPE_NETDEV) {
+				/* For netdev scope, id is optional */
 				if (argc > 1 && strcmp(argv[1], "id") == 0) {
-					NEXT_ARG(); /* move to "id" */
-					NEXT_ARG(); /* move to id value */
+					NEXT_ARG();
+					NEXT_ARG();
 					if (get_unsigned(&handle_id, *argv, 10)) {
 						fprintf(stderr, "Invalid handle id\n");
 						return -1;
 					}
 				}
-			} else if (strcmp(*argv, "queue") == 0) {
-				handle_scope = NET_SHAPER_SCOPE_QUEUE;
-				/* For queue scope, id is required */
-				NEXT_ARG();
-				if (strcmp(*argv, "id") != 0) {
-					fprintf(stderr, "What is \"%s\"\n", *argv);
-					usage();
-					return -1;
-				}
-				NEXT_ARG();
-				if (get_unsigned(&handle_id, *argv, 10)) {
-					fprintf(stderr, "Invalid handle id\n");
-					return -1;
-				}
-			} else if (strcmp(*argv, "node") == 0) {
-				handle_scope = NET_SHAPER_SCOPE_NODE;
-				/* For node scope, id is required */
+			} else {
+				/* For queue/node scope, id is required */
 				NEXT_ARG();
 				if (strcmp(*argv, "id") != 0) {
 					fprintf(stderr, "What is \"%s\"\n", *argv);
@@ -173,9 +181,6 @@ static int do_cmd(int argc, char **argv, int cmd)
 					fprintf(stderr, "Invalid handle id\n");
 					return -1;
 				}
-			} else {
-				fprintf(stderr, "Invalid scope\n");
-				return -1;
 			}
 		} else {
 			fprintf(stderr, "What is \"%s\"\n", *argv);
-- 
2.52.0

[PATCH iproute2-next 2/5] netshaper: Add bw-min and weight parameter support

[Mohsin Bashir]

Add bw-min and weight parameters to the set command. Previously
only bw-max was supported. Update the set validation to accept
any of bw-max, bw-min, or weight. Update usage text to reflect
the new parameters.

Before:
 ./netshaper set dev eth2 handle scope queue id 0 bw-min 100kbit
What is "bw-min"
Usage: netshaper [ OPTIONS ] { COMMAND | help }
OPTIONS := { -V[ersion] | -c[olor] | -help }
COMMAND := { set | get | delete } dev DEVNAME
	    handle scope HANDLE_SCOPE [id HANDLE_ID]
	    [bw-max BW_MAX]
Where: DEVNAME         := STRING
       HANDLE_SCOPE    := { netdev | queue | node }
       HANDLE_ID       := UINT (required for queue/node, optional for netdev)
       BW_MAX          := UINT{ kbit | mbit | gbit }

After:
 ./netshaper set dev eth2 handle scope queue id 0 bw-min 100kbit

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Mohsin Bashir <mohsin.bashr@gmail.com>
---
 netshaper/netshaper.c | 38 ++++++++++++++++++++++++++++----------
 1 file changed, 28 insertions(+), 10 deletions(-)

diff --git a/netshaper/netshaper.c b/netshaper/netshaper.c
index 1603e6e5..4ee30525 100644
--- a/netshaper/netshaper.c
+++ b/netshaper/netshaper.c
@@ -33,11 +33,12 @@ static void usage(void)
 		"OPTIONS := { -V[ersion] | -c[olor] | -help }\n"
 		"COMMAND := { set | get | delete } dev DEVNAME\n"
 		"	    handle scope HANDLE_SCOPE [id HANDLE_ID]\n"
-		"	    [bw-max BW_MAX]\n"
+		"	    [bw-max BW_MAX] [bw-min BW_MIN] [weight WEIGHT]\n"
 		"Where: DEVNAME         := STRING\n"
 		"       HANDLE_SCOPE    := { netdev | queue | node }\n"
 		"       HANDLE_ID       := UINT (required for queue/node, optional for netdev)\n"
-		"       BW_MAX          := UINT{ kbit | mbit | gbit }\n");
+		"       BW_MAX/BW_MIN   := UINT{ kbit | mbit | gbit }\n"
+		"       WEIGHT          := UINT\n");
 }
 
 static const char *net_shaper_scope_names[NET_SHAPER_SCOPE_MAX + 1] = {
@@ -125,13 +126,13 @@ static int do_cmd(int argc, char **argv, int cmd)
 	GENL_REQUEST(req, 1024, genl_family, 0, NET_SHAPER_FAMILY_VERSION, cmd,
 		     NLM_F_REQUEST | NLM_F_ACK);
 
-	struct nlmsghdr *answer;
-	__u64 bw_max_bps = 0;
-	int ifindex = -1;
+	bool has_bw_max = false, has_bw_min = false, has_weight = false;
 	int handle_scope = NET_SHAPER_SCOPE_UNSPEC;
-	__u32 handle_id = 0;
+	__u64 bw_max_bps = 0, bw_min_bps = 0;
+	__u32 handle_id = 0, weight = 0;
 	bool handle_present = false;
-	int err;
+	struct nlmsghdr *answer;
+	int err, ifindex = -1;
 
 	while (argc > 0) {
 		if (strcmp(*argv, "dev") == 0) {
@@ -141,6 +142,19 @@ static int do_cmd(int argc, char **argv, int cmd)
 			NEXT_ARG();
 			if (parse_rate(*argv, &bw_max_bps))
 				return -1;
+			has_bw_max = true;
+		} else if (strcmp(*argv, "bw-min") == 0) {
+			NEXT_ARG();
+			if (parse_rate(*argv, &bw_min_bps))
+				return -1;
+			has_bw_min = true;
+		} else if (strcmp(*argv, "weight") == 0) {
+			NEXT_ARG();
+			if (get_unsigned(&weight, *argv, 10)) {
+				fprintf(stderr, "Invalid weight value\n");
+				return -1;
+			}
+			has_weight = true;
 		} else if (strcmp(*argv, "handle") == 0) {
 			handle_present = true;
 			NEXT_ARG();
@@ -197,8 +211,8 @@ static int do_cmd(int argc, char **argv, int cmd)
 	if (!handle_present)
 		missarg("handle");
 
-	if (cmd == NET_SHAPER_CMD_SET && bw_max_bps == 0)
-		missarg("bw-max");
+	if (cmd == NET_SHAPER_CMD_SET && !has_bw_max && !has_bw_min && !has_weight)
+		missarg("bw-max, bw-min, or weight");
 
 	addattr32(&req.n, sizeof(req), NET_SHAPER_A_IFINDEX, ifindex);
 
@@ -208,8 +222,12 @@ static int do_cmd(int argc, char **argv, int cmd)
 	addattr32(&req.n, sizeof(req), NET_SHAPER_A_HANDLE_ID, handle_id);
 	addattr_nest_end(&req.n, handle);
 
-	if (cmd == NET_SHAPER_CMD_SET)
+	if (has_bw_max)
 		addattr64(&req.n, sizeof(req), NET_SHAPER_A_BW_MAX, bw_max_bps);
+	if (has_bw_min)
+		addattr64(&req.n, sizeof(req), NET_SHAPER_A_BW_MIN, bw_min_bps);
+	if (has_weight)
+		addattr32(&req.n, sizeof(req), NET_SHAPER_A_WEIGHT, weight);
 
 	err = rtnl_talk(&gen_rth, &req.n, &answer);
 	if (err < 0) {
-- 
2.52.0

[PATCH iproute2-next 3/5] netshaper: Extend show output with parent, bw-min and weight

[Mohsin Bashir]

Extend print_netshaper_attrs() to display parent scope/id, bw-min,
and weight fields in the show output. Replace the switch-based
iteration with direct attribute checks for cleaner output formatting.
Verify the support by setting the rate for a queue and attempt to read it
back.

Before:
 ./netshaper set dev eth2 handle scope queue id 0 bw-min 10mbit weight 5
 ./netshaper show dev eth2 handle scope queue id 0
scope: queue
id: 0
dev: eth2

After:
 ./netshaper set dev eth2 handle scope queue id 0 bw-min 10mbit weight 5
 ./netshaper show dev eth2 handle scope queue id 0
dev: eth2 scope queue id 1 parent scope netdev bw-min 10 mbps weight 5

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Mohsin Bashir <mohsin.bashr@gmail.com>
---
 netshaper/netshaper.c | 89 ++++++++++++++++++++++++++-----------------
 1 file changed, 55 insertions(+), 34 deletions(-)

diff --git a/netshaper/netshaper.c b/netshaper/netshaper.c
index 4ee30525..53a5eae3 100644
--- a/netshaper/netshaper.c
+++ b/netshaper/netshaper.c
@@ -72,53 +72,74 @@ static int parse_rate(const char *str, __u64 *rate_bps)
 
 static void print_netshaper_attrs(struct nlmsghdr *answer)
 {
+	struct rtattr *parent_tb[NET_SHAPER_A_HANDLE_MAX + 1] = {};
 	struct genlmsghdr *ghdr = NLMSG_DATA(answer);
 	int len = answer->nlmsg_len - NLMSG_LENGTH(GENL_HDRLEN);
 	struct rtattr *tb[NET_SHAPER_A_MAX + 1] = {};
 	struct rtattr *handle_tb[NET_SHAPER_A_HANDLE_MAX + 1] = {};
-	__u32 bw_max_mbps, scope, id;
-	__u64 bw_max_bps;
+	__u32 scope, id;
 	int ifindex;
 
 	parse_rtattr_flags(tb, NET_SHAPER_A_MAX,
 			   (struct rtattr *)((char *)ghdr + GENL_HDRLEN),
 			   len, NLA_F_NESTED);
 
-	for (int i = 1; i <= NET_SHAPER_A_MAX; ++i) {
-		if (!tb[i])
-			continue;
-		switch (i) {
-		case NET_SHAPER_A_BW_MAX:
-			bw_max_bps = rta_getattr_uint(tb[i]);
-			bw_max_mbps = (bw_max_bps / 1000000);
+	if (tb[NET_SHAPER_A_IFINDEX]) {
+		ifindex = rta_getattr_u32(tb[NET_SHAPER_A_IFINDEX]);
+		print_color_string(PRINT_ANY, COLOR_IFNAME, "dev",
+				   "dev: %s ", ll_index_to_name(ifindex));
+	}
 
-			print_uint(PRINT_ANY, "bw-max", "bw-max: %u mbps\n",
-				   bw_max_mbps);
-			break;
-		case NET_SHAPER_A_IFINDEX:
-			ifindex = rta_getattr_u32(tb[i]);
-			print_color_string(PRINT_ANY, COLOR_IFNAME, "dev",
-					   "dev: %s\n",
-					   ll_index_to_name(ifindex));
-			break;
-		case NET_SHAPER_A_HANDLE:
-			parse_rtattr_nested(handle_tb, NET_SHAPER_A_HANDLE_MAX,
-					    tb[NET_SHAPER_A_HANDLE]);
-			if (handle_tb[NET_SHAPER_A_HANDLE_SCOPE]) {
-				scope = rta_getattr_u32(handle_tb[NET_SHAPER_A_HANDLE_SCOPE]);
-				print_string(PRINT_ANY, "scope",
-					     "scope: %s\n",
-					     net_shaper_scope_names[scope]);
-			}
-			if (handle_tb[NET_SHAPER_A_HANDLE_ID]) {
-				id = rta_getattr_u32(handle_tb[NET_SHAPER_A_HANDLE_ID]);
-				print_uint(PRINT_ANY, "id", "id: %u\n", id);
-			}
-			break;
-		default:
-			break;
+	if (tb[NET_SHAPER_A_HANDLE]) {
+		parse_rtattr_nested(handle_tb, NET_SHAPER_A_HANDLE_MAX,
+				    tb[NET_SHAPER_A_HANDLE]);
+		if (handle_tb[NET_SHAPER_A_HANDLE_SCOPE]) {
+			scope = rta_getattr_u32(handle_tb[NET_SHAPER_A_HANDLE_SCOPE]);
+			print_string(PRINT_ANY, "scope", "scope %s ",
+				     net_shaper_scope_names[scope]);
+		}
+		if (handle_tb[NET_SHAPER_A_HANDLE_ID]) {
+			id = rta_getattr_u32(handle_tb[NET_SHAPER_A_HANDLE_ID]);
+			print_uint(PRINT_ANY, "id", "id %u ", id);
+		}
+	}
+
+	if (tb[NET_SHAPER_A_PARENT]) {
+		parse_rtattr_nested(parent_tb, NET_SHAPER_A_HANDLE_MAX,
+				    tb[NET_SHAPER_A_PARENT]);
+		if (parent_tb[NET_SHAPER_A_HANDLE_SCOPE]) {
+			scope = rta_getattr_u32(parent_tb[NET_SHAPER_A_HANDLE_SCOPE]);
+			print_string(PRINT_ANY, "parent-scope",
+				     "parent scope %s ",
+				     net_shaper_scope_names[scope]);
 		}
+		if (parent_tb[NET_SHAPER_A_HANDLE_ID]) {
+			id = rta_getattr_u32(parent_tb[NET_SHAPER_A_HANDLE_ID]);
+			print_uint(PRINT_ANY, "parent-id", "id %u ", id);
+		}
+	}
+
+	if (tb[NET_SHAPER_A_BW_MAX]) {
+		__u64 bw = rta_getattr_uint(tb[NET_SHAPER_A_BW_MAX]);
+
+		print_uint(PRINT_ANY, "bw-max", "bw-max %u mbps ",
+			   (__u32)(bw / 1000000));
 	}
+
+	if (tb[NET_SHAPER_A_BW_MIN]) {
+		__u64 bw = rta_getattr_uint(tb[NET_SHAPER_A_BW_MIN]);
+
+		print_uint(PRINT_ANY, "bw-min", "bw-min %u mbps ",
+			   (__u32)(bw / 1000000));
+	}
+
+	if (tb[NET_SHAPER_A_WEIGHT]) {
+		__u32 weight = rta_getattr_u32(tb[NET_SHAPER_A_WEIGHT]);
+
+		print_uint(PRINT_ANY, "weight", "weight %u ", weight);
+	}
+
+	printf("\n");
 }
 
 static int do_cmd(int argc, char **argv, int cmd)
-- 
2.52.0

[PATCH iproute2-next 4/5] netshaper: Make handle id optional for node scope

[Mohsin Bashir]

Make handle id optional for node scope so that omitting it tells
the kernel to create a new node and assign an id automatically,
while providing it updates an existing node. Queue scope still
requires an explicit id since it maps to a specific TX queue.
Only send the handle id in the netlink message when the user
explicitly provides one.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Mohsin Bashir <mohsin.bashr@gmail.com>
---
 netshaper/netshaper.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/netshaper/netshaper.c b/netshaper/netshaper.c
index 53a5eae3..fc68e735 100644
--- a/netshaper/netshaper.c
+++ b/netshaper/netshaper.c
@@ -152,6 +152,7 @@ static int do_cmd(int argc, char **argv, int cmd)
 	__u64 bw_max_bps = 0, bw_min_bps = 0;
 	__u32 handle_id = 0, weight = 0;
 	bool handle_present = false;
+	bool has_handle_id = false;
 	struct nlmsghdr *answer;
 	int err, ifindex = -1;
 
@@ -193,18 +194,8 @@ static int do_cmd(int argc, char **argv, int cmd)
 				return -1;
 			}
 
-			if (handle_scope == NET_SHAPER_SCOPE_NETDEV) {
-				/* For netdev scope, id is optional */
-				if (argc > 1 && strcmp(argv[1], "id") == 0) {
-					NEXT_ARG();
-					NEXT_ARG();
-					if (get_unsigned(&handle_id, *argv, 10)) {
-						fprintf(stderr, "Invalid handle id\n");
-						return -1;
-					}
-				}
-			} else {
-				/* For queue/node scope, id is required */
+			if (handle_scope == NET_SHAPER_SCOPE_QUEUE) {
+				/* For queue scope, id is required */
 				NEXT_ARG();
 				if (strcmp(*argv, "id") != 0) {
 					fprintf(stderr, "What is \"%s\"\n", *argv);
@@ -216,6 +207,16 @@ static int do_cmd(int argc, char **argv, int cmd)
 					fprintf(stderr, "Invalid handle id\n");
 					return -1;
 				}
+				has_handle_id = true;
+			} else if (argc > 1 && strcmp(argv[1], "id") == 0) {
+				/* For netdev and node scope, id is optional */
+				NEXT_ARG();
+				NEXT_ARG();
+				if (get_unsigned(&handle_id, *argv, 10)) {
+					fprintf(stderr, "Invalid handle id\n");
+					return -1;
+				}
+				has_handle_id = true;
 			}
 		} else {
 			fprintf(stderr, "What is \"%s\"\n", *argv);
@@ -240,7 +241,8 @@ static int do_cmd(int argc, char **argv, int cmd)
 	struct rtattr *handle = addattr_nest(&req.n, sizeof(req),
 					     NET_SHAPER_A_HANDLE | NLA_F_NESTED);
 	addattr32(&req.n, sizeof(req), NET_SHAPER_A_HANDLE_SCOPE, handle_scope);
-	addattr32(&req.n, sizeof(req), NET_SHAPER_A_HANDLE_ID, handle_id);
+	if (has_handle_id)
+		addattr32(&req.n, sizeof(req), NET_SHAPER_A_HANDLE_ID, handle_id);
 	addattr_nest_end(&req.n, handle);
 
 	if (has_bw_max)
-- 
2.52.0

[PATCH iproute2-next 5/5] netshaper: Add group command for creating scheduling hierarchies

[Mohsin Bashir]

Add the group command to create and update scheduling groups via the
NET_SHAPER_CMD_GROUP netlink operation. This enables building shaper
hierarchies by specifying a node handle, parent scope, rate parameters,
and a set of leaf shapers (queues or nodes) to attach.

Example usage:
  netshaper group dev eth0 handle scope node parent scope netdev \
      bw-max 1gbit leaves scope queue id 0 scope queue id 1

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Mohsin Bashir <mohsin.bashr@gmail.com>
---
 netshaper/netshaper.c | 206 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 205 insertions(+), 1 deletion(-)

diff --git a/netshaper/netshaper.c b/netshaper/netshaper.c
index fc68e735..7ec8220c 100644
--- a/netshaper/netshaper.c
+++ b/netshaper/netshaper.c
@@ -31,9 +31,15 @@ static void usage(void)
 	fprintf(stderr,
 		"Usage: netshaper [ OPTIONS ] { COMMAND | help }\n"
 		"OPTIONS := { -V[ersion] | -c[olor] | -help }\n"
-		"COMMAND := { set | get | delete } dev DEVNAME\n"
+		"COMMAND := { set | get | delete | group } dev DEVNAME\n"
 		"	    handle scope HANDLE_SCOPE [id HANDLE_ID]\n"
 		"	    [bw-max BW_MAX] [bw-min BW_MIN] [weight WEIGHT]\n"
+		"\n"
+		"netshaper group dev DEVNAME handle scope SCOPE [ id ID ]\n"
+		"          parent scope SCOPE [ id ID ]\n"
+		"          [ bw-max BW ] [ bw-min BW ] [ weight WEIGHT ]\n"
+		"          leaves { scope SCOPE id ID } [ ... ]\n"
+		"\n"
 		"Where: DEVNAME         := STRING\n"
 		"       HANDLE_SCOPE    := { netdev | queue | node }\n"
 		"       HANDLE_ID       := UINT (required for queue/node, optional for netdev)\n"
@@ -264,6 +270,202 @@ static int do_cmd(int argc, char **argv, int cmd)
 	return err;
 }
 
+static int do_group(int argc, char **argv)
+{
+	GENL_REQUEST(req, 4096, genl_family, 0, NET_SHAPER_FAMILY_VERSION,
+		     NET_SHAPER_CMD_GROUP, NLM_F_REQUEST | NLM_F_ACK);
+
+	bool has_bw_max = false, has_bw_min = false, has_weight = false;
+	bool parsing_leaves = false, has_hid = false, has_pid = false;
+	int parent_scope = -1, ifindex = -1, num_leaves = 0;
+	int err, handle_scope = NET_SHAPER_SCOPE_UNSPEC;
+	__u32 handle_id = 0, parent_id = 0, weight = 0;
+	__u64 bw_max_bps = 0, bw_min_bps = 0;
+	struct nlmsghdr *answer;
+
+	struct {
+		int scope;
+		__u32 id;
+	} leaves[128];
+
+	while (argc > 0) {
+		if (parsing_leaves) {
+			if (strcmp(*argv, "scope") == 0) {
+				int lscope;
+
+				NEXT_ARG();
+				lscope = parse_scope(*argv);
+				if (lscope < 0) {
+					fprintf(stderr, "Invalid leaf scope \"%s\"\n",
+						*argv);
+					return -1;
+				}
+
+				NEXT_ARG();
+				if (strcmp(*argv, "id") != 0) {
+					fprintf(stderr, "Expected \"id\" after leaf scope\n");
+					return -1;
+				}
+
+				NEXT_ARG();
+				leaves[num_leaves].scope = lscope;
+				if (get_unsigned(&leaves[num_leaves].id, *argv, 10)) {
+					fprintf(stderr, "Invalid leaf id\n");
+					return -1;
+				}
+				num_leaves++;
+				argc--;
+				argv++;
+				continue;
+			}
+			parsing_leaves = false;
+		}
+
+		if (strcmp(*argv, "dev") == 0) {
+			NEXT_ARG();
+			ifindex = ll_name_to_index(*argv);
+			if (ifindex == 0) {
+				fprintf(stderr, "Device \"%s\" not found\n", *argv);
+				return -1;
+			}
+		} else if (strcmp(*argv, "bw-max") == 0) {
+			NEXT_ARG();
+			if (parse_rate(*argv, &bw_max_bps))
+				return -1;
+			has_bw_max = true;
+		} else if (strcmp(*argv, "bw-min") == 0) {
+			NEXT_ARG();
+			if (parse_rate(*argv, &bw_min_bps))
+				return -1;
+			has_bw_min = true;
+		} else if (strcmp(*argv, "weight") == 0) {
+			NEXT_ARG();
+			if (get_unsigned(&weight, *argv, 10)) {
+				fprintf(stderr, "Invalid weight value\n");
+				return -1;
+			}
+			has_weight = true;
+		} else if (strcmp(*argv, "handle") == 0) {
+			NEXT_ARG();
+			if (strcmp(*argv, "scope") != 0) {
+				fprintf(stderr, "Expected \"scope\" after \"handle\"\n");
+				return -1;
+			}
+			NEXT_ARG();
+			handle_scope = parse_scope(*argv);
+			if (handle_scope < 0) {
+				fprintf(stderr, "Invalid handle scope \"%s\"\n",
+					*argv);
+				return -1;
+			}
+
+			if (argc > 1 && strcmp(argv[1], "id") == 0) {
+				NEXT_ARG();
+				NEXT_ARG();
+				if (get_unsigned(&handle_id, *argv, 10)) {
+					fprintf(stderr, "Invalid handle id\n");
+					return -1;
+				}
+				has_hid = true;
+			}
+		} else if (strcmp(*argv, "parent") == 0) {
+			NEXT_ARG();
+			if (strcmp(*argv, "scope") != 0) {
+				fprintf(stderr, "Expected \"scope\" after \"parent\"\n");
+				return -1;
+			}
+			NEXT_ARG();
+			parent_scope = parse_scope(*argv);
+			if (parent_scope < 0) {
+				fprintf(stderr, "Invalid parent scope \"%s\"\n",
+					*argv);
+				return -1;
+			}
+
+			if (argc > 1 && strcmp(argv[1], "id") == 0) {
+				NEXT_ARG();
+				NEXT_ARG();
+				if (get_unsigned(&parent_id, *argv, 10)) {
+					fprintf(stderr, "Invalid parent id\n");
+					return -1;
+				}
+				has_pid = true;
+			}
+		} else if (strcmp(*argv, "leaves") == 0) {
+			parsing_leaves = true;
+			argc--;
+			argv++;
+			continue;
+		} else {
+			fprintf(stderr, "What is \"%s\"\n", *argv);
+			usage();
+			return -1;
+		}
+		argc--;
+		argv++;
+	}
+
+	if (ifindex == -1)
+		missarg("dev");
+	if (handle_scope == NET_SHAPER_SCOPE_UNSPEC)
+		missarg("handle");
+	if (parent_scope < 0)
+		missarg("parent");
+	if (num_leaves == 0)
+		missarg("leaves");
+
+	addattr32(&req.n, sizeof(req), NET_SHAPER_A_IFINDEX, ifindex);
+
+	struct rtattr *parent = addattr_nest(&req.n, sizeof(req),
+					     NET_SHAPER_A_PARENT | NLA_F_NESTED);
+	addattr32(&req.n, sizeof(req), NET_SHAPER_A_HANDLE_SCOPE, parent_scope);
+	if (has_pid)
+		addattr32(&req.n, sizeof(req), NET_SHAPER_A_HANDLE_ID, parent_id);
+	addattr_nest_end(&req.n, parent);
+
+	struct rtattr *handle = addattr_nest(&req.n, sizeof(req),
+					     NET_SHAPER_A_HANDLE | NLA_F_NESTED);
+	addattr32(&req.n, sizeof(req), NET_SHAPER_A_HANDLE_SCOPE, handle_scope);
+	if (has_hid)
+		addattr32(&req.n, sizeof(req), NET_SHAPER_A_HANDLE_ID, handle_id);
+	addattr_nest_end(&req.n, handle);
+
+	if (has_bw_max)
+		addattr64(&req.n, sizeof(req), NET_SHAPER_A_BW_MAX, bw_max_bps);
+	if (has_bw_min)
+		addattr64(&req.n, sizeof(req), NET_SHAPER_A_BW_MIN, bw_min_bps);
+	if (has_weight)
+		addattr32(&req.n, sizeof(req), NET_SHAPER_A_WEIGHT, weight);
+
+	if (has_bw_max || has_bw_min)
+		addattr32(&req.n, sizeof(req), NET_SHAPER_A_METRIC,
+			  NET_SHAPER_METRIC_BPS);
+
+	for (int i = 0; i < num_leaves; i++) {
+		struct rtattr *leaf, *leaf_handle;
+
+		leaf = addattr_nest(&req.n, sizeof(req),
+				    NET_SHAPER_A_LEAVES | NLA_F_NESTED);
+		leaf_handle = addattr_nest(&req.n, sizeof(req),
+					   NET_SHAPER_A_HANDLE | NLA_F_NESTED);
+		addattr32(&req.n, sizeof(req), NET_SHAPER_A_HANDLE_SCOPE,
+			  leaves[i].scope);
+		addattr32(&req.n, sizeof(req), NET_SHAPER_A_HANDLE_ID,
+			  leaves[i].id);
+		addattr_nest_end(&req.n, leaf_handle);
+		addattr_nest_end(&req.n, leaf);
+	}
+
+	err = rtnl_talk(&gen_rth, &req.n, &answer);
+	if (err < 0) {
+		fprintf(stderr, "Kernel command failed: %d\n", err);
+		return err;
+	}
+
+	print_netshaper_attrs(answer);
+	return 0;
+}
+
 int main(int argc, char **argv)
 {
 	int color = default_color_opt();
@@ -308,6 +510,8 @@ int main(int argc, char **argv)
 			return do_cmd(argc - 1, argv + 1, NET_SHAPER_CMD_DELETE);
 		if (strcmp(*argv, "show") == 0)
 			return do_cmd(argc - 1, argv + 1, NET_SHAPER_CMD_GET);
+		if (strcmp(*argv, "group") == 0)
+			return do_group(argc - 1, argv + 1);
 		if (strcmp(*argv, "help") == 0) {
 			usage();
 			return 0;
-- 
2.52.0

Re: [PATCH iproute2-next 0/5] netshaper: Extend netshaper support

[Stephen Hemminger]

On Thu, 30 Apr 2026 18:16:06 -0700
Mohsin Bashir <mohsin.bashr@gmail.com> wrote:

> From: Mohsin Bashir <hmohsin@meta.com>
> 
> This series extends the netshaper CLI with missing parameter support
> and adds the group command for building scheduling hierarchies.
> 
> The existing netshaper tool only supports setting bw-max on individual
> shapers. This series adds the remaining shaper attributes (bw-min,
> weight, priority) needed for TX scheduling, and introduces the
> group command which ties leaf shapers to a parent node in a single
> operation.
> 
> Mohsin Bashir (5):
>   netshaper: Extract parse_scope() and parse_rate() helpers
>   netshaper: Add bw-min and weight parameter support
>   netshaper: Extend show output with parent, bw-min and weight
>   netshaper: Make handle id optional for node scope
>   netshaper: Add group command for creating scheduling hierarchies
> 
>  netshaper/netshaper.c | 398 ++++++++++++++++++++++++++++++++++--------
>  1 file changed, 324 insertions(+), 74 deletions(-)
> 

AI review liked the patch but found a JSON break
As always there are some noise things here.
Like the suggestion about noreturn attribute.

Subject: Re: [PATCH iproute2] netshaper: Add group command and parameter support

On Thu, 30 Apr 2026, Mohsin Bashir wrote:
> This series adds netshaper support for scheduling hierarchies, bw-min/weight
> parameters, and improved output formatting.

Overall the patches look good and follow most iproute2 conventions properly.
I especially appreciate that all new code correctly uses strcmp() instead of
matches() for argument parsing.

However, there are a few minor issues that should be addressed:

> @@ -47,55 +54,98 @@ static const char *net_shaper_scope_names[NET_SHAPER_SCOPE_MAX + 1] = {
>  static void print_netshaper_attrs(struct nlmsghdr *answer)
> [...]
> +	printf("\n");
>  }

The raw printf("\n") at the end of print_netshaper_attrs() breaks JSON output.
All output should use the print_XXX() helpers to maintain proper JSON formatting.
This should be:

	print_nl();

or handled by the print functions themselves. The print helpers ensure that
JSON output remains valid when the -j flag is used.

> @@ -103,13 +153,14 @@ static int do_cmd(int argc, char **argv, int cmd)
> [...]
>  	while (argc > 0) {
>  		if (strcmp(*argv, "dev") == 0) {
>  			NEXT_ARG();
>  			ifindex = ll_name_to_index(*argv);

In do_cmd(), the return value of ll_name_to_index() is not properly validated.
When the device doesn't exist, this function returns 0. The code should check:

		ifindex = ll_name_to_index(*argv);
		if (ifindex == 0) {
			fprintf(stderr, "Device \"%s\" not found\n", *argv);
			return -1;
		}

This validation is correctly done in do_group() but missing in do_cmd().

> @@ -31,13 +31,20 @@ static void usage(void)
>  {
>  	fprintf(stderr,
>  		"Usage: netshaper [ OPTIONS ] { COMMAND | help }\n"

The usage() function should be marked with __attribute__((noreturn)) as per
iproute2 coding conventions:

static void usage(void) __attribute__((noreturn));

static void usage(void)
{
	...
	exit(-1);
}

This helps the compiler with optimization and static analysis.

These are all minor issues in an otherwise well-structured patch series.
The code properly uses designated initializers, validates input with
appropriate helpers, and follows the error handling patterns correctly.

With these small fixes, the series would be ready for inclusion.

Best regards

Re: [PATCH iproute2-next 1/5] netshaper: Extract parse_scope() and parse_rate() helpers

[David Ahern]

On 4/30/26 7:16 PM, Mohsin Bashir wrote:
> diff --git a/netshaper/netshaper.c b/netshaper/netshaper.c
> index 47fb805e..1603e6e5 100644
> --- a/netshaper/netshaper.c
> +++ b/netshaper/netshaper.c
> @@ -47,6 +47,28 @@ static const char *net_shaper_scope_names[NET_SHAPER_SCOPE_MAX + 1] = {
>  	"node"
>  };
>  
> +static int parse_scope(const char *str)
> +{
> +	if (strcmp(str, "netdev") == 0)
> +		return NET_SHAPER_SCOPE_NETDEV;
> +	if (strcmp(str, "queue") == 0)
> +		return NET_SHAPER_SCOPE_QUEUE;
> +	if (strcmp(str, "node") == 0)
> +		return NET_SHAPER_SCOPE_NODE;
> +	return -1;

net_shaper_scope_names exists. Create 1 table used for both parsing and
printing.

> +}
> +
> +static int parse_rate(const char *str, __u64 *rate_bps)
> +{
> +	if (get_rate64(rate_bps, str)) {
> +		fprintf(stderr, "Invalid rate value \"%s\"\n", str);
> +		return -1;
> +	}
> +	/* get_rate64 returns bytes/sec, convert to bits/sec */
> +	*rate_bps *= 8;

no magic numbers. Add a define for BITS_PER_BYTE to include/utils.h
above BIT()

Re: [PATCH iproute2-next 2/5] netshaper: Add bw-min and weight parameter support

[David Ahern]

On 4/30/26 7:16 PM, Mohsin Bashir wrote:
> diff --git a/netshaper/netshaper.c b/netshaper/netshaper.c
> index 1603e6e5..4ee30525 100644
> --- a/netshaper/netshaper.c
> +++ b/netshaper/netshaper.c
> @@ -33,11 +33,12 @@ static void usage(void)
>  		"OPTIONS := { -V[ersion] | -c[olor] | -help }\n"
>  		"COMMAND := { set | get | delete } dev DEVNAME\n"
>  		"	    handle scope HANDLE_SCOPE [id HANDLE_ID]\n"
> -		"	    [bw-max BW_MAX]\n"
> +		"	    [bw-max BW_MAX] [bw-min BW_MIN] [weight WEIGHT]\n"

natural order is min then max

>  		"Where: DEVNAME         := STRING\n"
>  		"       HANDLE_SCOPE    := { netdev | queue | node }\n"
>  		"       HANDLE_ID       := UINT (required for queue/node, optional for netdev)\n"
> -		"       BW_MAX          := UINT{ kbit | mbit | gbit }\n");
> +		"       BW_MAX/BW_MIN   := UINT{ kbit | mbit | gbit }\n"
> +		"       WEIGHT          := UINT\n");
>  }
>  
>  static const char *net_shaper_scope_names[NET_SHAPER_SCOPE_MAX + 1] = {
> @@ -125,13 +126,13 @@ static int do_cmd(int argc, char **argv, int cmd)
>  	GENL_REQUEST(req, 1024, genl_family, 0, NET_SHAPER_FAMILY_VERSION, cmd,
>  		     NLM_F_REQUEST | NLM_F_ACK);
>  
> -	struct nlmsghdr *answer;
> -	__u64 bw_max_bps = 0;
> -	int ifindex = -1;
> +	bool has_bw_max = false, has_bw_min = false, has_weight = false;

can `bw_{max,min}_bps != 0` be used instead of a separate bool?
similarly for weight

Re: [PATCH iproute2-next 0/5] netshaper: Extend netshaper support

[David Ahern]

On 4/30/26 7:16 PM, Mohsin Bashir wrote:
> From: Mohsin Bashir <hmohsin@meta.com>
> 
> This series extends the netshaper CLI with missing parameter support
> and adds the group command for building scheduling hierarchies.
> 
> The existing netshaper tool only supports setting bw-max on individual
> shapers. This series adds the remaining shaper attributes (bw-min,
> weight, priority) needed for TX scheduling, and introduces the
> group command which ties leaf shapers to a parent node in a single
> operation.
> 
> Mohsin Bashir (5):
>   netshaper: Extract parse_scope() and parse_rate() helpers
>   netshaper: Add bw-min and weight parameter support
>   netshaper: Extend show output with parent, bw-min and weight
>   netshaper: Make handle id optional for node scope
>   netshaper: Add group command for creating scheduling hierarchies
> 
>  netshaper/netshaper.c | 398 ++++++++++++++++++++++++++++++++++--------
>  1 file changed, 324 insertions(+), 74 deletions(-)
> 

man page?

Re: [PATCH iproute2-next 0/5] netshaper: Extend netshaper support

[Mohsin Bashir]

> This helps the compiler with optimization and static analysis.
> 
> These are all minor issues in an otherwise well-structured patch series.
> The code properly uses designated initializers, validates input with
> appropriate helpers, and follows the error handling patterns correctly.
> 
> With these small fixes, the series would be ready for inclusion.
> 
> Best regards

Thanks for the feedback Stephen and David. I'll address these points in V2.