Re: [PATCH v2 bpf] xsk: fix immature cq descriptor production

[Stanislav Fomichev <stfomichev@gmail.com>]

On 07/07, Alexander Lobakin wrote:
> From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> Date: Sat,  5 Jul 2025 15:55:12 +0200
> 
> > Eryk reported an issue that I have put under Closes: tag, related to
> > umem addrs being prematurely produced onto pool's completion queue.
> > Let us make the skb's destructor responsible for producing all addrs
> > that given skb used.
> > 
> > Commit from fixes tag introduced the buggy behavior, it was not broken
> > from day 1, but rather when xsk multi-buffer got introduced.
> > 
> > Introduce a struct which will carry descriptor count with array of
> > addresses taken from processed descriptors that will be carried via
> > skb_shared_info::destructor_arg. This way we can refer to it within
> > xsk_destruct_skb().
> > 
> > To summarize, behavior is changed from:
> > - produce addr to cq, increase cq's cached_prod
> > - increment descriptor count and store it on
> > - (xmit and rest of path...)
> >   skb_shared_info::destructor_arg
> > - use destructor_arg on skb destructor to update global state of cq
> >   producer
> > 
> > to the following:
> > - increment cq's cached_prod
> > - increment descriptor count, save xdp_desc::addr in custom array and
> >   store this custom array on skb_shared_info::destructor_arg
> > - (xmit and rest of path...)
> > - use destructor_arg on skb destructor to walk the array of addrs and
> >   write them to cq and finally update global state of cq producer
> > 
> > Fixes: b7f72a30e9ac ("xsk: introduce wrappers and helpers for supporting multi-buffer in Tx path")
> > Reported-by: Eryk Kubanski <e.kubanski@partner.samsung.com>
> > Closes: https://lore.kernel.org/netdev/20250530103456.53564-1-e.kubanski@partner.samsung.com/
> > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > ---
> > v1:
> > https://lore.kernel.org/bpf/20250702101648.1942562-1-maciej.fijalkowski@intel.com/
> > 
> > v1->v2:
> > * store addrs in array carried via destructor_arg instead having them
> >   stored in skb headroom; cleaner and less hacky approach;
> 
> Might look cleaner, but what about the performance given that you're
> adding a memory allocation?
> 
> (I realize that's only for the skb mode, still)
> 
> Yeah we anyway allocate an skb and may even copy the whole frame, just
> curious.
> I could recommend using skb->cb for that, but its 48 bytes would cover
> only 6 addresses =\

Can we pre-allocate an array of xsk_addrs during xsk_bind (the number of
xsk_addrs is bound by the tx ring size)? Then we can remove the alloc on tx
and replace it with some code to manage that pool of xsk_addrs..