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

[Pin-yen Lin]

On Tue, Oct 27, 2026 at 9:42 AM <netdev-bot+sashiko@xxxxxxxxxx> wrote:
>
> 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.

We will make gve_get_base_stats() include stats from the XDP queue in v4.
>
> > @@ -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

Regards,
Pin-yen