| Message ID | 56014CC0.3050201@huawei.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Weidong Wang <wangweidong1@huawei.com> Date: Tue, 22 Sep 2015 20:42:40 +0800 > @@ -880,6 +882,7 @@ bnx2_alloc_mem(struct bnx2 *bp) > } > } > > + spin_lock(&bp->stats64_lock); > bp->stats_blk = status_blk + status_blk_size; > > bp->stats_blk_mapping = bp->status_blk_mapping + status_blk_size; > @@ -894,20 +897,23 @@ bnx2_alloc_mem(struct bnx2 *bp) > &bp->ctx_blk_mapping[i], > GFP_KERNEL); > if (bp->ctx_blk[i] == NULL) > - goto alloc_mem_err; > + goto free_stats64_lock; > } > } > > err = bnx2_alloc_rx_mem(bp); > if (err) > - goto alloc_mem_err; > + goto free_stats64_lock; You're holding a spinlock while doing GFP_KERNEL allocations. Second of all, taking a spinlock in get_stats64() defeats the whole intention of making statistics acquisition as fast and as SMP scalable as possible. -- 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
On 2015/9/24 6:31, David Miller wrote: > From: Weidong Wang <wangweidong1@huawei.com> > Date: Tue, 22 Sep 2015 20:42:40 +0800 > >> @@ -880,6 +882,7 @@ bnx2_alloc_mem(struct bnx2 *bp) >> } >> } >> >> + spin_lock(&bp->stats64_lock); >> bp->stats_blk = status_blk + status_blk_size; >> >> bp->stats_blk_mapping = bp->status_blk_mapping + status_blk_size; >> @@ -894,20 +897,23 @@ bnx2_alloc_mem(struct bnx2 *bp) >> &bp->ctx_blk_mapping[i], >> GFP_KERNEL); >> if (bp->ctx_blk[i] == NULL) >> - goto alloc_mem_err; >> + goto free_stats64_lock; >> } >> } >> >> err = bnx2_alloc_rx_mem(bp); >> if (err) >> - goto alloc_mem_err; >> + goto free_stats64_lock; > > You're holding a spinlock while doing GFP_KERNEL allocations. > hm, yep, I should move it after the allocations. Like this: @@ -880,7 +882,9 @@ bnx2_alloc_mem(struct bnx2 *bp) } } + spin_lock(&bp->stats64_lock); bp->stats_blk = status_blk + status_blk_size; + spin_unlock(&bp->stats64_lock); the allocations won't use the stats_blk, so I shouldn't hold the lock while doing allocations. > Second of all, taking a spinlock in get_stats64() defeats the whole > intention of making statistics acquisition as fast and as SMP scalable > as possible. > It does affect the intention. Although, the problem exists then makes the system panic within some case. Do you have any idea about it? Best Regards, Weidong > . > -- 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
From: Weidong Wang <wangweidong1@huawei.com> Date: Thu, 24 Sep 2015 10:00:45 +0800 > It does affect the intention. Although, the problem exists then makes the > system panic within some case. > > Do you have any idea about it? Allocate the statistics block at probe time so that this problem is impossible. -- 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
On 2015/9/24 13:34, David Miller wrote: > From: Weidong Wang <wangweidong1@huawei.com> > Date: Thu, 24 Sep 2015 10:00:45 +0800 > >> It does affect the intention. Although, the problem exists then makes the >> system panic within some case. >> >> Do you have any idea about it? > > Allocate the statistics block at probe time so that this problem is > impossible. > It is a good idea. Yet, what is the intention of the dynamic to alloc/free stats_block? what will be affected by allocating the statistics block. Best Regards, Weidong > -- 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
diff --git a/drivers/net/ethernet/broadcom/bnx2.c b/drivers/net/ethernet/broadcom/bnx2.c index 2b66ef3..aec4081 100644 --- a/drivers/net/ethernet/broadcom/bnx2.c +++ b/drivers/net/ethernet/broadcom/bnx2.c @@ -830,11 +830,13 @@ bnx2_free_mem(struct bnx2 *bp) } } if (bnapi->status_blk.msi) { + spin_lock(&bp->stats64_lock); dma_free_coherent(&bp->pdev->dev, bp->status_stats_size, bnapi->status_blk.msi, bp->status_blk_mapping); bnapi->status_blk.msi = NULL; bp->stats_blk = NULL; + spin_unlock(&bp->stats64_lock); } } @@ -880,6 +882,7 @@ bnx2_alloc_mem(struct bnx2 *bp) } } + spin_lock(&bp->stats64_lock); bp->stats_blk = status_blk + status_blk_size; bp->stats_blk_mapping = bp->status_blk_mapping + status_blk_size; @@ -894,20 +897,23 @@ bnx2_alloc_mem(struct bnx2 *bp) &bp->ctx_blk_mapping[i], GFP_KERNEL); if (bp->ctx_blk[i] == NULL) - goto alloc_mem_err; + goto free_stats64_lock; } } err = bnx2_alloc_rx_mem(bp); if (err) - goto alloc_mem_err; + goto free_stats64_lock; err = bnx2_alloc_tx_mem(bp); if (err) - goto alloc_mem_err; + goto free_stats64_lock; + spin_unlock(&bp->stats64_lock); return 0; +free_stats64_lock: + spin_unlock(&bp->stats64_lock); alloc_mem_err: bnx2_free_mem(bp); return -ENOMEM; @@ -6756,10 +6762,14 @@ bnx2_close(struct net_device *dev) static void bnx2_save_stats(struct bnx2 *bp) { - u32 *hw_stats = (u32 *) bp->stats_blk; - u32 *temp_stats = (u32 *) bp->temp_stats_blk; + u32 *hw_stats; + u32 *temp_stats; int i; + spin_lock(&bp->stats64_lock); + hw_stats = (u32 *) bp->stats_blk; + temp_stats = (u32 *) bp->temp_stats_blk; + /* The 1st 10 counters are 64-bit counters */ for (i = 0; i < 20; i += 2) { u32 hi; @@ -6775,6 +6785,8 @@ bnx2_save_stats(struct bnx2 *bp) for ( ; i < sizeof(struct statistics_block) / 4; i++) temp_stats[i] += hw_stats[i]; + + spin_unlock(&bp->stats64_lock); } #define GET_64BIT_NET_STATS64(ctr) \ @@ -6793,8 +6805,11 @@ bnx2_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *net_stats) { struct bnx2 *bp = netdev_priv(dev); - if (bp->stats_blk == NULL) + spin_lock(&bp->stats64_lock); + if (bp->stats_blk == NULL) { + spin_unlock(&bp->stats64_lock); return net_stats; + } net_stats->rx_packets = GET_64BIT_NET_STATS(stat_IfHCInUcastPkts) + @@ -6858,6 +6873,7 @@ bnx2_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *net_stats) GET_32BIT_NET_STATS(stat_IfInMBUFDiscards) + GET_32BIT_NET_STATS(stat_FwRxDrop); + spin_unlock(&bp->stats64_lock); return net_stats; } @@ -7634,13 +7650,17 @@ bnx2_get_ethtool_stats(struct net_device *dev, { struct bnx2 *bp = netdev_priv(dev); int i; - u32 *hw_stats = (u32 *) bp->stats_blk; - u32 *temp_stats = (u32 *) bp->temp_stats_blk; + u32 *hw_stats; + u32 *temp_stats; u8 *stats_len_arr = NULL; + spin_lock(&bp->stats64_lock); + hw_stats = (u32 *) bp->stats_blk; + temp_stats = (u32 *) bp->temp_stats_blk; + if (hw_stats == NULL) { memset(buf, 0, sizeof(u64) * BNX2_NUM_STATS); - return; + goto free_stats64_lock; } if ((BNX2_CHIP_ID(bp) == BNX2_CHIP_ID_5706_A0) || @@ -7673,6 +7693,9 @@ bnx2_get_ethtool_stats(struct net_device *dev, (((u64) *(temp_stats + offset)) << 32) + *(temp_stats + offset + 1); } + +free_stats64_lock: + spin_unlock(&bp->stats64_lock); } static int @@ -8125,6 +8148,7 @@ bnx2_init_board(struct pci_dev *pdev, struct net_device *dev) spin_lock_init(&bp->phy_lock); spin_lock_init(&bp->indirect_lock); + spin_lock_init(&bp->stats64_lock); #ifdef BCM_CNIC mutex_init(&bp->cnic_lock); #endif diff --git a/drivers/net/ethernet/broadcom/bnx2.h b/drivers/net/ethernet/broadcom/bnx2.h index f92f76c..c88c21b 100644 --- a/drivers/net/ethernet/broadcom/bnx2.h +++ b/drivers/net/ethernet/broadcom/bnx2.h @@ -6928,6 +6928,7 @@ struct bnx2 { dma_addr_t status_blk_mapping; + spinlock_t stats64_lock; struct statistics_block *stats_blk; struct statistics_block *temp_stats_blk; dma_addr_t stats_blk_mapping;
we have two processes to do: P1#: ifconfig eth0 down; which will call bnx2_close, then will , and set Null to stats_blk P2#: ifconfig eth0; which will call bnx2_get_stats64, it will use stats_blk. In one case: --P1#-- --P2#-- stats_blk(no null) bnx2_free_mem ->bp->stats_blk = NULL GET_64BIT_NET_STATS then it will cause 'NULL Pointer' Problem. it is as well with 'ethtool -S ethx'. BTW, the other branch has this problem as well. So we add a spin_lock to protect stats_blk. Signed-off-by: Wang Weidong <wangweidong1@huawei.com> --- drivers/net/ethernet/broadcom/bnx2.c | 42 ++++++++++++++++++++++++++++-------- drivers/net/ethernet/broadcom/bnx2.h | 1 + 2 files changed, 34 insertions(+), 9 deletions(-)