eth: bnxt: improve the timing of stats

Kernel selftests wait 1.25x of the promised stats refresh time
(as read from ethtool -c). bnxt reports 1sec by default, but
the stats update process has two steps. First device DMAs the
new values, then the service task performs update in full-width
SW counters. So the worst case delay is actually 2x.

Note that the behavior is different for ring stats and port stats.
Port stats are fetched synchronously by the service worker, so
there's no risk of doubling up the delay there.

The problem of stale stats impacts not only tests but real workloads
which monitor egress bandwidth of a NIC. The inaccuracy causes double
counting in the next cycle and spurious overload alarms.

Try to read from the DMA buffer more aggressively, to mitigate
timing issues between DMA and service task. The SW update should
be cheap.

Fixes: 51f307856b60 ("bnxt_en: Allow statistics DMA to be configurable using ethtool -C.")
Reviewed-by: Michael Chan <michael.chan@broadcom.com>
Link: https://patch.msgid.link/20260619191538.104165-1-kuba@kernel.org
Signed-off-by: Jakub Kicinski <kuba@kernel.org>

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 055e93a417b65b9ed461dcf7e2d50e26f2885b88..7513618793daf3972acf339481fb3b85470069a2 100644 (file)
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -10530,7 +10530,7 @@ static void bnxt_accumulate_stats(struct bnxt_stats_mem *stats)
                                stats->hw_masks, stats->len / 8, false);
 }
 
-static void bnxt_accumulate_all_stats(struct bnxt *bp)
+static void bnxt_accumulate_ring_stats(struct bnxt *bp)
 {
        struct bnxt_stats_mem *ring0_stats;
        bool ignore_zero = false;
@@ -10553,6 +10553,10 @@ static void bnxt_accumulate_all_stats(struct bnxt *bp)
                                        ring0_stats->hw_masks,
                                        ring0_stats->len / 8, ignore_zero);
        }
+}
+
+static void bnxt_accumulate_port_stats(struct bnxt *bp)
+{
        if (bp->flags & BNXT_FLAG_PORT_STATS) {
                struct bnxt_stats_mem *stats = &bp->port_stats;
                __le64 *hw_stats = stats->hw_stats;
@@ -10575,6 +10579,41 @@ static void bnxt_accumulate_all_stats(struct bnxt *bp)
        }
 }
 
+static void bnxt_accumulate_all_stats(struct bnxt *bp)
+{
+       bnxt_accumulate_ring_stats(bp);
+       bnxt_accumulate_port_stats(bp);
+}
+
+/* Re-accumulate ring stats from DMA buffers if stale.
+ * uAPIs for reading sw_stats should call this first.
+ *
+ * We promise user space update frequency of bp->stats_coal_ticks but
+ * the update is a two step process - first device updates the DMA buffer,
+ * then we have to update from that buffer to driver stats in the service work.
+ * Worst case we would be 2x off from the desired frequency.
+ * Sync the stats sooner, if stale. The 20% threshold was chosen arbitrarily.
+ *
+ * Ideally we would split the user-configured time into two portions,
+ * i.e. also lower the DMA period by the 20%. But the DMA timer seems to have
+ * too coarse granularity to play such tricks.
+ */
+void bnxt_sync_ring_stats(struct bnxt *bp)
+{
+       unsigned long stale;
+
+       if (!netif_running(bp->dev) || !bp->stats_coal_ticks)
+               return;
+
+       spin_lock(&bp->stats_lock);
+       stale = usecs_to_jiffies(bp->stats_coal_ticks / 5);
+       if (time_after_eq(jiffies, bp->stats_updated_jiffies + stale)) {
+               bnxt_accumulate_ring_stats(bp);
+               bp->stats_updated_jiffies = jiffies;
+       }
+       spin_unlock(&bp->stats_lock);
+}
+
 static int bnxt_hwrm_port_qstats(struct bnxt *bp, u8 flags)
 {
        struct hwrm_port_qstats_input *req;
@@ -13577,6 +13616,7 @@ bnxt_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
                return;
        }
 
