-
Notifications
You must be signed in to change notification settings - Fork 260
Add simple interface for busy_poll on Linux #607
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
Conversation
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.
Can we have a test for this as well? Since it requires CAP_NET_ADMIN
, we probably need to ignore it and try it locally, see for example set_mark
.
src/socket.rs
Outdated
/// - Requires `CAP_NET_ADMIN` capability to increase the value | ||
/// - Default value is controlled by `/proc/sys/net/core/busy_read` | ||
/// - Available since Linux 3.11 | ||
#[cfg(target_os = "linux")] |
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.
This needs to be behind the all
feature, see other functions that have limited cross-OS availability.
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'm not familiar with all the targets, but I'll try?
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 manually searched and used the API and found that none of the targets listed in the README support busy polling. Here is the link to the documentation I found manually. So, I guess there is nothing else.
Apple
Windows
android
FreeBSD
Fuchsia
illumos
NetBSD
Redox
Solaris
OpenHarmony
I found a great list btw: https://docs.rs/rustix/latest/rustix/net/sockopt/index.html
src/socket.rs
Outdated
/// - Default value is controlled by `/proc/sys/net/core/busy_read` | ||
/// - Available since Linux 3.11 | ||
#[cfg(target_os = "linux")] | ||
pub fn set_busy_poll(&self, busy_poll: u16) -> io::Result<()> { |
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.
Why u16
?
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.
OK, this is my question. When I saw that most C code used int, I was confused why it was not unsigned int. I thought this type problem should be corrected, so I filled in u16. Now I will change it to u32 next time.
src/socket.rs
Outdated
#[cfg(target_os = "linux")] | ||
pub fn set_busy_poll(&self, busy_poll: u16) -> io::Result<()> { | ||
sys::set_busy_poll(self.as_raw(), busy_poll) | ||
} |
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.
Can we have a getting for this as well?
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.
Well, if you could specify I'd appreciate it, basically I copied the code from set_nonblocking.
Oh, set_mark is that one, that makes things easier.
src/socket.rs
Outdated
/// | ||
/// - Requires `CAP_NET_ADMIN` capability to increase the value | ||
/// - Default value is controlled by `/proc/sys/net/core/busy_read` | ||
/// - Available since Linux 3.11 |
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.
Can we replace these notes with only calling out SO_BUSY_POLL
, similar to the other functions. I don't want to copy all notes from the man page.
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.
Yeah, if it's your wish.
* Change the documentation comment of `set_busy_poll` * Move setter into unix under `all` feature * Add a getter
Thanks @nan-mu |
For #606