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 #53824

Open
ianlancetaylor opened this issue Jul 13, 2022 · 18 comments
Open

runtime: use epoll_pwait2 where available #53824

ianlancetaylor opened this issue Jul 13, 2022 · 18 comments
Assignees
Labels
compiler/runtime help wanted NeedsFix Performance
Milestone

Comments

@ianlancetaylor
Copy link
Contributor

ianlancetaylor commented Jul 13, 2022

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 labels Jul 13, 2022
@ianlancetaylor ianlancetaylor added this to the Backlog milestone Jul 13, 2022
@gopherbot gopherbot added the compiler/runtime label Jul 13, 2022
@prattmic
Copy link
Member

prattmic commented Jul 13, 2022

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

@dAdAbird
Copy link
Contributor

dAdAbird commented Jul 15, 2022

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

@ianlancetaylor
Copy link
Contributor Author

ianlancetaylor commented Jul 15, 2022

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

dAdAbird commented Jul 15, 2022

@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

prattmic commented Jul 15, 2022

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

dAdAbird commented Jul 15, 2022

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

@aclements
Copy link
Member

aclements commented Jul 16, 2022

@dAdAbird
Copy link
Contributor

dAdAbird commented Jul 18, 2022

Oh, cool, thanks for the advice Austin!

@dAdAbird
Copy link
Contributor

dAdAbird commented Jul 22, 2022

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

ianlancetaylor commented Jul 22, 2022

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

aclements commented Jul 22, 2022

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

dAdAbird commented Jul 25, 2022

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

dAdAbird commented Jul 25, 2022

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

prattmic commented Jul 25, 2022

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

dAdAbird commented Jul 29, 2022

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

gopherbot commented Aug 8, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime help wanted NeedsFix Performance
Projects
Status: In Progress
Development

No branches or pull requests

5 participants