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 epoll_pwait2() if available #295

Closed
wants to merge 1 commit into from

Conversation

panjf2000
Copy link
Contributor

@panjf2000 panjf2000 commented Apr 11, 2024

epoll_pwait2() was introduced in Linux kernel 5.11, it extends epoll_wait()/epoll_pwait() by allowing the timeout to be specified with a higher resolution, as a timespec type, providing nanosecond precision.

By using epoll_pwait2(), we can resolve the problem in redis/redis#8764 faultlessly, once and for all. Furthermore, it would also offer the possibility for us to handle timers in a higher resolution in the future: from microseconds to nanoseconds (if necessary).

`epoll_pwait2()` was introduced in Linux kernel 5.11, it extends
`epoll_wait()`/`epoll_pwait()` by allowing the `timeout` to be specified with a
higher resolution, as a `timespec` type, providing nanosecond precision.

By using `epoll_pwait2()`, we can resolve the problem in #8764 faultlessly,
once and for all. Furthermore, it would also offer the possibility for us to
handle timers in a higher resolution in the future: from microseconds to
nanoseconds (if necessary).

Signed-off-by: Andy Pan <[email protected]>
@panjf2000
Copy link
Contributor Author

See also #296

@panjf2000
Copy link
Contributor Author

Hi @zuiderkwast, since you've reviewed redis/redis#13005 before, and we're now outside the redis scope, I'd like you to take a look at this, I still reckon the current code with epoll_wait() as a workaround and epoll_pwait2() is the fundamental solution.

@zuiderkwast
Copy link
Contributor

zuiderkwast commented Apr 15, 2024

Hi @panjf2000,

I still reckon the current code with epoll_wait() as a workaround

As a fallback, not workaround. That's why we need to keep the code with epoll_wait(). Like Oran's wrote:

as soon as we merge this PR, the current code becomes fallback, and therefore untested. i'd argue that that is unacceptable. if a bug is introduced, we won't find it.

So, to test it, we need some CI job which runs on a an old linux kernel without epoll_pwait2 to make sure it is not untested. I don't know if we have that.

Furthermore, it adds complexity in the makefile and conditional C code, but there is no real benefit such as performance optimization.

if this PR would have add some performance optimization, then i'd probably accept that risk, but i argue that it doesn't, and therefore i argue that it does more harm than good.

let's meet again in 10 years and merge it. 😉

What he means is that when nobody is using old linux without epoll_wait2, we can just replace epoll_wait with epoll_wait2 without conditional compilation.

@zuiderkwast zuiderkwast added the wontfix This will not be worked on label Apr 15, 2024
@panjf2000
Copy link
Contributor Author

panjf2000 commented Apr 15, 2024

When I said workaround, I meant it's a workaround to the issue that it's trying to fix, not as a workaround compared to epoll_pwait2().

So, to test it, we need some CI job which runs on a an old linux kernel without epoll_pwait2 to make sure it is not untested. I don't know if we have that.

I really don't understand this one. The current code has been being tested on kernel >=5.11 or <5.11 for years and will continue to run on kernel <5.11. For kernel >=5.11, it switches to epoll_pwait2 which will be tested thoroughly.

There are irreconcilable differences between us on this matter, and I kept trying and failing to get your points, now I'm going to give up and let it go.

@panjf2000 panjf2000 closed this Apr 15, 2024
@panjf2000 panjf2000 deleted the epoll_pwait2 branch April 15, 2024 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants