Re: [PATCH net v2 1/9] gve: increment work_done for XDP and error packets

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 2 · 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] The commit message describes only a NAPI `work_done`
  accounting fix, but the patch also changes the semantics of the…
- [Medium] The newly added receive packet/byte accounting for XDP-
  consumed packets is unreachable for AF_XDP zero-copy buffers, so the…

--

gve: increment work_done for XDP and error packets

The DQO RX poll loop now increments work_done for every end-of-packet
completion instead of only for completions that produced an SKB, so XDP
and error packets are charged against the NAPI budget. The packet and
byte counters were moved into a separate rx_packets accumulator plus a
new increment on the XDP non-PASS path.

> diff --git a/drivers/net/ethernet/google/gve/gve_rx_dqo.c b/drivers/net/ethernet/google/gve/gve_rx_dqo.c
> index 5cf242b285577..c3f4a76b0fac5 100644
> --- a/drivers/net/ethernet/google/gve/gve_rx_dqo.c
> +++ b/drivers/net/ethernet/google/gve/gve_rx_dqo.c
> @@ -932,6 +932,10 @@ static int gve_rx_dqo(struct napi_struct *napi, struct gve_rx_ring *rx,
>  		if (xdp_act != XDP_PASS) {
>  			gve_xdp_done_dqo(priv, rx, &gve_xdp.xdp, xprog, xdp_act,
>  					 buf_state);
> +			u64_stats_update_begin(&rx->statss);
> +			rx->rpackets++;
> +			rx->rbytes += compl_desc->packet_len;
> +			u64_stats_update_end(&rx->statss);
>  			return 0;
>  		}

[Severity: Medium]
Is this new accounting reachable for AF_XDP zero-copy buffers?

gve_rx_dqo() dispatches those buffers before the xprog block above:

	xprog = READ_ONCE(priv->xdp_prog);
	if (buf_state->xsk_buff)
		return gve_rx_xsk_dqo(napi, rx, compl_desc, buf_state, xprog);

and gve_rx_xsk_dqo() takes its own non-PASS exit, which updates only
xdp_actions[] via gve_xsk_done_dqo() and leaves rx->ctx.skb_head NULL:

	if (xprog) {
		xdp_act = bpf_prog_run_xdp(xprog, xdp);
		buf_len = xdp->data_end - xdp->data;
		if (xdp_act != XDP_PASS) {
			gve_xsk_done_dqo(priv, rx, xdp, xprog, xdp_act);
			gve_free_buf_state(rx, buf_state);
			return 0;
		}
	}

With the reworked loop below, such a completion bumps work_done and then
hits "if (!rx->ctx.skb_head) continue;", so it is never added to
rx_packets/bytes either. Should a DQO queue running zero-copy with a
program that mostly returns XDP_DROP/XDP_TX/XDP_REDIRECT keep reporting
0 rx_packets/rx_bytes while the same program on the copy path now
reports them?

The GQI path counts XDP-consumed packets in rpackets/rbytes
(gve_rx_no_hsplit() does "ctx->total_size += frag_size; goto
finish_ok_pkt;"), so the DQO zero-copy path looks like the only
remaining outlier.

[Severity: Medium]
The commit message only describes the work_done change:

    Ensure every packet is accounted for in work_done by incrementing
    work_done before checking for the existence of a SKB.

Could it also mention that rx->rpackets/rx->rbytes semantics change?
XDP_DROP/XDP_ABORTED/XDP_TX/XDP_REDIRECT packets on the non-zero-copy
DQO path did not previously contribute to these counters, and
gve_get_stats() folds them into rtnl_link_stats64 rx_packets/rx_bytes:

	packets = priv->rx[ring].rpackets;
	bytes = priv->rx[ring].rbytes;
	...
	s->rx_packets += packets;
	s->rx_bytes += bytes;

The note "corrected stat counting for packets relative to work_done"
sits below the --- separator, so it will not be part of the git history,
and the patch carries Fixes: 293b49361f91 plus Cc: stable@vger.kernel.org.

> @@ -1090,6 +1094,7 @@ int gve_rx_poll_dqo(struct gve_notify_block *block, int budget)
>  	struct gve_rx_ring *rx;
>  	struct gve_priv *priv;
>  	u64 xdp_redirects;
> +	u32 rx_packets = 0;
>  	u32 work_done = 0;
>  	u64 bytes = 0;
>  	u64 xdp_txs;
> @@ -1150,13 +1155,14 @@ int gve_rx_poll_dqo(struct gve_notify_block *block, int budget)
>  		/* Free running counter of completed descriptors */
>  		rx->cnt++;
>  
> -		if (!rx->ctx.skb_head)
> -			continue;
> -
>  		if (!compl_desc->end_of_packet)
>  			continue;
>  
>  		work_done++;
> +
> +		if (!rx->ctx.skb_head)
> +			continue;
> +
>  		pkt_bytes = rx->ctx.skb_head->len;
>  		/* The ethernet header (first ETH_HLEN bytes) is snipped off
>  		 * by eth_type_trans.
> @@ -1164,6 +1170,9 @@ int gve_rx_poll_dqo(struct gve_notify_block *block, int budget)
>  		if (skb_headlen(rx->ctx.skb_head))
>  			pkt_bytes += ETH_HLEN;
>  
> +		rx_packets++;
> +		bytes += pkt_bytes;
> +
>  		/* gve_rx_complete_skb() will consume skb if successful */
>  		if (gve_rx_complete_skb(rx, napi, compl_desc, feat) != 0) {
>  			gve_rx_free_skb(napi, rx);

[Severity: Medium]
Related to the commit message question above: moving "bytes += pkt_bytes"
ahead of the gve_rx_complete_skb() failure branch means packets the
driver drops here (and also counted in rx_desc_err_dropped_pkt) now add
their bytes to rx_bytes, where previously they did not. Is that change
intended, and should it be described in the changelog?

> @@ -1173,7 +1182,6 @@ int gve_rx_poll_dqo(struct gve_notify_block *block, int budget)
>  			continue;
>  		}
>  
> -		bytes += pkt_bytes;
>  		rx->ctx.skb_head = NULL;
>  		rx->ctx.skb_tail = NULL;
>  	}
> @@ -1187,7 +1195,7 @@ int gve_rx_poll_dqo(struct gve_notify_block *block, int budget)
>  	gve_rx_post_buffers_dqo(rx);
>  
>  	u64_stats_update_begin(&rx->statss);
> -	rx->rpackets += work_done;
> +	rx->rpackets += rx_packets;
>  	rx->rbytes += bytes;
>  	u64_stats_update_end(&rx->statss);
>

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