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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] When XDP is attached, the new base snapshot counts XDP TX ring
  traffic only at teardown, so device-level netdev qstats tx-packets/tx-…

--

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

This patch keeps baseline rtnl and qstats counters in gve_priv so that
device statistics do not go backwards across queue teardown. Ring counters
are folded into the base in gve_queues_mem_remove() and in
gve_rx_queue_stop(). gve_get_base_stats() now reports the saved values
instead of zeros.

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

[ ... ]

> @@ -646,6 +688,9 @@ static void gve_queues_mem_remove(struct gve_priv *priv)
>  	gve_get_curr_alloc_cfgs(priv, &tx_alloc_cfg, &rx_alloc_cfg);
>  
>  	spin_lock_bh(&priv->stats_lock);
> +	gve_get_ring_stats(priv, &priv->base_net_stats);
> +	gve_get_ring_err_stats(priv, &priv->base_ring_err_stats);
> +
>  	priv->tx = NULL;
>  	priv->rx = NULL;
>  	spin_unlock_bh(&priv->stats_lock);

[Severity: Medium]
With an XDP program attached, will the device-level qstats tx-packets and
tx-bytes jump forward on every ifdown or reconfiguration?

The TX loop in gve_get_ring_stats() goes up to gve_num_tx_queues(), and
that count includes the XDP TX rings:

    return priv->tx_cfg.num_queues + priv->tx_cfg.num_xdp_queues;

As a result, the full pkt_done/bytes_done history of the XDP TX rings
ends up in base_net_stats.tx_*. Those counters are updated by
gve_clean_xdp_done() and by the DQO XDP path.

While the interface is up, qstats never reads those rings live:

gve_queues_start()
    netif_set_real_num_tx_queues(dev, priv->tx_cfg.num_queues);

netdev_nl_stats_by_netdev()
    netdev_stat_queue_sum(netdev, 0, netdev->real_num_rx_queues, &rx_sum,
                          0, netdev->real_num_tx_queues, &tx_sum);

So gve_get_tx_queue_stats() is only called for the non-XDP TX queues. The
new gve_get_base_stats() returns only the saved snapshot.

The qstats device total therefore leaves out live XDP TX traffic. Then,
when gve_close() or gve_adjust_config() reaches gve_queues_mem_remove(),
all of that traffic shows up at once. gve_get_stats() counts the live XDP
rings the whole time, so rtnl and qstats report different totals while
the interface is up.

The netdev_stat_ops kernel-doc says get_base_stats "can also be used to
report any miscellaneous packets transferred outside of the main set of
queues used by the networking stack".

Should gve_get_base_stats() also add the live counters of
tx[tx_cfg.num_queues .. gve_num_tx_queues())? The other option is to
limit the teardown snapshot's TX loop to tx_cfg.num_queues.

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

[ ... ]

> +	spin_lock_bh(&priv->stats_lock);
> +	rx->packets = base_stats->rx_packets;
> +	rx->bytes = base_stats->rx_bytes;
> +	rx->alloc_fail = base_err_stats->rx_alloc_fails;
>  
> -	tx->packets = 0;
> -	tx->bytes = 0;
> +	tx->packets = base_stats->tx_packets;
> +	tx->bytes = base_stats->tx_bytes;
> +	spin_unlock_bh(&priv->stats_lock);
>  }

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