[PATCH net v3 0/5] net: mvneta: fix the TX queue arbitration and the mqprio offload

[PATCH net v3 0/5] net: mvneta: fix the TX queue arbitration and the mqprio offload

[Josef Schlehofer]

The NETA TX arbiter is left at its reset default, which serves the TX
queues in fixed priority. Under load a busy queue starves the others.
OpenWrt has worked around this since 2017 by sending all TX traffic
of its 32-bit boards through queue 0 [1].

Patch 5 serves the queues in weighted round robin. mvpp2 also sets its
TX queues to round robin, in mvpp2_defaults_set(). An mqprio offload
keeps the fixed priority, as its traffic classes may rely on it. Without
the offload the queues are served in WRR, also when skbedit or a
software mqprio picks them.

While working on it, four older bugs in the mqprio offload showed up.
Turning the shaper off leaves the per-queue rate limits in place
(patch 1). Deleting the qdisc never reaches the driver (patch 2). A
failed setup is not undone (patch 3). Opening the interface or changing
its MTU sets every rate limit to 10.23 Mbit/s (patch 4).

The series was tested on a Turris Omnia (Armada 385). With an mqprio
offload and "queues 4@0 4@4" on a 100 Mbit/s link, one stream on queues
4 to 7 got 94 Mbit/s and three streams on queues 0 to 3 got 0.2 Mbit/s
together. The other NETA variants were not tested, but the driver
already programs registers of the same block (MVNETA_TX_MTU and the
token sizes) on all of them. Suspend and resume were not tested.

With patch 2, replacing an offloaded mqprio qdisc under a new handle
with another qdisc that sets traffic classes loses them. The new qdisc
is set up before the old one is torn down, and the driver now acts on
that teardown. The core does the same when the old qdisc is a software
mqprio. A taprio qdisc left without traffic classes sends nothing.

Changes in v3:
- Patch 5 (patch 3 in v2): keep the fixed priority while an mqprio
  offload is set up. Moved to the end of the series.
- Patch 4: braces around the loop bodies.
- Patch 2: the commit message also mentions a taprio replacement. The
  code is unchanged.
- Patches 1 to 3: Reviewed-by tags added.
- v2: https://lore.kernel.org/netdev/20261103125021.70845-1-pepe.schlehofer@gmail.com/
- v1: https://lore.kernel.org/netdev/20261102112816.12322-1-pepe.schlehofer@gmail.com/

[1] https://github.com/openwrt/openwrt/blob/a5e96c837ead549820867bb83c1ad471e39f5ace/target/linux/mvebu/patches-6.18/700-mvneta-tx-queue-workaround.patch

Josef Schlehofer (5):
  net: mvneta: lift the per-queue TX rate limits
  net: mvneta: fix the removal of the mqprio offload
  net: mvneta: unwind a failed mqprio setup
  net: mvneta: keep the TX rate limits across open and MTU change
  net: mvneta: serve the TX queues in weighted round robin

 drivers/net/ethernet/marvell/mvneta.c | 112 +++++++++++++++++++++++---
 1 file changed, 100 insertions(+), 12 deletions(-)


base-commit: f56daaadfecae1f760d0fb34385b256d3af68970
-- 
2.54.0 (Apple Git-157)

[PATCH net v3 1/5] net: mvneta: lift the per-queue TX rate limits

[Josef Schlehofer]

The per-queue rate limits set by mqprio stay in effect after the
shaper is turned off.

mvneta_disable_per_queue_rate_limit() only changes the mode bits in
MVNETA_TXQ_CMD1_REG. The token buckets keep limiting the TX queues
with that mode as well, and their refill rate still holds the old
limit.

Refill the token buckets of all TX queues at full rate when the rate
limit is disabled. Deleting the qdisc reaches this code only with the
next patch.

Tested on a Turris Omnia (Armada 385) with 10 Mbit/s limits on a
100 Mbit/s link. When mqprio was set up again without a shaper, each
TX queue stayed at 10 Mbit/s. With this patch the link reaches
94 Mbit/s.

Fixes: 2551dc9e398c ("net: mvneta: Add TC traffic shaping offload")
Signed-off-by: Josef Schlehofer <pepe.schlehofer@gmail.com>
Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/ethernet/marvell/mvneta.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index ea755cf05b9e..9b9efac2846e 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -5204,9 +5204,16 @@ static int mvneta_enable_per_queue_rate_limit(struct mvneta_port *pp)
 static void mvneta_disable_per_queue_rate_limit(struct mvneta_port *pp)
 {
 	u32 val = mvreg_read(pp, MVNETA_TXQ_CMD1_REG);
+	int queue;
 
 	val |= (MVNETA_TXQ_CMD1_BW_LIM_SEL_V1 | MVNETA_TXQ_CMD1_BW_LIM_EN);
 	mvreg_write(pp, MVNETA_TXQ_CMD1_REG, val);
+
+	/* The token buckets still limit the queues, refill them at full rate */
+	val = (1 << MVNETA_TXQ_BUCKET_REFILL_PERIOD_SHIFT) |
+	      MVNETA_TXQ_BUCKET_REFILL_VALUE_MAX;
+	for (queue = 0; queue < txq_number; queue++)
+		mvreg_write(pp, MVNETA_TXQ_BUCKET_REFILL_REG(queue), val);
 }
 
 static int mvneta_setup_queue_rates(struct mvneta_port *pp, int queue,
-- 
2.54.0 (Apple Git-157)

Re: [PATCH net v3 1/5] net: mvneta: lift the per-queue TX rate limits

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] In drivers/net/ethernet/marvell/mvneta.c,
  mvneta_disable_per_queue_rate_limit() now writes every…

--

net: mvneta: lift the per-queue TX rate limits

When the per-queue TX rate limit is disabled, this commit sets the token
bucket refill registers of all TX queues to full rate. Without it, the
limits set by an earlier mqprio shaper stayed in effect after the shaper
was turned off.

> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> index ea755cf05b9ef..9b9efac2846ef 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -5204,9 +5204,16 @@ static int mvneta_enable_per_queue_rate_limit(struct mvneta_port *pp)
>  static void mvneta_disable_per_queue_rate_limit(struct mvneta_port *pp)
>  {
>  	u32 val = mvreg_read(pp, MVNETA_TXQ_CMD1_REG);
> +	int queue;
>  
>  	val |= (MVNETA_TXQ_CMD1_BW_LIM_SEL_V1 | MVNETA_TXQ_CMD1_BW_LIM_EN);
>  	mvreg_write(pp, MVNETA_TXQ_CMD1_REG, val);
> +
> +	/* The token buckets still limit the queues, refill them at full rate */
> +	val = (1 << MVNETA_TXQ_BUCKET_REFILL_PERIOD_SHIFT) |
> +	      MVNETA_TXQ_BUCKET_REFILL_VALUE_MAX;
> +	for (queue = 0; queue < txq_number; queue++)
> +		mvreg_write(pp, MVNETA_TXQ_BUCKET_REFILL_REG(queue), val);

[Severity: Low]
Does "full rate" here depend on MVNETA_REFILL_NUM_CLK_REG having been
programmed first?

A period of 1 means one base refill period. The length of that period
comes from MVNETA_REFILL_NUM_CLK_REG. As far as I can see, the only write
to that register in the driver is in mvneta_enable_per_queue_rate_limit():

	/* Set the base refill rate */
	mvreg_write(pp, MVNETA_REFILL_NUM_CLK_REG, refill_cycles);

mvneta_setup_mqprio() can reach the disable path without the enable path
ever having run:

	if (!num_tc) {
		mvneta_disable_per_queue_rate_limit(pp);
		...

	if (mqprio->shaper != TC_MQPRIO_SHAPER_BW_RATE) {
		mvneta_disable_per_queue_rate_limit(pp);
		return 0;
	}

One example is the first offloaded mqprio after boot with no bw_rlimit
shaper, such as one that only maps VLAN priorities to RX queues. Later
in the series, deleting such an offload, or a setup that fails before
mvneta_enable_per_queue_rate_limit() runs, also takes this path.

In those cases the per-queue refill registers are overwritten while
MVNETA_REFILL_NUM_CLK_REG still holds its hardware reset value. Before
this patch they kept their defaults on a port where the shaper was never
enabled.

If the reset period is long, could this cap the TX queues below line
rate? For example, with NUM_CLK at 0xffff cycles, MAX tokens per period
works out to roughly 2 Gbit/s, which is below a 2.5G port's rate.

This depends on the reset value of MVNETA_REFILL_NUM_CLK_REG, which
can't be checked from the code. Period 1 with the maximum value is the
fastest setting the register can encode, so this may never be visible in
practice.

Would it make sense to also program MVNETA_REFILL_NUM_CLK_REG on the
disable path, so the "full rate" setting doesn't depend on the enable
path having run first?

>  }
>  
>  static int mvneta_setup_queue_rates(struct mvneta_port *pp, int queue,

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261104224518.66487-1-pepe.schlehofer%40gmail.com

[PATCH net v3 2/5] net: mvneta: fix the removal of the mqprio offload

[Josef Schlehofer]

Deleting an offloaded mqprio qdisc leaves its traffic classes and
per-queue rate limits in place.

mqprio_disable_offload() passes an all-zero configuration, so qopt.hw
is 0 and mvneta_setup_mqprio() returns before it reaches the teardown
code.

Check qopt.hw only when traffic classes are set up.

A replacement qdisc is set up before the old one is torn down.
Replacing an offloaded mqprio qdisc under a new handle with another
qdisc that sets traffic classes therefore loses them, as with software
mqprio. A taprio qdisc left without traffic classes sends nothing.

Tested on a Turris Omnia (Armada 385) with 10 Mbit/s limits on a
100 Mbit/s link. After the qdisc was deleted, only TX queues 0-3 were
used and the link carried 19 Mbit/s. With this patch and the previous
one, the queues above 3 are used again and the link reaches 94 Mbit/s.

Fixes: e7ca75fe6662 ("net: mvneta: Don't force-set the offloading flag")
Signed-off-by: Josef Schlehofer <pepe.schlehofer@gmail.com>
Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/ethernet/marvell/mvneta.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 9b9efac2846e..79711a117c39 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -5251,11 +5251,12 @@ static int mvneta_setup_mqprio(struct net_device *dev,
 	int rxq, txq, tc, ret;
 	u8 num_tc;
 
-	if (mqprio->qopt.hw != TC_MQPRIO_HW_OFFLOAD_TCS)
-		return 0;
-
 	num_tc = mqprio->qopt.num_tc;
 
+	/* On removal the qdisc passes an all-zero qopt, with hw 0 */
+	if (num_tc && mqprio->qopt.hw != TC_MQPRIO_HW_OFFLOAD_TCS)
+		return 0;
+
 	if (num_tc > rxq_number)
 		return -EINVAL;
 
-- 
2.54.0 (Apple Git-157)

Re: [PATCH net v3 2/5] net: mvneta: fix the removal of the mqprio offload

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] With this patch, an all-zero mqprio teardown always reaches
  the global hardware reset in mvneta_setup_mqprio()…

--

net: mvneta: fix the removal of the mqprio offload

This patch lets mvneta_setup_mqprio() reach its teardown code when
mqprio_disable_offload() passes an all-zero configuration. Deleting an
offloaded mqprio qdisc now clears the traffic classes and the per-queue
rate limits. The qopt.hw check now applies only when num_tc is non-zero.

> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> index 9b9efac2846ef..79711a117c39c 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -5251,11 +5251,12 @@ static int mvneta_setup_mqprio(struct net_device *dev,
>  	int rxq, txq, tc, ret;
>  	u8 num_tc;
>  
> -	if (mqprio->qopt.hw != TC_MQPRIO_HW_OFFLOAD_TCS)
> -		return 0;
> -
>  	num_tc = mqprio->qopt.num_tc;
>  
> +	/* On removal the qdisc passes an all-zero qopt, with hw 0 */
> +	if (num_tc && mqprio->qopt.hw != TC_MQPRIO_HW_OFFLOAD_TCS)
> +		return 0;
> +

[Severity: Medium]

Can this teardown wipe out the configuration of a replacement mqprio
qdisc?

Take an offloaded mqprio root that gets replaced under a new handle with
something like:

  tc qdisc replace dev ethX root handle 2: mqprio ... hw 1 \
      mode channel shaper bw_rate max_rate ...

The core sets up the new qdisc before it destroys the old one:

tc_modify_qdisc()
  qdisc_create()
    mqprio_init()
      mqprio_enable_offload()
        mvneta_setup_mqprio()        /* num_tc = N, hw = 1 */
  qdisc_graft()
    notify_and_destroy()
      mqprio_destroy()               /* old qdisc, hw_offload == 1 */
        mqprio_disable_offload()
          mvneta_setup_mqprio()      /* all-zero qopt */

Before this patch, the hw == 0 early return ignored the second call. Now
that call goes through to the port-wide reset in mvneta_setup_mqprio():

	mvneta_clear_rx_prio_map(pp);

	if (!num_tc) {
		mvneta_disable_per_queue_rate_limit(pp);
		netdev_reset_tc(dev);
		return 0;
	}

The reset does three things:

  - it writes 0 to MVNETA_VLAN_PRIO_TO_RXQ
  - it sets TXQ_CMD1 back to the V1 limiter and refills every TXQ bucket
    at MVNETA_TXQ_BUCKET_REFILL_VALUE_MAX
  - it resets dev->num_tc

The replacement qdisc had just programmed all of this state. With the
rest of the series, the zero call also clears txq_fixed_prio in
mvneta_setup_tc().

The new qdisc still has priv->hw_offload set, so tc shows "hw 1" and the
configured max_rate values. The hardware, however, does no shaping and
steers every VLAN priority to RX queue 0.

mqprio has no .change op. Replacing under a new handle is therefore the
only way to change the configuration without deleting the qdisc first.

The commit message says:

  "Replacing an offloaded mqprio qdisc under a new handle with another
  qdisc that sets traffic classes therefore loses them, as with software
  mqprio."

Software mqprio has no hardware shaper or RX steering to lose. Does this
description cover the mqprio-over-mqprio case, where the rates and RX
steering are dropped while tc still reports them?

The later patch "net: mvneta: keep the TX rate limits across open and
MTU change" treats the same symptom ("tc still shows the configured
rates" while they are not applied) as a bug. No later patch in the series
seems to handle this replace path.

Other mqprio offload drivers, such as enetc_setup_tc_mqprio() and
am65_cpsw_setup_mqprio(), also treat num_tc == 0 as an unconditional
teardown. The problem comes from the core's create-before-destroy
ordering, which those drivers share.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261104224518.66487-1-pepe.schlehofer%40gmail.com

[PATCH net v3 3/5] net: mvneta: unwind a failed mqprio setup

[Josef Schlehofer]

A failed mqprio offload setup leaves part of its configuration in the
hardware. For example, when one max_rate is not a multiple of
10 kbit/s, the command fails but the TX queues set up before it stay
rate limited.

mvneta_setup_mqprio() returns on an invalid queue range or rate after
it has already mapped RX queues, switched the rate limit mode and set
the rates of some TX queues. The qdisc then fails to attach and the
driver is not called again, so nothing removes them.

Undo the RX queue mapping, the rate limits and the traffic classes
when the setup fails.

Tested on a Turris Omnia (Armada 385). After mqprio failed on a
max_rate of 10001kbit, TX queues 0-3 stayed limited to 10 Mbit/s and
the VLAN priority map stayed set. With this patch neither is left
behind.

Fixes: e9f7099d0730 ("net: mvneta: Allow having more than one queue per TC")
Fixes: 2551dc9e398c ("net: mvneta: Add TC traffic shaping offload")
Signed-off-by: Josef Schlehofer <pepe.schlehofer@gmail.com>
Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/ethernet/marvell/mvneta.c | 28 +++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 79711a117c39..171111d7fe57 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -5277,8 +5277,10 @@ static int mvneta_setup_mqprio(struct net_device *dev,
 		for (rxq = mqprio->qopt.offset[tc];
 		     rxq < mqprio->qopt.count[tc] + mqprio->qopt.offset[tc];
 		     rxq++) {
-			if (rxq >= rxq_number)
-				return -EINVAL;
+			if (rxq >= rxq_number) {
+				ret = -EINVAL;
+				goto err;
+			}
 
 			mvneta_map_vlan_prio_to_rxq(pp, tc, rxq);
 		}
@@ -5289,29 +5291,39 @@ static int mvneta_setup_mqprio(struct net_device *dev,
 		return 0;
 	}
 
-	if (mqprio->qopt.num_tc > txq_number)
-		return -EINVAL;
+	if (mqprio->qopt.num_tc > txq_number) {
+		ret = -EINVAL;
+		goto err;
+	}
 
 	ret = mvneta_enable_per_queue_rate_limit(pp);
 	if (ret)
-		return ret;
+		goto err;
 
 	for (tc = 0; tc < mqprio->qopt.num_tc; tc++) {
 		for (txq = mqprio->qopt.offset[tc];
 		     txq < mqprio->qopt.count[tc] + mqprio->qopt.offset[tc];
 		     txq++) {
-			if (txq >= txq_number)
-				return -EINVAL;
+			if (txq >= txq_number) {
+				ret = -EINVAL;
+				goto err;
+			}
 
 			ret = mvneta_setup_queue_rates(pp, txq,
 						       mqprio->min_rate[tc],
 						       mqprio->max_rate[tc]);
 			if (ret)
-				return ret;
+				goto err;
 		}
 	}
 
 	return 0;
+
+err:
+	mvneta_clear_rx_prio_map(pp);
+	mvneta_disable_per_queue_rate_limit(pp);
+	netdev_reset_tc(dev);
+	return ret;
 }
 
 static int mvneta_setup_tc(struct net_device *dev, enum tc_setup_type type,
-- 
2.54.0 (Apple Git-157)

Re: [PATCH net v3 3/5] net: mvneta: unwind a failed mqprio setup

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In drivers/net/ethernet/marvell/mvneta.c, the new `err:` label
  in mvneta_setup_mqprio() does not put back the hardware state that…

--

net: mvneta: unwind a failed mqprio setup

This commit adds an err: label to mvneta_setup_mqprio(). When a mqprio
offload setup fails, the driver now clears the VLAN priority to RX queue
map, disables the per-queue TX rate limits and resets the netdev traffic
classes, so no partial configuration is left in hardware.

> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> index 79711a117c39c..171111d7fe57d 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c

[ ... ]

> @@ -5289,29 +5291,39 @@ static int mvneta_setup_mqprio(struct net_device *dev,

[ ... ]

>  			ret = mvneta_setup_queue_rates(pp, txq,
>  						       mqprio->min_rate[tc],
>  						       mqprio->max_rate[tc]);
>  			if (ret)
> -				return ret;
> +				goto err;
>  		}
>  	}
>  
>  	return 0;
> +
> +err:
> +	mvneta_clear_rx_prio_map(pp);
> +	mvneta_disable_per_queue_rate_limit(pp);
> +	netdev_reset_tc(dev);
> +	return ret;
>  }

[Severity: Medium]
Can this unwind also remove the TX rate limits of an mqprio qdisc that is
already attached, when replacing it fails?

For a failed fresh attach, going back to "no offload" is right. Now take
a shaped, offloaded mqprio that is root as 1:. It gets replaced under a
new handle with a bad parameter, for example:

  tc qdisc replace dev eth0 root handle 2: mqprio ... hw 1 mode channel \
     shaper bw_rate max_rate 10001kbit ...

The new qdisc is initialised while 1: is still grafted:

tc_modify_qdisc()
  create_n_graft:
  qdisc_create()
    mqprio_init()
      mqprio_enable_offload()
        mvneta_setup_mqprio()
          mvneta_setup_queue_rates() fails
          goto err
            mvneta_disable_per_queue_rate_limit()

qdisc_create() then calls mqprio_destroy() on the new qdisc.
mqprio_enable_offload() sets priv->hw_offload only after ndo_setup_tc
succeeds, so the driver is not called again and 1: stays root.

Before this patch, these failures returned before any of 1:'s per-queue
TX rates were overwritten:

  - the RX queue range check
  - num_tc > txq_number
  - mvneta_enable_per_queue_rate_limit()
  - the first TX queue

Now every one of those paths goes through err: and refills all TX buckets
at full rate. Meanwhile tc qdisc show still lists 1: with its max_rate
values.

The replace case was already partly inconsistent before this patch, since
the RX prio map is cleared before validation. Losing 1:'s TX rate limits
is new, though.

"net: mvneta: lift the per-queue TX rate limits", earlier in the series,
made mvneta_disable_per_queue_rate_limit() also clear
pp->txqs[].bucket_refill:

	for (queue = 0; queue < txq_number; queue++) {
		pp->txqs[queue].bucket_refill = 0;
		mvreg_write(pp, MVNETA_TXQ_BUCKET_REFILL_REG(queue), val);
	}

Because of that, mvneta_restore_per_queue_rate_limit() will not bring 1:'s
limits back on the next open either.

The earlier series commit "net: mvneta: fix the removal of the mqprio
offload" says that "A replacement qdisc is set up before the old one is
torn down". This commit message only covers the fresh attach case.

Would it work to validate the whole configuration before touching the
hardware? That would cover the RX/TX queue ranges, num_tc against
txq_number, min_rate, and the resolution and range of each max_rate. It
would also need to happen before the mvneta_clear_rx_prio_map() and
netdev_set_num_tc() calls at the top of mvneta_setup_mqprio(). A rejected
configuration would then leave the previous offload in place.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261104224518.66487-1-pepe.schlehofer%40gmail.com

[PATCH net v3 4/5] net: mvneta: keep the TX rate limits across open and MTU change

[Josef Schlehofer]

After the interface is brought up again or its MTU is changed, every
per-queue rate limit set by mqprio runs at 10.23 Mbit/s, whatever rate
was configured. tc still shows the configured rates.

mvneta_txq_hw_init() writes the token configuration of the legacy rate
limiter for each TX queue. This also sets the refill value of the
queue's token bucket to 1023, which is 10.23 Mbit/s at the refill
period that mqprio uses.

Remember the refill of each rate limited queue and program the rate
limits again when the port is started. Resume also starts the port
with mvneta_start_dev().

Tested on a Turris Omnia (Armada 385) with 1 Mbit/s limits on a
100 Mbit/s link. After the link was set down and up, or the MTU was
changed, each busy TX queue carried about 10 Mbit/s. With this patch it
stays at about 1 Mbit/s.

Fixes: 2551dc9e398c ("net: mvneta: Add TC traffic shaping offload")
Signed-off-by: Josef Schlehofer <pepe.schlehofer@gmail.com>
---
 drivers/net/ethernet/marvell/mvneta.c | 31 ++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 171111d7fe57..2f8c134a921e 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -683,6 +683,9 @@ struct mvneta_tx_queue {
 
 	u32 done_pkts_coal;
 
+	/* Bucket refill of the mqprio rate limit, 0 when not limited */
+	u32 bucket_refill;
+
 	/* Virtual address of the TX DMA descriptors array */
 	struct mvneta_tx_desc *descs;
 
@@ -3752,6 +3755,8 @@ static int mvneta_config_interface(struct mvneta_port *pp,
 	return ret;
 }
 
+static void mvneta_restore_per_queue_rate_limit(struct mvneta_port *pp);
+
 static void mvneta_start_dev(struct mvneta_port *pp)
 {
 	int cpu;
@@ -3760,6 +3765,7 @@ static void mvneta_start_dev(struct mvneta_port *pp)
 
 	mvneta_max_rx_size_set(pp, pp->pkt_size);
 	mvneta_txq_max_tx_size_set(pp, pp->pkt_size);
+	mvneta_restore_per_queue_rate_limit(pp);
 
 	/* start the Rx/Tx activity */
 	mvneta_port_enable(pp);
@@ -5212,8 +5218,10 @@ static void mvneta_disable_per_queue_rate_limit(struct mvneta_port *pp)
 	/* The token buckets still limit the queues, refill them at full rate */
 	val = (1 << MVNETA_TXQ_BUCKET_REFILL_PERIOD_SHIFT) |
 	      MVNETA_TXQ_BUCKET_REFILL_VALUE_MAX;
-	for (queue = 0; queue < txq_number; queue++)
+	for (queue = 0; queue < txq_number; queue++) {
+		pp->txqs[queue].bucket_refill = 0;
 		mvreg_write(pp, MVNETA_TXQ_BUCKET_REFILL_REG(queue), val);
+	}
 }
 
 static int mvneta_setup_queue_rates(struct mvneta_port *pp, int queue,
@@ -5239,11 +5247,32 @@ static int mvneta_setup_queue_rates(struct mvneta_port *pp, int queue,
 	val |= (MVNETA_TXQ_BUCKET_REFILL_PERIOD <<
 		MVNETA_TXQ_BUCKET_REFILL_PERIOD_SHIFT);
 
+	pp->txqs[queue].bucket_refill = val;
 	mvreg_write(pp, MVNETA_TXQ_BUCKET_REFILL_REG(queue), val);
 
 	return 0;
 }
 
+/* The TX queue setup overwrites the rate limits, program them again */
+static void mvneta_restore_per_queue_rate_limit(struct mvneta_port *pp)
+{
+	int queue;
+
+	for (queue = 0; queue < txq_number; queue++) {
+		if (pp->txqs[queue].bucket_refill)
+			break;
+	}
+
+	if (queue == txq_number || mvneta_enable_per_queue_rate_limit(pp))
+		return;
+
+	for (; queue < txq_number; queue++) {
+		if (pp->txqs[queue].bucket_refill)
+			mvreg_write(pp, MVNETA_TXQ_BUCKET_REFILL_REG(queue),
+				    pp->txqs[queue].bucket_refill);
+	}
+}
+
 static int mvneta_setup_mqprio(struct net_device *dev,
 			       struct tc_mqprio_qopt_offload *mqprio)
 {
-- 
2.54.0 (Apple Git-157)

Re: [PATCH net v3 4/5] net: mvneta: keep the TX rate limits across open and MTU change

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message for "net: mvneta: keep the TX rate limits
  across open and MTU change" is a bug fix (it has a Fixes: 2551dc9e398c…

--

net: mvneta: keep the TX rate limits across open and MTU change

This patch saves the bucket refill value of each mqprio rate limited TX
queue in struct mvneta_tx_queue. When mvneta_start_dev() runs, it calls
the new mvneta_restore_per_queue_rate_limit() to program those values
again. The reason is that mvneta_txq_hw_init() resets the refill to
10.23 Mbit/s on open, on MTU change and on resume.

> Tested on a Turris Omnia (Armada 385) with 1 Mbit/s limits on a
> 100 Mbit/s link. After the link was set down and up, or the MTU was
> changed, each busy TX queue carried about 10 Mbit/s. With this patch it
> stays at about 1 Mbit/s.
>
> Fixes: 2551dc9e398c ("net: mvneta: Add TC traffic shaping offload")

[Severity: Low]
This isn't a bug, but could the commit message say how the issue was
found? The patch carries a Fixes: tag, and the "Bug fixes" section of
Documentation/process/maintainer-netdev.rst says:

    All bug fixes must state how the issue was discovered and validated,
    and how the fix was tested.

The message explains the validation and names the hardware used for
testing. It doesn't say how the bug was discovered, for example seen in
real use, found by reading the code, or found by a tool. There is also no
Reported-by or similar trailer that would imply it.

The same section also asks for a statement about selftests:

    The testing statement should explain whether the fix was tested against the
    reproducer and against kernel selftests.

Could a line be added saying whether the kernel selftests were run with
this fix?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261104224518.66487-1-pepe.schlehofer%40gmail.com

[PATCH net v3 5/5] net: mvneta: serve the TX queues in weighted round robin

[Josef Schlehofer]

Under load, some TX queues get no service at all. With four TCP streams
on a 100 Mbit/s link, up to three streams sent nothing for 20 seconds.

The TX queue arbiter is left at its reset default, which serves all
queues in fixed priority. The stack spreads the flows over the queues,
so a busy queue starves the others.

Give the TX queues the same WRR weight and clear the fixed priority.
Use the smallest weight that is larger than the largest packet.

Keep the fixed priority while an mqprio offload is set up, as its
traffic classes may rely on it. Clear it when the offload is removed or
its setup fails. Starting the port sets the mode again. The fixed
priority is set per queue, so the queues of one traffic class still
starve each other.

Tested on a Turris Omnia (Armada 385). With this patch each busy TX
queue gets an equal share of the link and no stream stalls. With an
mqprio offload, a higher queue still has priority over a lower one.

Fixes: ee40a116ebf1 ("net: mvneta: fix improper tx queue usage in mvneta_tx()")
Suggested-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Signed-off-by: Josef Schlehofer <pepe.schlehofer@gmail.com>
---
 drivers/net/ethernet/marvell/mvneta.c | 41 ++++++++++++++++++++++++++-
 1 file changed, 40 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 2f8c134a921e..6fff17a3e4de 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -253,6 +253,8 @@
 #define MVNETA_TXQ_CMD1_REG			 0x3e00
 #define      MVNETA_TXQ_CMD1_BW_LIM_SEL_V1	 BIT(3)
 #define      MVNETA_TXQ_CMD1_BW_LIM_EN		 BIT(0)
+#define MVNETA_TXQ_FIXED_PRIO_REG		 0x3e04
+#define      MVNETA_TXQ_FIXED_PRIO_ALL		 0xff
 #define MVNETA_REFILL_NUM_CLK_REG		 0x3e08
 #define      MVNETA_REFILL_MAX_NUM_CLK		 0x0000ffff
 #define MVNETA_TX_MTU                            0x3e0c
@@ -264,6 +266,8 @@
 #define      MVNETA_TXQ_BUCKET_REFILL_VALUE_MAX	 0x0007ffff
 #define MVNETA_TXQ_TOKEN_SIZE_REG(q)             (0x3e40 + ((q) << 2))
 #define      MVNETA_TXQ_TOKEN_SIZE_MAX           0x7fffffff
+#define MVNETA_TXQ_WRR_ARB_REG(q)		 (0x3e80 + ((q) << 2))
+#define      MVNETA_TXQ_WRR_WEIGHT_MASK		 0xff
 
 /* The values of the bucket refill base period and refill period are taken from
  * the reference manual, and adds up to a base resolution of 10Kbps. This allows
@@ -518,6 +522,8 @@ struct mvneta_port {
 	 */
 	spinlock_t lock;
 	bool is_stopped;
+	/* An mqprio offload keeps the TX queues in fixed priority */
+	bool txq_fixed_prio;
 
 	u32 cause_rx_tx;
 	struct napi_struct napi;
@@ -1632,6 +1638,29 @@ static void mvneta_txq_max_tx_size_set(struct mvneta_port *pp, int max_tx_size)
 	}
 }
 
+/* With the default fixed priority, a busy queue starves the others */
+static void mvneta_txq_wrr_set(struct mvneta_port *pp, int max_tx_size)
+{
+	/* In units of 256 bytes, must exceed the largest packet */
+	u32 weight = max_tx_size / 256 + 1;
+	int queue;
+	u32 val;
+
+	for (queue = 0; queue < txq_number; queue++) {
+		val = mvreg_read(pp, MVNETA_TXQ_WRR_ARB_REG(queue));
+		val &= ~MVNETA_TXQ_WRR_WEIGHT_MASK;
+		val |= weight;
+		mvreg_write(pp, MVNETA_TXQ_WRR_ARB_REG(queue), val);
+	}
+}
+
+/* Only the traffic classes of an mqprio offload keep the fixed priority */
+static void mvneta_txq_fixed_prio_set(struct mvneta_port *pp)
+{
+	mvreg_write(pp, MVNETA_TXQ_FIXED_PRIO_REG,
+		    pp->txq_fixed_prio ? MVNETA_TXQ_FIXED_PRIO_ALL : 0);
+}
+
 /* Set unicast address */
 static void mvneta_set_ucast_addr(struct mvneta_port *pp, u8 last_nibble,
 				  int queue)
@@ -3765,6 +3794,8 @@ static void mvneta_start_dev(struct mvneta_port *pp)
 
 	mvneta_max_rx_size_set(pp, pp->pkt_size);
 	mvneta_txq_max_tx_size_set(pp, pp->pkt_size);
+	mvneta_txq_wrr_set(pp, pp->pkt_size);
+	mvneta_txq_fixed_prio_set(pp);
 	mvneta_restore_per_queue_rate_limit(pp);
 
 	/* start the Rx/Tx activity */
@@ -5358,9 +5389,17 @@ err:
 static int mvneta_setup_tc(struct net_device *dev, enum tc_setup_type type,
 			   void *type_data)
 {
+	struct mvneta_port *pp = netdev_priv(dev);
+	struct tc_mqprio_qopt_offload *mqprio;
+	int ret;
+
 	switch (type) {
 	case TC_SETUP_QDISC_MQPRIO:
-		return mvneta_setup_mqprio(dev, type_data);
+		mqprio = type_data;
+		ret = mvneta_setup_mqprio(dev, mqprio);
+		pp->txq_fixed_prio = !ret && mqprio->qopt.num_tc;
+		mvneta_txq_fixed_prio_set(pp);
+		return ret;
 	default:
 		return -EOPNOTSUPP;
 	}
-- 
2.54.0 (Apple Git-157)