[PATCH net 0/3] net: mvneta: fix the TX queue arbitration and the mqprio teardown

[PATCH net 0/3] net: mvneta: fix the TX queue arbitration and the mqprio teardown

[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 3 serves the queues in weighted round robin. While testing it
with the mqprio offload, two older bugs showed up. Deleting the qdisc
never reaches the driver, and turning the shaper off leaves the
per-queue rate limits in place. Patches 1 and 2 fix them.

The series was tested on a Turris Omnia (Armada 385). 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.

Replacing an offloaded mqprio qdisc under a new handle still loses the
new configuration, because the old qdisc is torn down after the new
one is set up. Software mqprio behaves the same way, so this series
leaves it alone.

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

Josef Schlehofer (3):
  net: mvneta: lift the per-queue TX rate limits
  net: mvneta: fix the removal of the mqprio offload
  net: mvneta: serve the TX queues in weighted round robin

 drivers/net/ethernet/marvell/mvneta.c | 36 ++++++++++++++++++++++++---
 1 file changed, 33 insertions(+), 3 deletions(-)


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

[PATCH net 1/3] 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.

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>
---
 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)

[PATCH net 2/3] 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.

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>
---
 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)

[PATCH net 3/3] 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.
Like the Marvell reference driver, use the smallest weight that is
larger than the largest packet.
The mqprio offload never configured the TX scheduling, so its traffic
classes lose the fixed priority of the reset default as well.

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.

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

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 79711a117c39..75d46349df37 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -253,6 +253,7 @@
 #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_REFILL_NUM_CLK_REG		 0x3e08
 #define      MVNETA_REFILL_MAX_NUM_CLK		 0x0000ffff
 #define MVNETA_TX_MTU                            0x3e0c
@@ -264,6 +265,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
@@ -1629,6 +1632,24 @@ 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);
+	}
+
+	mvreg_write(pp, MVNETA_TXQ_FIXED_PRIO_REG, 0);
+}
+
 /* Set unicast address */
 static void mvneta_set_ucast_addr(struct mvneta_port *pp, u8 last_nibble,
 				  int queue)
@@ -3760,6 +3781,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_txq_wrr_set(pp, pp->pkt_size);
 
 	/* start the Rx/Tx activity */
 	mvneta_port_enable(pp);
-- 
2.54.0 (Apple Git-157)

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 (2) · Low: 1

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] At this commit, the new token-bucket reset in
  mvneta_disable_per_queue_rate_limit()…
