bnx2x: Fix statistics locking scheme
authorYuval Mintz <Yuval.Mintz@qlogic.com>
Mon, 23 Mar 2015 08:56:14 +0000 (10:56 +0200)
committerDavid S. Miller <davem@davemloft.net>
Tue, 24 Mar 2015 02:38:24 +0000 (22:38 -0400)
Statistics' state-machine in bnx2x driver must be synced with various driver
flows, but its current locking scheme manages to be wasteful [using 2 locks +
additional local variable] and prone to race-conditions at the same time,
as the state-machine and 'action' are being accessed under different locks.

In addition, current 'safe exec' isn't in fact safe, since the only guarantee
it gives is that DMA transactions are over, but ramrods might still be running.

This patch cleans up said logic, leaving us with a single lock for the entire
flow and removing the possible races.

Changes from v2:
- Switched into mutex locking from semaphore locking.
- Release locks on error flows.

Changes from v1:
Failure to acquire lock fails flow instead of printing a warning and
allowing access to the critical section.

Signed-off-by: Yuval Mintz <Yuval.Mintz@qlogic.com>
Signed-off-by: Ariel Elior <Ariel.Elior@qlogic.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c
drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c
drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.h

index 756053c028becff93c62cfcdbd6e1f635594cc01..4085c4b310470b6ebf718121fa423d2995dcb8ef 100644 (file)
@@ -1811,7 +1811,7 @@ struct bnx2x {
        int                     stats_state;
 
        /* used for synchronization of concurrent threads statistics handling */
-       spinlock_t              stats_lock;
+       struct mutex            stats_lock;
 
        /* used by dmae command loader */
        struct dmae_command     stats_dmae;
@@ -1935,8 +1935,6 @@ struct bnx2x {
 
        int fp_array_size;
        u32 dump_preset_idx;
-       bool                                    stats_started;
-       struct semaphore                        stats_sema;
 
        u8                                      phys_port_id[ETH_ALEN];
 
index 996e215fc3246c4fba6f1714ddceb69b88f179be..ae571a199f2fe82f7e8e1112479b12e8857955b5 100644 (file)
@@ -12037,9 +12037,8 @@ static int bnx2x_init_bp(struct bnx2x *bp)
        mutex_init(&bp->port.phy_mutex);
        mutex_init(&bp->fw_mb_mutex);
        mutex_init(&bp->drv_info_mutex);
+       mutex_init(&bp->stats_lock);
        bp->drv_info_mng_owner = false;
-       spin_lock_init(&bp->stats_lock);
-       sema_init(&bp->stats_sema, 1);
 
        INIT_DELAYED_WORK(&bp->sp_task, bnx2x_sp_task);
        INIT_DELAYED_WORK(&bp->sp_rtnl_task, bnx2x_sp_rtnl_task);
@@ -13668,9 +13667,9 @@ static int bnx2x_eeh_nic_unload(struct bnx2x *bp)
        cancel_delayed_work_sync(&bp->sp_task);
        cancel_delayed_work_sync(&bp->period_task);
 
-       spin_lock_bh(&bp->stats_lock);
+       mutex_lock(&bp->stats_lock);
        bp->stats_state = STATS_STATE_DISABLED;
-       spin_unlock_bh(&bp->stats_lock);
+       mutex_unlock(&bp->stats_lock);
 
        bnx2x_save_statistics(bp);
 
index e5aca2de1871350f3e47a788c4360b461c02a039..cfe3c7695455e16eab516e0f99db6ee6b7e58879 100644 (file)
@@ -2238,7 +2238,9 @@ int bnx2x_vf_close(struct bnx2x *bp, struct bnx2x_virtf *vf)
 
                cookie.vf = vf;
                cookie.state = VF_ACQUIRED;
-               bnx2x_stats_safe_exec(bp, bnx2x_set_vf_state, &cookie);
+               rc = bnx2x_stats_safe_exec(bp, bnx2x_set_vf_state, &cookie);
+               if (rc)
+                       goto op_err;
        }
 
        DP(BNX2X_MSG_IOV, "set state to acquired\n");
index d1608297c773746237a8faf3697277afcbe45918..800ab44a07cecd721c8b12f0c5b8602e1124a0dd 100644 (file)
@@ -123,36 +123,28 @@ static void bnx2x_dp_stats(struct bnx2x *bp)
  */
 static void bnx2x_storm_stats_post(struct bnx2x *bp)
 {
-       if (!bp->stats_pending) {
-               int rc;
+       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++);
+       if (bp->stats_pending)
+               return;
 
-               DP(BNX2X_MSG_STATS, "Sending statistics ramrod %d\n",
-                  le16_to_cpu(bp->fw_stats_req->hdr.drv_stats_counter));
+       bp->fw_stats_req->hdr.drv_stats_counter =
+               cpu_to_le16(bp->stats_counter++);
 
-               /* adjust the ramrod to include VF queues statistics */
-               bnx2x_iov_adjust_stats_req(bp);
-               bnx2x_dp_stats(bp);
+       DP(BNX2X_MSG_STATS, "Sending statistics ramrod %d\n",
+          le16_to_cpu(bp->fw_stats_req->hdr.drv_stats_counter));
 
-               /* 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)
-                       bp->stats_pending = 1;
+       /* adjust the ramrod to include VF queues statistics */
+       bnx2x_iov_adjust_stats_req(bp);
+       bnx2x_dp_stats(bp);
 
-               spin_unlock_bh(&bp->stats_lock);
-       }
+       /* 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)
+               bp->stats_pending = 1;
 }
 
 static void bnx2x_hw_stats_post(struct bnx2x *bp)
@@ -221,7 +213,7 @@ static void bnx2x_stats_comp(struct bnx2x *bp)
  */
 
 /* should be called under stats_sema */
-static void __bnx2x_stats_pmf_update(struct bnx2x *bp)
+static void bnx2x_stats_pmf_update(struct bnx2x *bp)
 {
        struct dmae_command *dmae;
        u32 opcode;
@@ -519,7 +511,7 @@ static void bnx2x_func_stats_init(struct bnx2x *bp)
 }
 
 /* should be called under stats_sema */
-static void __bnx2x_stats_start(struct bnx2x *bp)
+static void bnx2x_stats_start(struct bnx2x *bp)
 {
        if (IS_PF(bp)) {
                if (bp->port.pmf)
@@ -531,34 +523,13 @@ static void __bnx2x_stats_start(struct bnx2x *bp)
                bnx2x_hw_stats_post(bp);
                bnx2x_storm_stats_post(bp);
        }
-
-       bp->stats_started = true;
-}
-
-static void bnx2x_stats_start(struct bnx2x *bp)
-{
-       if (down_timeout(&bp->stats_sema, HZ/10))
-               BNX2X_ERR("Unable to acquire stats lock\n");
-       __bnx2x_stats_start(bp);
-       up(&bp->stats_sema);
 }
 
 static void bnx2x_stats_pmf_start(struct bnx2x *bp)
 {
-       if (down_timeout(&bp->stats_sema, HZ/10))
-               BNX2X_ERR("Unable to acquire stats lock\n");
        bnx2x_stats_comp(bp);
-       __bnx2x_stats_pmf_update(bp);
-       __bnx2x_stats_start(bp);
-       up(&bp->stats_sema);
-}
-
-static void bnx2x_stats_pmf_update(struct bnx2x *bp)
-{
-       if (down_timeout(&bp->stats_sema, HZ/10))
-               BNX2X_ERR("Unable to acquire stats lock\n");
-       __bnx2x_stats_pmf_update(bp);
-       up(&bp->stats_sema);
+       bnx2x_stats_pmf_update(bp);
+       bnx2x_stats_start(bp);
 }
 
 static void bnx2x_stats_restart(struct bnx2x *bp)
@@ -568,11 +539,9 @@ static void bnx2x_stats_restart(struct bnx2x *bp)
         */
        if (IS_VF(bp))
                return;
-       if (down_timeout(&bp->stats_sema, HZ/10))
-               BNX2X_ERR("Unable to acquire stats lock\n");
+
        bnx2x_stats_comp(bp);
-       __bnx2x_stats_start(bp);
-       up(&bp->stats_sema);
+       bnx2x_stats_start(bp);
 }
 
 static void bnx2x_bmac_stats_update(struct bnx2x *bp)
