[PATCH net-next] net: systemport: Do not block interrupts in TX reclaim

[PATCH net-next] net: systemport: Do not block interrupts in TX reclaim

[Florian Fainelli]

There is no need to disable interrupts with a spin_lock_irqsave() in
bcm_sysport_tx_poll() since we are in softIRQ context already. Leave
interrupts enabled, thus giving a chance for the RX interrupts to be
processed.

This now makes bcm_sysport_tx_reclaim() equivalent to
bcm_sysport_tx_clean(), thus remove the former, and make
bcm_sysport_tx_reclaim_all() to use the latter.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/broadcom/bcmsysport.c | 30 ++++++----------------
 1 file changed, 8 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index f07ac0e0af59..dfff0657ce8f 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -925,26 +925,6 @@ static unsigned int __bcm_sysport_tx_reclaim(struct bcm_sysport_priv *priv,
 	return pkts_compl;
 }
 
-/* Locked version of the per-ring TX reclaim routine */
-static unsigned int bcm_sysport_tx_reclaim(struct bcm_sysport_priv *priv,
-					   struct bcm_sysport_tx_ring *ring)
-{
-	struct netdev_queue *txq;
-	unsigned int released;
-	unsigned long flags;
-
-	txq = netdev_get_tx_queue(priv->netdev, ring->index);
-
-	spin_lock_irqsave(&ring->lock, flags);
-	released = __bcm_sysport_tx_reclaim(priv, ring);
-	if (released)
-		netif_tx_wake_queue(txq);
-
-	spin_unlock_irqrestore(&ring->lock, flags);
-
-	return released;
-}
-
 /* Locked version of the per-ring TX reclaim, but does not wake the queue */
 static void bcm_sysport_tx_clean(struct bcm_sysport_priv *priv,
 				 struct bcm_sysport_tx_ring *ring)