- [Medium] Only mvneta_disable_per_queue_rate_limit() resets the buckets.
- [Low] The new 'full rate' bucket value in
  mvneta_disable_per_queue_rate_limit() is `(1 <<…

Pre-existing issues:
- [Medium] mvneta_setup_mqprio() calls
  mvneta_enable_per_queue_rate_limit() (line 5294), which switches…
- [Medium] mvneta_resume() never re-applies the TX shaper state
  programmed by mvneta_setup_mqprio(): the MVNETA_TXQ_CMD1_REG mode…

--

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

This patch changes mvneta_disable_per_queue_rate_limit(). It now also
reprograms the token bucket refill register of every TX queue to one base
period and MVNETA_TXQ_BUCKET_REFILL_VALUE_MAX tokens. As a result, the
per-queue limits set by an mqprio shaper no longer stay in effect once the
shaper is 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;

[Severity: Low]
Is this value still full rate if MVNETA_REFILL_NUM_CLK_REG was never
programmed?

The refill period here is one base period, and MVNETA_REFILL_NUM_CLK_REG
sets the base period. Only mvneta_enable_per_queue_rate_limit() writes
that register:

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

mvneta_disable_per_queue_rate_limit() also runs on ports where the shaper
was never enabled. One example is a first "tc qdisc add ... mqprio ... hw 1"
with no shaper:

drivers/net/ethernet/marvell/mvneta.c:mvneta_setup_mqprio() {
	...
	if (mqprio->shaper != TC_MQPRIO_SHAPER_BW_RATE) {
		mvneta_disable_per_queue_rate_limit(pp);
		return 0;
	}
	...
}

On those ports, every bucket refill register loses its reset default. The
new value has a real ceiling that depends on the reset value of NUM_CLK.

If the 16 bit field is at 0xffff cycles with a clock of about 250 MHz, this
still allows about 2 Gbit/s. That is below line rate only on 2500Base-X. A
reset value of 0 has undefined effect. The testing in the commit message
ran after the shaper had been enabled, so NUM_CLK was already programmed
there.

For comparison, mvpp2_defaults_set() in the mvpp2 driver programs the base
period together with refill period 1 and max tokens:

	mvpp2_write(port->priv, MVPP2_TXP_SCHED_PERIOD_REG,
		    port->priv->tclk / USEC_PER_SEC);

Could the disable path program MVNETA_REFILL_NUM_CLK_REG as well, so the
result does not depend on the hardware reset value?

> +	for (queue = 0; queue < txq_number; queue++)
> +		mvreg_write(pp, MVNETA_TXQ_BUCKET_REFILL_REG(queue), val);
>  }

[Severity: Medium]
Does this reset run when an offloaded mqprio qdisc with a shaper is
deleted?

On deletion, mqprio_destroy()->mqprio_disable_offload() passes an
all-zero offload struct to the driver:

net/sched/sch_mqprio.c:mqprio_disable_offload() {
	struct tc_mqprio_qopt_offload mqprio = { { 0 } };
	...
}

qopt.hw is 0, so mvneta_setup_mqprio() returns before it reaches the
!num_tc branch that calls mvneta_disable_per_queue_rate_limit():

drivers/net/ethernet/marvell/mvneta.c:mvneta_setup_mqprio() {
	...
	if (mqprio->qopt.hw != TC_MQPRIO_HW_OFFLOAD_TCS)
		return 0;
	...
	if (!num_tc) {
		mvneta_disable_per_queue_rate_limit(pp);
		netdev_reset_tc(dev);
		return 0;
	}
	...
}

After "tc qdisc del dev X root" of an mqprio with "shaper bw_rlimit", the
TX queues seem to stay in V3 mode with the old refill values. The commit
message says:

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

At this commit, only the path that sets up mqprio again without a shaper
gets the new reset.

The next patch in the series, "net: mvneta: fix the removal of the mqprio
offload", changes the check to:

	if (num_tc && mqprio->qopt.hw != TC_MQPRIO_HW_OFFLOAD_TCS)
		return 0;

With that change, the all-zero teardown request reaches the reset. Could
the commit message mention this dependency, or could the two patches be
reordered?

[Severity: Medium]
Only the disable path resets the buckets. At this commit, can queues
outside the new TC ranges keep stale limits?

mvneta_enable_per_queue_rate_limit() does not touch the per-queue bucket
registers. The setup loop in mvneta_setup_mqprio() reprograms only the
queues inside the new offset/count ranges:

	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++) {
			...
			ret = mvneta_setup_queue_rates(pp, txq, ...);

Take shaper config A (for example queues 0-7 at 10 Mbit/s) and shaper
config B (only queue 0). mqprio has no change operation, so A has to be
deleted before B is installed. As described above, the driver ignores
that deletion, and queues 1-7 keep A's refill values.

Traffic that picks a TX queue without regard to TC is then still
throttled. Examples are XDP_TX and ndo_xdp_xmit, which use
pp->txqs[cpu % txq_number].

Once "net: mvneta: fix the removal of the mqprio offload" is applied, the
destroy of A reaches the num_tc == 0 branch and every queue is refilled at
full rate. So the issue is gone by the end of the series.

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch.
However, the new full rate reset is not used on the error paths of
mvneta_setup_mqprio(). Can a rejected shaper config leave queues
throttled?

mvneta_setup_mqprio() switches to V3 mode before all TCs are validated:

	ret = mvneta_enable_per_queue_rate_limit(pp);
	if (ret)
		return ret;

	for (tc = 0; tc < mqprio->qopt.num_tc; tc++) {
		...
			ret = mvneta_setup_queue_rates(pp, txq, ...);
			if (ret)
				return ret;

Suppose TC0 has a valid max_rate and TC1 fails in
mvneta_setup_queue_rates(). TC1 fails if min_rate is non-zero, or if
max_rate is zero, not a multiple of 10 kbit/s, or above
MVNETA_TXQ_BUCKET_REFILL_VALUE_MAX. The function then returns -EINVAL,
TC0's queues are already programmed, and nothing is rolled back.

sch_mqprio sets priv->hw_offload only when ndo_setup_tc() succeeds. So
mqprio_destroy() takes the netdev_set_num_tc(dev, 0) branch and never
calls back into the driver:

	if (priv->hw_offload && dev->netdev_ops->ndo_setup_tc)
		mqprio_disable_offload(sch);
	else
		netdev_set_num_tc(dev, 0);

The missing rollback dates back to 2551dc9e398c and is still there at the
end of the series. Now that mvneta_disable_per_queue_rate_limit() resets
the buckets, could the error paths call it?

[Severity: Medium]
This isn't a bug introduced by this patch, but does mvneta_resume()
restore the shaper registers?

Three things hold the shaper state:

- the MVNETA_TXQ_CMD1_REG mode bits
- MVNETA_REFILL_NUM_CLK_REG
- the per-queue MVNETA_TXQ_BUCKET_REFILL_REG values

They are written only by mvneta_enable_per_queue_rate_limit(),
mvneta_disable_per_queue_rate_limit() and mvneta_setup_queue_rates(). All
three are called from mvneta_setup_mqprio(). The resume path
re-initializes the port defaults and rings but calls none of them.

If the controller loses register state across suspend, the hardware
shaper goes back to reset defaults. tc would still report the mqprio
shaper configuration.

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

Re: [PATCH net 2/3] 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] After this patch, an mqprio teardown request (all-zero qopt,
  num_tc == 0) always reaches the device-wide reset in…

--

net: mvneta: fix the removal of the mqprio offload

When an offloaded mqprio qdisc is deleted, mqprio_disable_offload()
passes an all-zero qopt with hw set to 0. As a result,
mvneta_setup_mqprio() used to return before reaching its teardown code.
This patch checks qopt.hw only when num_tc is non-zero, so the teardown
path is reached on removal.

> 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 a root qdisc replace now wipe out the configuration that the new
mqprio qdisc has just offloaded?

With this change, every all-zero teardown request reaches the reset that
covers the whole device, further down 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;
	}

Take the case where offloaded mqprio 1: is installed and the admin runs
something like "tc qdisc replace dev X root handle 2: mqprio ... hw 1".
__tc_modify_qdisc() does not find handle 2:, so it goes to create_n_graft.
The new qdisc is then initialised before the old one is destroyed:

__tc_modify_qdisc()
  qdisc_create()
    mqprio_init()
      mqprio_enable_offload()
        mvneta_setup_mqprio(num_tc = N, hw = 1)
          programs TC map, VLAN prio to RXQ map, per-queue rate limits
  qdisc_graft()
    notify_and_destroy(old)
      qdisc_put(old)
        mqprio_destroy(old)
          mqprio_disable_offload()
            mvneta_setup_mqprio(num_tc = 0)
              clears RX prio map, rate limits, netdev TC state
    new->ops->attach(new)
      mqprio_attach()

mqprio_destroy() takes this path because the old qdisc has hw_offload
set:

	if (priv->hw_offload && dev->netdev_ops->ndo_setup_tc)
		mqprio_disable_offload(sch);

mqprio_attach() only grafts the child qdiscs and does not reapply the
configuration. The replace returns success and the new qdisc still
reports hw_offload, but its rate limits, RX steering and netdev TC
mapping are gone.

Before this patch, the hw != TC_MQPRIO_HW_OFFLOAD_TCS check turned the old
qdisc's teardown into a no-op, so the new qdisc's configuration was kept.

The same init-before-destroy ordering already resets state in software
mode, since mqprio_destroy() calls netdev_set_num_tc(dev, 0). It also
affects other drivers that reset on num_tc == 0, and mvneta behaved this
way before e7ca75fe6662. On mvneta, though, this patch is what makes the
case reachable again.

>  	if (num_tc > rxq_number)
>  		return -EINVAL;
>

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