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

Use the proper netlink buffer size with large kernel pages #258

Merged
merged 1 commit into from
May 30, 2023

Conversation

refi64
Copy link
Contributor

@refi64 refi64 commented May 27, 2023

The recommended netlink buffer size is based on the system's page size, which means that the current size is far too small for systems with 16k or 64k pages, such as Asahi Linux or RHEL's kernel-64k for ARM64. On these systems, the server fails to start with errors like this:

Error: Decode error occurred: invalid netlink buffer: length field says 1444 the buffer is 1260 bytes long

Instead, follow the kernel's own netlink docs to compute the buffer size. The approach here matches the approach merged into Chromium recently:

https://chromium-review.googlesource.com/c/chromium/src/+/4312885

Copy link
Member

@bschwind bschwind left a comment

Choose a reason for hiding this comment

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

Good catch here! The code looks good, I just have some mostly trivial comments on naming and such.

netlink-request/src/lib.rs Outdated Show resolved Hide resolved
netlink-request/src/lib.rs Outdated Show resolved Hide resolved
@refi64 refi64 force-pushed the netlink-buffer-size branch from 18b1c1f to a1062df Compare May 28, 2023 23:03
@bschwind bschwind requested review from strohel and mcginty May 29, 2023 05:26
Copy link
Member

@bschwind bschwind left a comment

Choose a reason for hiding this comment

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

LGTM++, I'll wait for at least another review before merging.

Copy link
Member

@strohel strohel left a comment

Choose a reason for hiding this comment

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

Looking great! Just one little suggestion wrt dependencies.

I guess the sysconf() call is rather cheap and we don't have to think about caching it (beyond local variable caching already present in this PR)?

netlink-request/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Collaborator

@mcginty mcginty left a comment

Choose a reason for hiding this comment

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

Nice comments, nice fix!

+1 for @strohel's suggestion of not using default features in nix.

On my machine, sysconf is <1ms but is called >200 times when running innernet up <network> given that it's called in a pretty low-level function. I don't think it's a bad idea just from a cleanliness perspective to throw it in a OnceCell, especially since Rust will likely stabilize a similar API in std soon enough.

The recommended netlink buffer size is based on the system's page size,
which means that the current size is far too small for systems with 16k
or 64k pages, such as Asahi Linux or RHEL's kernel-64k for ARM64. On
these systems, the server fails to start with errors like this:

Error: Decode error occurred: invalid netlink buffer: length field says 1444 the buffer is 1260 bytes long

Instead, follow the kernel's own netlink docs to compute the buffer
size. The approach here matches the approach merged into Chromium
recently:

https://chromium-review.googlesource.com/c/chromium/src/+/4312885
@refi64 refi64 force-pushed the netlink-buffer-size branch from a1062df to 8485d96 Compare May 30, 2023 00:19
Copy link
Collaborator

@mcginty mcginty left a comment

Choose a reason for hiding this comment

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

OnceCell change looks good!

Copy link
Member

@strohel strohel left a comment

Choose a reason for hiding this comment

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

Thanks, looking great now. Merging.

@strohel strohel merged commit f67457e into tonarino:main May 30, 2023
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.

4 participants