+       bnxt_sync_ring_stats(bp);
        bnxt_get_ring_stats(bp, stats);
        bnxt_add_prev_stats(bp, stats);
 
@@ -14753,7 +14793,10 @@ static void bnxt_sp_task(struct work_struct *work)
        if (test_and_clear_bit(BNXT_PERIODIC_STATS_SP_EVENT, &bp->sp_event)) {
                bnxt_hwrm_port_qstats(bp, 0);
                bnxt_hwrm_port_qstats_ext(bp, 0);
+               spin_lock(&bp->stats_lock);
                bnxt_accumulate_all_stats(bp);
+               bp->stats_updated_jiffies = jiffies;
+               spin_unlock(&bp->stats_lock);
        }
 
        if (test_and_clear_bit(BNXT_LINK_CHNG_SP_EVENT, &bp->sp_event)) {
@@ -15488,6 +15531,7 @@ static int bnxt_init_board(struct pci_dev *pdev, struct net_device *dev)
        INIT_DELAYED_WORK(&bp->fw_reset_task, bnxt_fw_reset_task);
 
        spin_lock_init(&bp->ntp_fltr_lock);
+       spin_lock_init(&bp->stats_lock);
 #if BITS_PER_LONG == 32
        spin_lock_init(&bp->db_lock);
 #endif
@@ -16056,6 +16100,7 @@ static void bnxt_get_queue_stats_rx(struct net_device *dev, int i,
        if (!bp->bnapi)
                return;
 
+       bnxt_sync_ring_stats(bp);
        cpr = &bp->bnapi[i]->cp_ring;
        sw = cpr->stats.sw_stats;
 
@@ -16084,6 +16129,7 @@ static void bnxt_get_queue_stats_tx(struct net_device *dev, int i,
        if (!bp->tx_ring)
                return;
 
+       bnxt_sync_ring_stats(bp);
        bnapi = bp->tx_ring[bp->tx_ring_map[i]].bnapi;
        sw = bnapi->cp_ring.stats.sw_stats;
 

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index 6d312259f852789b70b62321605e20fb197668f3..6335dfc14c985ba11cd5dd7cf6ce415e1aeb9dd1 100644 (file)
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -2620,6 +2620,10 @@ struct bnxt {
 #define BNXT_MIN_STATS_COAL_TICKS        250000
 #define BNXT_MAX_STATS_COAL_TICKS       1000000
 
+       /* Protects stats_updated_jiffies and writes to sw_stats */
+       spinlock_t              stats_lock;
+       unsigned long           stats_updated_jiffies;
+
        struct work_struct      sp_task;
        unsigned long           sp_event;
 #define BNXT_RX_NTP_FLTR_SP_EVENT      1
@@ -3027,6 +3031,7 @@ void bnxt_reenable_sriov(struct bnxt *bp);
 void bnxt_close_nic(struct bnxt *, bool, bool);
 void bnxt_get_ring_drv_stats(struct bnxt *bp,
                             struct bnxt_total_ring_drv_stats *stats);
+void bnxt_sync_ring_stats(struct bnxt *bp);
 bool bnxt_rfs_capable(struct bnxt *bp, bool new_rss_ctx);
 int bnxt_dbg_hwrm_rd_reg(struct bnxt *bp, u32 reg_off, u16 num_words,
                         u32 *reg_buf);

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index 56d74a3c24b7c6cb95774a7e11088f65b0509ddf..62bc9cae613c38487168c5e8ae53031d1239949d 100644 (file)
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -606,6 +606,7 @@ static void bnxt_get_ethtool_stats(struct net_device *dev,
                goto skip_ring_stats;
        }
 
+       bnxt_sync_ring_stats(bp);
        tpa_stats = bnxt_get_num_tpa_ring_stats(bp);
        for (i = 0; i < bp->cp_nr_rings; i++) {
                struct bnxt_napi *bnapi = bp->bnapi[i];