Re: [PATCH net-next v8 2/5] net: stmmac: drop split frames on AF_XDP zero-copy RX

[Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>]

[-- Attachment #1: Type: text/plain, Size: 9692 bytes --]

> 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 changes the discard test from `status == discard_frame`
>   to `status & discard_frame` in both stmmac_rx() and stmmac_rx_zc().
> - [Low] After this patch, stmmac_rx_zc() counts the same event (a frame
>   split across RX descriptors on an AF_XDP zero-copy queue) differently…
> - [Low] In stmmac_rx_zc() on dwmac4/XGMAC, a split frame can now be
>   counted in both rx_dropped and rx_errors.
> 
> Pre-existing issues:
> - [High] At this commit, stmmac_rx() (non-ZC, XDP attached, GMAC4/XGMAC
>   whose non-last descriptors report bare rx_not_ls) still drops a non-…
> - [Medium] stmmac_rx_zc() and stmmac_rx() add their local
>   rx_dropped/rx_errors into the device-wide `priv->xstats.rx_dropped`…
> 
> --
> 
> net: stmmac: drop split frames on AF_XDP zero-copy RX
> 
> In stmmac_rx_zc(), when the 1:1 XSK mapping check drops a non-last
> descriptor, this patch now marks the frame as errored. The rest of the
> split frame is then drained, and its tail is no longer handled as a new
> frame. The patch also makes the legacy enhanced and normal descriptor
> callbacks report rx_not_ls together with discard_frame, and changes the
> discard test in stmmac_rx_zc() and stmmac_rx() from == to &.
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/enh_desc.c b/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
> > index 0512536012251..297e2428a8f9b 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
> > @@ -184,7 +184,7 @@ static int enh_desc_get_rx_status(struct stmmac_extra_stats *x,
> >  
> >  	if (unlikely(!(rdes0 & RDES0_LAST_DESCRIPTOR))) {
> >  		x->rx_length++;
> > -		return discard_frame;
> > +		return discard_frame | rx_not_ls;
> >  	}
> 
> [Severity: Low]
> 
> With this change and the matching one in ndesc_get_rx_status(), does
> stmmac_rx_zc() now count the same split frame differently depending on
> the descriptor format?
> 
> On dwmac4 and xgmac, dwmac4_wrback_get_rx_status() and
> dwxgmac2_get_rx_status() return a bare rx_not_ls for a non-last
> descriptor:
> 
> 	if (likely(!(rdes3 & RDES3_LAST_DESCRIPTOR)))
> 		return rx_not_ls;
> 
> So the frame reaches the new 1:1 mapping drop and rx_dropped is
> incremented once.
> 
> On dwmac100/dwmac1000, non-last descriptors now carry discard_frame. The
> status & discard_frame block in stmmac_rx_zc() runs first and increments
> rx_errors for each non-last descriptor (when hwts_rx_en is off). The new
> rx_dropped++ in the 1:1 path is never reached on these cores.
> 
> An N-descriptor jumbo frame would then show up as 1 rx_dropped on
> GMAC4/XGMAC, but as N-1 rx_errors on dwmac1000. Both counters are
> exported through stmmac_get_stats64(). Is this asymmetry intended?
> 
> The legacy cores already counted rx_errors per descriptor before this
> patch; what is new is the different rx_dropped accounting. Later in the
> series, "net: stmmac: rework stmmac_rx to support XDP rx multi-buff"
> changes stmmac_rx() to test !error && (status & discard_frame), so it
> counts once per frame. stmmac_rx_zc() still counts per descriptor at the
> end of the series.
> 
> [ ... ]
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 8fd2acb25fb60..efa88cd359dac 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> 
> [ ... ]
> 
> > @@ -5715,8 +5718,9 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
> >  		if (likely(status & rx_not_ls)) {
> >  			xsk_buff_free(buf->xdp);
> >  			buf->xdp = NULL;
> > +			rx_dropped++;
> > +			error = 1;
> >  			dirty++;
> > -			count++;
> >  			goto read_again;
> >  		}
> >  
> 
> [Severity: Low]
> 
> Can one split frame on dwmac4/XGMAC now be counted in both rx_dropped and
> rx_errors?

ack, I will fix it in the next revision.

> 
> The first non-last descriptor takes this path, which increments
> rx_dropped and sets error = 1. Suppose the last descriptor of the same
> frame then reports discard_frame (RDES3_ERROR_SUMMARY, for example a CRC
> or giant error on an oversized frame). The earlier block in
> stmmac_rx_zc() still runs:
> 
> 		if (unlikely(status & discard_frame)) {
> 			error = 1;
> 			if (!priv->hwts_rx_en)
> 				rx_errors++;
> 		}
> 
> This happens even though error is already set for this frame.
> stmmac_get_stats64() exports both totals as rx_dropped and rx_errors.
> 
> Later in the series, "net: stmmac: rework stmmac_rx to support XDP rx
> multi-buff" avoids this in stmmac_rx() by testing !error && (status &
> discard_frame). stmmac_rx_zc() is not changed, so the double count is
> still there at the end of the series.
> 
> [Severity: Medium]
> 
> This is a pre-existing issue and was not introduced by this patch. The
> new rx_dropped++ feeds this accumulation at the end of stmmac_rx_zc():

Since this issue has not been introduced by this patch I guess we can fix it
with a dedicated patch.
> 
> 	priv->xstats.rx_dropped += rx_dropped;
> 	priv->xstats.rx_errors += rx_errors;
> 
> These are plain unsigned long fields in struct stmmac_extra_stats,
> shared by the whole device. Each RX channel has its own NAPI instance,
> so stmmac_rx_zc() and stmmac_rx() can run at the same time for different
> queues on different CPUs.
> 
> Can these non-atomic read-modify-writes lose updates?
> stmmac_get_stats64() also reads the fields without any synchronization.
> 
> stmmac_rx() does the same accumulation, and it is unchanged at the end
> of the series.
> 
> > @@ -5883,7 +5887,8 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
> >  
> >  		if (priv->extend_desc)
> >  			stmmac_rx_extended_status(priv, &priv->xstats, rx_q->dma_erx + entry);
> > -		if (unlikely(status == discard_frame)) {
> > +
> > +		if (unlikely(status & discard_frame)) {
> 
> [Severity: Low]
> 
> Changing == to & here and in stmmac_rx_zc() also changes the normal RX
> path on dwmac4/GMAC4/GMAC5. Should the commit message mention this?
> 
> dwmac4_wrback_get_rx_status() sets ret = discard_frame when
> RDES3_ERROR_SUMMARY is set. It then ORs in csum_none for IP header or
> payload errors:
> 
> 	if (rdes1 & RDES1_IP_HDR_ERROR) {
> 		x->ip_hdr_err++;
> 		ret |= csum_none;
> 	}
> 
> So a frame with both an L2 error and an IP checksum error reports
> discard_frame | csum_none.
> 
> Before this patch, such a frame failed the == test. It was run through
> XDP and passed to napi_gro_receive() with CHECKSUM_NONE. Now it is
> dropped and counted in rx_errors.
> 
> The new behaviour looks correct. It stops errored frames from reaching
> the stack, which has been possible since commit 644b8437ccef9e ("net:
> stmmac: dwmac4: report Rx checksum errors in status").

I will add the missing Fixes tag in the next revision.

> 
> However, the commit message presents the operator change only as a
> result of the enh_desc/norm_desc normalization, and limits the scope to
> AF_XDP zero-copy. Could it describe this GMAC4 change and reference
> 644b8437ccef9e?
> 
> >  			page_pool_put_page(rx_q->page_pool, buf->page, 0, true);
> >  			buf->page = NULL;
> >  			error = 1;
> 
> [Severity: High]
> 
> This isn't a bug introduced by this patch. The commit message says the
> change lets stmmac_rx() drain split frames, which holds for
> dwmac100/dwmac1000.

Since this issue has not been introduced by this patch I guess we can fix it
with a dedicated patch.

Regards,
Lorenzo

> 
> GMAC4/XGMAC report non-last descriptors as a bare rx_not_ls. On those
> cores, with an XDP program attached, are there still paths in
> stmmac_rx() that drop a non-last descriptor without setting error?
> 
> When the XDP verdict is STMMAC_XDP_CONSUMED:
> 
> 				if (xdp_res & STMMAC_XDP_CONSUMED) {
> 					...
> 					skb = NULL;
> 
> 					if (unlikely((status & rx_not_ls)))
> 						goto read_again;
> 
> error is still 0 here, so the next fragment goes through XDP as a new
> frame. With a 1-3 byte tail, wouldn't buf1_len -= ETH_FCS_LEN underflow?
> 
> For XDP_TX/XDP_REDIRECT:
> 
> 				} else if (xdp_res & (STMMAC_XDP_TX |
> 						      STMMAC_XDP_REDIRECT)) {
> 					xdp_status |= xdp_res;
> 					buf->page = NULL;
> 					goto next;
> 				}
> 
> goto next clears in_progress even when rx_not_ls is set. The loop head
> then resets error and len, and the tail is reparsed with len == 0.
> 
> When napi_build_skb() fails:
> 
> 				rx_dropped++;
> 				count++;
> 				goto drain_data;
> 
> error is not set here either, so a later descriptor can build an skb from
> only the end of the frame.
> 
> This is the same class of bug the patch fixes in stmmac_rx_zc(). An
> on-link peer can trigger it, since GMAC_CONFIG_JE is always set on
> GMAC4.
> 
> It looks like later patches in the same series fix it: "net: stmmac:
> drop multi-buffer frames in XDP mode" and "net: stmmac: rework stmmac_rx
> to support XDP rx multi-buff". After those, XDP and stmmac_build_skb()
> only run on the last descriptor, and failures go to error_free_frag.
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261018-stmmac-rx-mb-v8-0-5f58c0b25849%40oss.qualcomm.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]