* [PATCH net] bnx2x: guard stats_pending with stats_lock in all cases
@ 2013-08-12 19:08 Neal Cardwell
2013-08-12 20:52 ` Dmitry Kravkov
0 siblings, 1 reply; 3+ messages in thread
From: Neal Cardwell @ 2013-08-12 19:08 UTC (permalink / raw)
To: David Miller
Cc: netdev, Neal Cardwell, Eilon Greenstein, Vladislav Zolotarov,
Yaniv Rosner, Merav Sicron, Eric Dumazet, Tom Herbert,
Havard Skinnemoen, Sanjay Hortikar
As 9bcb8018cf ("bnx2x: Fix the race on bp->stats_pending") noted, we
can have a race on bp->stats_pending between the timer and a
STATS_EVENT_LINK_UP event handler.
But it seems we can also have the following two races on
bp->stats_pending between the timer and a STATS_EVENT_STOP handler:
Scenario A:
-----------
thread 1: bnx2x_timer() thread in bnx2x_storm_stats_post():
/* send FW stats ramrod */
rc = bnx2x_sp_post(bp, RAMROD_CMD_ID_COMMON_STAT_QUERY, 0,
U64_HI(bp->fw_stats_req_mapping),
U64_LO(bp->fw_stats_req_mapping),
NONE_CONNECTION_TYPE);
if (rc == 0)
thread 2: ethtool thread:
bnx2x_stats_handle(bp, STATS_EVENT_STOP)
-> bnx2x_stats_stop()
-> bnx2x_storm_stats_update()
bp->stats_pending = 0;
thread 1: bnx2x_timer() thread in bnx2x_storm_stats_post():
bp->stats_pending = 1;
If the action interleaves in this way we end up with a
bp->stats_pending value of 1 when there are no stats pending.
Scenario B:
-----------
thread 1: bnx2x_timer() thread in bnx2x_stats_update():
if (bnx2x_storm_stats_update(bp)) {
if (bp->stats_pending == 3) { // false
reg1 = bp->stats_pending // reg1=1
thread 2: ethtool thread:
bnx2x_stats_handle(bp, STATS_EVENT_STOP)
-> bnx2x_stats_stop()
-> bnx2x_storm_stats_update()
bp->stats_pending = 0;
thread 1: bnx2x_timer() thread in bnx2x_stats_update():
reg1++ // reg1=2
bp->stats_pending = reg1; // stats_pending=2
return;
If the action interleaves in this way we end up with a non-zero
bp->stats_pending value when there are no stats pending.
-----------
These are suspected as possible contributors to occasional
bnx2x_panic() dumps we see of the form:
storm stats were not updated for 3 times
When these panic messages happen we see the NIC hang.
The proposed fix is for all, not just some, of the reads and writes to
stats_pending to be guarded by stats_lock. So now all accesses to
stats_pending and stats_couner are guarded by stats_lock.
Signed-off-by: Neal Cardwell <ncardwell@google.com>
Cc: Eilon Greenstein <eilong@broadcom.com>
Cc: Vladislav Zolotarov <vladz@broadcom.com>
Cc: Yaniv Rosner <yanivr@broadcom.com>
Cc: Merav Sicron <meravs@broadcom.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Tom Herbert <therbert@google.com>
Cc: Havard Skinnemoen <hskinnemoen@google.com>
Cc: Sanjay Hortikar <horti@google.com>
Change-Id: Ic389c4355ef4a3e01da8f1f4c2019e75dbb5ddf6
---
drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c | 47 +++++++++++++----------
1 file changed, 27 insertions(+), 20 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c
index 98366ab..d72a630 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c
@@ -123,16 +123,11 @@ static void bnx2x_dp_stats(struct bnx2x *bp)
*/
static void bnx2x_storm_stats_post(struct bnx2x *bp)
{
+ spin_lock_bh(&bp->stats_lock);
+
if (!bp->stats_pending) {
int rc;
- spin_lock_bh(&bp->stats_lock);
-
- if (bp->stats_pending) {
- spin_unlock_bh(&bp->stats_lock);
- return;
- }
-
bp->fw_stats_req->hdr.drv_stats_counter =
cpu_to_le16(bp->stats_counter++);
@@ -151,8 +146,9 @@ static void bnx2x_storm_stats_post(struct bnx2x *bp)
if (rc == 0)
bp->stats_pending = 1;
- spin_unlock_bh(&bp->stats_lock);
}
+
+ spin_unlock_bh(&bp->stats_lock);
}
static void bnx2x_hw_stats_post(struct bnx2x *bp)
@@ -888,9 +884,7 @@ static int bnx2x_storm_stats_validate_counters(struct bnx2x *bp)
/* Make sure we use the value of the counter
* used for sending the last stats ramrod.
*/
- spin_lock_bh(&bp->stats_lock);
cur_stats_counter = bp->stats_counter - 1;
- spin_unlock_bh(&bp->stats_lock);
/* are storm stats valid? */
if (le16_to_cpu(counters->xstats_counter) != cur_stats_counter) {
@@ -923,7 +917,8 @@ static int bnx2x_storm_stats_validate_counters(struct bnx2x *bp)
return 0;
}
-static int bnx2x_storm_stats_update(struct bnx2x *bp)
+static int bnx2x_storm_stats_update(struct bnx2x *bp,
+ bool track_pending)
{
struct tstorm_per_port_stats *tport =
&bp->fw_stats_data->port.tstorm_port_statistics;
@@ -934,9 +929,24 @@ static int bnx2x_storm_stats_update(struct bnx2x *bp)
struct bnx2x_eth_stats_old *estats_old = &bp->eth_stats_old;
int i;
+ spin_lock_bh(&bp->stats_lock);
+
/* vfs stat counter is managed by pf */
- if (IS_PF(bp) && bnx2x_storm_stats_validate_counters(bp))
+ if (IS_PF(bp) && bnx2x_storm_stats_validate_counters(bp)) {
+ bool pending_timeout = false;
+
+ if (track_pending && (bp->stats_pending++ == 3))
+ pending_timeout = true;
+
+ spin_unlock_bh(&bp->stats_lock);
+
+ if (pending_timeout) {
+ BNX2X_ERR("storm stats were not updated for 3 times\n");
+ bnx2x_panic();
+ }
+
return -EAGAIN;
+ }
estats->error_bytes_received_hi = 0;
estats->error_bytes_received_lo = 0;
@@ -1118,6 +1128,8 @@ static int bnx2x_storm_stats_update(struct bnx2x *bp)
bp->stats_pending = 0;
+ spin_unlock_bh(&bp->stats_lock);
+
return 0;
}
@@ -1237,18 +1249,13 @@ static void bnx2x_stats_update(struct bnx2x *bp)
if (bp->port.pmf)
bnx2x_hw_stats_update(bp);
- if (bnx2x_storm_stats_update(bp)) {
- if (bp->stats_pending++ == 3) {
- BNX2X_ERR("storm stats were not updated for 3 times\n");
- bnx2x_panic();
- }
+ if (bnx2x_storm_stats_update(bp, true))
return;
- }
} else {
/* vf doesn't collect HW statistics, and doesn't get completions
* perform only update
*/
- bnx2x_storm_stats_update(bp);
+ bnx2x_storm_stats_update(bp, false);
}
bnx2x_net_stats_update(bp);
@@ -1337,7 +1344,7 @@ static void bnx2x_stats_stop(struct bnx2x *bp)
if (bp->port.pmf)
update = (bnx2x_hw_stats_update(bp) == 0);
- update |= (bnx2x_storm_stats_update(bp) == 0);
+ update |= (bnx2x_storm_stats_update(bp, false) == 0);
if (update) {
bnx2x_net_stats_update(bp);
--
1.8.3
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH net] bnx2x: guard stats_pending with stats_lock in all cases
2013-08-12 19:08 [PATCH net] bnx2x: guard stats_pending with stats_lock in all cases Neal Cardwell
@ 2013-08-12 20:52 ` Dmitry Kravkov
2013-08-13 17:57 ` Neal Cardwell
0 siblings, 1 reply; 3+ messages in thread
From: Dmitry Kravkov @ 2013-08-12 20:52 UTC (permalink / raw)
To: Neal Cardwell
Cc: David Miller, netdev@vger.kernel.org, Eilon Greenstein,
Vladislav Zolotarov, Yaniv Rosner, Merav Sicron, Eric Dumazet,
Tom Herbert, Havard Skinnemoen, Sanjay Hortikar, Dmitry Kravkov
On Mon, Aug 12, 2013 at 10:08 PM, Neal Cardwell <ncardwell@google.com> wrote:
>
> As 9bcb8018cf ("bnx2x: Fix the race on bp->stats_pending") noted, we
> can have a race on bp->stats_pending between the timer and a
> STATS_EVENT_LINK_UP event handler.
>
> But it seems we can also have the following two races on
> bp->stats_pending between the timer and a STATS_EVENT_STOP handler:
>
> Scenario A:
> -----------
>
> thread 1: bnx2x_timer() thread in bnx2x_storm_stats_post():
> /* send FW stats ramrod */
> rc = bnx2x_sp_post(bp, RAMROD_CMD_ID_COMMON_STAT_QUERY, 0,
> U64_HI(bp->fw_stats_req_mapping),
> U64_LO(bp->fw_stats_req_mapping),
> NONE_CONNECTION_TYPE);
> if (rc == 0)
>
> thread 2: ethtool thread:
> bnx2x_stats_handle(bp, STATS_EVENT_STOP)
> -> bnx2x_stats_stop()
> -> bnx2x_storm_stats_update()
> bp->stats_pending = 0;
>
> thread 1: bnx2x_timer() thread in bnx2x_storm_stats_post():
> bp->stats_pending = 1;
>
> If the action interleaves in this way we end up with a
> bp->stats_pending value of 1 when there are no stats pending.
>
> Scenario B:
> -----------
> thread 1: bnx2x_timer() thread in bnx2x_stats_update():
> if (bnx2x_storm_stats_update(bp)) {
> if (bp->stats_pending == 3) { // false
> reg1 = bp->stats_pending // reg1=1
>
> thread 2: ethtool thread:
> bnx2x_stats_handle(bp, STATS_EVENT_STOP)
> -> bnx2x_stats_stop()
> -> bnx2x_storm_stats_update()
> bp->stats_pending = 0;
>
> thread 1: bnx2x_timer() thread in bnx2x_stats_update():
> reg1++ // reg1=2
> bp->stats_pending = reg1; // stats_pending=2
> return;
>
> If the action interleaves in this way we end up with a non-zero
> bp->stats_pending value when there are no stats pending.
>
> -----------
>
> These are suspected as possible contributors to occasional
> bnx2x_panic() dumps we see of the form:
>
> storm stats were not updated for 3 times
>
> When these panic messages happen we see the NIC hang.
>
> The proposed fix is for all, not just some, of the reads and writes to
> stats_pending to be guarded by stats_lock. So now all accesses to
> stats_pending and stats_couner are guarded by stats_lock.
>
Neal, thanks for looking into this.
There's another race, where two flows
stats_update() and stats_start() may send two queries in parallel
which will cause FW to assert.
I'm preparing the set for net, which includes the fix for this race
also(will be out in a couple of hours)
I will be more than happy if you will take a look.
Thanks
Dmitry
> Signed-off-by: Neal Cardwell <ncardwell@google.com>
> Cc: Eilon Greenstein <eilong@broadcom.com>
> Cc: Vladislav Zolotarov <vladz@broadcom.com>
> Cc: Yaniv Rosner <yanivr@broadcom.com>
> Cc: Merav Sicron <meravs@broadcom.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Tom Herbert <therbert@google.com>
> Cc: Havard Skinnemoen <hskinnemoen@google.com>
> Cc: Sanjay Hortikar <horti@google.com>
> Change-Id: Ic389c4355ef4a3e01da8f1f4c2019e75dbb5ddf6
> ---
> drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c | 47 +++++++++++++----------
> 1 file changed, 27 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c
> index 98366ab..d72a630 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c
> @@ -123,16 +123,11 @@ static void bnx2x_dp_stats(struct bnx2x *bp)
> */
> static void bnx2x_storm_stats_post(struct bnx2x *bp)
> {
> + spin_lock_bh(&bp->stats_lock);
> +
> if (!bp->stats_pending) {
> int rc;
>
> - spin_lock_bh(&bp->stats_lock);
> -
> - if (bp->stats_pending) {
> - spin_unlock_bh(&bp->stats_lock);
> - return;
> - }
> -
> bp->fw_stats_req->hdr.drv_stats_counter =
> cpu_to_le16(bp->stats_counter++);
>
> @@ -151,8 +146,9 @@ static void bnx2x_storm_stats_post(struct bnx2x *bp)
> if (rc == 0)
> bp->stats_pending = 1;
>
> - spin_unlock_bh(&bp->stats_lock);
> }
> +
> + spin_unlock_bh(&bp->stats_lock);
> }
>
> static void bnx2x_hw_stats_post(struct bnx2x *bp)
> @@ -888,9 +884,7 @@ static int bnx2x_storm_stats_validate_counters(struct bnx2x *bp)
> /* Make sure we use the value of the counter
> * used for sending the last stats ramrod.
> */
> - spin_lock_bh(&bp->stats_lock);
> cur_stats_counter = bp->stats_counter - 1;
> - spin_unlock_bh(&bp->stats_lock);
>
> /* are storm stats valid? */
> if (le16_to_cpu(counters->xstats_counter) != cur_stats_counter) {
> @@ -923,7 +917,8 @@ static int bnx2x_storm_stats_validate_counters(struct bnx2x *bp)
> return 0;
> }
>
> -static int bnx2x_storm_stats_update(struct bnx2x *bp)
> +static int bnx2x_storm_stats_update(struct bnx2x *bp,
> + bool track_pending)
> {
> struct tstorm_per_port_stats *tport =
> &bp->fw_stats_data->port.tstorm_port_statistics;
> @@ -934,9 +929,24 @@ static int bnx2x_storm_stats_update(struct bnx2x *bp)
> struct bnx2x_eth_stats_old *estats_old = &bp->eth_stats_old;
> int i;
>
> + spin_lock_bh(&bp->stats_lock);
> +
> /* vfs stat counter is managed by pf */
> - if (IS_PF(bp) && bnx2x_storm_stats_validate_counters(bp))
> + if (IS_PF(bp) && bnx2x_storm_stats_validate_counters(bp)) {
> + bool pending_timeout = false;
> +
> + if (track_pending && (bp->stats_pending++ == 3))
> + pending_timeout = true;
> +
> + spin_unlock_bh(&bp->stats_lock);
> +
> + if (pending_timeout) {
> + BNX2X_ERR("storm stats were not updated for 3 times\n");
> + bnx2x_panic();
> + }
> +
> return -EAGAIN;
> + }
>
> estats->error_bytes_received_hi = 0;
> estats->error_bytes_received_lo = 0;
> @@ -1118,6 +1128,8 @@ static int bnx2x_storm_stats_update(struct bnx2x *bp)
>
> bp->stats_pending = 0;
>
> + spin_unlock_bh(&bp->stats_lock);
> +
> return 0;
> }
>
> @@ -1237,18 +1249,13 @@ static void bnx2x_stats_update(struct bnx2x *bp)
> if (bp->port.pmf)
> bnx2x_hw_stats_update(bp);
>
> - if (bnx2x_storm_stats_update(bp)) {
> - if (bp->stats_pending++ == 3) {
> - BNX2X_ERR("storm stats were not updated for 3 times\n");
> - bnx2x_panic();
> - }
> + if (bnx2x_storm_stats_update(bp, true))
> return;
> - }
> } else {
> /* vf doesn't collect HW statistics, and doesn't get completions
> * perform only update
> */
> - bnx2x_storm_stats_update(bp);
> + bnx2x_storm_stats_update(bp, false);
> }
>
> bnx2x_net_stats_update(bp);
> @@ -1337,7 +1344,7 @@ static void bnx2x_stats_stop(struct bnx2x *bp)
> if (bp->port.pmf)
> update = (bnx2x_hw_stats_update(bp) == 0);
>
> - update |= (bnx2x_storm_stats_update(bp) == 0);
> + update |= (bnx2x_storm_stats_update(bp, false) == 0);
>
> if (update) {
> bnx2x_net_stats_update(bp);
> --
> 1.8.3
>
> --
> 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
--
Best regards,
Dmitry
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH net] bnx2x: guard stats_pending with stats_lock in all cases
2013-08-12 20:52 ` Dmitry Kravkov
@ 2013-08-13 17:57 ` Neal Cardwell
0 siblings, 0 replies; 3+ messages in thread
From: Neal Cardwell @ 2013-08-13 17:57 UTC (permalink / raw)
To: Dmitry Kravkov
Cc: David Miller, netdev@vger.kernel.org, Eilon Greenstein,
Vladislav Zolotarov, Yaniv Rosner, Merav Sicron, Eric Dumazet,
Tom Herbert, Havard Skinnemoen, Sanjay Hortikar, Dmitry Kravkov
On Mon, Aug 12, 2013 at 4:52 PM, Dmitry Kravkov <dkravkov@gmail.com> wrote:
> Neal, thanks for looking into this.
>
> There's another race, where two flows
> stats_update() and stats_start() may send two queries in parallel
> which will cause FW to assert.
>
> I'm preparing the set for net, which includes the fix for this race
> also(will be out in a couple of hours)
>
> I will be more than happy if you will take a look.
>
> Thanks
> Dmitry
Thanks! Your "bnx2x: protect different statistics flows" patch looks
like a superset of the synchronization I was proposing in this patch,
so we can drop mine.
Thanks,
neal
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-08-13 17:57 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-08-12 19:08 [PATCH net] bnx2x: guard stats_pending with stats_lock in all cases Neal Cardwell
2013-08-12 20:52 ` Dmitry Kravkov
2013-08-13 17:57 ` Neal Cardwell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox