| Message ID | 20200124235930.640-1-f.fainelli@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] net: systemport: Do not block interrupts in TX reclaim | expand |
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)
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!
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)
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(-)