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

runtime: use epoll_pwait2 where available for sub-millisecond timeout precision on Linux #53824

Open
ianlancetaylor opened this issue Jul 13, 2022 · 24 comments
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. help wanted NeedsFix The path to resolution is known, but the work has not been done. Performance
Milestone

Comments

@ianlancetaylor
Copy link
Contributor

Linux kernel 5.11 (February, 2021) added the epoll_pwait2 system call. It is exactly like epoll_pwait except that the timeout argument is a struct timespec. This permits specifying a timeout with nanosecond precision, where epoll_pwait and epoll_wait only permit millisecond precision. We should change runtime/netpoll_epoll.go to use epoll_pwait2 when available to get higher precision delays.

CC @golang/runtime

@ianlancetaylor ianlancetaylor added Performance help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Jul 13, 2022
@ianlancetaylor ianlancetaylor added this to the Backlog milestone Jul 13, 2022
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 13, 2022
@prattmic
Copy link
Member

Specifically, this will help with #44343 (comment) on Linux.

@dAdAbird
Copy link
Contributor

Does anybody work on this? I can take on it.

@ianlancetaylor
Copy link
Contributor Author

Thanks. I suggest that you sketch out what you will do before implementing, to make sure that it seems like a good approach.

@dAdAbird
Copy link
Contributor

@ianlancetaylor, I've just discovered the patch from @prattmic: https://go.dev/cl/363417.
As far as I can tell, it only lacks runtime·epollwait2 implementations for other than amd64 platforms. Are there some reasons you didn't move with the CL 363417 forward? Thanks!

@prattmic
Copy link
Member

Mostly because I simply haven't gotten around to it. You are welcome to base a change off of my CL.

I also did want to do some benchmarking (probably with https://cs.opensource.google/go/x/benchmarks/+/master:sweet/) to check for performance regressions, since more precise timeouts could potentially lead to more frequent wakeups.

@dAdAbird
Copy link
Contributor

Thanks, @prattmic. I'll prepare CL then and will do some benchmarking.

@aclements
Copy link
Member

aclements commented Jul 16, 2022 via email

@dAdAbird
Copy link
Contributor

Oh, cool, thanks for the advice Austin!

@dAdAbird
Copy link
Contributor

I have a few questions. @prattmic, in your CL you use _ENOSYS to define if the syscall exists. But I cannot find its definition anywhere other than in the "golang.org.x/sys/unix" package (ENOSYS there).

And in a broader context, if we go with runtime/internal/syscall, where should syscall numbers (e.g. SYS_epoll_pwait = 281, SYS_epoll_pwait2 = 441 for amd64) be defined? Currently, they are in runtime/sys_linux_xxx.s along with the syscall assembly implementations. I reckon all these definitions should end up in runtime/internal/syscall at the end of the day. But what to do with epoll_pwait2 while it's not there? I don't think using definitions from "golang.org.x/sys/unix" in runtime is a good idea. Or is it? Should I (try to) add sysnums to the runtime/internal/syscall?

Thanks!

@ianlancetaylor
Copy link
Contributor Author

The constant _ENOSYS was removed from the runtime package in https://go.dev/cl/389354. It would be fine to revert that part of that CL for this purpose.

@aclements
Copy link
Member

For the syscall number constants, I would create new per-arch files in runtime/internal/syscall to define those constants. Eventually, we'll want to shift all of the constants out of sys_linux_xxx.s and into that package (and ideally generate them).

Which packages the runtime can import are very restricted, so it wouldn't be able to import golang.org/x/sys/unix. We might eventually want to use the same code generation mechanism for both, but that's down the road.

@dAdAbird
Copy link
Contributor

For the syscall number constants, I would create new per-arch files in runtime/internal/syscall to define those constants. Eventually, we'll want to shift all of the constants out of sys_linux_xxx.s and into that package (and ideally generate them).

Yes, this is what I was thinking to do. To generate those constants (like it's done in golang.org/x/sys/unix) and put them into runtime/internal/syscall.

Or in this CL I should only have SYS_epoll_pwait, SYS_epoll_pwait2 constants. And all the rest should be generated in the separate CL later on?

@dAdAbird
Copy link
Contributor

Ah, as far as I can see there is no generator for the runtime now. Hence no easy way to generate syscall defs.
So I'll probably just add constants manually. Still, should be there only SYS_EPOLL_PWAIT and SYS_EPOLL_PWAIT2, or copy all syscalls from golang.org/x/sys/unix's zsysnum_linux_xxx.go files?

@prattmic
Copy link
Member

I think for now you can just add the epoll constants you need for the CL.

@dAdAbird
Copy link
Contributor

dAdAbird commented Jul 29, 2022

Hi, I'm again with questions.

  1. I made functions for epoll_pwait and epoll_pwait2 calls in runtime/netpoll_epoll.go instead of assembler once, which looks like this:
var _zero uintptr
func epollwait2(epfd int, events []epollevent, maxev int, ts *timespec) (n, errno int) {
	var ev unsafe.Pointer
	if len(events) > 0 {
		ev = unsafe.Pointer(&events[0])
	} else {
		ev = unsafe.Pointer(&_zero)
	}
	r1, _, e := syscall.Syscall6(syscall.SYS_EPOLL_PWAIT2, uintptr(epfd), uintptr(ev), uintptr(maxev), uintptr(unsafe.Pointer(&ts)), 0, 0)
	return int(r1), int(e)
}

...

But then I thought, it makes sense to define these funcs in runtime/internal/syscall if we are switching to this package anyway. But for that, I'd have to move at least type epollevent struct definition from runtime/defs_linux_xxx.go to runtime/internal/syscall/defs_linux_xxx.go (which I've already created for SYS_EPOLL_PWAIT and SYS_EPOLL_PWAIT2 definitions). What do you think about that?
It that case, it's also worth moving into internal/syscall defs of _EINTR, _EAGAIN, _ENOMEM, _ENOSYS and making syscall.EpollPwait return Go error instead of errno. But it's for sure should be in a different CL, rather related to #51087.

  1. Is there any reason why arguments and return values of epoll* funcs are int32 not int? Should I stay with int32 or may I switch to int in new epollwait* funcs?

  2. I'd also redo other syscalls in runtime/netpoll_epoll.go to use runtime/internal/syscall for consistency reasons. But I'm not sure if to make it in this CL or make a new one (and an issue) for that.

Cheers!

@prattmic
Copy link
Member

prattmic commented Jul 29, 2022

But then I thought, it makes sense to define these funcs in runtime/internal/syscall if we are switching to this package anyway. But for that, I'd have to move at least type epollevent struct definition from runtime/defs_linux_xxx.go to runtime/internal/syscall/defs_linux_xxx.go (which I've already created for SYS_EPOLL_PWAIT and SYS_EPOLL_PWAIT2 definitions). What do you think about that?

