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

Eric Dumazet
2010-11-23T16:29:44Z

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 ?

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;