mlxsw: Support for nexthop group statistics

PATCH Applied Petr Machata 4 participants 22 messages Mar 8, 2024 – Mar 11, 2024

Subsystems (5)

Networking Network drivers Kselftest IPv4/IPv6 +1 more subsystems

lore

Share & tools

lore

Petr Machata

author maintainer

2024-03-08 13:02 UTC #0 lore raw

cover mlxsw: Support for nexthop group statistics

ECMP is a fundamental component in L3 designs. However, it's fragile. Many factors influence whether an ECMP group will operate as intended: hash policy (i.e. the set of fields that contribute to ECMP hash calculation), neighbor validity, hash seed (which might lead to polarization) or the type of ECMP group used (hash-threshold or resilient).

At the same time, collection of statistics that would help an operator determine that the group performs as desired, is difficult.

Support for nexthop group statistics and their HW collection has been introduced recently. In this patch set, add HW stats collection support to mlxsw.

This patchset progresses as follows:

- Patches #1 and #2 add nexthop IDs to notifiers.
- Patches #3 and #4 are code-shaping.
- Patches #5, #6 and #7 adjust the flow counter code.
- Patches #8 and #9 add HW nexthop counters.
- Patch #10 adjusts the HW counter code to allow sharing the same counter for several resilient group buckets with the same NH ID.
- Patch #11 adds a selftest.

Petr Machata (11):
net: nexthop: Initialize NH group ID in resilient NH group notifiers
net: nexthop: Have all NH notifiers carry NH ID
mlxsw: spectrum_router: Rename two functions
mlxsw: spectrum_router: Have mlxsw_sp_nexthop_counter_enable() return
int
mlxsw: spectrum: Allow fetch-and-clear of flow counters
mlxsw: spectrum_router: Avoid allocating NH counters twice
mlxsw: spectrum_router: Add helpers for nexthop counters
mlxsw: spectrum_router: Track NH ID's of group members
mlxsw: spectrum_router: Support nexthop group hardware statistics
mlxsw: spectrum_router: Share nexthop counters in resilient groups
selftests: forwarding: Add a test for NH group stats

 .../net/ethernet/mellanox/mlxsw/spectrum.c    |   8 +-
 .../net/ethernet/mellanox/mlxsw/spectrum.h    |   4 +-
 .../ethernet/mellanox/mlxsw/spectrum_acl.c    |   2 +-
 .../ethernet/mellanox/mlxsw/spectrum_dpipe.c  |  22 +-
 .../mellanox/mlxsw/spectrum_mr_tcam.c         |   2 +-
 .../ethernet/mellanox/mlxsw/spectrum_router.c | 312 ++++++++++++++++--
 .../ethernet/mellanox/mlxsw/spectrum_router.h |   6 +-
 include/net/nexthop.h                         |   2 +-
 net/ipv4/nexthop.c                            |   3 +-
 .../testing/selftests/net/forwarding/Makefile |   1 +
 tools/testing/selftests/net/forwarding/lib.sh |  34 ++
 .../net/forwarding/router_mpath_nh.sh         |  13 +
 .../net/forwarding/router_mpath_nh_lib.sh     | 129 ++++++++
 .../net/forwarding/router_mpath_nh_res.sh     |  13 +
 14 files changed, 505 insertions(+), 46 deletions(-)
 create mode 100644 tools/testing/selftests/net/forwarding/router_mpath_nh_lib.sh

Signature · 1 line

2.43.0

Petr Machata

author maintainer

2024-03-08 13:02 UTC #1 lore raw

PATCH 1/11 net: nexthop: Initialize NH group ID in resilient NH group notifiers

The NEXTHOP_EVENT_RES_TABLE_PRE_REPLACE notifier currently keeps the group ID unset. That makes it impossible to look up the group for which the notifier is intended. This is not an issue at the moment, because the only client is netdevsim, and that just so that it veto replacements, which is a static property not tied to a particular group. But for any practical use, the ID is necessary. Set it.

Signed-off-by Petr Machata <[email protected]>

Reviewed-by Ido Schimmel <[email protected]>

---
 net/ipv4/nexthop.c | 1 +
 1 file changed, 1 insertion(+)

net/ipv4/nexthop.c +1 -0

