Re: [PATCH net v4 2/4] gve: Fix backward stats when interface goes down or configuration is adjusted

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message and the Fixes tag describe only part of what
  the patch changes.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but on 32-bit
  SMP kernels tx->statss of an XDP TX ring has writers that are not…
- [Low] This problem wasn't introduced by this patch, but check_down() in
  tools/testing/selftests/drivers/net/stats.py has the docstring "Test…
- [Low] This problem wasn't introduced by this patch, but
  pkt_byte_sum.stat_cmp() in…
- [Low] This problem wasn't introduced by this patch, but the single-
  queue RX restart path never calls u64_stats_init() on the new ring's…
- [Low] This problem wasn't introduced by this patch, but
  gve_rx_post_buffers_dqo() updates rx->statss (rx_buf_alloc_fail)…

--

gve: Fix backward stats when interface goes down or configuration is adjusted

This patch keeps baseline rx/tx packet, byte and alloc-fail counters in
gve_priv. They are saved during full queue teardown in
gve_queues_mem_remove() and during a single RX queue stop in
gve_rx_queue_stop(). Both ndo_get_stats64 and the netdev_stat_ops base
stats report these baselines, and gve_get_base_stats() also reports the
live XDP TX ring counters.

> gve_get_base_stats() sets all the stats to 0, so the stats go backwards
> when interface goes down or configuration is adjusted.

[ ... ]

> Fixes: 2e5e0932dff5 ("gve: add support for basic queue stats")

[Severity: Low]

Does this description cover everything the patch changes? The message
blames gve_get_base_stats(). The patch also changes ndo_get_stats64:
gve_get_stats() now calls gve_add_base_stats().

That part fixes an older bug. In the tree before 2e5e0932dff5,
gve_close() already freed the rings on every ifdown:

gve_close()
    gve_queues_stop(priv);
    gve_queues_mem_remove(priv);

gve_queues_mem_remove() set priv->tx and priv->rx to NULL, and
gve_get_stats() only summed the live rings. So the ip -s link and
/proc/net/dev counters already went back to 0 on ifdown.
Documentation/networking/statistics.rst says:

  Statistics must persist across routine operations like bringing the
  interface down and up.

Could the changelog mention the rtnl counter change, and could the
Fixes: tag be revisited? With the current tag, stable backporters may
treat trees without netdev_stat_ops (older than 6.13) as unaffected,
even though they have the rtnl reset on ifdown.

> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index 26fdd22644f4d..b5e2f39a903c5 100644
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c

[ ... ]

> @@ -84,19 +112,41 @@ static void gve_get_stats(struct net_device *dev, struct rtnl_link_stats64 *s)

[ ... ]

> +static void gve_get_stats(struct net_device *dev, struct rtnl_link_stats64 *s)
> +{
> +	struct gve_priv *priv = netdev_priv(dev);
> +
> +	spin_lock_bh(&priv->stats_lock);
> +	gve_get_ring_stats(priv, s);
> +	gve_add_base_stats(priv, s);
>  	spin_unlock_bh(&priv->stats_lock);
>  }

[Severity: Low]

This is a pre-existing issue and was not introduced by this patch. The
commit message cites the check_down test case in
tools/testing/selftests/drivers/net/stats.py. Its docstring says:

    """ Test statistics (interface and qstat) are not impacted by ifdown """

However, check_down() only compares qstats from netfam.qstats_get()
before and after ip link set down. It never reads the rtnl stats64.

If the new gve_add_base_stats() call in gve_get_stats() regressed,
wouldn't check_down() still pass?

[ ... ]

