Marcin Knap via B4 Relay <[email protected]> writes:
> From: Marcin Knap <[email protected]> > > When an XDP program redirects a zero-copy buffer to an XSK and the > socket's Rx ring is full, xsk_rcv_zc() frees the buffer and returns > -ENOBUFS to xdp_do_redirect(). > > A failed xdp_do_redirect() leaves the buffer owned by the caller, and > most drivers rely on that: mlx5e treats the failure as XDP_DROP and > releases the buffer when the Rx WQE is refilled, while i40e, ice, > ixgbe, igb, igc, stmmac, idpf and others free it themselves. The > buffer is therefore freed twice. It gets reused for another Rx > descriptor, the second free puts an in-use buffer back on the pool > free list, and the same UMEM frame is later handed to user space > twice. ixgbe and igb keep the buffer in the ring on their need_wakeup > -ENOBUFS (XDP_EXIT) path and run it through XDP again on the next poll, > after the core has freed it. > > Leave the buffer with the caller on both the single-buffer and the > multi-buffer -ENOBUFS paths, as xsk_rcv() and xsk_rcv_check() already > do. A few drivers relied on the core freeing the buffer, so make them > free it: > > - i40e and ice consume the descriptor on XDP_EXIT without freeing it. > - gve DQO counts the redirect error without freeing the buffer. > - octeontx2 returns false on a failed redirect, so the caller goes on > to build an skb from the already freed buffer. Free the buffer and > report it as consumed instead. > > Seen on Mellanox ConnectX-6 DX MT2892 (FW 22.47.1026) with AF_XDP > zero-copy on 7.3.0-rc6 when the user-space consumer could not keep up > with the Rx ring. With this change no duplicate frames were seen with > striding RQ on or off. > > Fixes: 269009893146 ("xsk: recycle buffer in case Rx queue was full") > Signed-off-by: Marcin Knap <[email protected]> > --- > Tested on mlx5 (ConnectX-6 DX) in zero-copy single-buffer mode. > The i40e and ice changes and the multi-buffer path are build-tested > only, as I don't have that hardware available. > --- > Changes in v2: > - Also free the buffer on zero-copy redirect failure in gve DQO and > octeontx2, which relied on the core freeing it (AI review). > - Audited the remaining zero-copy drivers against Björn's table. > ixgbe and igb no longer run XDP on a freed buffer after > XDP_EXIT; making them drop and consume the descriptor (R2) is > left for a follow-up unless you prefer it in this patch. > - Rebased on current bpf. > - Link to v1: > https://patch.msgid.link/[email protected] Thank you for catching, and cleaning up the drivers. I can follow-up with the documentation [1], or if you respin, feel free to add it yourself. Reviewed-by: Björn Töpel <[email protected]> [1] https://gist.github.com/fb-bjorn/d6835c49650bc3433b427a9c9708fbe2
