Re: [PATCH net] net: bcmgenet: Use weighted round-robin TX DMA arbitration

[Justin Chen <justin.chen@broadcom.com>]

On 6/11/26 1:08 AM, Nicolai Buchwitz wrote:
> Hi Ovidiu
> 
> This seems to be a good follow-up to Justin's earlier series [1].
> 
> On 10.6.2026 10:52, Ovidiu Panait wrote:
>> Under heavy network traffic, we observed sporadic TX queue timeouts on 
>> the
>> Raspberry Pi 4. The timeouts can be reproduced by stress testing the TX
>> path with multiple concurrent iperf UDP streams:
>>
>>     iperf3 -c <ip> -u -b0 -P16 -t60
>>     NETDEV WATCHDOG: CPU: 0: transmit queue 0 timed out 2044 ms
>>     NETDEV WATCHDOG: CPU: 3: transmit queue 0 timed out 2004 ms
>>
>> Investigation showed that the timeouts are caused by the priority-based
>> arbiter. Under heavy load the highest priority queue starves the lower
>> priority ones, causing timeouts. The TX strict priority arbiter is not
>> suitable for the default use case where all the traffic gets spread
>> across all the TX queues.
>>
>> Therefore, to fix this, switch the TX DMA arbiter to Weighted Round- 
>> Robin,
>> which services all queues, so they do not stall. The weights were chosen
>> to follow the existing priority scheme: q0 gets the smallest weight, 
>> while
>> q1-4 get the bulk of the TX bandwidth.
>>
>> Fixes: 1c1008c793fa ("net: bcmgenet: add main driver file")
>> Signed-off-by: Ovidiu Panait <ovidiu.panait.rb@renesas.com>
>> ---
> 
> AFAIK the existing queue mechanism dates back to the STB QoS use cases 
> Florian
> had in mind when he wrote the driver, so let's hear what he has to say 
> on this.
> 
> The change itself looks correct to me. My concern is the targeting. This
> flips the default policy for every GENET user rather than fixing a specific
> defect, and Justin's series already called the persistent timeouts a design
> issue rather than a bug.
> 
> So isn't this more net-next material than net with a broad Fixes: tag?
> Please also add to the commit message that it drops the existing priority
> handling for rings 1-4.
> 

I'm ok with these changes. Internally we no longer require priority 
queues. It is a legacy use case we no longer have. My idea was to remove 
the queues entirely and have one big queue. But I figured I would wait 
for Nicolai's XDP changes so we do not need to remove and re-add queues 
for XDP. This could be a stop-gap solution until that is done.

Justin

>>  .../net/ethernet/broadcom/genet/bcmgenet.c    | 23 +++++++------------
>>  1 file changed, 8 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/ 
>> net/ethernet/broadcom/genet/bcmgenet.c
>> index 7c11cf916762..ad08c67269be 100644
>> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> @@ -40,9 +40,8 @@
>>
>>  #include "bcmgenet.h"
>>
>> -/* Default highest priority queue for multi queue support */
>> -#define GENET_Q1_PRIORITY    0
>> -#define GENET_Q0_PRIORITY    1
>> +#define GENET_Q0_WEIGHT        1
>> +#define GENET_Q1_WEIGHT        4
>>
>>  #define GENET_Q0_RX_BD_CNT    \
>>      (TOTAL_DESC - priv->hw_params->rx_queues * priv->hw_params- 
>> >rx_bds_per_q)
>> @@ -2129,13 +2128,6 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff 
>> *skb, struct net_device *dev)
>>      int i;
>>
>>      index = skb_get_queue_mapping(skb);
>> -    /* Mapping strategy:
>> -     * queue_mapping = 0, unclassified, packet xmited through ring 0
>> -     * queue_mapping = 1, goes to ring 1. (highest priority queue)
>> -     * queue_mapping = 2, goes to ring 2.
>> -     * queue_mapping = 3, goes to ring 3.
>> -     * queue_mapping = 4, goes to ring 4.
>> -     */
>>      ring = &priv->tx_rings[index];
>>      txq = netdev_get_tx_queue(dev, index);
>>
>> @@ -2881,8 +2873,9 @@ static int bcmgenet_rdma_disable(struct 
>> bcmgenet_priv *priv)
>>
>>  /* Initialize Tx queues
>>   *
>> - * Queues 1-4 are priority-based, each one has 32 descriptors,
>> - * with queue 1 being the highest priority queue.
>> + * Queues 1-4 are the priority queues, each one has 32 descriptors.
>> + * The weighted round-robin arbiter gives them a larger share of TX
>> + * bandwidth than the default queue 0.
>>   *
>>   * Queue 0 is the default Tx queue with
>>   * GENET_Q0_TX_BD_CNT = 256 - 4 * 32 = 128 descriptors.
>> @@ -2900,8 +2893,8 @@ static void bcmgenet_init_tx_queues(struct 
>> net_device *dev)
>>      unsigned int start = 0, end = GENET_Q0_TX_BD_CNT;
>>      u32 i, ring_mask, dma_priority[3] = {0, 0, 0};
>>
>> -    /* Enable strict priority arbiter mode */
>> -    bcmgenet_tdma_writel(priv, DMA_ARBITER_SP, DMA_ARB_CTRL);
>> +    /* Enable Weighted Round-Robin arbiter mode */
>> +    bcmgenet_tdma_writel(priv, DMA_ARBITER_WRR, DMA_ARB_CTRL);
>>
>>      /* Initialize Tx priority queues */
>>      for (i = 0; i <= priv->hw_params->tx_queues; i++) {
>> @@ -2909,7 +2902,7 @@ static void bcmgenet_init_tx_queues(struct 
>> net_device *dev)
>>          start = end;
>>          end += priv->hw_params->tx_bds_per_q;
>>          dma_priority[DMA_PRIO_REG_INDEX(i)] |=
>> -            (i ? GENET_Q1_PRIORITY : GENET_Q0_PRIORITY)
>> +            (i ? GENET_Q1_WEIGHT : GENET_Q0_WEIGHT)
>>              << DMA_PRIO_REG_SHIFT(i);
>>      }
> 
> [1] https://lore.kernel.org/netdev/20260406175756.134567-1- 
> justin.chen@broadcom.com/
> 
> Thanks
> Nicolai