@@ -960,9 +940,15 @@ static int bcm_sysport_tx_poll(struct napi_struct *napi, int budget)
 {
 	struct bcm_sysport_tx_ring *ring =
 		container_of(napi, struct bcm_sysport_tx_ring, napi);
+	struct bcm_sysport_priv *priv = ring->priv;
 	unsigned int work_done = 0;
 
-	work_done = bcm_sysport_tx_reclaim(ring->priv, ring);
+	spin_lock(&ring->lock);
+	work_done = __bcm_sysport_tx_reclaim(priv, ring);
+	if (work_done)
+		netif_tx_wake_queue(netdev_get_tx_queue(priv->netdev,
+							ring->index));
+	spin_unlock(&ring->lock);
 
 	if (work_done == 0) {
 		napi_complete(napi);
@@ -984,7 +970,7 @@ static void bcm_sysport_tx_reclaim_all(struct bcm_sysport_priv *priv)
 	unsigned int q;
 
 	for (q = 0; q < priv->netdev->num_tx_queues; q++)
-		bcm_sysport_tx_reclaim(priv, &priv->tx_rings[q]);
+		bcm_sysport_tx_clean(priv, &priv->tx_rings[q]);
 }
 
 static int bcm_sysport_poll(struct napi_struct *napi, int budget)
-- 
2.17.1

Re: [PATCH net-next] net: systemport: Do not block interrupts in TX reclaim

[Eric Dumazet]

On 1/24/20 3:59 PM, Florian Fainelli wrote:
> There is no need to disable interrupts with a spin_lock_irqsave() in
> bcm_sysport_tx_poll() since we are in softIRQ context already. Leave
> interrupts enabled, thus giving a chance for the RX interrupts to be
> processed.
> 
> This now makes bcm_sysport_tx_reclaim() equivalent to
> bcm_sysport_tx_clean(), thus remove the former, and make
> bcm_sysport_tx_reclaim_all() to use the latter.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/net/ethernet/broadcom/bcmsysport.c | 30 ++++++----------------
>  1 file changed, 8 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
> index f07ac0e0af59..dfff0657ce8f 100644
> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
> @@ -925,26 +925,6 @@ static unsigned int __bcm_sysport_tx_reclaim(struct bcm_sysport_priv *priv,
>  	return pkts_compl;
>  }
>  
> -/* Locked version of the per-ring TX reclaim routine */
> -static unsigned int bcm_sysport_tx_reclaim(struct bcm_sysport_priv *priv,
> -					   struct bcm_sysport_tx_ring *ring)
> -{
> -	struct netdev_queue *txq;
> -	unsigned int released;
> -	unsigned long flags;
> -
> -	txq = netdev_get_tx_queue(priv->netdev, ring->index);
> -
> -	spin_lock_irqsave(&ring->lock, flags);
> -	released = __bcm_sysport_tx_reclaim(priv, ring);
> -	if (released)
> -		netif_tx_wake_queue(txq);
> -
> -	spin_unlock_irqrestore(&ring->lock, flags);
> -
> -	return released;
> -}
> -
>  /* Locked version of the per-ring TX reclaim, but does not wake the queue */
>  static void bcm_sysport_tx_clean(struct bcm_sysport_priv *priv,
>  				 struct bcm_sysport_tx_ring *ring)
> @@ -960,9 +940,15 @@ static int bcm_sysport_tx_poll(struct napi_struct *napi, int budget)
>  {
>  	struct bcm_sysport_tx_ring *ring =
>  		container_of(napi, struct bcm_sysport_tx_ring, napi);
> +	struct bcm_sysport_priv *priv = ring->priv;
>  	unsigned int work_done = 0;
>  
> -	work_done = bcm_sysport_tx_reclaim(ring->priv, ring);
> +	spin_lock(&ring->lock);
> +	work_done = __bcm_sysport_tx_reclaim(priv, ring);
> +	if (work_done)
> +		netif_tx_wake_queue(netdev_get_tx_queue(priv->netdev,
> +							ring->index));
> +	spin_unlock(&ring->lock);
>  
>  	if (work_done == 0) {
>  		napi_complete(napi);
> @@ -984,7 +970,7 @@ static void bcm_sysport_tx_reclaim_all(struct bcm_sysport_priv *priv)
>  	unsigned int q;
>  
>  	for (q = 0; q < priv->netdev->num_tx_queues; q++)
> -		bcm_sysport_tx_reclaim(priv, &priv->tx_rings[q]);
> +		bcm_sysport_tx_clean(priv, &priv->tx_rings[q]);
>  }
>  
>  static int bcm_sysport_poll(struct napi_struct *napi, int budget)
> 

I am a bit confused by this patch, the changelog mixes hard and soft irqs.

This driver seems to call bcm_sysport_tx_reclaim_all() from hard irq handler 
(INTRL2_0_TX_RING_FULL condition)

So it looks you need to acquire ring->lock with some _irqsave() variant when
bcm_sysport_tx_poll() is running (from BH context)

Re: [PATCH net-next] net: systemport: Do not block interrupts in TX reclaim

[Florian Fainelli]

On 1/24/2020 5:35 PM, Eric Dumazet wrote:
> 
> 
> On 1/24/20 3:59 PM, Florian Fainelli wrote:
>> There is no need to disable interrupts with a spin_lock_irqsave() in
>> bcm_sysport_tx_poll() since we are in softIRQ context already. Leave
>> interrupts enabled, thus giving a chance for the RX interrupts to be
>> processed.
>>
>> This now makes bcm_sysport_tx_reclaim() equivalent to
>> bcm_sysport_tx_clean(), thus remove the former, and make
>> bcm_sysport_tx_reclaim_all() to use the latter.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>  drivers/net/ethernet/broadcom/bcmsysport.c | 30 ++++++----------------
>>  1 file changed, 8 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
>> index f07ac0e0af59..dfff0657ce8f 100644
>> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
>> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
>> @@ -925,26 +925,6 @@ static unsigned int __bcm_sysport_tx_reclaim(struct bcm_sysport_priv *priv,
>>  	return pkts_compl;
>>  }
>>  
>> -/* Locked version of the per-ring TX reclaim routine */
>> -static unsigned int bcm_sysport_tx_reclaim(struct bcm_sysport_priv *priv,
>> -					   struct bcm_sysport_tx_ring *ring)
>> -{
>> -	struct netdev_queue *txq;
>> -	unsigned int released;
>> -	unsigned long flags;
>> -
>> -	txq = netdev_get_tx_queue(priv->netdev, ring->index);
>> -
>> -	spin_lock_irqsave(&ring->lock, flags);
>> -	released = __bcm_sysport_tx_reclaim(priv, ring);
>> -	if (released)
>> -		netif_tx_wake_queue(txq);
>> -
>> -	spin_unlock_irqrestore(&ring->lock, flags);
>> -
>> -	return released;
>> -}
>> -
>>  /* Locked version of the per-ring TX reclaim, but does not wake the queue */
>>  static void bcm_sysport_tx_clean(struct bcm_sysport_priv *priv,
>>  				 struct bcm_sysport_tx_ring *ring)
>> @@ -960,9 +940,15 @@ static int bcm_sysport_tx_poll(struct napi_struct *napi, int budget)
>>  {
>>  	struct bcm_sysport_tx_ring *ring =
>>  		container_of(napi, struct bcm_sysport_tx_ring, napi);
>> +	struct bcm_sysport_priv *priv = ring->priv;
>>  	unsigned int work_done = 0;
>>  
>> -	work_done = bcm_sysport_tx_reclaim(ring->priv, ring);
>> +	spin_lock(&ring->lock);
>> +	work_done = __bcm_sysport_tx_reclaim(priv, ring);
>> +	if (work_done)
>> +		netif_tx_wake_queue(netdev_get_tx_queue(priv->netdev,
>> +							ring->index));
>> +	spin_unlock(&ring->lock);
>>  
>>  	if (work_done == 0) {
>>  		napi_complete(napi);
>> @@ -984,7 +970,7 @@ static void bcm_sysport_tx_reclaim_all(struct bcm_sysport_priv *priv)
>>  	unsigned int q;
>>  
>>  	for (q = 0; q < priv->netdev->num_tx_queues; q++)
>> -		bcm_sysport_tx_reclaim(priv, &priv->tx_rings[q]);
>> +		bcm_sysport_tx_clean(priv, &priv->tx_rings[q]);
>>  }
>>  
>>  static int bcm_sysport_poll(struct napi_struct *napi, int budget)
>>
> 
> I am a bit confused by this patch, the changelog mixes hard and soft irqs.
> 
> This driver seems to call bcm_sysport_tx_reclaim_all() from hard irq handler 
> (INTRL2_0_TX_RING_FULL condition)
> 
> So it looks you need to acquire ring->lock with some _irqsave() variant when
> bcm_sysport_tx_poll() is running (from BH context)

You are right, I completely missed that path and the very reason why
this is spin_lock_irqsave() in the first place... time to get some sleep.

Thanks!
-- 
Florian