diff mbox

[net-next] bnx2x: Resolving a possible dead-lock situation

Message ID 1290529777.3046.99.camel@edumazet-laptop
State Superseded, archived
Delegated to: David Miller
Headers show

Commit Message

Eric Dumazet Nov. 23, 2010, 4:29 p.m. UTC 
Le mardi 23 novembre 2010 à 18:15 +0200, Vladislav Zolotarov a écrit :
> There is a possible dead-lock situation between sch_direct_xmit() 
> (called from soft_IRQ context) and bnx2x_tx_int() when called from 
> an ethtool self-test flow (syscall context).
> 
> To prevent a dead-lock, disable bottom-halves on a local CPU when taking
> a tx_lock from bnx2x_tx_int() (use __netif_tx_lock_bh(txq)).
> 
> The flow in the bnx2x_tx_int() where tx_lock is taken should be hit
> very rarely thus performance penalty of this change should be minimal.
> 
> Signed-off-by: Vladislav Zolotarov <vladz@broadcom.com>
> Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
> ---
>  drivers/net/bnx2x/bnx2x_cmn.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/bnx2x/bnx2x_cmn.c b/drivers/net/bnx2x/bnx2x_cmn.c
> index 94d5f59..5189788 100644
> --- a/drivers/net/bnx2x/bnx2x_cmn.c
> +++ b/drivers/net/bnx2x/bnx2x_cmn.c
> @@ -144,14 +144,14 @@ int bnx2x_tx_int(struct bnx2x_fastpath *fp)
>  		 * stops the queue
>  		 */
>  
> -		__netif_tx_lock(txq, smp_processor_id());
> +		__netif_tx_lock_bh(txq);
>  
>  		if ((netif_tx_queue_stopped(txq)) &&
>  		    (bp->state == BNX2X_STATE_OPEN) &&
>  		    (bnx2x_tx_avail(fp) >= MAX_SKB_FRAGS + 3))
>  			netif_tx_wake_queue(txq);
>  
> -		__netif_tx_unlock(txq);
> +		__netif_tx_unlock_bh(txq);
>  	}
>  	return 0;
>  }

That seems strange. Even if performance penalty is not minimal, it
should be avoided.

If problem comes from ethtool, why not preventing BH in ethtool itself ?



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Eric Dumazet Nov. 23, 2010, 4:55 p.m. UTC | #1 
Le mardi 23 novembre 2010 à 08:36 -0800, Vladislav Zolotarov a écrit :
> > 
> > That seems strange. Even if performance penalty is not minimal, it
> > should be avoided.
> > 
> > If problem comes from ethtool, why not preventing BH in ethtool itself
> > ?
> 
> Looks good. Let me run a few checks before u submit a patch to Dave. Or do u prefer 
> me to push it on your behalf?

I was only suggesting an alternate patch, I am not able to test it ;)



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vladislav Zolotarov Nov. 23, 2010, 6:13 p.m. UTC | #2 
> I was only suggesting an alternate patch, I am not able to test it ;)


Sure ;) So, I'll test it and send a patch with u signed-off... 

> 

> 

>
diff mbox

Patch

diff --git a/drivers/net/bnx2x/bnx2x_ethtool.c b/drivers/net/bnx2x/bnx2x_ethtool.c
index d02ffbd..1f7d4e6 100644
--- a/drivers/net/bnx2x/bnx2x_ethtool.c
+++ b/drivers/net/bnx2x/bnx2x_ethtool.c
@@ -1499,9 +1499,11 @@  static int bnx2x_run_loopback(struct bnx2x *bp, int loopback_mode, u8 link_up)
 	 * updates that have been performed while interrupts were
 	 * disabled.
 	 */
-	if (bp->common.int_block == INT_BLOCK_IGU)
+	if (bp->common.int_block == INT_BLOCK_IGU) {
+		local_bh_disable();
 		bnx2x_tx_int(fp_tx);
-
+		local_bh_enable();
+	}
 	rx_idx = le16_to_cpu(*fp_rx->rx_cons_sb);
 	if (rx_idx != rx_start_idx + num_pkts)
 		goto test_loopback_exit;