[PATCH net-next 0/3] mlxsw: Offload root TBF as port shaper

From: Ido Schimmel <idosch@nvidia.com>

Petr says:

Egress configuration in an mlxsw deployment would generally have an ETS
qdisc at root, with a number of bands and a priority dispatch between them.
Some of those bands could then have a RED and/or TBF qdiscs attached.

When TBF is used like this, mlxsw configures shaper on a subgroup, which is
the pair of traffic classes (UC + BUM) corresponding to the band where TBF
is installed. This way it is possible to limit traffic on several bands
(subgroups) independently by configuring several TBF qdiscs, each on a
different band.

It is however not possible to limit traffic flowing through the port as
such. The ASIC supports this through port shapers (as opposed to the
abovementioned subgroup shapers). An obvious way to express this as a user
would be to configure a root TBF qdisc, and then add the whole ETS
hierarchy as its child.

TBF (and RED) can currently be used as a root qdisc. This usage has always
been accepted as a special case, when only one subgroup is configured, and
that is the subgroup that root TBF and RED configure. However it was never
possible to install ETS under that TBF.

In this patchset, this limitation is relaxed. TBF qdisc in root position is
now always offloaded as a port shaper. Such TBF qdisc does not limit
offload of further children. It is thus possible to configure the usual
priority classification through ETS, with RED and/or TBF on individual
bands, all that below a port-level TBF. For example:

    (1) # tc qdisc replace dev swp1 root handle 1: tbf rate 800mbit burst 16kb limit 1M
    (2) # tc qdisc replace dev swp1 parent 1:1 handle 11: ets strict 8 priomap 7 6 5 4 3 2 1 0
    (3) # tc qdisc replace dev swp1 parent 11:1 handle 111: tbf rate 600mbit burst 16kb limit 1M
    (4) # tc qdisc replace dev swp1 parent 11:2 handle 112: tbf rate 600mbit burst 16kb limit 1M

Here, (1) configures a 800-Mbps port shaper, (2) adds an ETS element with 8
strictly-prioritized bands, and (3) and (4) configure two more shapers,
each 600 Mbps, one under 11:1 (band 0, TCs 7 and 15), one under 11:2 (band
1, TCs 6 and 14). This way, traffic on bands 0 and 1 are each independently
capped at 600 Mbps, and at the same time, traffic through the port as a
whole is capped at 800 Mbps.

In patch #1, TBF is permitted as root qdisc, under which the usual qdisc
tree can be installed.

In patch #2, the qdisc offloadability selftest is extended to cover the
root TBF as well.

Patch #3 then tests that the offloaded TBF shapes as expected.

Petr Machata (3):
  mlxsw: spectrum_qdisc: Offload root TBF as port shaper
  selftests: mlxsw: Test offloadability of root TBF
  selftests: mlxsw: Test port shaper

 .../ethernet/mellanox/mlxsw/spectrum_qdisc.c  | 55 +++++++++++++------
 .../drivers/net/mlxsw/sch_offload.sh          | 14 +++++
 .../net/forwarding/sch_tbf_etsprio.sh         | 28 ++++++++++
 3 files changed, 79 insertions(+), 18 deletions(-)

-- 
2.31.1

From: Petr Machata <petrm@nvidia.com>

The Spectrum ASIC allows configuration of maximum shaper on all levels of
the scheduling hierarchy: TCs, subgroups, groups and also ports. Currently,
TBF always configures a subgroup. But a user could reasonably express the
intent to configure port shaper by putting TBF to a root position, around
ETS / PRIO. Accept this usage and offload appropriately.

Signed-off-by: Petr Machata <petrm@nvidia.com>
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 .../ethernet/mellanox/mlxsw/spectrum_qdisc.c  | 55 +++++++++++++------
 1 file changed, 37 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_qdisc.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_qdisc.c
index ddb5ad88b350..4243d3b883ff 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_qdisc.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_qdisc.c

