From: Maciej Fijalkowski <maciej.fijalkowski@xxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxx>
> Closes: https://lore.kernel.org/netdev/20250530103456.53564-1-e.kubanski@xxxxxxxxxxxxxxxxxxx/
> Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@xxxxxxxxx>
> ---
> v1:
> https://lore.kernel.org/bpf/20250702101648.1942562-1-maciej.fijalkowski@xxxxxxxxx/
>
> 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 =\
Headroom is no that hacky; we even place &xdp_frame there :D
> ---
> net/xdp/xsk.c | 79 ++++++++++++++++++++++++++++++++++-----------
> net/xdp/xsk_queue.h | 12 +++++++
> 2 files changed, 73 insertions(+), 18 deletions(-)
[...]
> @@ -619,6 +646,12 @@ static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs,
> return ERR_PTR(err);
>
> skb_reserve(skb, hr);
> +
> + addrs = kzalloc(sizeof(*addrs), GFP_KERNEL);
Are you sure you can use sleeping GFP_KERNEL here, not GFP_ATOMIC?
> + if (!addrs)
> + return ERR_PTR(-ENOMEM);
> +
> + xsk_set_destructor_arg(skb, addrs);
> }
>
> addr = desc->addr;
Thanks,
Olek
