-
Notifications
You must be signed in to change notification settings - Fork 1k
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
base: main
Are you sure you want to change the base?
Conversation
From the CI results (i686-unknown-linux-musl) it seems like the machine does not have a uapi header with the constant. |
for the time being, need to add those constants as exception (for CI), as if musl then ignore those two. |
@devnexen Got it. I was wondering if someone tried to update the kernel headers for the musl CI. |
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 |
nice, it might be best to create a PR for this musl update alone tough. cc @tgross35 |
There was a problem hiding this 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.
Needs this one to pass CI: #3921 |
There was a problem hiding this 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
?
Head branch was pushed to by a user without write access
I will double check CI |
So musl for some reason hard codes the socket constants instead of fetching them from the OS: I will look for a way to test these constants only on gnu libc |
Head branch was pushed to by a user without write access
8871da8
to
c4eeb30
Compare
☔ The latest upstream changes (presumably #4132) made this pull request unmergeable. Please resolve the merge conflicts. |
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. |
libc-test/semver/TODO-linux.txt
Outdated
@@ -31,6 +31,7 @@ SO_ATTACH_REUSEPORT_EBPF | |||
SO_BINDTOIFINDEX | |||
SO_BPF_EXTENSIONS | |||
SO_BSDCOMPAT | |||
SO_BUSY_POLL_BUDGET |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, will do!
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
d05a44c
to
72bf054
Compare
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]>
Remove the comment of these socket options.
Reference: https://elixir.bootlin.com/linux/latest/source/include/uapi/asm-generic/socket.h