libxdp: Add tx_metadata_len/xsk_umem__create_opts() to support AF_XDP Tx metadata

[tacslon]

Currently, we cannot assign metadata length in param config when calling xsk_umem__create(). This PR adds the support to assign tx_metadata_len when creating umem.
Also, adding xsk_umem__create_opts() to provide a new way to create umem by opts macro.

[magnus-karlsson]

Thanks for implementing this. Some comments that need addressing in the review.

[tohojo]

talon ***@***.***> writes:

> We reuse the opts macros from libbpf - see `struct xdp_program_opts` ``` #define xdp_program_opts__last_field fd #define DECLARE_LIBXDP_OPTS DECLARE_LIBBPF_OPTS ``` @tohojo Thanks, I can see macros listed above, how to use these macros to version the interface?

See example usage in xdp_program__create(). Unfortunately, the existing struct xsk_umem_config doesn't support the opts macro usage since it doesn't contain a 'sz' field as its first member. So to support this way of extending opts, a new version of that struct would need to be defined, and a new set of functions taking the opts instead of the old struct. So replacements for xsk_umem__create() and xsk_umem__create_with_fd(). This could be a single function, I suppose (xdp_umem__create_opts() or something similar?) with some/most of the function parameters being moved to be opts struct members. This is a matter of taste, so I'll let Magnus chime in here with his preference...

[magnus-karlsson]

On Tue, 15 Oct 2024 at 23:07, Toke Høiland-Jørgensen < ***@***.***> wrote: talon ***@***.***> writes: >> We reuse the opts macros from libbpf - see `struct xdp_program_opts` > ``` > #define xdp_program_opts__last_field fd > > #define DECLARE_LIBXDP_OPTS DECLARE_LIBBPF_OPTS > ``` > @tohojo Thanks, I can see macros listed above, how to use these macros to version the interface? See example usage in xdp_program__create(). Unfortunately, the existing struct xsk_umem_config doesn't support the opts macro usage since it doesn't contain a 'sz' field as its first member. So to support this way of extending opts, a new version of that struct would need to be defined, and a new set of functions taking the opts instead of the old struct. So replacements for xsk_umem__create() and xsk_umem__create_with_fd(). This could be a single function, I suppose (xdp_umem__create_opts() or something similar?) with some/most of the function parameters being moved to be opts struct members. This is a matter of taste, so I'll let Magnus chime in here with his preference... The least evil here seems to be a new function. The name Toke proposes is

fine. And add a comment that users should use the new version.

…

— Reply to this email directly, view it on GitHub <#443 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AASGUEM554MSG2UM7LSLHALZ3V7ZJAVCNFSM6AAAAABPWUWX3GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMJVGE3DEMRRHE> . You are receiving this because your review was requested.Message ID: ***@***.***>

[tacslon]

@tohojo Requested changes are committed, ptal, thanks!

[tohojo]

Only a few nits, otherwise the opts usage LGTM :)

[tacslon]

Only a few nits, otherwise the opts usage LGTM :)

@tohojo @magnus-karlsson New changes are committed, ptal, thanks!

[tohojo]

Alright, so from an opts API point of view, it basically looks good now. @magnus-karlsson could you please take a look as well and approve from an XSK PoV?

One issue to flag here is that we are technically changing the semantics of xsk_umem__create_with_fd() slightly: before, 0 was accepted as a valid socket fd, where now it isn't, even for the old function. I don't think this will be an issue in practice, as the only way this can happen is if socket() itself returns 0 as the fd, but wanted to flag it just the same. What is your feeling, @magnus-karlsson - is this OK?

[tacslon]

The 2 failing selftests are caused by failing to load xdp_handle_tcp_recv prog, maybe can be retested