I think that sounds good.

_It that case, it's also worth moving into internal/syscall defs of _EINTR, _EAGAIN, _ENOMEM, _ENOSYS and making syscall.EpollPwait return Go error instead of errno.

error is an interface type, which may not be safe to use everywhere in the runtime that we need to make syscalls. But we potentially use an explicit Errno type for a bit stronger typing than the existing uintptr.

  1. Is there any reason why arguments and return values of epoll* funcs are int32 not int? Should I stay with int32 or may I switch to int in new epollwait* funcs?

They use int32 because the kernel interface only accepts 32-bit integers. Note that int is 32-bits in the kernel on all architectures (see page 2).

  1. I'd also redo other syscalls in runtime/netpoll_epoll.go to use runtime/internal/syscall for consistency reasons. But I'm not sure if to make it in this CL or make a new one (and an issue) for that.

I think you should have at least 2 CLs: one (or more) moving things to runtime/internal/syscall with no behavior changes, and one adding and using epoll_pwait2.

@dAdAbird
Copy link
Contributor

Hi Michael, thank you for the feedback and explanations.

I think you should have at least 2 CLs: one (or more) moving things to runtime/internal/syscall with no behavior changes, and one adding and using epoll_pwait2.

Sounds good to me.

Cheers!

@prattmic prattmic modified the milestones: Backlog, Go1.20 Jul 29, 2022
@gopherbot
Copy link

Change https://go.dev/cl/421994 mentions this issue: runtime: move epoll syscalls to runtime/internal/syscall

gopherbot pushed a commit that referenced this issue Sep 30, 2022
This change moves Linux epoll's syscalls implementation to the
"runtime/internal/syscall" package. The intention in this CL was to
minimise behavioural changes but make the code more generalised. This
also will allow adding new syscalls (like epoll_pwait2) without the
need to implement assembly stubs for each arch.

It also drops epoll_create as not all architectures provide this call.
epoll_create1 was added to the kernel in version 2.6.27 and Go requires
Linux kernel version 2.6.32 or later since Go 1.18. So it is safe to
always use epoll_create1.