@@ -271,6 +271,7 @@ mlxsw_sp_qdisc_destroy(struct mlxsw_sp_port *mlxsw_sp_port,
 
 struct mlxsw_sp_qdisc_tree_validate {
 	bool forbid_ets;
+	bool forbid_root_tbf;
 	bool forbid_tbf;
 	bool forbid_red;
 };

@@ -310,18 +311,26 @@ __mlxsw_sp_qdisc_tree_validate(struct mlxsw_sp_qdisc *mlxsw_sp_qdisc,
 		if (validate.forbid_red)
 			return -EINVAL;
 		validate.forbid_red = true;
+		validate.forbid_root_tbf = true;
 		validate.forbid_ets = true;
 		break;
 	case MLXSW_SP_QDISC_TBF:
-		if (validate.forbid_tbf)
-			return -EINVAL;
-		validate.forbid_tbf = true;
-		validate.forbid_ets = true;
+		if (validate.forbid_root_tbf) {
+			if (validate.forbid_tbf)
+				return -EINVAL;
+			/* This is a TC TBF. */
+			validate.forbid_tbf = true;
+			validate.forbid_ets = true;
+		} else {
+			/* This is root TBF. */
+			validate.forbid_root_tbf = true;
+		}
 		break;
 	case MLXSW_SP_QDISC_PRIO:
 	case MLXSW_SP_QDISC_ETS:
 		if (validate.forbid_ets)
 			return -EINVAL;
+		validate.forbid_root_tbf = true;
 		validate.forbid_ets = true;
 		break;
 	default:

@@ -905,16 +914,34 @@ mlxsw_sp_setup_tc_qdisc_leaf_clean_stats(struct mlxsw_sp_port *mlxsw_sp_port,
 	mlxsw_sp_qdisc->stats_base.backlog = 0;
 }
 
+static enum mlxsw_reg_qeec_hr
+mlxsw_sp_qdisc_tbf_hr(struct mlxsw_sp_port *mlxsw_sp_port,
+		      struct mlxsw_sp_qdisc *mlxsw_sp_qdisc)
+{
+	if (mlxsw_sp_qdisc == &mlxsw_sp_port->qdisc->root_qdisc)
+		return MLXSW_REG_QEEC_HR_PORT;
+
+	/* Configure subgroup shaper, so that both UC and MC traffic is subject
+	 * to shaping. That is unlike RED, however UC queue lengths are going to
+	 * be different than MC ones due to different pool and quota
+	 * configurations, so the configuration is not applicable. For shaper on
+	 * the other hand, subjecting the overall stream to the configured
+	 * shaper makes sense. Also note that that is what we do for
+	 * ieee_setmaxrate().
+	 */
+	return MLXSW_REG_QEEC_HR_SUBGROUP;
+}
+
 static int
 mlxsw_sp_qdisc_tbf_destroy(struct mlxsw_sp_port *mlxsw_sp_port,
 			   struct mlxsw_sp_qdisc *mlxsw_sp_qdisc)
 {
+	enum mlxsw_reg_qeec_hr hr = mlxsw_sp_qdisc_tbf_hr(mlxsw_sp_port,
+							  mlxsw_sp_qdisc);
 	int tclass_num = mlxsw_sp_qdisc_get_tclass_num(mlxsw_sp_port,
 						       mlxsw_sp_qdisc);
 
-	return mlxsw_sp_port_ets_maxrate_set(mlxsw_sp_port,
-					     MLXSW_REG_QEEC_HR_SUBGROUP,
-					     tclass_num, 0,
+	return mlxsw_sp_port_ets_maxrate_set(mlxsw_sp_port, hr, tclass_num, 0,
 					     MLXSW_REG_QEEC_MAS_DIS, 0);
 }
 

@@ -996,6 +1023,8 @@ mlxsw_sp_qdisc_tbf_replace(struct mlxsw_sp_port *mlxsw_sp_port, u32 handle,
 			   struct mlxsw_sp_qdisc *mlxsw_sp_qdisc,
 			   void *params)
 {
+	enum mlxsw_reg_qeec_hr hr = mlxsw_sp_qdisc_tbf_hr(mlxsw_sp_port,
+							  mlxsw_sp_qdisc);
 	struct tc_tbf_qopt_offload_replace_params *p = params;
 	u64 rate_kbps = mlxsw_sp_qdisc_tbf_rate_kbps(p);
 	int tclass_num;

@@ -1016,17 +1045,7 @@ mlxsw_sp_qdisc_tbf_replace(struct mlxsw_sp_port *mlxsw_sp_port, u32 handle,
 		/* check_params above was supposed to reject this value. */
 		return -EINVAL;
 
-	/* Configure subgroup shaper, so that both UC and MC traffic is subject
-	 * to shaping. That is unlike RED, however UC queue lengths are going to
-	 * be different than MC ones due to different pool and quota
-	 * configurations, so the configuration is not applicable. For shaper on
-	 * the other hand, subjecting the overall stream to the configured
-	 * shaper makes sense. Also note that that is what we do for
-	 * ieee_setmaxrate().
-	 */
-	return mlxsw_sp_port_ets_maxrate_set(mlxsw_sp_port,
-					     MLXSW_REG_QEEC_HR_SUBGROUP,
-					     tclass_num, 0,
+	return mlxsw_sp_port_ets_maxrate_set(mlxsw_sp_port, hr, tclass_num, 0,
 					     rate_kbps, burst_size);
 }
 

-- 
2.31.1

From: Petr Machata <petrm@nvidia.com>

TBF can be used as a root qdisc, with the usual ETS/RED/TBF hierarchy below
it. This use should now be offloaded. Add a test that verifies that it is.

Signed-off-by: Petr Machata <petrm@nvidia.com>
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 .../selftests/drivers/net/mlxsw/sch_offload.sh     | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/tools/testing/selftests/drivers/net/mlxsw/sch_offload.sh b/tools/testing/selftests/drivers/net/mlxsw/sch_offload.sh
index ade79ef08de3..071a33d10c20 100755
--- a/tools/testing/selftests/drivers/net/mlxsw/sch_offload.sh
+++ b/tools/testing/selftests/drivers/net/mlxsw/sch_offload.sh

@@ -6,7 +6,9 @@
 
 ALL_TESTS="
 	test_root
+	test_port_tbf
 	test_etsprio
+	test_etsprio_port_tbf
 "
 NUM_NETIFS=1
 lib_dir=$(dirname $0)/../../../net/forwarding

@@ -221,6 +223,12 @@ test_root()
 	do_test_combinations 1 0
 }
 
+test_port_tbf()
+{
+	with_tbf 1: root \
+		do_test_combinations 8 1
+}
+
 do_test_etsprio()
 {
 	local parent=$1; shift

@@ -264,6 +272,12 @@ test_etsprio()
 	do_test_etsprio root ""
 }
 
+test_etsprio_port_tbf()
+{
+	with_tbf 1: root \
+		do_test_etsprio "parent 1:1" "-TBF"
+}
+
 cleanup()
 {
 	tc qdisc del dev $h1 root &>/dev/null

-- 
2.31.1

From: Petr Machata <petrm@nvidia.com>

TBF can be used as a root qdisc, in which case it is supposed to configure
port shaper. Add a test that verifies that this is so by installing a root
TBF with a ETS or PRIO below it, and then expecting individual bands to all
be shaped according to the root TBF configuration.

Signed-off-by: Petr Machata <petrm@nvidia.com>
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 .../net/forwarding/sch_tbf_etsprio.sh         | 28 +++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/tools/testing/selftests/net/forwarding/sch_tbf_etsprio.sh b/tools/testing/selftests/net/forwarding/sch_tbf_etsprio.sh
index 8bd85da1905a..75a37c189ef3 100644
--- a/tools/testing/selftests/net/forwarding/sch_tbf_etsprio.sh
+++ b/tools/testing/selftests/net/forwarding/sch_tbf_etsprio.sh

@@ -4,9 +4,12 @@
 ALL_TESTS="
 	ping_ipv4
 	tbf_test
+	tbf_root_test
 "
 source $lib_dir/sch_tbf_core.sh
 
+QDISC_TYPE=${QDISC% *}
+
 tbf_test_one()
 {
 	local bs=$1; shift

@@ -22,6 +25,8 @@ tbf_test_one()
 
 tbf_test()
 {
+	log_info "Testing root-$QDISC_TYPE-tbf"
+
 	# This test is used for both ETS and PRIO. Even though we only need two
 	# bands, PRIO demands a minimum of three.
 	tc qdisc add dev $swp2 root handle 10: $QDISC 3 priomap 2 1 0

@@ -29,6 +34,29 @@ tbf_test()
 	tc qdisc del dev $swp2 root
 }
 
+tbf_root_test()
+{
+	local bs=128K
+
+	log_info "Testing root-tbf-$QDISC_TYPE"
+
+	tc qdisc replace dev $swp2 root handle 1: \
+		tbf rate 400Mbit burst $bs limit 1M
+	tc qdisc replace dev $swp2 parent 1:1 handle 10: \
+		$QDISC 3 priomap 2 1 0
+	tc qdisc replace dev $swp2 parent 10:3 handle 103: \
+		bfifo limit 1M
+	tc qdisc replace dev $swp2 parent 10:2 handle 102: \
+		bfifo limit 1M
+	tc qdisc replace dev $swp2 parent 10:1 handle 101: \
+		bfifo limit 1M
+
+	do_tbf_test 10 400 $bs
+	do_tbf_test 11 400 $bs
+
+	tc qdisc del dev $swp2 root
+}
+
 trap cleanup EXIT
 
 setup_prepare

-- 
2.31.1

On Wed, 27 Oct 2021 18:19:59 +0300 Ido Schimmel wrote:

+	 * shaper makes sense. Also note that that is what we do for

That that that. checkpatch is sometimes useful.. :)

Hello:

This series was applied to netdev/net-next.git (master)
by Jakub Kicinski [off-list ref]:

On Wed, 27 Oct 2021 18:19:58 +0300 you wrote:

From: Ido Schimmel <idosch@nvidia.com>

Petr says:

Egress configuration in an mlxsw deployment would generally have an ETS
qdisc at root, with a number of bands and a priority dispatch between them.
Some of those bands could then have a RED and/or TBF qdiscs attached.

[...]

Here is the summary with links:
  - [net-next,1/3] mlxsw: spectrum_qdisc: Offload root TBF as port shaper
    https://git.kernel.org/netdev/net-next/c/48e4d00b1b93
  - [net-next,2/3] selftests: mlxsw: Test offloadability of root TBF
    https://git.kernel.org/netdev/net-next/c/3d5290ea1dae
  - [net-next,3/3] selftests: mlxsw: Test port shaper
    https://git.kernel.org/netdev/net-next/c/2b11e24ebaef

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Jakub Kicinski [off-list ref] writes:

On Wed, 27 Oct 2021 18:19:59 +0300 Ido Schimmel wrote:

quoted

+	 * shaper makes sense. Also note that that is what we do for

That that that. checkpatch is sometimes useful.. :)

I think that that that is legit. The first that in that that that is a
conjunction, the second a pronoun.