| From 57e7a0db16ae4d3a27271295c9b8a7f3207737c2 Mon Sep 17 00:00:00 2001 |
| From: Sasha Levin <sashal@kernel.org> |
| Date: Tue, 5 Jul 2022 19:22:22 +0800 |
| Subject: net: hinic: fix bug that ethtool get wrong stats |
| |
| From: Qiao Ma <mqaio@linux.alibaba.com> |
| |
| [ Upstream commit 67dffd3db98570af8ff54c934f7d14664c0d182a ] |
| |
| Function hinic_get_stats64() will do two operations: |
| 1. reads stats from every hinic_rxq/txq and accumulates them |
| 2. calls hinic_rxq/txq_clean_stats() to clean every rxq/txq's stats |
| |
| For hinic_get_stats64(), it could get right data, because it sums all |
| data to nic_dev->rx_stats/tx_stats. |
| But it is wrong for get_drv_queue_stats(), this function will read |
| hinic_rxq's stats, which have been cleared to zero by hinic_get_stats64(). |
| |
| I have observed hinic's cleanup operation by using such command: |
| > watch -n 1 "cat ethtool -S eth4 | tail -40" |
| |
| Result before: |
| ... |
| rxq7_pkts: 1 |
| rxq7_bytes: 90 |
| rxq7_errors: 0 |
| rxq7_csum_errors: 0 |
| rxq7_other_errors: 0 |
| ... |
| rxq9_pkts: 11 |
| rxq9_bytes: 726 |
| rxq9_errors: 0 |
| rxq9_csum_errors: 0 |
| rxq9_other_errors: 0 |
| ... |
| rxq11_pkts: 0 |
| rxq11_bytes: 0 |
| rxq11_errors: 0 |
| rxq11_csum_errors: 0 |
| rxq11_other_errors: 0 |
| |
| Result after a few seconds: |
| ... |
| rxq7_pkts: 0 |
| rxq7_bytes: 0 |
| rxq7_errors: 0 |
| rxq7_csum_errors: 0 |
| rxq7_other_errors: 0 |
| ... |
| rxq9_pkts: 2 |
| rxq9_bytes: 132 |
| rxq9_errors: 0 |
| rxq9_csum_errors: 0 |
| rxq9_other_errors: 0 |
| ... |
| rxq11_pkts: 1 |
| rxq11_bytes: 170 |
| rxq11_errors: 0 |
| rxq11_csum_errors: 0 |
| rxq11_other_errors: 0 |
| |
| To solve this problem, we just keep every queue's total stats in their own |
| queue (aka hinic_{rxq|txq}), and simply sum all per-queue stats every time |
| calling hinic_get_stats64(). |
| With that solution, there is no need to clean per-queue stats now, |
| and there is no need to maintain global hinic_dev.{tx|rx}_stats, too. |
| |
| Fixes: edd384f682cc ("net-next/hinic: Add ethtool and stats") |
| Signed-off-by: Qiao Ma <mqaio@linux.alibaba.com> |
| Reported-by: kernel test robot <lkp@intel.com> |
| Signed-off-by: David S. Miller <davem@davemloft.net> |
| Signed-off-by: Sasha Levin <sashal@kernel.org> |
| --- |
| drivers/net/ethernet/huawei/hinic/hinic_dev.h | 3 - |
| .../net/ethernet/huawei/hinic/hinic_main.c | 57 ++++++------------- |
| drivers/net/ethernet/huawei/hinic/hinic_rx.c | 2 - |
| drivers/net/ethernet/huawei/hinic/hinic_tx.c | 2 - |
| 4 files changed, 16 insertions(+), 48 deletions(-) |
| |
| diff --git a/drivers/net/ethernet/huawei/hinic/hinic_dev.h b/drivers/net/ethernet/huawei/hinic/hinic_dev.h |
| index fb3e89141a0d..a4fbf44f944c 100644 |
| --- a/drivers/net/ethernet/huawei/hinic/hinic_dev.h |
| +++ b/drivers/net/ethernet/huawei/hinic/hinic_dev.h |
| @@ -95,9 +95,6 @@ struct hinic_dev { |
| u16 sq_depth; |
| u16 rq_depth; |
| |
| - struct hinic_txq_stats tx_stats; |
| - struct hinic_rxq_stats rx_stats; |
| - |
| u8 rss_tmpl_idx; |
| u8 rss_hash_engine; |
| u16 num_rss; |
| diff --git a/drivers/net/ethernet/huawei/hinic/hinic_main.c b/drivers/net/ethernet/huawei/hinic/hinic_main.c |
| index 56a89793f47d..89dc52510fdc 100644 |
| --- a/drivers/net/ethernet/huawei/hinic/hinic_main.c |
| +++ b/drivers/net/ethernet/huawei/hinic/hinic_main.c |
| @@ -80,56 +80,44 @@ static int set_features(struct hinic_dev *nic_dev, |
| netdev_features_t pre_features, |
| netdev_features_t features, bool force_change); |
| |
| -static void update_rx_stats(struct hinic_dev *nic_dev, struct hinic_rxq *rxq) |
| +static void gather_rx_stats(struct hinic_rxq_stats *nic_rx_stats, struct hinic_rxq *rxq) |
| { |
| - struct hinic_rxq_stats *nic_rx_stats = &nic_dev->rx_stats; |
| struct hinic_rxq_stats rx_stats; |
| |
| - u64_stats_init(&rx_stats.syncp); |
| - |
| hinic_rxq_get_stats(rxq, &rx_stats); |
| |
| - u64_stats_update_begin(&nic_rx_stats->syncp); |
| nic_rx_stats->bytes += rx_stats.bytes; |
| nic_rx_stats->pkts += rx_stats.pkts; |
| nic_rx_stats->errors += rx_stats.errors; |
| nic_rx_stats->csum_errors += rx_stats.csum_errors; |
| nic_rx_stats->other_errors += rx_stats.other_errors; |
| - u64_stats_update_end(&nic_rx_stats->syncp); |
| - |
| - hinic_rxq_clean_stats(rxq); |
| } |
| |
| -static void update_tx_stats(struct hinic_dev *nic_dev, struct hinic_txq *txq) |
| +static void gather_tx_stats(struct hinic_txq_stats *nic_tx_stats, struct hinic_txq *txq) |
| { |
| - struct hinic_txq_stats *nic_tx_stats = &nic_dev->tx_stats; |
| struct hinic_txq_stats tx_stats; |
| |
| - u64_stats_init(&tx_stats.syncp); |
| - |
| hinic_txq_get_stats(txq, &tx_stats); |
| |
| - u64_stats_update_begin(&nic_tx_stats->syncp); |
| nic_tx_stats->bytes += tx_stats.bytes; |
| nic_tx_stats->pkts += tx_stats.pkts; |
| nic_tx_stats->tx_busy += tx_stats.tx_busy; |
| nic_tx_stats->tx_wake += tx_stats.tx_wake; |
| nic_tx_stats->tx_dropped += tx_stats.tx_dropped; |
| nic_tx_stats->big_frags_pkts += tx_stats.big_frags_pkts; |
| - u64_stats_update_end(&nic_tx_stats->syncp); |
| - |
| - hinic_txq_clean_stats(txq); |
| } |
| |
| -static void update_nic_stats(struct hinic_dev *nic_dev) |
| +static void gather_nic_stats(struct hinic_dev *nic_dev, |
| + struct hinic_rxq_stats *nic_rx_stats, |
| + struct hinic_txq_stats *nic_tx_stats) |
| { |
| int i, num_qps = hinic_hwdev_num_qps(nic_dev->hwdev); |
| |
| for (i = 0; i < num_qps; i++) |
| - update_rx_stats(nic_dev, &nic_dev->rxqs[i]); |
| + gather_rx_stats(nic_rx_stats, &nic_dev->rxqs[i]); |
| |
| for (i = 0; i < num_qps; i++) |
| - update_tx_stats(nic_dev, &nic_dev->txqs[i]); |
| + gather_tx_stats(nic_tx_stats, &nic_dev->txqs[i]); |
| } |
| |
| /** |
| @@ -558,8 +546,6 @@ int hinic_close(struct net_device *netdev) |
| netif_carrier_off(netdev); |
| netif_tx_disable(netdev); |
| |
| - update_nic_stats(nic_dev); |
| - |
| up(&nic_dev->mgmt_lock); |
| |
| if (!HINIC_IS_VF(nic_dev->hwdev->hwif)) |
| @@ -853,26 +839,23 @@ static void hinic_get_stats64(struct net_device *netdev, |
| struct rtnl_link_stats64 *stats) |
| { |
| struct hinic_dev *nic_dev = netdev_priv(netdev); |
| - struct hinic_rxq_stats *nic_rx_stats; |
| - struct hinic_txq_stats *nic_tx_stats; |
| - |
| - nic_rx_stats = &nic_dev->rx_stats; |
| - nic_tx_stats = &nic_dev->tx_stats; |
| + struct hinic_rxq_stats nic_rx_stats = {}; |
| + struct hinic_txq_stats nic_tx_stats = {}; |
| |
| down(&nic_dev->mgmt_lock); |
| |
| if (nic_dev->flags & HINIC_INTF_UP) |
| - update_nic_stats(nic_dev); |
| + gather_nic_stats(nic_dev, &nic_rx_stats, &nic_tx_stats); |
| |
| up(&nic_dev->mgmt_lock); |
| |
| - stats->rx_bytes = nic_rx_stats->bytes; |
| - stats->rx_packets = nic_rx_stats->pkts; |
| - stats->rx_errors = nic_rx_stats->errors; |
| + stats->rx_bytes = nic_rx_stats.bytes; |
| + stats->rx_packets = nic_rx_stats.pkts; |
| + stats->rx_errors = nic_rx_stats.errors; |
| |
| - stats->tx_bytes = nic_tx_stats->bytes; |
| - stats->tx_packets = nic_tx_stats->pkts; |
| - stats->tx_errors = nic_tx_stats->tx_dropped; |
| + stats->tx_bytes = nic_tx_stats.bytes; |
| + stats->tx_packets = nic_tx_stats.pkts; |
| + stats->tx_errors = nic_tx_stats.tx_dropped; |
| } |
| |
| static int hinic_set_features(struct net_device *netdev, |
| @@ -1171,8 +1154,6 @@ static void hinic_free_intr_coalesce(struct hinic_dev *nic_dev) |
| static int nic_dev_init(struct pci_dev *pdev) |
| { |
| struct hinic_rx_mode_work *rx_mode_work; |
| - struct hinic_txq_stats *tx_stats; |
| - struct hinic_rxq_stats *rx_stats; |
| struct hinic_dev *nic_dev; |
| struct net_device *netdev; |
| struct hinic_hwdev *hwdev; |
| @@ -1234,12 +1215,6 @@ static int nic_dev_init(struct pci_dev *pdev) |
| |
| sema_init(&nic_dev->mgmt_lock, 1); |
| |
| - tx_stats = &nic_dev->tx_stats; |
| - rx_stats = &nic_dev->rx_stats; |
| - |
| - u64_stats_init(&tx_stats->syncp); |
| - u64_stats_init(&rx_stats->syncp); |
| - |
| nic_dev->vlan_bitmap = devm_bitmap_zalloc(&pdev->dev, VLAN_N_VID, |
| GFP_KERNEL); |
| if (!nic_dev->vlan_bitmap) { |
| diff --git a/drivers/net/ethernet/huawei/hinic/hinic_rx.c b/drivers/net/ethernet/huawei/hinic/hinic_rx.c |
| index b33ed4d92b71..b6bce622a6a8 100644 |
| --- a/drivers/net/ethernet/huawei/hinic/hinic_rx.c |
| +++ b/drivers/net/ethernet/huawei/hinic/hinic_rx.c |
| @@ -73,7 +73,6 @@ void hinic_rxq_get_stats(struct hinic_rxq *rxq, struct hinic_rxq_stats *stats) |
| struct hinic_rxq_stats *rxq_stats = &rxq->rxq_stats; |
| unsigned int start; |
| |
| - u64_stats_update_begin(&stats->syncp); |
| do { |
| start = u64_stats_fetch_begin(&rxq_stats->syncp); |
| stats->pkts = rxq_stats->pkts; |
| @@ -83,7 +82,6 @@ void hinic_rxq_get_stats(struct hinic_rxq *rxq, struct hinic_rxq_stats *stats) |
| stats->csum_errors = rxq_stats->csum_errors; |
| stats->other_errors = rxq_stats->other_errors; |
| } while (u64_stats_fetch_retry(&rxq_stats->syncp, start)); |
| - u64_stats_update_end(&stats->syncp); |
| } |
| |
| /** |
| diff --git a/drivers/net/ethernet/huawei/hinic/hinic_tx.c b/drivers/net/ethernet/huawei/hinic/hinic_tx.c |
| index 8d59babbf476..082eb2a5088d 100644 |
| --- a/drivers/net/ethernet/huawei/hinic/hinic_tx.c |
| +++ b/drivers/net/ethernet/huawei/hinic/hinic_tx.c |
| @@ -98,7 +98,6 @@ void hinic_txq_get_stats(struct hinic_txq *txq, struct hinic_txq_stats *stats) |
| struct hinic_txq_stats *txq_stats = &txq->txq_stats; |
| unsigned int start; |
| |
| - u64_stats_update_begin(&stats->syncp); |
| do { |
| start = u64_stats_fetch_begin(&txq_stats->syncp); |
| stats->pkts = txq_stats->pkts; |
| @@ -108,7 +107,6 @@ void hinic_txq_get_stats(struct hinic_txq *txq, struct hinic_txq_stats *stats) |
| stats->tx_dropped = txq_stats->tx_dropped; |
| stats->big_frags_pkts = txq_stats->big_frags_pkts; |
| } while (u64_stats_fetch_retry(&txq_stats->syncp, start)); |
| - u64_stats_update_end(&stats->syncp); |
| } |
| |
| /** |
| -- |
| 2.35.1 |
| |