index e34466751d7b..0548d1b46708 100644
@@ -407,6 +407,7 @@ static int call_nexthop_res_table_notifiers(struct net *net, struct nexthop *nh,
 	struct nh_notifier_info info = {
 		.net = net,
 		.extack = extack,
+		.id = nh->id,
 	};
 	struct nh_group *nhg;
 	int err;

Signature · 1 line

2.43.0

Petr Machata

author maintainer

2024-03-08 13:02 UTC #2 lore raw

PATCH 2/11 net: nexthop: Have all NH notifiers carry NH ID

In reply to Petr Machata

When sending the notifications to collect NH statistics for resilient groups, the driver will need to know the nexthop IDs in individual buckets to look up the right counter. To that end, move the nexthop ID from struct nh_notifier_grp_entry_info to nh_notifier_single_info.

Signed-off-by Petr Machata <[email protected]>

Reviewed-by Ido Schimmel <[email protected]>

---
 include/net/nexthop.h | 2 +-
 net/ipv4/nexthop.c    | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

include/net/nexthop.h +1 -1

index 7ec9cc80f11c..7ca315ad500e 100644
@@ -185,6 +185,7 @@ struct nh_notifier_single_info {
 		__be32 ipv4;
 		struct in6_addr ipv6;
 	};
+	u32 id;
 	u8 is_reject:1,
 	   is_fdb:1,
 	   has_encap:1;

@@ -192,7 +193,6 @@ struct nh_notifier_single_info {
 
 struct nh_notifier_grp_entry_info {
 	u8 weight;
-	u32 id;
 	struct nh_notifier_single_info nh;
 };

net/ipv4/nexthop.c +1 -1

index 0548d1b46708..5d09270359b1 100644
@@ -104,6 +104,7 @@ __nh_notifier_single_info_init(struct nh_notifier_single_info *nh_info,
 	else if (nh_info->gw_family == AF_INET6)
 		nh_info->ipv6 = nhi->fib_nhc.nhc_gw.ipv6;
 
+	nh_info->id = nhi->nh_parent->id;
 	nh_info->is_reject = nhi->reject_nh;
 	nh_info->is_fdb = nhi->fdb_nh;
 	nh_info->has_encap = !!nhi->fib_nhc.nhc_lwtstate;

@@ -150,7 +151,6 @@ static int nh_notifier_mpath_info_init(struct nh_notifier_info *info,
 		struct nh_info *nhi;
 
 		nhi = rtnl_dereference(nhge->nh->nh_info);
-		info->nh_grp->nh_entries[i].id = nhge->nh->id;
 		info->nh_grp->nh_entries[i].weight = nhge->weight;
 		__nh_notifier_single_info_init(&info->nh_grp->nh_entries[i].nh,
 					       nhi);

Signature · 1 line

2.43.0

Petr Machata

author maintainer

2024-03-08 13:02 UTC #3 lore raw

PATCH 3/11 mlxsw: spectrum_router: Rename two functions

In reply to Petr Machata

The function mlxsw_sp_nexthop_counter_alloc() doesn't directly allocate anything, and mlxsw_sp_nexthop_counter_free() doesn't directly free. For the following patches, we will need names for functions that actually do those things. Therefore rename to mlxsw_sp_nexthop_counter_enable() and mlxsw_sp_nexthop_counter_disable() to free up the namespace.

Signed-off-by Petr Machata <[email protected]>

Reviewed-by Ido Schimmel <[email protected]>

---
 .../ethernet/mellanox/mlxsw/spectrum_dpipe.c  |  4 +--
 .../ethernet/mellanox/mlxsw/spectrum_router.c | 26 +++++++++----------
 .../ethernet/mellanox/mlxsw/spectrum_router.h |  8 +++---
 3 files changed, 19 insertions(+), 19 deletions(-)

drivers/net/ethernet/mellanox/mlxsw/spectrum_dpipe.c +2 -2

index c8a356accdf8..22e3dcb1d67a 100644
@@ -1193,9 +1193,9 @@ static int mlxsw_sp_dpipe_table_adj_counters_update(void *priv, bool enable)
 		mlxsw_sp_nexthop_indexes(nh, &adj_index, &adj_size,
 					 &adj_hash_index);
 		if (enable)
-			mlxsw_sp_nexthop_counter_alloc(mlxsw_sp, nh);
+			mlxsw_sp_nexthop_counter_enable(mlxsw_sp, nh);
 		else
-			mlxsw_sp_nexthop_counter_free(mlxsw_sp, nh);
+			mlxsw_sp_nexthop_counter_disable(mlxsw_sp, nh);
 		mlxsw_sp_nexthop_eth_update(mlxsw_sp,
 					    adj_index + adj_hash_index, nh,
 					    true, ratr_pl);

drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c +13 ~~-13~~

index 87617df694ab..9bb58fb0d1da 100644
@@ -3151,8 +3151,8 @@ struct mlxsw_sp_nexthop_group {
 	bool can_destroy;
 };
 
-void mlxsw_sp_nexthop_counter_alloc(struct mlxsw_sp *mlxsw_sp,
-				    struct mlxsw_sp_nexthop *nh)
+void mlxsw_sp_nexthop_counter_enable(struct mlxsw_sp *mlxsw_sp,
+				     struct mlxsw_sp_nexthop *nh)
 {
 	struct devlink *devlink;

@@ -3167,8 +3167,8 @@ void mlxsw_sp_nexthop_counter_alloc(struct mlxsw_sp *mlxsw_sp,
 	nh->counter_valid = true;
 }
 
-void mlxsw_sp_nexthop_counter_free(struct mlxsw_sp *mlxsw_sp,
-				   struct mlxsw_sp_nexthop *nh)
+void mlxsw_sp_nexthop_counter_disable(struct mlxsw_sp *mlxsw_sp,
+				      struct mlxsw_sp_nexthop *nh)
 {
 	if (!nh->counter_valid)
 		return;

@@ -4507,7 +4507,7 @@ static int mlxsw_sp_nexthop4_init(struct mlxsw_sp *mlxsw_sp,
 	if (err)
 		return err;
 
-	mlxsw_sp_nexthop_counter_alloc(mlxsw_sp, nh);
+	mlxsw_sp_nexthop_counter_enable(mlxsw_sp, nh);
 	list_add_tail(&nh->router_list_node, &mlxsw_sp->router->nexthop_list);
 
 	if (!dev)

@@ -4531,7 +4531,7 @@ static int mlxsw_sp_nexthop4_init(struct mlxsw_sp *mlxsw_sp,
 
 err_nexthop_neigh_init:
 	list_del(&nh->router_list_node);
-	mlxsw_sp_nexthop_counter_free(mlxsw_sp, nh);
+	mlxsw_sp_nexthop_counter_disable(mlxsw_sp, nh);
 	mlxsw_sp_nexthop_remove(mlxsw_sp, nh);
 	return err;
 }

@@ -4541,7 +4541,7 @@ static void mlxsw_sp_nexthop4_fini(struct mlxsw_sp *mlxsw_sp,
 {
 	mlxsw_sp_nexthop_type_fini(mlxsw_sp, nh);
 	list_del(&nh->router_list_node);
-	mlxsw_sp_nexthop_counter_free(mlxsw_sp, nh);
+	mlxsw_sp_nexthop_counter_disable(mlxsw_sp, nh);
 	mlxsw_sp_nexthop_remove(mlxsw_sp, nh);
 }

@@ -5006,7 +5006,7 @@ mlxsw_sp_nexthop_obj_init(struct mlxsw_sp *mlxsw_sp,
 		break;
 	}
 
-	mlxsw_sp_nexthop_counter_alloc(mlxsw_sp, nh);
+	mlxsw_sp_nexthop_counter_enable(mlxsw_sp, nh);
 	list_add_tail(&nh->router_list_node, &mlxsw_sp->router->nexthop_list);
 	nh->ifindex = dev->ifindex;

@@ -5030,7 +5030,7 @@ mlxsw_sp_nexthop_obj_init(struct mlxsw_sp *mlxsw_sp,
 
 err_type_init:
 	list_del(&nh->router_list_node);
-	mlxsw_sp_nexthop_counter_free(mlxsw_sp, nh);
+	mlxsw_sp_nexthop_counter_disable(mlxsw_sp, nh);
 	return err;
 }

@@ -5041,7 +5041,7 @@ static void mlxsw_sp_nexthop_obj_fini(struct mlxsw_sp *mlxsw_sp,
 		mlxsw_sp_nexthop_obj_blackhole_fini(mlxsw_sp, nh);
 	mlxsw_sp_nexthop_type_fini(mlxsw_sp, nh);
 	list_del(&nh->router_list_node);
-	mlxsw_sp_nexthop_counter_free(mlxsw_sp, nh);
+	mlxsw_sp_nexthop_counter_disable(mlxsw_sp, nh);
 	nh->should_offload = 0;
 }

@@ -6734,7 +6734,7 @@ static int mlxsw_sp_nexthop6_init(struct mlxsw_sp *mlxsw_sp,
 #if IS_ENABLED(CONFIG_IPV6)
 	nh->neigh_tbl = &nd_tbl;
 #endif
-	mlxsw_sp_nexthop_counter_alloc(mlxsw_sp, nh);
+	mlxsw_sp_nexthop_counter_enable(mlxsw_sp, nh);
 
 	list_add_tail(&nh->router_list_node, &mlxsw_sp->router->nexthop_list);

@@ -6750,7 +6750,7 @@ static int mlxsw_sp_nexthop6_init(struct mlxsw_sp *mlxsw_sp,
 
 err_nexthop_type_init:
 	list_del(&nh->router_list_node);
-	mlxsw_sp_nexthop_counter_free(mlxsw_sp, nh);
+	mlxsw_sp_nexthop_counter_disable(mlxsw_sp, nh);
 	return err;
 }

@@ -6759,7 +6759,7 @@ static void mlxsw_sp_nexthop6_fini(struct mlxsw_sp *mlxsw_sp,
 {
 	mlxsw_sp_nexthop_type_fini(mlxsw_sp, nh);
 	list_del(&nh->router_list_node);
-	mlxsw_sp_nexthop_counter_free(mlxsw_sp, nh);
+	mlxsw_sp_nexthop_counter_disable(mlxsw_sp, nh);
 }
 
 static bool mlxsw_sp_rt6_is_gateway(const struct mlxsw_sp *mlxsw_sp,

drivers/net/ethernet/mellanox/mlxsw/spectrum_router.h +4 -4

index ed3b628caafe..bc5894c405a6 100644
@@ -156,10 +156,10 @@ int mlxsw_sp_nexthop_counter_get(struct mlxsw_sp *mlxsw_sp,
 int mlxsw_sp_nexthop_eth_update(struct mlxsw_sp *mlxsw_sp, u32 adj_index,
 				struct mlxsw_sp_nexthop *nh, bool force,
 				char *ratr_pl);
-void mlxsw_sp_nexthop_counter_alloc(struct mlxsw_sp *mlxsw_sp,
-				    struct mlxsw_sp_nexthop *nh);
-void mlxsw_sp_nexthop_counter_free(struct mlxsw_sp *mlxsw_sp,
-				   struct mlxsw_sp_nexthop *nh);
+void mlxsw_sp_nexthop_counter_enable(struct mlxsw_sp *mlxsw_sp,
+				     struct mlxsw_sp_nexthop *nh);
+void mlxsw_sp_nexthop_counter_disable(struct mlxsw_sp *mlxsw_sp,
+				      struct mlxsw_sp_nexthop *nh);
 
 static inline bool mlxsw_sp_l3addr_eq(const union mlxsw_sp_l3addr *addr1,
 				      const union mlxsw_sp_l3addr *addr2)

Signature · 1 line

2.43.0

Petr Machata

author maintainer

2024-03-08 13:03 UTC #4 lore raw

PATCH 4/11 mlxsw: spectrum_router: Have mlxsw_sp_nexthop_counter_enable() return int

In reply to Petr Machata

In order to be able to diagnose failures in counter allocation, have the function mlxsw_sp_nexthop_counter_enable() return an error code.

Signed-off-by Petr Machata <[email protected]>

Reviewed-by Ido Schimmel <[email protected]>

---
 .../ethernet/mellanox/mlxsw/spectrum_dpipe.c  | 20 +++++++++++++---
 .../ethernet/mellanox/mlxsw/spectrum_router.c | 24 +++++++++++++------
 .../ethernet/mellanox/mlxsw/spectrum_router.h |  4 ++--
 3 files changed, 36 insertions(+), 12 deletions(-)

drivers/net/ethernet/mellanox/mlxsw/spectrum_dpipe.c +17 -3

index 22e3dcb1d67a..ca80af06465f 100644
@@ -1181,9 +1181,11 @@ static int mlxsw_sp_dpipe_table_adj_counters_update(void *priv, bool enable)
 	char ratr_pl[MLXSW_REG_RATR_LEN];
 	struct mlxsw_sp *mlxsw_sp = priv;
 	struct mlxsw_sp_nexthop *nh;
+	unsigned int n_done = 0;
 	u32 adj_hash_index = 0;
 	u32 adj_index = 0;
 	u32 adj_size = 0;
+	int err;
 
 	mlxsw_sp_nexthop_for_each(nh, mlxsw_sp->router) {
 		if (!mlxsw_sp_nexthop_is_forward(nh) ||

@@ -1192,15 +1194,27 @@ static int mlxsw_sp_dpipe_table_adj_counters_update(void *priv, bool enable)
 
 		mlxsw_sp_nexthop_indexes(nh, &adj_index, &adj_size,
 					 &adj_hash_index);
-		if (enable)
-			mlxsw_sp_nexthop_counter_enable(mlxsw_sp, nh);
-		else
+		if (enable) {
+			err = mlxsw_sp_nexthop_counter_enable(mlxsw_sp, nh);
+			if (err)
+				goto err_counter_enable;
+		} else {
 			mlxsw_sp_nexthop_counter_disable(mlxsw_sp, nh);
+		}
 		mlxsw_sp_nexthop_eth_update(mlxsw_sp,
 					    adj_index + adj_hash_index, nh,
 					    true, ratr_pl);
+		n_done++;
 	}
 	return 0;
+
+err_counter_enable:
+	mlxsw_sp_nexthop_for_each(nh, mlxsw_sp->router) {
+		if (!n_done--)
+			break;
+		mlxsw_sp_nexthop_counter_disable(mlxsw_sp, nh);
+	}
+	return err;
 }
 
 static u64

drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c +17 -7

index 9bb58fb0d1da..23b54a4040af 100644
@@ -3151,20 +3151,23 @@ struct mlxsw_sp_nexthop_group {
 	bool can_destroy;
 };
 
-void mlxsw_sp_nexthop_counter_enable(struct mlxsw_sp *mlxsw_sp,
-				     struct mlxsw_sp_nexthop *nh)
+int mlxsw_sp_nexthop_counter_enable(struct mlxsw_sp *mlxsw_sp,
+				    struct mlxsw_sp_nexthop *nh)
 {
 	struct devlink *devlink;
+	int err;
 
 	devlink = priv_to_devlink(mlxsw_sp->core);
 	if (!devlink_dpipe_table_counter_enabled(devlink,
 						 MLXSW_SP_DPIPE_TABLE_NAME_ADJ))
-		return;
+		return 0;
 
-	if (mlxsw_sp_flow_counter_alloc(mlxsw_sp, &nh->counter_index))
-		return;
+	err = mlxsw_sp_flow_counter_alloc(mlxsw_sp, &nh->counter_index);
+	if (err)
+		return err;
 
 	nh->counter_valid = true;
+	return 0;
 }
 
 void mlxsw_sp_nexthop_counter_disable(struct mlxsw_sp *mlxsw_sp,

@@ -4507,7 +4510,10 @@ static int mlxsw_sp_nexthop4_init(struct mlxsw_sp *mlxsw_sp,
 	if (err)
 		return err;
 
-	mlxsw_sp_nexthop_counter_enable(mlxsw_sp, nh);
+	err = mlxsw_sp_nexthop_counter_enable(mlxsw_sp, nh);
+	if (err)
+		goto err_counter_enable;
+
 	list_add_tail(&nh->router_list_node, &mlxsw_sp->router->nexthop_list);
 
 	if (!dev)

@@ -4532,6 +4538,7 @@ static int mlxsw_sp_nexthop4_init(struct mlxsw_sp *mlxsw_sp,
 err_nexthop_neigh_init:
 	list_del(&nh->router_list_node);
 	mlxsw_sp_nexthop_counter_disable(mlxsw_sp, nh);
+err_counter_enable:
 	mlxsw_sp_nexthop_remove(mlxsw_sp, nh);
 	return err;
 }

@@ -6734,7 +6741,10 @@ static int mlxsw_sp_nexthop6_init(struct mlxsw_sp *mlxsw_sp,
 #if IS_ENABLED(CONFIG_IPV6)
 	nh->neigh_tbl = &nd_tbl;
 #endif
-	mlxsw_sp_nexthop_counter_enable(mlxsw_sp, nh);
+
+	err = mlxsw_sp_nexthop_counter_enable(mlxsw_sp, nh);
+	if (err)
+		return err;
 
 	list_add_tail(&nh->router_list_node, &mlxsw_sp->router->nexthop_list);

drivers/net/ethernet/mellanox/mlxsw/spectrum_router.h +2 -2

index bc5894c405a6..0432c7cc6b07 100644
@@ -156,8 +156,8 @@ int mlxsw_sp_nexthop_counter_get(struct mlxsw_sp *mlxsw_sp,
 int mlxsw_sp_nexthop_eth_update(struct mlxsw_sp *mlxsw_sp, u32 adj_index,
 				struct mlxsw_sp_nexthop *nh, bool force,
 				char *ratr_pl);
-void mlxsw_sp_nexthop_counter_enable(struct mlxsw_sp *mlxsw_sp,
-				     struct mlxsw_sp_nexthop *nh);
+int mlxsw_sp_nexthop_counter_enable(struct mlxsw_sp *mlxsw_sp,
+				    struct mlxsw_sp_nexthop *nh);
 void mlxsw_sp_nexthop_counter_disable(struct mlxsw_sp *mlxsw_sp,
 				      struct mlxsw_sp_nexthop *nh);

Signature · 1 line

2.43.0

Petr Machata

author maintainer

2024-03-08 13:03 UTC #5 lore raw

PATCH 5/11 mlxsw: spectrum: Allow fetch-and-clear of flow counters

In reply to Petr Machata

For the report_delta-like interface like a previous patch has added for collection of NH group statistics, it's easiest to read the counter and have the HW clear it right away. Thus, change mlxsw_sp_flow_counter_get() to take a bool indicating whether this should be done.

Signed-off-by Petr Machata <[email protected]>

Reviewed-by Ido Schimmel <[email protected]>

---
 drivers/net/ethernet/mellanox/mlxsw/spectrum.c         | 8 +++++---
 drivers/net/ethernet/mellanox/mlxsw/spectrum.h         | 4 ++--
 drivers/net/ethernet/mellanox/mlxsw/spectrum_acl.c     | 2 +-
 drivers/net/ethernet/mellanox/mlxsw/spectrum_mr_tcam.c | 2 +-
 drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c  | 4 ++--
 5 files changed, 11 insertions(+), 9 deletions(-)

drivers/net/ethernet/mellanox/mlxsw/spectrum.c +5 -3

index ecde2086c703..bb642e9bb6cf 100644
@@ -176,13 +176,15 @@ MLXSW_ITEM32(tx, hdr, fid, 0x08, 16, 16);
 MLXSW_ITEM32(tx, hdr, type, 0x0C, 0, 4);
 
 int mlxsw_sp_flow_counter_get(struct mlxsw_sp *mlxsw_sp,
-			      unsigned int counter_index, u64 *packets,
-			      u64 *bytes)
+			      unsigned int counter_index, bool clear,
+			      u64 *packets, u64 *bytes)
 {
+	enum mlxsw_reg_mgpc_opcode op = clear ? MLXSW_REG_MGPC_OPCODE_CLEAR :
+						MLXSW_REG_MGPC_OPCODE_NOP;
 	char mgpc_pl[MLXSW_REG_MGPC_LEN];
 	int err;
 
-	mlxsw_reg_mgpc_pack(mgpc_pl, counter_index, MLXSW_REG_MGPC_OPCODE_NOP,
+	mlxsw_reg_mgpc_pack(mgpc_pl, counter_index, op,
 			    MLXSW_REG_FLOW_COUNTER_SET_TYPE_PACKETS_BYTES);
 	err = mlxsw_reg_query(mlxsw_sp->core, MLXSW_REG(mgpc), mgpc_pl);
 	if (err)

drivers/net/ethernet/mellanox/mlxsw/spectrum.h +2 -2

index 898d24232935..3beb5d0847ab 100644
@@ -706,8 +706,8 @@ int mlxsw_sp_port_kill_vid(struct net_device *dev,
 int mlxsw_sp_port_vlan_set(struct mlxsw_sp_port *mlxsw_sp_port, u16 vid_begin,
 			   u16 vid_end, bool is_member, bool untagged);
 int mlxsw_sp_flow_counter_get(struct mlxsw_sp *mlxsw_sp,
-			      unsigned int counter_index, u64 *packets,
-			      u64 *bytes);
+			      unsigned int counter_index, bool clear,
+			      u64 *packets, u64 *bytes);
 int mlxsw_sp_flow_counter_alloc(struct mlxsw_sp *mlxsw_sp,
 				unsigned int *p_counter_index);
 void mlxsw_sp_flow_counter_free(struct mlxsw_sp *mlxsw_sp,

drivers/net/ethernet/mellanox/mlxsw/spectrum_acl.c +1 -1

index b01b000bc71c..3e70cee4d2f3 100644
@@ -1024,7 +1024,7 @@ int mlxsw_sp_acl_rule_get_stats(struct mlxsw_sp *mlxsw_sp,
 	rulei = mlxsw_sp_acl_rule_rulei(rule);
 	if (rulei->counter_valid) {
 		err = mlxsw_sp_flow_counter_get(mlxsw_sp, rulei->counter_index,
-						&current_packets,
+						false, &current_packets,
 						&current_bytes);
 		if (err)
 			return err;

drivers/net/ethernet/mellanox/mlxsw/spectrum_mr_tcam.c +1 -1

index 221aa6a474eb..01d81ae3662a 100644
@@ -361,7 +361,7 @@ static int mlxsw_sp_mr_tcam_route_stats(struct mlxsw_sp *mlxsw_sp,
 	struct mlxsw_sp_mr_tcam_route *route = route_priv;
 
 	return mlxsw_sp_flow_counter_get(mlxsw_sp, route->counter_index,
-					 packets, bytes);
+					 false, packets, bytes);
 }
 
 static int

drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c +2 -2

index 23b54a4040af..2df95b5a444f 100644
@@ -2251,7 +2251,7 @@ int mlxsw_sp_neigh_counter_get(struct mlxsw_sp *mlxsw_sp,
 		return -EINVAL;
 
 	return mlxsw_sp_flow_counter_get(mlxsw_sp, neigh_entry->counter_index,
-					 p_counter, NULL);
+					 false, p_counter, NULL);
 }
 
 static struct mlxsw_sp_neigh_entry *

@@ -3186,7 +3186,7 @@ int mlxsw_sp_nexthop_counter_get(struct mlxsw_sp *mlxsw_sp,
 		return -EINVAL;
 
 	return mlxsw_sp_flow_counter_get(mlxsw_sp, nh->counter_index,
-					 p_counter, NULL);
+					 false, p_counter, NULL);
 }
 
 struct mlxsw_sp_nexthop *mlxsw_sp_nexthop_next(struct mlxsw_sp_router *router,

Signature · 1 line

2.43.0

Petr Machata

author maintainer

2024-03-08 13:03 UTC #6 lore raw

PATCH 6/11 mlxsw: spectrum_router: Avoid allocating NH counters twice

In reply to Petr Machata

mlxsw_sp_nexthop_counter_disable() decays to a nop when called on a disabled counter, but mlxsw_sp_nexthop_counter_enable() can't similarly be called on an enabled counter. This would be useful in the following patches. Add the missing condition.

Signed-off-by Petr Machata <[email protected]>

Reviewed-by Ido Schimmel <[email protected]>

---
 drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c +3 -0

index 2df95b5a444f..a724484614ab 100644
@@ -3157,6 +3157,9 @@ int mlxsw_sp_nexthop_counter_enable(struct mlxsw_sp *mlxsw_sp,
 	struct devlink *devlink;
 	int err;
 
+	if (nh->counter_valid)
+		return 0;
+
 	devlink = priv_to_devlink(mlxsw_sp->core);
 	if (!devlink_dpipe_table_counter_enabled(devlink,
 						 MLXSW_SP_DPIPE_TABLE_NAME_ADJ))

Signature · 1 line

2.43.0

Petr Machata

author maintainer

2024-03-08 13:03 UTC #7 lore raw

PATCH 7/11 mlxsw: spectrum_router: Add helpers for nexthop counters

In reply to Petr Machata

The next patch will add the ability to share nexthop counters among mlxsw nexthops backed by the same core nexthop. To have a place to store reference count, the counter should be kept in a dedicated structure. In this patch, introduce the structure together with the related helpers, sans the refcount, which comes in the next patch.

Signed-off-by Petr Machata <[email protected]>

Reviewed-by Ido Schimmel <[email protected]>

---
 .../ethernet/mellanox/mlxsw/spectrum_router.c | 65 ++++++++++++++-----
 1 file changed, 50 insertions(+), 15 deletions(-)

drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c +50 ~~-15~~

index a724484614ab..73a16c328252 100644
@@ -3049,6 +3049,8 @@ struct mlxsw_sp_nexthop_key {
 	struct fib_nh *fib_nh;
 };
 
+struct mlxsw_sp_nexthop_counter;
+
 struct mlxsw_sp_nexthop {
 	struct list_head neigh_list_node; /* member of neigh entry list */
 	struct list_head crif_list_node;

@@ -3080,8 +3082,7 @@ struct mlxsw_sp_nexthop {
 		struct mlxsw_sp_neigh_entry *neigh_entry;
 		struct mlxsw_sp_ipip_entry *ipip_entry;
 	};
-	unsigned int counter_index;
-	bool counter_valid;
+	struct mlxsw_sp_nexthop_counter *counter;
 };
 
 static struct net_device *

@@ -3151,13 +3152,46 @@ struct mlxsw_sp_nexthop_group {
 	bool can_destroy;
 };
 
+struct mlxsw_sp_nexthop_counter {
+	unsigned int counter_index;
+};
+
+static struct mlxsw_sp_nexthop_counter *
+mlxsw_sp_nexthop_counter_alloc(struct mlxsw_sp *mlxsw_sp)
+{
+	struct mlxsw_sp_nexthop_counter *nhct;
+	int err;
+
+	nhct = kzalloc(sizeof(*nhct), GFP_KERNEL);
+	if (!nhct)
+		return ERR_PTR(-ENOMEM);
+
+	err = mlxsw_sp_flow_counter_alloc(mlxsw_sp, &nhct->counter_index);
+	if (err)
+		goto err_counter_alloc;
+
+	return nhct;
+
+err_counter_alloc:
+	kfree(nhct);
+	return ERR_PTR(err);
+}
+
+static void
+mlxsw_sp_nexthop_counter_free(struct mlxsw_sp *mlxsw_sp,
+			      struct mlxsw_sp_nexthop_counter *nhct)
+{
+	mlxsw_sp_flow_counter_free(mlxsw_sp, nhct->counter_index);
+	kfree(nhct);
+}
+
 int mlxsw_sp_nexthop_counter_enable(struct mlxsw_sp *mlxsw_sp,
 				    struct mlxsw_sp_nexthop *nh)
 {
+	struct mlxsw_sp_nexthop_counter *nhct;
 	struct devlink *devlink;
-	int err;
 
-	if (nh->counter_valid)
+	if (nh->counter)
 		return 0;
 
 	devlink = priv_to_devlink(mlxsw_sp->core);

@@ -3165,30 +3199,30 @@ int mlxsw_sp_nexthop_counter_enable(struct mlxsw_sp *mlxsw_sp,
 						 MLXSW_SP_DPIPE_TABLE_NAME_ADJ))
 		return 0;
 
-	err = mlxsw_sp_flow_counter_alloc(mlxsw_sp, &nh->counter_index);
-	if (err)
-		return err;
+	nhct = mlxsw_sp_nexthop_counter_alloc(mlxsw_sp);
+	if (IS_ERR(nhct))
+		return PTR_ERR(nhct);
 
-	nh->counter_valid = true;
+	nh->counter = nhct;
 	return 0;
 }
 
 void mlxsw_sp_nexthop_counter_disable(struct mlxsw_sp *mlxsw_sp,
 				      struct mlxsw_sp_nexthop *nh)
 {
-	if (!nh->counter_valid)
+	if (!nh->counter)
 		return;
-	mlxsw_sp_flow_counter_free(mlxsw_sp, nh->counter_index);
-	nh->counter_valid = false;
+	mlxsw_sp_nexthop_counter_free(mlxsw_sp, nh->counter);
+	nh->counter = NULL;
 }
 
 int mlxsw_sp_nexthop_counter_get(struct mlxsw_sp *mlxsw_sp,
 				 struct mlxsw_sp_nexthop *nh, u64 *p_counter)
 {
-	if (!nh->counter_valid)
+	if (!nh->counter)
 		return -EINVAL;
 
-	return mlxsw_sp_flow_counter_get(mlxsw_sp, nh->counter_index,
+	return mlxsw_sp_flow_counter_get(mlxsw_sp, nh->counter->counter_index,
 					 false, p_counter, NULL);
 }

@@ -3662,8 +3696,9 @@ static int __mlxsw_sp_nexthop_eth_update(struct mlxsw_sp *mlxsw_sp,
 		WARN_ON_ONCE(1);
 		return -EINVAL;
 	}
-	if (nh->counter_valid)
-		mlxsw_reg_ratr_counter_pack(ratr_pl, nh->counter_index, true);
+	if (nh->counter)
+		mlxsw_reg_ratr_counter_pack(ratr_pl, nh->counter->counter_index,
+					    true);
 	else
 		mlxsw_reg_ratr_counter_pack(ratr_pl, 0, false);

Signature · 1 line

2.43.0

Petr Machata

author maintainer

2024-03-08 13:03 UTC #8 lore raw

PATCH 8/11 mlxsw: spectrum_router: Track NH ID's of group members

In reply to Petr Machata

The core interfaces for collecting per-NH statistics are built around nexthops even for resilient groups. Because mlxsw models each bucket as a nexthop, the core next hop that a given bucket contributes to needs to be looked up. In order to be able to match the two up, we need to track nexthop ID for members of group nexthop objects. For simplicity, do it for all nexthop objects, not just group members.

Signed-off-by Petr Machata <[email protected]>

Reviewed-by Ido Schimmel <[email protected]>

---
 drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c +2 -0

index 73a16c328252..922dcd9c0b5a 100644
@@ -3083,6 +3083,7 @@ struct mlxsw_sp_nexthop {
 		struct mlxsw_sp_ipip_entry *ipip_entry;
 	};
 	struct mlxsw_sp_nexthop_counter *counter;
+	u32 id;		/* NH ID for members of a NH object group. */
 };
 
 static struct net_device *

@@ -5054,6 +5055,7 @@ mlxsw_sp_nexthop_obj_init(struct mlxsw_sp *mlxsw_sp,
 	mlxsw_sp_nexthop_counter_enable(mlxsw_sp, nh);
 	list_add_tail(&nh->router_list_node, &mlxsw_sp->router->nexthop_list);
 	nh->ifindex = dev->ifindex;
+	nh->id = nh_obj->id;
 
 	err = mlxsw_sp_nexthop_type_init(mlxsw_sp, nh, dev);
 	if (err)

Signature · 1 line

2.43.0

Petr Machata

author maintainer

2024-03-08 13:03 UTC #9 lore raw

PATCH 9/11 mlxsw: spectrum_router: Support nexthop group hardware statistics

In reply to Petr Machata

When hw_stats is set on a group, install nexthop counters on members of a group.

Counter allocation request is moved from nexthop object initialization to the update code. The previous placement made sense: when the counters are enabled by dpipe, the counters are installed to all existing nexthops and all nexthops created from then on get them. For the finer-grained nexthop group statistics, this is unsuitable. The existing placement was kept for the IPv4 and IPv6 nexthops.

Resilient group replacement emits a pre_replace notification, and then any bucket_replace notifications if there were any replacements at all. If the group is balanced and the nexthop composition of the replaced group didn't change, there will be no such notifiers. Therefore hook to the pre_replace notifier and mark all buckets for update, to un/install the counters.

When reporting deltas for resilient groups, use the nexthop ID that we stored in a previous patch to look up to which nexthop a bucket contributes.

Co-developed-by Ido Schimmel <[email protected]>

Signed-off-by Ido Schimmel <[email protected]>

Signed-off-by Petr Machata <[email protected]>

---
 .../ethernet/mellanox/mlxsw/spectrum_router.c | 148 +++++++++++++++++-
 1 file changed, 142 insertions(+), 6 deletions(-)

drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c +142 -6

index 922dcd9c0b5a..93ead5ccf864 100644
@@ -3108,7 +3108,8 @@ struct mlxsw_sp_nexthop_group_info {
 	int sum_norm_weight;
 	u8 adj_index_valid:1,
 	   gateway:1, /* routes using the group use a gateway */
-	   is_resilient:1;
+	   is_resilient:1,
+	   hw_stats:1;
 	struct list_head list; /* member in nh_res_grp_list */
 	struct mlxsw_sp_nexthop nexthops[] __counted_by(count);
 };

@@ -3189,15 +3190,17 @@ mlxsw_sp_nexthop_counter_free(struct mlxsw_sp *mlxsw_sp,
 int mlxsw_sp_nexthop_counter_enable(struct mlxsw_sp *mlxsw_sp,
 				    struct mlxsw_sp_nexthop *nh)
 {
+	const char *table_adj = MLXSW_SP_DPIPE_TABLE_NAME_ADJ;
 	struct mlxsw_sp_nexthop_counter *nhct;
 	struct devlink *devlink;
+	bool dpipe_stats;
 
 	if (nh->counter)
 		return 0;
 
 	devlink = priv_to_devlink(mlxsw_sp->core);
-	if (!devlink_dpipe_table_counter_enabled(devlink,
-						 MLXSW_SP_DPIPE_TABLE_NAME_ADJ))
+	dpipe_stats = devlink_dpipe_table_counter_enabled(devlink, table_adj);
+	if (!(nh->nhgi->hw_stats || dpipe_stats))
 		return 0;
 
 	nhct = mlxsw_sp_nexthop_counter_alloc(mlxsw_sp);

@@ -3217,6 +3220,15 @@ void mlxsw_sp_nexthop_counter_disable(struct mlxsw_sp *mlxsw_sp,
 	nh->counter = NULL;
 }
 
+static int mlxsw_sp_nexthop_counter_update(struct mlxsw_sp *mlxsw_sp,
+					   struct mlxsw_sp_nexthop *nh)
+{
+	if (nh->nhgi->hw_stats)
+		return mlxsw_sp_nexthop_counter_enable(mlxsw_sp, nh);
+	mlxsw_sp_nexthop_counter_disable(mlxsw_sp, nh);
+	return 0;
+}
+
 int mlxsw_sp_nexthop_counter_get(struct mlxsw_sp *mlxsw_sp,
 				 struct mlxsw_sp_nexthop *nh, u64 *p_counter)
 {

@@ -3224,7 +3236,7 @@ int mlxsw_sp_nexthop_counter_get(struct mlxsw_sp *mlxsw_sp,
 		return -EINVAL;
 
 	return mlxsw_sp_flow_counter_get(mlxsw_sp, nh->counter->counter_index,
-					 false, p_counter, NULL);
+					 true, p_counter, NULL);
 }
 
 struct mlxsw_sp_nexthop *mlxsw_sp_nexthop_next(struct mlxsw_sp_router *router,

@@ -3786,6 +3798,7 @@ mlxsw_sp_nexthop_group_update(struct mlxsw_sp *mlxsw_sp,
 		nh = &nhgi->nexthops[i];
 
 		if (!nh->should_offload) {
+			mlxsw_sp_nexthop_counter_disable(mlxsw_sp, nh);
 			nh->offloaded = 0;
 			continue;
 		}

@@ -3793,6 +3806,10 @@ mlxsw_sp_nexthop_group_update(struct mlxsw_sp *mlxsw_sp,
 		if (nh->update || reallocate) {
 			int err = 0;
 
+			err = mlxsw_sp_nexthop_counter_update(mlxsw_sp, nh);
+			if (err)
+				return err;
+
 			err = mlxsw_sp_nexthop_update(mlxsw_sp, adj_index, nh,
 						      true, ratr_pl);
 			if (err)

@@ -5052,7 +5069,6 @@ mlxsw_sp_nexthop_obj_init(struct mlxsw_sp *mlxsw_sp,
 		break;
 	}
 
-	mlxsw_sp_nexthop_counter_enable(mlxsw_sp, nh);
 	list_add_tail(&nh->router_list_node, &mlxsw_sp->router->nexthop_list);
 	nh->ifindex = dev->ifindex;
 	nh->id = nh_obj->id;

@@ -5077,7 +5093,6 @@ mlxsw_sp_nexthop_obj_init(struct mlxsw_sp *mlxsw_sp,
 
 err_type_init:
 	list_del(&nh->router_list_node);
-	mlxsw_sp_nexthop_counter_disable(mlxsw_sp, nh);
 	return err;
 }

@@ -5100,6 +5115,7 @@ mlxsw_sp_nexthop_obj_group_info_init(struct mlxsw_sp *mlxsw_sp,
 	struct mlxsw_sp_nexthop_group_info *nhgi;
 	struct mlxsw_sp_nexthop *nh;
 	bool is_resilient = false;
+	bool hw_stats = false;
 	unsigned int nhs;
 	int err, i;

@@ -5109,9 +5125,11 @@ mlxsw_sp_nexthop_obj_group_info_init(struct mlxsw_sp *mlxsw_sp,
 		break;
 	case NH_NOTIFIER_INFO_TYPE_GRP:
 		nhs = info->nh_grp->num_nh;
+		hw_stats = info->nh_grp->hw_stats;
 		break;
 	case NH_NOTIFIER_INFO_TYPE_RES_TABLE:
 		nhs = info->nh_res_table->num_nh_buckets;
+		hw_stats = info->nh_res_table->hw_stats;
 		is_resilient = true;
 		break;
 	default:

@@ -5126,6 +5144,7 @@ mlxsw_sp_nexthop_obj_group_info_init(struct mlxsw_sp *mlxsw_sp,
 	nhgi->gateway = mlxsw_sp_nexthop_obj_is_gateway(mlxsw_sp, info);
 	nhgi->is_resilient = is_resilient;
 	nhgi->count = nhs;
+	nhgi->hw_stats = hw_stats;
 	for (i = 0; i < nhgi->count; i++) {
 		struct nh_notifier_single_info *nh_obj;
 		int weight;

@@ -5347,6 +5366,43 @@ mlxsw_sp_nexthop_obj_group_replace(struct mlxsw_sp *mlxsw_sp,
 	return err;
 }
 
+static int mlxsw_sp_nexthop_obj_res_group_pre(struct mlxsw_sp *mlxsw_sp,
+					      struct nh_notifier_info *info)
+{
+	struct nh_notifier_grp_info *grp_info = info->nh_grp;
+	struct mlxsw_sp_nexthop_group_info *nhgi;
+	struct mlxsw_sp_nexthop_group *nh_grp;
+	int err;
+	int i;
+
+	nh_grp = mlxsw_sp_nexthop_obj_group_lookup(mlxsw_sp, info->id);
+	if (!nh_grp)
+		return 0;
+	nhgi = nh_grp->nhgi;
+
+	if (nhgi->hw_stats == grp_info->hw_stats)
+		return 0;
+
+	nhgi->hw_stats = grp_info->hw_stats;
+
+	for (i = 0; i < nhgi->count; i++) {
+		struct mlxsw_sp_nexthop *nh = &nhgi->nexthops[i];
+
+		if (nh->offloaded)
+			nh->update = 1;
+	}
+
+	err = mlxsw_sp_nexthop_group_refresh(mlxsw_sp, nh_grp);
+	if (err)
+		goto err_group_refresh;
+
+	return 0;
+
+err_group_refresh:
+	nhgi->hw_stats = !grp_info->hw_stats;
+	return err;
+}
+
 static int mlxsw_sp_nexthop_obj_new(struct mlxsw_sp *mlxsw_sp,
 				    struct nh_notifier_info *info)
 {

@@ -5523,6 +5579,79 @@ static int mlxsw_sp_nexthop_obj_bucket_replace(struct mlxsw_sp *mlxsw_sp,
 	return err;
 }
 
+static void
+mlxsw_sp_nexthop_obj_mp_hw_stats_get(struct mlxsw_sp *mlxsw_sp,
+				     struct mlxsw_sp_nexthop_group_info *nhgi,
+				     struct nh_notifier_grp_hw_stats_info *info)
+{
+	int nhi;
+
+	for (nhi = 0; nhi < info->num_nh; nhi++) {
+		struct mlxsw_sp_nexthop *nh = &nhgi->nexthops[nhi];
+		u64 packets;
+		int err;
+
+		err = mlxsw_sp_nexthop_counter_get(mlxsw_sp, nh, &packets);
+		if (err)
+			continue;
+
+		nh_grp_hw_stats_report_delta(info, nhi, packets);
+	}
+}
+
+static void
+mlxsw_sp_nexthop_obj_res_hw_stats_get(struct mlxsw_sp *mlxsw_sp,
+				      struct mlxsw_sp_nexthop_group_info *nhgi,
+				      struct nh_notifier_grp_hw_stats_info *info)
+{
+	int nhi = -1;
+	int bucket;
+
+	for (bucket = 0; bucket < nhgi->count; bucket++) {
+		struct mlxsw_sp_nexthop *nh = &nhgi->nexthops[bucket];
+		u64 packets;
+		int err;
+
+		if (nhi == -1 || info->stats[nhi].id != nh->id) {
+			for (nhi = 0; nhi < info->num_nh; nhi++)
+				if (info->stats[nhi].id == nh->id)
+					break;
+			if (WARN_ON_ONCE(nhi == info->num_nh)) {
+				nhi = -1;
+				continue;
+			}
+		}
+
+		err = mlxsw_sp_nexthop_counter_get(mlxsw_sp, nh, &packets);
+		if (err)
+			continue;
+
+		nh_grp_hw_stats_report_delta(info, nhi, packets);
+	}
+}
+
+static void mlxsw_sp_nexthop_obj_hw_stats_get(struct mlxsw_sp *mlxsw_sp,
+					      struct nh_notifier_info *info)
+{
+	struct mlxsw_sp_nexthop_group_info *nhgi;
+	struct mlxsw_sp_nexthop_group *nh_grp;
+
+	if (info->type != NH_NOTIFIER_INFO_TYPE_GRP_HW_STATS)
+		return;
+
+	nh_grp = mlxsw_sp_nexthop_obj_group_lookup(mlxsw_sp, info->id);
+	if (!nh_grp)
+		return;
+	nhgi = nh_grp->nhgi;
+
+	if (nhgi->is_resilient)
+		mlxsw_sp_nexthop_obj_res_hw_stats_get(mlxsw_sp, nhgi,
+						      info->nh_grp_hw_stats);
+	else
+		mlxsw_sp_nexthop_obj_mp_hw_stats_get(mlxsw_sp, nhgi,
+						     info->nh_grp_hw_stats);
+}
+
 static int mlxsw_sp_nexthop_obj_event(struct notifier_block *nb,
 				      unsigned long event, void *ptr)
 {

@@ -5538,6 +5667,10 @@ static int mlxsw_sp_nexthop_obj_event(struct notifier_block *nb,
 	mutex_lock(&router->lock);
 
 	switch (event) {
+	case NEXTHOP_EVENT_RES_TABLE_PRE_REPLACE:
+		err = mlxsw_sp_nexthop_obj_res_group_pre(router->mlxsw_sp,
+							 info);
+		break;
 	case NEXTHOP_EVENT_REPLACE:
 		err = mlxsw_sp_nexthop_obj_new(router->mlxsw_sp, info);
 		break;

@@ -5548,6 +5681,9 @@ static int mlxsw_sp_nexthop_obj_event(struct notifier_block *nb,
 		err = mlxsw_sp_nexthop_obj_bucket_replace(router->mlxsw_sp,
 							  info);
 		break;
+	case NEXTHOP_EVENT_HW_STATS_REPORT_DELTA:
+		mlxsw_sp_nexthop_obj_hw_stats_get(router->mlxsw_sp, info);
+		break;
 	default:
 		break;
 	}

Signature · 1 line

2.43.0

Petr Machata

author maintainer

2024-03-08 13:03 UTC #10 lore raw

PATCH 10/11 mlxsw: spectrum_router: Share nexthop counters in resilient groups

In reply to Petr Machata

For resilient groups, we can reuse the same counter for all the buckets that share the same nexthop. Keep a reference count per counter, and keep all these counters in a per-next hop group xarray, which serves as a NHID->counter cache. If a counter is already present for a given NHID, just take a reference and use the same counter.

Signed-off-by Petr Machata <[email protected]>

Reviewed-by Ido Schimmel <[email protected]>

---
 .../ethernet/mellanox/mlxsw/spectrum_router.c | 70 ++++++++++++++++++-
 1 file changed, 68 insertions(+), 2 deletions(-)

drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c +68 -2

index 93ead5ccf864..40ba314fbc72 100644
@@ -19,6 +19,7 @@
 #include <linux/net_namespace.h>
 #include <linux/mutex.h>
 #include <linux/genalloc.h>
+#include <linux/xarray.h>
 #include <net/netevent.h>
 #include <net/neighbour.h>
 #include <net/arp.h>

@@ -3111,6 +3112,7 @@ struct mlxsw_sp_nexthop_group_info {
 	   is_resilient:1,
 	   hw_stats:1;
 	struct list_head list; /* member in nh_res_grp_list */
+	struct xarray nexthop_counters;
 	struct mlxsw_sp_nexthop nexthops[] __counted_by(count);
 };

@@ -3156,6 +3158,7 @@ struct mlxsw_sp_nexthop_group {
 
 struct mlxsw_sp_nexthop_counter {
 	unsigned int counter_index;
+	refcount_t ref_count;
 };
 
 static struct mlxsw_sp_nexthop_counter *

@@ -3172,6 +3175,7 @@ mlxsw_sp_nexthop_counter_alloc(struct mlxsw_sp *mlxsw_sp)
 	if (err)
 		goto err_counter_alloc;
 
+	refcount_set(&nhct->ref_count, 1);
 	return nhct;
 
 err_counter_alloc:

@@ -3187,6 +3191,56 @@ mlxsw_sp_nexthop_counter_free(struct mlxsw_sp *mlxsw_sp,
 	kfree(nhct);
 }
 
+static struct mlxsw_sp_nexthop_counter *
+mlxsw_sp_nexthop_sh_counter_get(struct mlxsw_sp *mlxsw_sp,
+				struct mlxsw_sp_nexthop *nh)
+{
+	struct mlxsw_sp_nexthop_group *nh_grp = nh->nhgi->nh_grp;
+	struct mlxsw_sp_nexthop_counter *nhct;
+	void *ptr;
+	int err;
+
+	nhct = xa_load(&nh_grp->nhgi->nexthop_counters, nh->id);
+	if (nhct) {
+		refcount_inc(&nhct->ref_count);
+		return nhct;
+	}
+
+	nhct = mlxsw_sp_nexthop_counter_alloc(mlxsw_sp);
+	if (IS_ERR(nhct))
+		return nhct;
+
+	ptr = xa_store(&nh_grp->nhgi->nexthop_counters, nh->id, nhct,
+		       GFP_KERNEL);
+	if (IS_ERR(ptr)) {
+		err = PTR_ERR(ptr);
+		goto err_store;
+	}
+
+	return nhct;
+
+err_store:
+	mlxsw_sp_nexthop_counter_free(mlxsw_sp, nhct);
+	return ERR_PTR(err);
+}
+
+static void mlxsw_sp_nexthop_sh_counter_put(struct mlxsw_sp *mlxsw_sp,
+					    struct mlxsw_sp_nexthop *nh)
+{
+	struct mlxsw_sp_nexthop_group *nh_grp = nh->nhgi->nh_grp;
+	struct mlxsw_sp_nexthop_counter *nhct;
+
+	nhct = xa_load(&nh_grp->nhgi->nexthop_counters, nh->id);
+	if (WARN_ON(!nhct))
+		return;
+
+	if (!refcount_dec_and_test(&nhct->ref_count))
+		return;
+
+	xa_erase(&nh_grp->nhgi->nexthop_counters, nh->id);
+	mlxsw_sp_nexthop_counter_free(mlxsw_sp, nhct);
+}
+
 int mlxsw_sp_nexthop_counter_enable(struct mlxsw_sp *mlxsw_sp,
 				    struct mlxsw_sp_nexthop *nh)
 {

@@ -3203,7 +3257,10 @@ int mlxsw_sp_nexthop_counter_enable(struct mlxsw_sp *mlxsw_sp,
 	if (!(nh->nhgi->hw_stats || dpipe_stats))
 		return 0;
 
-	nhct = mlxsw_sp_nexthop_counter_alloc(mlxsw_sp);
+	if (nh->id)
+		nhct = mlxsw_sp_nexthop_sh_counter_get(mlxsw_sp, nh);
+	else
+		nhct = mlxsw_sp_nexthop_counter_alloc(mlxsw_sp);
 	if (IS_ERR(nhct))
 		return PTR_ERR(nhct);

@@ -3216,7 +3273,11 @@ void mlxsw_sp_nexthop_counter_disable(struct mlxsw_sp *mlxsw_sp,
 {
 	if (!nh->counter)
 		return;
-	mlxsw_sp_nexthop_counter_free(mlxsw_sp, nh->counter);
+
+	if (nh->id)
+		mlxsw_sp_nexthop_sh_counter_put(mlxsw_sp, nh);
+	else
+		mlxsw_sp_nexthop_counter_free(mlxsw_sp, nh->counter);
 	nh->counter = NULL;
 }

@@ -5145,6 +5206,9 @@ mlxsw_sp_nexthop_obj_group_info_init(struct mlxsw_sp *mlxsw_sp,
 	nhgi->is_resilient = is_resilient;
 	nhgi->count = nhs;
 	nhgi->hw_stats = hw_stats;
+
+	xa_init_flags(&nhgi->nexthop_counters, XA_FLAGS_ALLOC1);
+
 	for (i = 0; i < nhgi->count; i++) {
 		struct nh_notifier_single_info *nh_obj;
 		int weight;

@@ -5227,6 +5291,8 @@ mlxsw_sp_nexthop_obj_group_info_fini(struct mlxsw_sp *mlxsw_sp,
 	}
 	mlxsw_sp_nexthop_group_refresh(mlxsw_sp, nh_grp);
 	WARN_ON_ONCE(nhgi->adj_index_valid);
+	WARN_ON(!xa_empty(&nhgi->nexthop_counters));
+	xa_destroy(&nhgi->nexthop_counters);
 	kfree(nhgi);
 }

Signature · 1 line

2.43.0

Petr Machata

author maintainer

2024-03-08 13:03 UTC #11 lore raw

PATCH 11/11 selftests: forwarding: Add a test for NH group stats

In reply to Petr Machata

Add to lib.sh support for fetching NH stats, and a new library, router_mpath_nh_lib.sh, with the common code for testing NH stats. Use the latter from router_mpath_nh.sh and router_mpath_nh_res.sh.

The test works by sending traffic through a NH group, and checking that the reported values correspond to what the link that ultimately receives the traffic reports having seen.

Signed-off-by Petr Machata <[email protected]>

---
 .../testing/selftests/net/forwarding/Makefile |   1 +
 tools/testing/selftests/net/forwarding/lib.sh |  34 +++++
 .../net/forwarding/router_mpath_nh.sh         |  13 ++
 .../net/forwarding/router_mpath_nh_lib.sh     | 129 ++++++++++++++++++
 .../net/forwarding/router_mpath_nh_res.sh     |  13 ++
 5 files changed, 190 insertions(+)
 create mode 100644 tools/testing/selftests/net/forwarding/router_mpath_nh_lib.sh

tools/testing/selftests/net/forwarding/Makefile +1 -0

index cdefc9a5ec34..535865b3d1d6 100644
@@ -123,6 +123,7 @@ TEST_FILES := devlink_lib.sh \
 	mirror_gre_topo_lib.sh \
 	mirror_lib.sh \
 	mirror_topo_lib.sh \
+	router_mpath_nh_lib.sh \
 	sch_ets_core.sh \
 	sch_ets_tests.sh \
 	sch_tbf_core.sh \

tools/testing/selftests/net/forwarding/lib.sh +34 -0

index d1bf39eaf2b3..e579c2e0c462 100644
@@ -900,6 +900,33 @@ hw_stats_get()
 		jq ".[0].stats64.$dir.$stat"
 }
 
+__nh_stats_get()
+{
+	local key=$1; shift
+	local group_id=$1; shift
+	local member_id=$1; shift
+
+	ip -j -s -s nexthop show id $group_id |
+	    jq --argjson member_id "$member_id" --arg key "$key" \
+	       '.[].group_stats[] | select(.id == $member_id) | .[$key]'
+}
+
+nh_stats_get()
+{
+	local group_id=$1; shift
+	local member_id=$1; shift
+
+	__nh_stats_get packets "$group_id" "$member_id"
+}
+
+nh_stats_get_hw()
+{
+	local group_id=$1; shift
+	local member_id=$1; shift
+
+	__nh_stats_get packets_hw "$group_id" "$member_id"
+}
+
 humanize()
 {
 	local speed=$1; shift

@@ -2010,3 +2037,10 @@ bail_on_lldpad()
 		fi
 	fi
 }
+
+absval()
+{
+	local v=$1; shift
+
+	echo $((v > 0 ? v : -v))
+}

tools/testing/selftests/net/forwarding/router_mpath_nh.sh +13 -0

index 982e0d098ea9..3f0f5dc95542 100755
@@ -7,9 +7,12 @@ ALL_TESTS="
 	multipath_test
 	ping_ipv4_blackhole
 	ping_ipv6_blackhole
+	nh_stats_test_v4
+	nh_stats_test_v6
 "
 NUM_NETIFS=8
 source lib.sh
+source router_mpath_nh_lib.sh
 
 h1_create()
 {

@@ -325,6 +328,16 @@ ping_ipv6_blackhole()
 	ip -6 nexthop del id 1001
 }
 
+nh_stats_test_v4()
+{
+	__nh_stats_test_v4 mpath
+}
+
+nh_stats_test_v6()
+{
+	__nh_stats_test_v6 mpath
+}
+
 setup_prepare()
 {
 	h1=${NETIFS[p1]}

tools/testing/selftests/net/forwarding/router_mpath_nh_lib.sh new file +129 -0

129 lines not on this page · raw message

tools/testing/selftests/net/forwarding/router_mpath_nh_res.sh +13 -0

index a60ff54723b7..4b483d24ad00 100755
@@ -5,9 +5,12 @@ ALL_TESTS="
 	ping_ipv4
 	ping_ipv6
 	multipath_test
+	nh_stats_test_v4
+	nh_stats_test_v6
 "
 NUM_NETIFS=8
 source lib.sh
+source router_mpath_nh_lib.sh
 
 h1_create()
 {

@@ -333,6 +336,16 @@ multipath_test()
 	ip nexthop replace id 106 group 104,1/105,1 type resilient
 }
 
+nh_stats_test_v4()
+{
+	__nh_stats_test_v4 resilient
+}
+
+nh_stats_test_v6()
+{
+	__nh_stats_test_v6 resilient
+}
+
 setup_prepare()
 {
 	h1=${NETIFS[p1]}

Signature · 1 line

2.43.0

Jakub Kicinski

maintainer

2024-03-08 17:03 UTC #12 lore raw

Re: [PATCH net-next 11/11] selftests: forwarding: Add a test for NH group stats

Quoted 7 lines · On Fri, 8 Mar 2024 13:59:55 +0100 Petr Machata wrote:

Add to lib.sh support for fetching NH stats, and a new library, router_mpath_nh_lib.sh, with the common code for testing NH stats. Use the latter from router_mpath_nh.sh and router_mpath_nh_res.sh.
The test works by sending traffic through a NH group, and checking that the reported values correspond to what the link that ultimately receives the traffic reports having seen.

Are the iproute2 patches still on their way? I can't find them and the test is getting skipped.

Petr Machata

author maintainer

2024-03-08 22:33 UTC #13 lore raw

Re: [PATCH net-next 11/11] selftests: forwarding: Add a test for NH group stats

Quoted 11 lines · Jakub Kicinski

Quoted 7 lines · On Fri, 8 Mar 2024 13:59:55 +0100 Petr Machata wrote:
Add to lib.sh support for fetching NH stats, and a new library, router_mpath_nh_lib.sh, with the common code for testing NH stats. Use the latter from router_mpath_nh.sh and router_mpath_nh_res.sh.
The test works by sending traffic through a NH group, and checking that the reported values correspond to what the link that ultimately receives the traffic reports having seen.
Are the iproute2 patches still on their way? I can't find them and the test is getting skipped.

I only just sent it. The code is here FWIW:

https://github.com/pmachata/iproute2/commits/nh_stats

Jakub Kicinski

maintainer

2024-03-09 03:48 UTC #14 lore raw

Re: [PATCH net-next 11/11] selftests: forwarding: Add a test for NH group stats

Quoted 6 lines · On Fri, 8 Mar 2024 23:31:57 +0100 Petr Machata wrote:

Quoted 2 lines
Are the iproute2 patches still on their way? I can't find them and the test is getting skipped.
I only just sent it. The code is here FWIW:
https://github.com/pmachata/iproute2/commits/nh_stats

I tried but I'll need to also sync the kernel headers. Maybe I'll wait until David applies :S
Will the test still skip, tho, when running on veth?

David Ahern

maintainer

2024-03-09 04:47 UTC #15 lore raw

Re: [PATCH net-next 11/11] selftests: forwarding: Add a test for NH group stats

Quoted 11 lines · Jakub Kicinski

Quoted 6 lines · On Fri, 8 Mar 2024 23:31:57 +0100 Petr Machata wrote:
Quoted 2 lines
Are the iproute2 patches still on their way? I can't find them and the test is getting skipped.
I only just sent it. The code is here FWIW:
https://github.com/pmachata/iproute2/commits/nh_stats
I tried but I'll need to also sync the kernel headers. Maybe I'll wait until David applies :S
Will the test still skip, tho, when running on veth?

marked the set; will try to get to it tomorrow.

Petr Machata

author maintainer

2024-03-11 10:59 UTC #16 lore raw

Re: [PATCH net-next 11/11] selftests: forwarding: Add a test for NH group stats

In reply to Jakub Kicinski

Quoted 11 lines · Jakub Kicinski

Quoted 6 lines · On Fri, 8 Mar 2024 23:31:57 +0100 Petr Machata wrote:
Quoted 2 lines
Are the iproute2 patches still on their way? I can't find them and the test is getting skipped.
I only just sent it. The code is here FWIW:
https://github.com/pmachata/iproute2/commits/nh_stats
I tried but I'll need to also sync the kernel headers. Maybe I'll wait until David applies :S
Will the test still skip, tho, when running on veth?

It should run the SW parts and skip the HW ones.

Petr Machata

author maintainer

2024-03-11 11:02 UTC #17 lore raw

Re: [PATCH net-next 11/11] selftests: forwarding: Add a test for NH group stats

Quoted 8 lines · Petr Machata

Quoted 6 lines · Jakub Kicinski
Quoted 5 lines · On Fri, 8 Mar 2024 23:31:57 +0100 Petr Machata wrote:
Quoted 2 lines
Are the iproute2 patches still on their way? I can't find them and the test is getting skipped.
I only just sent it. The code is here FWIW:

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
https://github.com/pmachata/iproute2/commits/nh_stats
I tried but I'll need to also sync the kernel headers. Maybe I'll wait until David applies :S
Will the test still skip, tho, when running on veth?
It should run the SW parts and skip the HW ones.

In fact why don't I paste the run I still have in the terminal from last week:

[root@virtme-ng forwarding]# TESTS=" nh_stats_test_v4 nh_stats_test_v6 " ./router_mpath_nh_res.sh
TEST: NH stats test IPv4 [ OK ]
TEST: HW stats not offloaded on veth topology [SKIP]
TEST: NH stats test IPv6 [ OK ]
TEST: HW stats not offloaded on veth topology [SKIP]

Jakub Kicinski

maintainer

2024-03-11 15:59 UTC #18 lore raw

Re: [PATCH net-next 11/11] selftests: forwarding: Add a test for NH group stats

Quoted 10 lines · On Mon, 11 Mar 2024 12:00:59 +0100 Petr Machata wrote:

Quoted 1 line
It should run the SW parts and skip the HW ones.
In fact why don't I paste the run I still have in the terminal from last week:
[root@virtme-ng forwarding]# TESTS=" nh_stats_test_v4 nh_stats_test_v6 " ./router_mpath_nh_res.sh
TEST: NH stats test IPv4 [ OK ]
TEST: HW stats not offloaded on veth topology [SKIP]
TEST: NH stats test IPv6 [ OK ]
TEST: HW stats not offloaded on veth topology [SKIP]

SKIP beats PASS so the result for the entire test (according to our local result parser) will be SKIP. Can we switch to XFAIL?

Petr Machata

author maintainer

2024-03-11 17:13 UTC #19 lore raw

Re: [PATCH net-next 11/11] selftests: forwarding: Add a test for NH group stats

Quoted 6 lines · Jakub Kicinski

Quoted 5 lines · On Mon, 11 Mar 2024 12:00:59 +0100 Petr Machata wrote:
Quoted 1 line
It should run the SW parts and skip the HW ones.
In fact why don't I paste the run I still have in the terminal from last week:

Quoted 8 lines

Quoted 5 lines
[root@virtme-ng forwarding]# TESTS=" nh_stats_test_v4 nh_stats_test_v6 " ./router_mpath_nh_res.sh
TEST: NH stats test IPv4 [ OK ]
TEST: HW stats not offloaded on veth topology [SKIP]
TEST: NH stats test IPv6 [ OK ]
TEST: HW stats not offloaded on veth topology [SKIP]
SKIP beats PASS so the result for the entire test (according to our local result parser) will be SKIP. Can we switch to XFAIL?

Sure.

But I don't think SKIP should trump PASS. IMHO SKIP is the weakest status, soon as you have one PASS, that's what the overall outcome should be. I guess the logic behind it was that it's useful to see the tests that skip?

Jakub Kicinski

maintainer

2024-03-11 17:52 UTC #20 lore raw

Re: [PATCH net-next 11/11] selftests: forwarding: Add a test for NH group stats

Quoted 4 lines · On Mon, 11 Mar 2024 17:30:54 +0100 Petr Machata wrote:

But I don't think SKIP should trump PASS. IMHO SKIP is the weakest status, soon as you have one PASS, that's what the overall outcome should be. I guess the logic behind it was that it's useful to see the tests that skip?

SKIP is a signal that something could not be run. People maintaining CIs will try to investigate why. The signal for "everything is working as expected" is PASS, possibly XFAIL if you want to signal that the test didn't run/pass but that's expected.

[email protected]

CI Applied

2024-03-11 22:00 UTC #21 lore raw

In reply to Petr Machata

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

Hello:

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

Quoted 10 lines · On Fri, 8 Mar 2024 13:59:44 +0100 you wrote:

ECMP is a fundamental component in L3 designs. However, it's fragile. Many factors influence whether an ECMP group will operate as intended: hash policy (i.e. the set of fields that contribute to ECMP hash calculation), neighbor validity, hash seed (which might lead to polarization) or the type of ECMP group used (hash-threshold or resilient).
At the same time, collection of statistics that would help an operator determine that the group performs as desired, is difficult.
[...]

Here is the summary with links:
- [net-next,01/11] net: nexthop: Initialize NH group ID in resilient NH group notifiers

    https://git.kernel.org/netdev/net-next/c/2d32c49386cb
  - [net-next,02/11] net: nexthop: Have all NH notifiers carry NH ID
    https://git.kernel.org/netdev/net-next/c/e99eb57e9b14
  - [net-next,03/11] mlxsw: spectrum_router: Rename two functions
    https://git.kernel.org/netdev/net-next/c/64f962c65fe3
  - [net-next,04/11] mlxsw: spectrum_router: Have mlxsw_sp_nexthop_counter_enable() return int
    https://git.kernel.org/netdev/net-next/c/8acb480e43c8
  - [net-next,05/11] mlxsw: spectrum: Allow fetch-and-clear of flow counters
    https://git.kernel.org/netdev/net-next/c/6fb88aaf272a
  - [net-next,06/11] mlxsw: spectrum_router: Avoid allocating NH counters twice
    https://git.kernel.org/netdev/net-next/c/79fa52145e19
  - [net-next,07/11] mlxsw: spectrum_router: Add helpers for nexthop counters
    https://git.kernel.org/netdev/net-next/c/10bf92fd775e
  - [net-next,08/11] mlxsw: spectrum_router: Track NH ID's of group members
    https://git.kernel.org/netdev/net-next/c/41acb5549e60
  - [net-next,09/11] mlxsw: spectrum_router: Support nexthop group hardware statistics
    https://git.kernel.org/netdev/net-next/c/5a5a98e5176e
  - [net-next,10/11] mlxsw: spectrum_router: Share nexthop counters in resilient groups
    https://git.kernel.org/netdev/net-next/c/44c2fbebe18a
  - [net-next,11/11] selftests: forwarding: Add a test for NH group stats
    https://git.kernel.org/netdev/net-next/c/a22b042660ca

You are awesome, thank you!

Signature · 2 lines

Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html