Re: [PATCH net v2 4/4] net: bcmgenet: relax the xmit ring full case

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

On 4/1/26 12:47 AM, Nicolai Buchwitz wrote:
> On 1.4.2026 02:38, Justin Chen wrote:
>> We have a queue size of 32. If a packet is multiple fragments we can
>> run through this queue really quickly. Currently we stop the xmit
>> queue at SKB_FRAG_SIZE which by default can take up half the queue.
>> Instead lets just look at the incoming packet and freeze the queue
>> if the incoming packet has more fragments than free_bds. This will
>> relieve some of the queue timeouts we have been seeing.
>>
>> Fixes: 1c1008c793fa ("net: bcmgenet: add main driver file")
>> Signed-off-by: Justin Chen <justin.chen@broadcom.com>
>> ---
>>  drivers/net/ethernet/broadcom/genet/bcmgenet.c | 9 +++------
>>  1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/ 
>> net/ethernet/broadcom/genet/bcmgenet.c
>> index 54f71b1e85fc..a1aa1278842e 100644
>> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> @@ -2018,10 +2018,10 @@ static int bcmgenet_tx_poll(struct napi_struct 
>> *napi, int budget)
>>
>>      spin_lock(&ring->lock);
>>      work_done = __bcmgenet_tx_reclaim(ring->priv->dev, ring);
>> -    if (ring->free_bds > (MAX_SKB_FRAGS + 1)) {
>> -        txq = netdev_get_tx_queue(ring->priv->dev, ring->index);
>> +    txq = netdev_get_tx_queue(ring->priv->dev, ring->index);
>> +    if (netif_tx_queue_stopped(txq))
>>          netif_tx_wake_queue(txq);
>> -    }
>> +
>>      spin_unlock(&ring->lock);
>>
>>      if (work_done == 0) {
>> @@ -2224,9 +2224,6 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff 
>> *skb, struct net_device *dev)
>>
>>      netdev_tx_sent_queue(txq, GENET_CB(skb)->bytes_sent);
>>
>> -    if (ring->free_bds <= (MAX_SKB_FRAGS + 1))
>> -        netif_tx_stop_queue(txq);
>> -
> 
> With this removed, the only queue stop is the NETDEV_TX_BUSY path at
> entry. That means every ring-full hits BUSY and requeues rather than
> stopping proactively on the previous transmit.
> 
> Would it work to keep the proactive stop but base it on the actual
> packet size instead of MAX_SKB_FRAGS?
> 
>      if (ring->free_bds <= (nr_frags + 1))
>          netif_tx_stop_queue(txq);
> 
> That avoids reserving half the ring for the worst case while still
> preventing BUSY from becoming the normal path.
> 
>>      if (!netdev_xmit_more() || netif_xmit_stopped(txq))
>>          /* Packets are ready, update producer index */
>>          bcmgenet_tdma_ring_writel(priv, ring->index,
> 
> This wakes unconditionally even if only 1 BD was reclaimed. Combined
> with the above, it can create a tight stop/wake/BUSY thrash when the
> ring is near-full.
> 

That is fair. Let me drop this patch entirely. Seems like we are trading 
one problem for another problem. Maybe removing the priority queues 
entirely is the way to go. I will wait for the XDP patches to be applied 
first to avoid conflicts.

Thanks,
Justin

> Thanks
> 
> Nicolai