@@ -1246,18 +1215,12 @@ static void bnx2x_stats_update(struct bnx2x *bp)
 {
        u32 *stats_comp = bnx2x_sp(bp, stats_comp);
 
-       /* we run update from timer context, so give up
-        * if somebody is in the middle of transition
-        */
-       if (down_trylock(&bp->stats_sema))
+       if (bnx2x_edebug_stats_stopped(bp))
                return;
 
-       if (bnx2x_edebug_stats_stopped(bp) || !bp->stats_started)
-               goto out;
-
        if (IS_PF(bp)) {
                if (*stats_comp != DMAE_COMP_VAL)
-                       goto out;
+                       return;
 
                if (bp->port.pmf)
                        bnx2x_hw_stats_update(bp);
@@ -1267,7 +1230,7 @@ static void bnx2x_stats_update(struct bnx2x *bp)
                                BNX2X_ERR("storm stats were not updated for 3 times\n");
                                bnx2x_panic();
                        }
-                       goto out;
+                       return;
                }
        } else {
                /* vf doesn't collect HW statistics, and doesn't get completions
@@ -1281,7 +1244,7 @@ static void bnx2x_stats_update(struct bnx2x *bp)
 
        /* vf is done */
        if (IS_VF(bp))
-               goto out;
+               return;
 
        if (netif_msg_timer(bp)) {
                struct bnx2x_eth_stats *estats = &bp->eth_stats;
@@ -1292,9 +1255,6 @@ static void bnx2x_stats_update(struct bnx2x *bp)
 
        bnx2x_hw_stats_post(bp);
        bnx2x_storm_stats_post(bp);
-
-out:
-       up(&bp->stats_sema);
 }
 
 static void bnx2x_port_stats_stop(struct bnx2x *bp)
@@ -1358,12 +1318,7 @@ static void bnx2x_port_stats_stop(struct bnx2x *bp)
 
 static void bnx2x_stats_stop(struct bnx2x *bp)
 {
-       int update = 0;
-
-       if (down_timeout(&bp->stats_sema, HZ/10))
-               BNX2X_ERR("Unable to acquire stats lock\n");
-
-       bp->stats_started = false;
+       bool update = false;
 
        bnx2x_stats_comp(bp);
 
@@ -1381,8 +1336,6 @@ static void bnx2x_stats_stop(struct bnx2x *bp)
                bnx2x_hw_stats_post(bp);
                bnx2x_stats_comp(bp);
        }
-
-       up(&bp->stats_sema);
 }
 
 static void bnx2x_stats_do_nothing(struct bnx2x *bp)
