Skip to contentSkip to conversation

Support for nexthop group statistics

PATCH Superseded Petr Machata 3 participants 11 messages Mar 8, 2024 – Mar 11, 2024

Subsystems (2)

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
#0 lore raw

cover Support for nexthop group statistics

Next hop group stats allow verification of balancedness of a next hop group. The feature was merged in kernel commit 7cf497e5a122 ("Merge branch 'nexthop-group-stats'"). This patchset adds to ip the corresponding support.

NH group stats come in two flavors: as statistics for SW and for HW datapaths. The former is shown when -s is given to "ip nexthop". The latter implies more work from the kernel, and possibly driver and HW, and might not be always necessary. Therefore tie it to -s -s, similarly to how ip link shows more detailed stats when -s is given twice.

Here's an example usage:

 # ip link add name gre1 up type gre \
      local 172.16.1.1 remote 172.16.1.2 tos inherit
 # ip nexthop replace id 1001 dev gre1
 # ip nexthop replace id 1002 dev gre1
 # ip nexthop replace id 1111 group 1001/1002 hw_stats on
 # ip -s -s -j -p nexthop show id 1111
 [ {
 	[ ...snip... ]
         "hw_stats": {
             "enabled": true,
             "used": true
         },
         "group_stats": [ {
                 "id": 1001,
                 "packets": 0,
                 "packets_hw": 0
             },{
                 "id": 1002,
                 "packets": 0,
                 "packets_hw": 0
             } ]
     } ]

hw_stats.enabled shows whether hw_stats have been requested for the given group. hw_stats.used shows whether any driver actually implemented the counter. group_stats[].packets show the total stats, packets_hw only the HW-datapath stats.

Petr Machata (4):
libnetlink: Add rta_getattr_uint()
ip: ipnexthop: Support dumping next hop group stats
ip: ipnexthop: Support dumping next hop group HW stats
ip: ipnexthop: Allow toggling collection of nexthop group HW
statistics

 include/libnetlink.h  |   6 ++
 ip/ipnexthop.c        | 127 ++++++++++++++++++++++++++++++++++++++++++
 ip/nh_common.h        |  11 ++++
 man/man8/ip-nexthop.8 |   2 +
 4 files changed, 146 insertions(+)
Signature · 1 line
2.43.0
#1 lore raw

PATCH 1/4 libnetlink: Add rta_getattr_uint()

NLA_UINT attributes have a 4- or 8-byte payload. Add a function to extract these.

---
 include/libnetlink.h | 6 ++++++
 1 file changed, 6 insertions(+)
include/libnetlink.h +6 -0
index ad7e7127..a1233659 100644
@@ -260,6 +260,12 @@ static inline __u64 rta_getattr_u64(const struct rtattr *rta)
 	memcpy(&tmp, RTA_DATA(rta), sizeof(__u64));
 	return tmp;
 }
+static inline __u64 rta_getattr_uint(const struct rtattr *rta)
+{
+	if (RTA_PAYLOAD(rta) == sizeof(__u32))
+		return rta_getattr_u32(rta);
+	return rta_getattr_u64(rta);
+}
 static inline __s32 rta_getattr_s32(const struct rtattr *rta)
 {
 	return *(__s32 *)RTA_DATA(rta);
Signature · 1 line
2.43.0
#2 lore raw

Re: [PATCH iproute2-next 1/4] libnetlink: Add rta_getattr_uint()

include/libnetlink.h static inline __u64 rta_getattr_u64(const struct rtattr *rta) · line 267

Quoted 5 lines · On Fri, 8 Mar 2024 23:29:06 +0100 Petr Machata <[email protected]> wrote:
include/libnetlink.h +5 -0
+static inline __u64 rta_getattr_uint(const struct rtattr *rta)
+{
+	if (RTA_PAYLOAD(rta) == sizeof(__u32))
+		return rta_getattr_u32(rta);
+	return rta_getattr_u64(rta);

Don't understand the use case here.
The kernel always sends the same payload size for the same attribute.

maintainer
#3 lore raw

Re: [PATCH iproute2-next 1/4] libnetlink: Add rta_getattr_uint()

Quoted 8 lines · On Fri, 8 Mar 2024 14:58:59 -0800 Stephen Hemminger wrote:

include/libnetlink.h static inline __u64 rta_getattr_u64(const struct rtattr *rta) · line 267

Quoted 5 lines
include/libnetlink.h +5 -0
+static inline __u64 rta_getattr_uint(const struct rtattr *rta)
+{
+	if (RTA_PAYLOAD(rta) == sizeof(__u32))
+		return rta_getattr_u32(rta);
+	return rta_getattr_u64(rta);  

Don't understand the use case here.
The kernel always sends the same payload size for the same attribute.

Please see commit 374d345d9b5e13380c in the kernel.

#4 lore raw

Re: [PATCH iproute2-next 1/4] libnetlink: Add rta_getattr_uint()

Quoted 11 lines · On Fri, 8 Mar 2024 19:43:34 -0800 Jakub Kicinski <[email protected]> wrote:
Quoted 8 lines · On Fri, 8 Mar 2024 14:58:59 -0800 Stephen Hemminger wrote:

include/libnetlink.h static inline __u64 rta_getattr_u64(const struct rtattr *rta) · line 267

Quoted 5 lines
include/libnetlink.h +5 -0
+static inline __u64 rta_getattr_uint(const struct rtattr *rta)
+{
+	if (RTA_PAYLOAD(rta) == sizeof(__u32))
+		return rta_getattr_u32(rta);
+	return rta_getattr_u64(rta);    

Don't understand the use case here.
The kernel always sends the same payload size for the same attribute.

Please see commit 374d345d9b5e13380c in the kernel.

Ok, but maybe go further and handle u16 and u8

maintainer
#5 lore raw

Re: [PATCH iproute2-next 1/4] libnetlink: Add rta_getattr_uint()

Quoted 6 lines · On Sat, 9 Mar 2024 09:21:58 -0800 Stephen Hemminger wrote:
Quoted 4 lines
Quoted 2 lines

Don't understand the use case here.
The kernel always sends the same payload size for the same attribute.

Please see commit 374d345d9b5e13380c in the kernel.

Ok, but maybe go further and handle u16 and u8

I guess you can if that's helpful in iproute2, as a "universal getter", cost of a few branches. In the kernel I try hard to convince people to never use u8 and u16 as they simply waste space on padding, and the bits may turn out to be needed later.

#6 lore raw

Re: [PATCH iproute2-next 1/4] libnetlink: Add rta_getattr_uint()

In reply to Stephen Hemminger
Quoted 13 lines · Stephen Hemminger
Quoted 10 lines · On Fri, 8 Mar 2024 19:43:34 -0800 Jakub Kicinski <[email protected]> wrote:
Quoted 8 lines · On Fri, 8 Mar 2024 14:58:59 -0800 Stephen Hemminger wrote:

include/libnetlink.h static inline __u64 rta_getattr_u64(const struct rtattr *rta) · line 267

Quoted 5 lines
include/libnetlink.h +5 -0
+static inline __u64 rta_getattr_uint(const struct rtattr *rta)
+{
+	if (RTA_PAYLOAD(rta) == sizeof(__u32))
+		return rta_getattr_u32(rta);
+	return rta_getattr_u64(rta);    

Don't understand the use case here.
The kernel always sends the same payload size for the same attribute.

Quoted 3 lines
Quoted 1 line

Please see commit 374d345d9b5e13380c in the kernel.

Ok, but maybe go further and handle u16 and u8

I intended this as a getter of the corresponding kernel attribute type, which only ever sends 4- and 8-byte payloads, but sure, I can add that.

#7 lore raw

PATCH 2/4 ip: ipnexthop: Support dumping next hop group stats

In reply to Petr Machata

Next hop group stats allow verification of balancedness of a next hop group. The feature was merged in kernel commit 7cf497e5a122 ("Merge branch 'nexthop-group-stats'"). Add to ip the corresponding support. The statistics are requested if "ip nexthop" is started with -s.

---
 ip/ipnexthop.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++
 ip/nh_common.h |  6 ++++
 2 files changed, 93 insertions(+)
ip/ipnexthop.c +87 -0
index e946d6f9..8aa89546 100644
@@ -25,6 +25,7 @@ static struct {
 	unsigned int fdb;
 	unsigned int id;
 	unsigned int nhid;
+	unsigned int op_flags;
 } filter;
 
 enum {
@@ -92,6 +93,14 @@ static int nh_dump_filter(struct nlmsghdr *nlh, int reqlen)
 			return err;
 	}
 
+	if (filter.op_flags) {
+		__u32 op_flags = filter.op_flags;
+
+		err = addattr32(nlh, reqlen, NHA_OP_FLAGS, op_flags);
+		if (err)
+			return err;
+	}
+
 	return 0;
 }
 
@@ -296,6 +305,31 @@ static void parse_nh_res_group_rta(const struct rtattr *res_grp_attr,
 	}
 }
 
+static void parse_nh_group_stats_rta(const struct rtattr *grp_stats_attr,
+				     struct nh_entry *nhe)
+{
+	const struct rtattr *pos;
+	int i = 0;
+
+	rtattr_for_each_nested(pos, grp_stats_attr) {
+		struct nh_grp_stats *nh_grp_stats = &nhe->nh_grp_stats[i++];
+		struct rtattr *tb[NHA_GROUP_STATS_ENTRY_MAX + 1];
+		struct rtattr *rta;
+
+		parse_rtattr_nested(tb, NHA_GROUP_STATS_ENTRY_MAX, pos);
+
+		if (tb[NHA_GROUP_STATS_ENTRY_ID]) {
+			rta = tb[NHA_GROUP_STATS_ENTRY_ID];
+			nh_grp_stats->nh_id = rta_getattr_u32(rta);
+		}
+
+		if (tb[NHA_GROUP_STATS_ENTRY_PACKETS]) {
+			rta = tb[NHA_GROUP_STATS_ENTRY_PACKETS];
+			nh_grp_stats->packets = rta_getattr_uint(rta);
+		}
+	}
+}
+
 static void print_nh_res_group(const struct nha_res_grp *res_grp)
 {
 	struct timeval tv;
@@ -343,8 +377,33 @@ static void print_nh_res_bucket(FILE *fp, const struct rtattr *res_bucket_attr)
 	close_json_object();
 }
 
+static void print_nh_grp_stats(const struct nh_entry *nhe)
+{
+	int i;
+
+	if (!show_stats)
+		return;
+
+	open_json_array(PRINT_JSON, "group_stats");
+	print_string(PRINT_FP, NULL, "\n  stats:\n", NULL);
+	for (i = 0; i < nhe->nh_groups_cnt; i++) {
+		open_json_object(NULL);
+
+		print_uint(PRINT_ANY, "id", "    id %u",
+			   nhe->nh_grp_stats[i].nh_id);
+		print_u64(PRINT_ANY, "packets", " packets %llu",
+			  nhe->nh_grp_stats[i].packets);
+
+		if (i != nhe->nh_groups_cnt - 1)
+			print_string(PRINT_FP, NULL, "\n", NULL);
+		close_json_object();
+	}
+	close_json_array(PRINT_JSON, NULL);
+}
+
 static void ipnh_destroy_entry(struct nh_entry *nhe)
 {
+	free(nhe->nh_grp_stats);
 	free(nhe->nh_encap);
 	free(nhe->nh_groups);
 }
@@ -418,6 +477,16 @@ static int ipnh_parse_nhmsg(FILE *fp, const struct nhmsg *nhm, int len,
 		nhe->nh_has_res_grp = true;
 	}
 
+	if (tb[NHA_GROUP_STATS]) {
+		nhe->nh_grp_stats = calloc(nhe->nh_groups_cnt,
+					   sizeof(*nhe->nh_grp_stats));
+		if (!nhe->nh_grp_stats) {
+			err = -ENOMEM;
+			goto out_err;
+		}
+		parse_nh_group_stats_rta(tb[NHA_GROUP_STATS], nhe);
+	}
+
 	nhe->nh_blackhole = !!tb[NHA_BLACKHOLE];
 	nhe->nh_fdb = !!tb[NHA_FDB];
 
@@ -484,9 +553,23 @@ static void __print_nexthop_entry(FILE *fp, const char *jsobj,
 	if (nhe->nh_fdb)
 		print_null(PRINT_ANY, "fdb", "fdb", NULL);
 
+	if (nhe->nh_grp_stats)
+		print_nh_grp_stats(nhe);
+
 	close_json_object();
 }
 
+static __u32 ipnh_get_op_flags(void)
+{
+	__u32 op_flags = 0;
+
+	if (show_stats) {
+		op_flags |= NHA_OP_FLAG_DUMP_STATS;
+	}
+
+	return op_flags;
+}
+
 static int  __ipnh_get_id(struct rtnl_handle *rthp, __u32 nh_id,
 			  struct nlmsghdr **answer)
 {
@@ -500,8 +583,10 @@ static int  __ipnh_get_id(struct rtnl_handle *rthp, __u32 nh_id,
 		.n.nlmsg_type	= RTM_GETNEXTHOP,
 		.nhm.nh_family	= preferred_family,
 	};
+	__u32 op_flags = ipnh_get_op_flags();
 
 	addattr32(&req.n, sizeof(req), NHA_ID, nh_id);
+	addattr32(&req.n, sizeof(req), NHA_OP_FLAGS, op_flags);
 
 	return rtnl_talk(rthp, &req.n, answer);
 }
@@ -1093,6 +1178,8 @@ static int ipnh_list_flush(int argc, char **argv, int action)
 		argc--; argv++;
 	}
 
+	filter.op_flags = ipnh_get_op_flags();
+
 	if (action == IPNH_FLUSH)
 		return ipnh_flush(all);
 
ip/nh_common.h +6 -0
index 4d6677e6..e2f74ec5 100644
@@ -13,6 +13,11 @@ struct nha_res_grp {
 	__u64			unbalanced_time;
 };
 
+struct nh_grp_stats {
+	__u32			nh_id;
+	__u64			packets;
+};
+
 struct nh_entry {
 	struct hlist_node	nh_hash;
 
@@ -44,6 +49,7 @@ struct nh_entry {
 
 	int			nh_groups_cnt;
 	struct nexthop_grp	*nh_groups;
+	struct nh_grp_stats	*nh_grp_stats;
 };
 
 void print_cache_nexthop_id(FILE *fp, const char *fp_prefix, const char *jsobj,
Signature · 1 line
2.43.0
#8 lore raw

Re: [PATCH iproute2-next 2/4] ip: ipnexthop: Support dumping next hop group stats

ip/ipnexthop.c static void print_nh_res_bucket(FILE *fp, const struct rtattr *res_bucket_attr) · line 388

Quoted 10 lines · On Fri, 8 Mar 2024 23:29:07 +0100 Petr Machata <[email protected]> wrote:
ip/ipnexthop.c +9 -0
+static void print_nh_grp_stats(const struct nh_entry *nhe)
+{
+	int i;
+
+	if (!show_stats)
+		return;
+
+	open_json_array(PRINT_JSON, "group_stats");
+	print_string(PRINT_FP, NULL, "\n  stats:\n", NULL);

Use print_nl() to handle single line mode.

#9 lore raw

PATCH 3/4 ip: ipnexthop: Support dumping next hop group HW stats

In reply to Petr Machata

Besides SW datapath stats, the kernel also support collecting statistics from HW datapath, for nexthop groups offloaded to HW. Request that these be collected when ip is given "-s -s", similarly to how "ip link" shows more statistics in that case.

Besides the statistics themselves, also show whether the collection of HW statistics was in fact requested, and whether any driver actually implemented the request.

---
 ip/ipnexthop.c | 28 ++++++++++++++++++++++++++++
 ip/nh_common.h |  5 +++++
 2 files changed, 33 insertions(+)
ip/ipnexthop.c +28 -0
index 8aa89546..573f1abb 100644
@@ -327,6 +327,11 @@ static void parse_nh_group_stats_rta(const struct rtattr *grp_stats_attr,
 			rta = tb[NHA_GROUP_STATS_ENTRY_PACKETS];
 			nh_grp_stats->packets = rta_getattr_uint(rta);
 		}
+
+		if (tb[NHA_GROUP_STATS_ENTRY_PACKETS_HW]) {
+			rta = tb[NHA_GROUP_STATS_ENTRY_PACKETS_HW];
+			nh_grp_stats->packets_hw = rta_getattr_uint(rta);
+		}
 	}
 }
 
@@ -393,6 +398,9 @@ static void print_nh_grp_stats(const struct nh_entry *nhe)
 			   nhe->nh_grp_stats[i].nh_id);
 		print_u64(PRINT_ANY, "packets", " packets %llu",
 			  nhe->nh_grp_stats[i].packets);
+		if (show_stats > 1)
+			print_u64(PRINT_ANY, "packets_hw", " packets_hw %llu",
+				  nhe->nh_grp_stats[i].packets_hw);
 
 		if (i != nhe->nh_groups_cnt - 1)
 			print_string(PRINT_FP, NULL, "\n", NULL);
@@ -477,6 +485,15 @@ static int ipnh_parse_nhmsg(FILE *fp, const struct nhmsg *nhm, int len,
 		nhe->nh_has_res_grp = true;
 	}
 
+	if (tb[NHA_HW_STATS_ENABLE]) {
+		nhe->nh_hw_stats_supported = true;
+		nhe->nh_hw_stats_enabled =
+			!!rta_getattr_u32(tb[NHA_HW_STATS_ENABLE]);
+	}
+
+	if (tb[NHA_HW_STATS_USED])
+		nhe->nh_hw_stats_used = !!rta_getattr_u32(tb[NHA_HW_STATS_USED]);
+
 	if (tb[NHA_GROUP_STATS]) {
 		nhe->nh_grp_stats = calloc(nhe->nh_groups_cnt,
 					   sizeof(*nhe->nh_grp_stats));
@@ -553,6 +570,15 @@ static void __print_nexthop_entry(FILE *fp, const char *jsobj,
 	if (nhe->nh_fdb)
 		print_null(PRINT_ANY, "fdb", "fdb", NULL);
 
+	if ((show_details > 0 || show_stats) && nhe->nh_hw_stats_supported) {
+		open_json_object("hw_stats");
+		print_on_off(PRINT_ANY, "enabled", "hw_stats %s ",
+			     nhe->nh_hw_stats_enabled);
+		print_on_off(PRINT_ANY, "used", "used %s ",
+			     nhe->nh_hw_stats_used);
+		close_json_object();
+	}
+
 	if (nhe->nh_grp_stats)
 		print_nh_grp_stats(nhe);
 
@@ -565,6 +591,8 @@ static __u32 ipnh_get_op_flags(void)
 
 	if (show_stats) {
 		op_flags |= NHA_OP_FLAG_DUMP_STATS;
+		if (show_stats > 1)
+			op_flags |= NHA_OP_FLAG_DUMP_HW_STATS;
 	}
 
 	return op_flags;
ip/nh_common.h +5 -0
index e2f74ec5..33b1d847 100644
@@ -16,6 +16,7 @@ struct nha_res_grp {
 struct nh_grp_stats {
 	__u32			nh_id;
 	__u64			packets;
+	__u64			packets_hw;
 };
 
 struct nh_entry {
@@ -32,6 +33,10 @@ struct nh_entry {
 	bool			nh_blackhole;
 	bool			nh_fdb;
 
+	bool			nh_hw_stats_supported;
+	bool			nh_hw_stats_enabled;
+	bool			nh_hw_stats_used;
+
 	int			nh_gateway_len;
 	union {
 		__be32		ipv4;
Signature · 1 line
2.43.0
#10 lore raw

PATCH 4/4 ip: ipnexthop: Allow toggling collection of nexthop group HW statistics

In reply to Petr Machata

Besides SW datapath stats, the kernel also support collecting statistics from HW datapath, for nexthop groups offloaded to HW. Since collection of these statistics may consume HW resources, there is an interface to request that the HW stats be recorded. Add this toggle to "ip nexthop".

---
 ip/ipnexthop.c        | 12 ++++++++++++
 man/man8/ip-nexthop.8 |  2 ++
 2 files changed, 14 insertions(+)
ip/ipnexthop.c +12 -0
index 573f1abb..74685c49 100644
@@ -56,6 +56,7 @@ static void usage(void)
 		"        [ encap ENCAPTYPE ENCAPHDR ] |\n"
 		"        group GROUP [ fdb ] [ type TYPE [ TYPE_ARGS ] ] }\n"
 		"GROUP := [ <id[,weight]>/<id[,weight]>/... ]\n"
+		"         [ hw_stats {off|on} ]\n"
 		"TYPE := { mpath | resilient }\n"
 		"TYPE_ARGS := [ RESILIENT_ARGS ]\n"
 		"RESILIENT_ARGS := [ buckets BUCKETS ] [ idle_timer IDLE ]\n"
@@ -1100,6 +1101,17 @@ static int ipnh_modify(int cmd, unsigned int flags, int argc, char **argv)
 			if (rtnl_rtprot_a2n(&prot, *argv))
 				invarg("\"protocol\" value is invalid\n", *argv);
 			req.nhm.nh_protocol = prot;
+		} else if (!strcmp(*argv, "hw_stats")) {
+			bool hw_stats;
+			int ret;
+
+			NEXT_ARG();
+			hw_stats = parse_on_off("hw_stats", *argv, &ret);
+			if (ret)
+				return ret;
+
+			addattr32(&req.n, sizeof(req), NHA_HW_STATS_ENABLE,
+				  hw_stats);
 		} else if (strcmp(*argv, "help") == 0) {
 			usage();
 		} else {
man/man8/ip-nexthop.8 +2 -0
index f81a5910..aad68696 100644
@@ -68,6 +68,8 @@ ip-nexthop \- nexthop object management
 .BR fdb " ] | "
 .B  group
 .IR GROUP " [ "
+.BR hw_stats " { "
+.BR on " | " off " }  ] [ "
 .BR fdb " ] [ "
 .B type
 .IR TYPE " [ " TYPE_ARGS " ] ] }"
Signature · 1 line
2.43.0