For #53824
For #51087

Change-Id: I9a6a26b7f2075a38e041de1bab4691da0ecb94fc
Reviewed-on: https://go-review.googlesource.com/c/go/+/421994
Reviewed-by: Michael Pratt <mpratt@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>
Auto-Submit: Michael Pratt <mpratt@google.com>
@gopherbot
Copy link

Change https://go.dev/cl/437295 mentions this issue: Revert "runtime: move epoll syscalls to runtime/internal/syscall"

gopherbot pushed a commit that referenced this issue Sep 30, 2022
This reverts CL 421994.

Reason for revert: breaks runtime.TestCheckPtr2

For #53824
For #51087

Change-Id: I044ea4d6efdffe0a4b7fb0d2bb3717d9f391fc59
Reviewed-on: https://go-review.googlesource.com/c/go/+/437295
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Michael Pratt <mpratt@google.com>
Auto-Submit: Michael Pratt <mpratt@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
@gopherbot
Copy link

Change https://go.dev/cl/440115 mentions this issue: runtime: move epoll syscalls to runtime/internal/syscall

gopherbot pushed a commit that referenced this issue Oct 7, 2022
This change moves Linux epoll's syscalls implementation to the
"runtime/internal/syscall" package. The intention in this CL was to
minimise behavioural changes but make the code more generalised. This
also will allow adding new syscalls (like epoll_pwait2) without the
need to implement assembly stubs for each arch.

It also drops epoll_create as not all architectures provide this call.
epoll_create1 was added to the kernel in version 2.6.27 and Go requires
Linux kernel version 2.6.32 or later since Go 1.18. So it is safe to
always use epoll_create1.

This is a resubmit as the previous CL 421994 was reverted due to test
failures after the merge with the master. The issue was fixed in
CL 438615

For #53824
For #51087

Change-Id: I1bd0f23a85b4f9b80178c5dd36fd3e95ff4f9648
Reviewed-on: https://go-review.googlesource.com/c/go/+/440115
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Michael Pratt <mpratt@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>
romaindoumenc pushed a commit to TroutSoftware/go that referenced this issue Nov 3, 2022
This change moves Linux epoll's syscalls implementation to the
"runtime/internal/syscall" package. The intention in this CL was to
minimise behavioural changes but make the code more generalised. This
also will allow adding new syscalls (like epoll_pwait2) without the
need to implement assembly stubs for each arch.

It also drops epoll_create as not all architectures provide this call.
epoll_create1 was added to the kernel in version 2.6.27 and Go requires
Linux kernel version 2.6.32 or later since Go 1.18. So it is safe to
always use epoll_create1.

This is a resubmit as the previous CL 421994 was reverted due to test
failures after the merge with the master. The issue was fixed in
CL 438615

For golang#53824
For golang#51087

Change-Id: I1bd0f23a85b4f9b80178c5dd36fd3e95ff4f9648
Reviewed-on: https://go-review.googlesource.com/c/go/+/440115
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Michael Pratt <mpratt@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>
@gopherbot
Copy link

Change https://go.dev/cl/449815 mentions this issue: runtime: use epoll_pwait2 where available

@prattmic prattmic modified the milestones: Go1.20, Go1.21 Jan 11, 2023
@mknyszek
Copy link
Contributor

mknyszek commented Jun 9, 2023

It seems this has missed 2 releases. Moving to backlog.

Please comment if you plan to work on it in Go 1.22 and I'm happy to move it there. Thanks!

@mknyszek mknyszek modified the milestones: Go1.21, Backlog Jun 9, 2023
@gopherbot
Copy link

Change https://go.dev/cl/514275 mentions this issue: time: mention time resolution in docs

gopherbot pushed a commit that referenced this issue Aug 1, 2023
Updates #44343
Updates #53824

Change-Id: Ia7234fac4b1b88b3c331328aaa98dc85205e09ba
Reviewed-on: https://go-review.googlesource.com/c/go/+/514275
Reviewed-by: David Chase <drchase@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Quim Muntal <quimmuntal@gmail.com>
@thepudds thepudds changed the title runtime: use epoll_pwait2 where available runtime: use epoll_pwait2 where available for sub-millisecond timeout precision on Linux Feb 2, 2024
@thepudds
Copy link
Contributor

thepudds commented Feb 2, 2024

Hi @dAdAbird, are you planning on returning to this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. help wanted NeedsFix The path to resolution is known, but the work has not been done. Performance
Projects
Status: In Progress
Development

No branches or pull requests

7 participants