@@ -1410,18 +1363,28 @@ static const struct {
 
 void bnx2x_stats_handle(struct bnx2x *bp, enum bnx2x_stats_event event)
 {
-       enum bnx2x_stats_state state;
-       void (*action)(struct bnx2x *bp);
+       enum bnx2x_stats_state state = bp->stats_state;
+
        if (unlikely(bp->panic))
                return;
 
-       spin_lock_bh(&bp->stats_lock);
-       state = bp->stats_state;
+       /* Statistics update run from timer context, and we don't want to stop
+        * that context in case someone is in the middle of a transition.
+        * For other events, wait a bit until lock is taken.
+        */
+       if (!mutex_trylock(&bp->stats_lock)) {
+               if (event == STATS_EVENT_UPDATE)
+                       return;
+
+               DP(BNX2X_MSG_STATS,
+                  "Unlikely stats' lock contention [event %d]\n", event);
+               mutex_lock(&bp->stats_lock);
+       }
+
+       bnx2x_stats_stm[state][event].action(bp);
        bp->stats_state = bnx2x_stats_stm[state][event].next_state;
-       action = bnx2x_stats_stm[state][event].action;
-       spin_unlock_bh(&bp->stats_lock);
 
-       action(bp);
+       mutex_unlock(&bp->stats_lock);
 
        if ((event != STATS_EVENT_UPDATE) || netif_msg_timer(bp))
                DP(BNX2X_MSG_STATS, "state %d -> event %d -> state %d\n",
@@ -1998,13 +1961,34 @@ void bnx2x_afex_collect_stats(struct bnx2x *bp, void *void_afex_stats,
        }
 }
 
-void bnx2x_stats_safe_exec(struct bnx2x *bp,
-                          void (func_to_exec)(void *cookie),
-                          void *cookie){
-       if (down_timeout(&bp->stats_sema, HZ/10))
-               BNX2X_ERR("Unable to acquire stats lock\n");
+int bnx2x_stats_safe_exec(struct bnx2x *bp,
+                         void (func_to_exec)(void *cookie),
+                         void *cookie)
+{
+       int cnt = 10, rc = 0;
+
+       /* Wait for statistics to end [while blocking further requests],
+        * then run supplied function 'safely'.
+        */
+       mutex_lock(&bp->stats_lock);
+
        bnx2x_stats_comp(bp);
+       while (bp->stats_pending && cnt--)
+               if (bnx2x_storm_stats_update(bp))
+                       usleep_range(1000, 2000);
+       if (bp->stats_pending) {
+               BNX2X_ERR("Failed to wait for stats pending to clear [possibly FW is stuck]\n");
+               rc = -EBUSY;
+               goto out;
+       }
+
        func_to_exec(cookie);
-       __bnx2x_stats_start(bp);
-       up(&bp->stats_sema);
+
+out:
+       /* No need to restart statistics - if they're enabled, the timer
+        * will restart the statistics.
+        */
+       mutex_unlock(&bp->stats_lock);
+
+       return rc;
 }
index 2beceaefdeea7aa5ac53f6a3028cd0fbcacbc51a..965539a9dabe7e4702e1ba6b382aefc69fb26fac 100644 (file)
@@ -539,9 +539,9 @@ struct bnx2x;
 void bnx2x_memset_stats(struct bnx2x *bp);
 void bnx2x_stats_init(struct bnx2x *bp);
 void bnx2x_stats_handle(struct bnx2x *bp, enum bnx2x_stats_event event);
-void bnx2x_stats_safe_exec(struct bnx2x *bp,
-                          void (func_to_exec)(void *cookie),
-                          void *cookie);
+int bnx2x_stats_safe_exec(struct bnx2x *bp,
+                         void (func_to_exec)(void *cookie),
+                         void *cookie);
 
 /**
  * bnx2x_save_statistics - save statistics when unloading.