Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libxdp: Use opts to create XDP socket #454

Merged
merged 1 commit into from
Dec 12, 2024

Conversation

tacslon
Copy link
Contributor

@tacslon tacslon commented Nov 20, 2024

Introduce a new API xsk_socket__create_opts() to create XDP socket using opts struct. Note that if one of fill and comp is unset, the semantic of the new API is the same as xsk_socket__create(), otherwise the same as xsk_socket__create_shared().

Copy link
Member

@tohojo tohojo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - @magnus-karlsson PTAL

Copy link
Member

@magnus-karlsson magnus-karlsson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this implementation. Looks good, just some small things to fix.

* The following fields should not be NULL at the same time:
*
* @rx, @tx
* At least one traffic direction should be assigned for an XSk.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

XSk -> xsk

*
* @fill, @comp, @rx_size, @tx_size, @libbpf_flags, @libxdp_flags,
* @xdp_flags, @bind_flags
* If a socket with exclusive ownership of a umem are going to be
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are -> is

* If the remaining fields are unset, they will be set to
* default value (see `xsk_set_xdp_socket_config()`).
*
* Except for the fields mentioned above, none field can be set.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"none field"? What does that mean?

__u32 rx_size;
__u32 tx_size;
union {
__u32 libbpf_flags;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this new interface, let us delete libbpf_flags and be done with the union. libbpf_flags was there just for compatibility with libbpf version prior to 1.0. For this new interface, we can skip this.

lib/libxdp/xsk.c Outdated

if (usr_cfg->libbpf_flags & ~XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD)
libbpf_flags = OPTS_GET(opts, libbpf_flags, 0);
if (libbpf_flags & ~XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use LIBXDP instead of LIBBPF in this whole function. Again, the LIBBPF stuff in the interface is only there for some old libbpf compatibility. The local variable can be libxdp_flags too since we are in libxdp :-).

Introduce a new API xsk_socket__create_opts() to create XDP socket using opts struct.
Note that if one of fill and comp is unset, the semantic of the new API is the same
as xsk_socket__create(), otherwise the same as xsk_socket__create_shared().

Signed-off-by: Muyang Tian <[email protected]>
@tohojo
Copy link
Member

tohojo commented Dec 12, 2024

Updated changes LGTM - @magnus-karlsson WDYT?

Copy link
Member

@magnus-karlsson magnus-karlsson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you!

@tacslon
Copy link
Contributor Author

tacslon commented Dec 12, 2024

The test is failing, can be re-run @magnus-karlsson @tohojo
selftest (6.12.0-65.fc42, 17) (pull_request)

@tohojo tohojo merged commit abff006 into xdp-project:master Dec 12, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants