[RFC] Re: Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses.

[Pavel Machek]

On Fri 2016-12-02 15:05:21, Lino Sanfilippo wrote:
> Hi,
> 
> 
> > 
> > There's nothing that protect stmmac_poll() from running concurently
> > with stmmac_dma_interrupt(), right?
> > 
> 
> could it be that there is also another issue concerned locking?:
> The tx completion handler takes the xmit_lock in case that the
> netif_queue is stopped. This is AFAICS unnecessary, since both
> xmit and completion handler are already synchronized by the private
> tx lock. But it is IMHO also dangerous:
> 
> In the xmit handler we have the locking order
> 1. xmit_lock
> 2. private tx lock
> 
> while in the completion handler its the reverse:
> 
> 1. private tx lock
> 2. xmit lock.
> 
> Do I miss something?

No, it seems you are right. Something like this?

Hmm. And can priv->tx_lock be removed, as we already rely on
netif_tx_lock?

(I copied the "lock already held" annotations from forcedeth. I hope
they are right....)

Best regards,
								Pavel


commit a6f21255dfc11fcadc5062dfd0c5f3d77ca4f634
Author: Pavel <pavel@xxxxxx>
Date:   Wed Dec 7 13:29:15 2016 +0100

    Reported-by: Lino Sanfilippo <LinoSanfilippo@xxxxxx>
    
    The tx completion handler takes the xmit_lock in case that the
    netif_queue is stopped. This is AFAICS unnecessary, since both
    xmit and completion handler are already synchronized by the private
    tx lock. But it is IMHO also dangerous:
    
    In the xmit handler we have the locking order
    1. xmit_lock
    2. private tx lock
    
    while in the completion handler its the reverse:
    
    1. private tx lock
    2. xmit lock.
    
    Signed-off-by: Pavel Machek <pavel@xxxxxxx>

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 982c952..5df9bb3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1380,14 +1380,9 @@ static void stmmac_tx_clean(struct stmmac_priv *priv)
 
 	if (unlikely(netif_queue_stopped(priv->dev) &&
 		     stmmac_tx_avail(priv) > STMMAC_TX_THRESH)) {
-		netif_tx_lock(priv->dev);
-		if (netif_queue_stopped(priv->dev) &&
-		    stmmac_tx_avail(priv) > STMMAC_TX_THRESH) {
-			netif_dbg(priv, tx_done, priv->dev,
-				  "%s: restart transmit\n", __func__);
-			netif_wake_queue(priv->dev);
-		}
-		netif_tx_unlock(priv->dev);
+		netif_dbg(priv, tx_done, priv->dev,
+			  "%s: restart transmit\n", __func__);
+		netif_wake_queue(priv->dev);
 	}
 
 	if ((priv->eee_enabled) && (!priv->tx_path_in_lpi_mode)) {
@@ -1630,7 +1625,9 @@ static void stmmac_tx_timer(unsigned long data)
 {
 	struct stmmac_priv *priv = (struct stmmac_priv *)data;
 
+	netif_tx_lock_bh(priv->dev);
 	stmmac_tx_clean(priv);
+	netif_tx_unlock_bh(priv->dev);
 }
 
 /**
@@ -1994,7 +1991,8 @@ static void stmmac_tso_allocator(struct stmmac_priv *priv, unsigned int des,
  *   --------
  *
  * mss is fixed when enable tso, so w/o programming the TDES3 ctx field.
- */
+ *
+ *  Called with netif_tx_lock held.                                                                 */
 static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	u32 pay_len, mss;
@@ -2174,6 +2172,7 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
  *  Description : this is the tx entry point of the driver.
  *  It programs the chain or the ring and supports oversized frames
  *  and SG feature.
+ *  Called with netif_tx_lock held. 
  */
 static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
 {
@@ -2684,7 +2683,9 @@ static int stmmac_poll(struct napi_struct *napi, int budget)
 	int work_done = 0;
 
 	priv->xstats.napi_poll++;
+	netif_tx_lock_bh(priv->dev);
 	stmmac_tx_clean(priv);
+	netif_tx_unlock_bh(priv->dev);
 
 	work_done = stmmac_rx(priv, budget);
 	if (work_done < budget) {
@@ -2701,6 +2702,7 @@ static int stmmac_poll(struct napi_struct *napi, int budget)
  *   complete within a reasonable time. The driver will mark the error in the
  *   netdev structure and arrange for the device to be reset to a sane state
  *   in order to transmit a new packet.
+ * Called with netif_tx_lock held.
  */
 static void stmmac_tx_timeout(struct net_device *dev)
 {


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Attachment: signature.asc
Description: Digital signature