> @@ -2744,7 +2809,9 @@ static int gve_rx_queue_start(struct net_device *dev,
>  		return -EAGAIN;
>  
>  	gve_per_q_mem = (struct gve_rx_ring *)per_q_mem;
> +	spin_lock_bh(&priv->stats_lock);
>  	priv->rx[idx] = *gve_per_q_mem;
> +	spin_unlock_bh(&priv->stats_lock);

[Severity: Low]

This isn't a bug introduced by this patch, but is rx->statss ever
initialized on the single-queue restart path?

gve_rx_queue_mem_alloc() calls gve_rx_alloc_ring_gqi() or
gve_rx_alloc_ring_dqo(), which zero the ring. The only u64_stats_init()
calls for RX rings are in gve_init_sync_stats():

	for (i = 0; i < priv->rx_cfg.num_queues; i++)
		u64_stats_init(&priv->rx[i].statss);

That function only runs on a full queue start. gve_rx_queue_start() then
copies the uninitialized statss into priv->rx[idx].

On 32-bit kernels with CONFIG_DEBUG_LOCK_ALLOC, the seqcount's lockdep
map is never initialized. Can later stats updates or reads then trigger
lockdep warnings? The stats_lock added around this copy doesn't change
that.

[Severity: Low]

This is also a pre-existing issue and not introduced by this patch. Later
in gve_rx_queue_start(), the DQO path calls gve_rx_post_buffers_dqo()
from preemptible process context:

	else
		gve_rx_post_buffers_dqo(&priv->rx[idx]);

When a buffer allocation fails, that function writes rx->statss:

		if (unlikely(gve_alloc_buffer(rx, desc))) {
			u64_stats_update_begin(&rx->statss);
			rx->rx_buf_alloc_fail++;
			u64_stats_update_end(&rx->statss);

On 32-bit non-RT kernels, __u64_stats_update_begin() uses
preempt_disable_nested(). That only asserts that preemption is already
disabled; it does not disable it.

If the writer is preempted while the sequence is odd, can a reader on
the same CPU spin until the writer runs again? gve_get_stats() under
spin_lock_bh(&priv->stats_lock) is one such reader. Lockdep builds would
also warn here. gve_create_rings() reaches the same code regardless of
this patch.

[ ... ]

> @@ -2844,12 +2913,30 @@ static void gve_get_base_stats(struct net_device *dev,
>  			       struct netdev_queue_stats_rx *rx,
>  			       struct netdev_queue_stats_tx *tx)
>  {

[ ... ]

> +	tx->packets = base_stats->tx_packets;
> +	tx->bytes = base_stats->tx_bytes;
> +	/* XDP TX rings are not exposed as netdev queues, so report their live
> +	 * counters here to keep the device totals consistent with
> +	 * ndo_get_stats64.
> +	 */
> +	if (priv->tx)
> +		gve_add_tx_ring_range_stats(priv, priv->tx_cfg.num_queues,
> +					    gve_num_tx_queues(priv),
> +					    &tx->packets, &tx->bytes);
> +	spin_unlock_bh(&priv->stats_lock);
>  }

[Severity: Low]

This is a pre-existing issue in the selftest and was not introduced by
this patch. Does pkt_byte_sum() in stats.py really check that these
totals match ndo_get_stats64? stat_cmp() returns at the first key that
differs:

    def stat_cmp(rstat, qstat):
        for key in ['tx-packets', 'tx-bytes', 'rx-packets', 'rx-bytes']:
            if rstat[key] != qstat[key]:
                return rstat[key] - qstat[key]
        return 0

With live TX traffic, a correctly increasing tx-packets decides the
result before tx-bytes or the RX counters are compared. The test can
therefore pass while rtnl and qstats disagree in the later fields. That
includes the XDP TX contribution added here.

[Severity: Medium]

This isn't a bug introduced by this patch, but are the writers of
tx->statss on the XDP TX rings serialized on 32-bit SMP kernels?

gve_xdp_xmit_dqo() picks the ring as smp_processor_id() %
num_xdp_queues, so several CPUs can share one ring. It updates statss
after releasing the lock that serializes those CPUs:

	spin_unlock(&tx->dqo_tx.xdp_lock);

	u64_stats_update_begin(&tx->statss);
	tx->xdp_xmit += n;
	tx->xdp_xmit_errors += n - i;
	u64_stats_update_end(&tx->statss);

gve_xdp_xmit() in gve_tx.c does the same after
spin_unlock(&tx->xdp_lock). gve_clean_tx_done_dqo() also updates the
same statss from NAPI:

	u64_stats_update_begin(&tx->statss);
	tx->bytes_done += pkt_compl_bytes + reinject_compl_bytes;
	tx->pkt_done += pkt_compl_pkts + reinject_compl_pkts;

Could overlapping write_seqcount_begin()/end() calls tear the 64-bit
reads? Could they lose a sequence increment and leave the count odd? If
so, the retry loop in gve_add_tx_ring_range_stats() would spin forever
with stats_lock held and BH disabled.

gve_get_stats() and gve_get_ethtool_stats() already read these counters
before this patch, so the new reader in gve_get_base_stats() neither
creates nor widens the exposure. Hitting it needs a 32-bit kernel and
XDP_REDIRECT into gve from several CPUs while completions are running.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029211634.2232784-1-hramamurthy%40google.com