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

Add SO_PREFER_BUSY_POLL and SO_BUSY_POLL_BUDGET #3917

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

tammela
Copy link
Contributor

@tammela tammela commented Sep 11, 2024

Remove the comment of these socket options.
Reference: https://elixir.bootlin.com/linux/latest/source/include/uapi/asm-generic/socket.h

@rustbot
Copy link
Collaborator

rustbot commented Sep 11, 2024

r? @tgross35

rustbot has assigned @tgross35.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@tammela
Copy link
Contributor Author

tammela commented Sep 11, 2024

From the CI results (i686-unknown-linux-musl) it seems like the machine does not have a uapi header with the constant.
They were added in 5.11 and the sanitized headers are in 4.19: https://github.com/rust-lang/libc/blob/main/ci/install-musl.sh#L68

@devnexen
Copy link
Contributor

devnexen commented Sep 11, 2024

for the time being, need to add those constants as exception (for CI), as if musl then ignore those two.

@tammela
Copy link
Contributor Author

tammela commented Sep 12, 2024

@devnexen Got it. I was wondering if someone tried to update the kernel headers for the musl CI.
Alpine uses musl as well and they don't seem to use sabotage-linux headers but rather roll their own (currently targeting 6.6 soon to be 6.10):
https://git.alpinelinux.org/aports/tree/main/linux-headers?h=master

@tammela
Copy link
Contributor Author

tammela commented Sep 12, 2024

I updated the CI to use the alpine headers here: tammela@17af7ba

Seems to be passing the CI scripts, let me know if it's worth the shot and I can open up a PR

@devnexen
Copy link
Contributor

nice, it might be best to create a PR for this musl update alone tough. cc @tgross35

Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

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

Huh, no clue why those were commented out but looks like it has been that way since #2135. Anyway lgtm, thanks for including the link.

@tgross35 tgross35 enabled auto-merge September 30, 2024 16:13
@tgross35 tgross35 added the stable-nominated This PR should be considered for cherry-pick to libc's stable release branch label Sep 30, 2024
@tammela
Copy link
Contributor Author

tammela commented Sep 30, 2024

Needs this one to pass CI: #3921

Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

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

#3921 should be merged soon, would you mind rebasing to resolve conflicts?

Also, could you add these to the semver tests at libc-test/semver/linux.txt?

auto-merge was automatically disabled November 21, 2024 16:26

Head branch was pushed to by a user without write access

@tgross35 tgross35 enabled auto-merge November 21, 2024 19:00
@tammela
Copy link
Contributor Author

tammela commented Nov 21, 2024

I will double check CI

@tammela
Copy link
Contributor Author

tammela commented Nov 21, 2024

So musl for some reason hard codes the socket constants instead of fetching them from the OS:
https://git.musl-libc.org/cgit/musl/tree/include/sys/socket.h#n293

I will look for a way to test these constants only on gnu libc

auto-merge was automatically disabled November 26, 2024 19:19

Head branch was pushed to by a user without write access

@tammela tammela force-pushed the patch-1 branch 2 times, most recently from 8871da8 to c4eeb30 Compare November 26, 2024 19:30
@bors
Copy link
Contributor

bors commented Nov 27, 2024

☔ The latest upstream changes (presumably #4132) made this pull request unmergeable. Please resolve the merge conflicts.

@tgross35
Copy link
Contributor

It looks like CI is passing - is this ready to be merged or is there something else to be done? (I've held off because of the labels)

@tammela
Copy link
Contributor Author

tammela commented Nov 27, 2024

It looks like CI is passing - is this ready to be merged or is there something else to be done? (I've held off because of the labels)

I gave in and gated the new constants for !musl. So it should be good from my side.
Musl made the decision to hardcode the SO_ constants instead of inherit them from the OS via #include <asm-generic/socket.h>.
Essentially updating the kernel headers is not enough, you have to hope a recent musl version kept up with the new constants...
The next musl version update will likely fill in the gap (till 70), but linux already added a bunch more :)

@@ -31,6 +31,7 @@ SO_ATTACH_REUSEPORT_EBPF
SO_BINDTOIFINDEX
SO_BPF_EXTENSIONS
SO_BSDCOMPAT
SO_BUSY_POLL_BUDGET
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you instead add these to linux-gnu.txt or linux-gnu-x86_64.txt instead?

Since there is something left to do, maybe leave a // FIXME(musl) in the .rs file rather than using the TODO semver.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, will do!

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it needs to go in one of the linux-gnu* files rather than linux-{arch} since these aren't yet available on musl

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added to the x86 one, but it should be fine for everything except musl, but it doesn't seem to have such a file

@tammela tammela force-pushed the patch-1 branch 2 times, most recently from d05a44c to 72bf054 Compare November 27, 2024 20:31
Remove the comment of these socket options.
Reference: https://elixir.bootlin.com/linux/latest/source/include/uapi/asm-generic/socket.h

Note, musl hardcodes 'SO_*' constants instead of inheriting them from the OS.

Signed-off-by: Pedro Tammela <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-linux O-unix S-waiting-on-author stable-nominated This PR should be considered for cherry-pick to libc's stable release branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants