Re: [PATCH net v3 3/5] eth: fbnic: return a pending Rx packet before reusing its buffer

[netdev-bot+sashiko]

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 patch adds a second place where an Rx packet gets thrown
  away, and that path is not counted in any statistic.

--

eth: fbnic: return a pending Rx packet before reusing its buffer

This patch changes fbnic_pkt_prepare() so it no longer only warns when a
new header completion arrives while a packet is still pending. It now
returns the pending header page and any attached fragments to the page
pool, then clears data_hard_start. To make this possible,
fbnic_put_pkt_buff() is moved above fbnic_pkt_prepare().

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> index 7dda59bba14ee..47ded7a9e0a81 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c

[ ... ]

> @@ -1025,8 +1052,12 @@ static void fbnic_pkt_prepare(struct fbnic_napi_vector *nv, u64 rcd,
>  	unsigned char *hdr_start;
>  	struct page *page;
>  
> -	/* data_hard_start should always be NULL when this is called */
> -	WARN_ON_ONCE(pkt->buff.data_hard_start);
> +	/* a new header while one is pending: release the old one */
> +	if (unlikely(pkt->buff.data_hard_start)) {
> +		WARN_ON_ONCE(1);
> +		fbnic_put_pkt_buff(qt, pkt, 1);
> +		pkt->buff.data_hard_start = NULL;
> +	}

[Severity: Low]

Should this new drop path also be counted in the Rx drop statistics?

Every other place that discards a packet in the META handling of
fbnic_clean_rcq() counts it before calling fbnic_put_pkt_buff(). That
includes a META completion that arrives with no header:

drivers/net/ethernet/meta/fbnic/fbnic_txrx.c:fbnic_clean_rcq() {
    ...
			} else {
				if (!skb)
					alloc_failed++;

				if (skb == ERR_PTR(-FBNIC_XDP_LEN_ERR))
					length_errors++;
				else
					dropped++;

				fbnic_put_pkt_buff(qt, pkt, 1);
				goto next_dont_count;
			}
    ...
}

Suppose the device sends HDR_AL(A), [PAY_AL ...], HDR_AL(B), ..., META.
Packet A is then freed here and nothing increments dropped, length_errors
or alloc_failed. So rcq->stats.dropped, rx_dropped and the per-queue stats
do not show it. Only the first such event per boot leaves a trace, through
WARN_ON_ONCE().

Before this patch the packet was also not counted, because it was leaked.
Now that it is dropped on purpose, should fbnic_pkt_prepare() update
rcq->stats (or report back to fbnic_clean_rcq() so it can bump dropped)?
That would match how the other drop paths are counted.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1791775409.git.yhlee%40isslab.korea.ac.kr