* [PATCH net-next] bnx2x: Resolving a possible dead-lock situation
@ 2010-11-23 16:15 Vladislav Zolotarov
2010-11-23 16:29 ` Eric Dumazet
0 siblings, 1 reply; 6+ messages in thread
From: Vladislav Zolotarov @ 2010-11-23 16:15 UTC (permalink / raw)
To: Dave Miller; +Cc: Eilon Greenstein, netdev list
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;
}
--
1.7.0.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] bnx2x: Resolving a possible dead-lock situation
2010-11-23 16:15 [PATCH net-next] bnx2x: Resolving a possible dead-lock situation Vladislav Zolotarov
@ 2010-11-23 16:29 ` Eric Dumazet
2010-11-23 16:36 ` Vladislav Zolotarov
0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2010-11-23 16:29 UTC (permalink / raw)
To: Vladislav Zolotarov; +Cc: Dave Miller, Eilon Greenstein, netdev list
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;
^ permalink raw reply related [flat|nested] 6+ messages in thread
* RE: [PATCH net-next] bnx2x: Resolving a possible dead-lock situation
2010-11-23 16:29 ` Eric Dumazet
@ 2010-11-23 16:36 ` Vladislav Zolotarov
2010-11-23 16:55 ` Eric Dumazet
0 siblings, 1 reply; 6+ messages in thread
From: Vladislav Zolotarov @ 2010-11-23 16:36 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Dave Miller, Eilon Greenstein, netdev list
>
> 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?
Thanks,
vlad
>
> 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;
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH net-next] bnx2x: Resolving a possible dead-lock situation
2010-11-23 16:36 ` Vladislav Zolotarov
@ 2010-11-23 16:55 ` Eric Dumazet
2010-11-23 18:13 ` Vladislav Zolotarov
2010-11-23 18:15 ` Vladislav Zolotarov
0 siblings, 2 replies; 6+ messages in thread
From: Eric Dumazet @ 2010-11-23 16:55 UTC (permalink / raw)
To: Vladislav Zolotarov; +Cc: Dave Miller, Eilon Greenstein, netdev list
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 ;)
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH net-next] bnx2x: Resolving a possible dead-lock situation
2010-11-23 16:55 ` Eric Dumazet
@ 2010-11-23 18:13 ` Vladislav Zolotarov
2010-11-23 18:15 ` Vladislav Zolotarov
1 sibling, 0 replies; 6+ messages in thread
From: Vladislav Zolotarov @ 2010-11-23 18:13 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Dave Miller, Eilon Greenstein, netdev list
> 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...
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH net-next] bnx2x: Resolving a possible dead-lock situation
2010-11-23 16:55 ` Eric Dumazet
2010-11-23 18:13 ` Vladislav Zolotarov
@ 2010-11-23 18:15 ` Vladislav Zolotarov
1 sibling, 0 replies; 6+ messages in thread
From: Vladislav Zolotarov @ 2010-11-23 18:15 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Dave Miller, Eilon Greenstein, netdev list
>
> I was only suggesting an alternate patch, I am not able to test it ;)
I think I forgot to thank u for your suggestion... ;)
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-11-23 18:15 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-11-23 16:15 [PATCH net-next] bnx2x: Resolving a possible dead-lock situation Vladislav Zolotarov
2010-11-23 16:29 ` Eric Dumazet
2010-11-23 16:36 ` Vladislav Zolotarov
2010-11-23 16:55 ` Eric Dumazet
2010-11-23 18:13 ` Vladislav Zolotarov
2010-11-23 18:15 ` Vladislav Zolotarov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox