Use epoll_create() as a fallback for epoll_create1() #1590

link2xt · 2022-07-02T23:11:48Z

Thomasdezeeuw

@link2xt as I've said it can't impact the performance of existing code, which this does. This will not be accepted as is.

link2xt · 2022-07-03T09:47:54Z

@Thomasdezeeuw How does this impact the performance? Is epoll_create such a frequent operation that not calling fcntl saves any cycles? The crate doesn't have any benchmark, how do you measure it?

Thomasdezeeuw · 2022-07-03T09:49:19Z

@Thomasdezeeuw How does this impact the performance? Is epoll_create such a frequent operation that not calling fcntl saves any cycles? The crate doesn't have any benchmark, how do you measure it?

I'm not willing to revert to epoll_create on all platforms, I've stated this before.

link2xt · 2022-07-03T09:53:47Z

Threre is zero difference in syscall implementation, both call do_epoll_create. epoll_create1 is not faster than epoll_create:
https://elixir.bootlin.com/linux/v5.19-rc4/source/fs/eventpoll.c#L2010

The only difference is that there is an additional fcntl syscall to set a flag once at the beginning of event loop.

Thomasdezeeuw · 2022-07-03T10:19:47Z

Threre is zero difference in syscall implementation, both call do_epoll_create. epoll_create1 is not faster than epoll_create: https://elixir.bootlin.com/linux/v5.19-rc4/source/fs/eventpoll.c#L2010

The only difference is that there is an additional fcntl syscall to set a flag once at the beginning of event loop.

Can I just say that this is becoming really tiring...

They reason that epoll_create1 was introduced is the same reason we use it. We don't control the entire program, so it's possible to fork between epoll_create and the next call to fcntl, causing the file descriptor to be leaked. We don't want this so we use epoll_create1.

notgull · 2022-07-03T15:29:07Z

Is there any particular reason we can't use what the polling crate does for epoll, where it tries to use a syscall to call epoll_create1, and if that fails with ENOSYS, it falls back onto the epoll_create implementation? That would be the best of both worlds.

link2xt · 2022-07-03T16:32:55Z

Is there any particular reason we can't use what the polling crate does for epoll, where it tries to use a syscall to call epoll_create1, and if that fails with ENOSYS, it falls back onto the epoll_create implementation? That would be the best of both worlds.

Sounds good, and should not degrade performance at all thanks to branch prediction: on systems with epoll_create1 it will always succeed and on older systems we don't care.

Noah-Kennedy · 2022-07-03T17:54:59Z

I think that would be fair to do. It shouldn't degrade performance or worsen anything for system which support the newer API, and would provide support for the older systems.

Thomasdezeeuw · 2022-07-04T09:47:00Z

That should fine with the addition of some documentation that we fall back to a racy version using epoll_create.

link2xt · 2022-07-04T12:25:24Z

Note that kqueue selector is also racy. AFAIK only NetBSD offers kqueue1 that allows to set CLOEXEC atomically.

Thomasdezeeuw · 2022-07-04T13:01:39Z

Note that kqueue selector is also racy. AFAIK only NetBSD offers kqueue1 that allows to set CLOEXEC atomically.

True, but for kqueue we can't do better (except for NetBSD, but that's not a tier1 platform for Mio).

link2xt · 2022-07-09T14:18:35Z

I have updated the PR to use epoll_create1 when it is available.

notgull · 2022-07-09T14:26:24Z

Hmm, I don't think this works. I think you need to use the libc::syscall function to call epoll_create1. Otherwise, it'll try to link to the epoll_create1 function in libc, which may not exist.

link2xt · 2022-07-09T14:26:54Z

src/sys/unix/selector/epoll.rs

-            #[cfg(debug_assertions)]
-            has_waker: AtomicBool::new(false),
-        })
+        syscall!(syscall(libc::SYS_epoll_create1, flag))


This is probably even faster, because we no longer use epoll_create1 C function, but epoll_create1 syscall.

link2xt · 2022-07-09T14:32:45Z

Fails on Illumos currently, because they have epoll_create1 function, but no syscall: https://github.com/illumos/illumos-gate/blob/29ed14768e5e76bb8c95f0a28b4d21d8741dfabe/usr/src/lib/libc/port/sys/epoll.c#L104

Illumos does not have epoll syscalls.

link2xt · 2022-07-09T14:58:46Z

Hmm, I don't think this works. I think you need to use the libc::syscall function to call epoll_create1. Otherwise, it'll try to link to the epoll_create1 function in libc, which may not exist.

It's fixed now.

Thomasdezeeuw

This is going into the right direction. However we're not going to use raw system calls. Also please add the the documentation to Poll::new to I asked for regarding the possible data race.

Thomasdezeeuw · 2022-07-09T15:48:01Z

src/sys/unix/selector/epoll.rs

+
+        // Try epoll_create1 syscall with an epoll_create fallback.
+        #[cfg(not(any(target_os = "illumos", target_os = "redox")))]
+        let ep = syscall!(syscall(libc::SYS_epoll_create1, flag))


We're not going to use raw system calls. Please add the epoll_create1 function the libc crate instead.

The problem it started with is that Android API level 16 does not have epoll_create1 library function in the libc, so the linker fails if we call epoll_create1 libc function. The solution is to try calling raw system call epoll_create1 and use a fallback if it falls with ENOSYS.

In my case the linker is not even called by cargo, because cargo is only used to build static .a library, so there is no way to detect whether libc has an epoll_create function from within the compiler.

Is there any problem with using a raw system call?

Is there any problem with using a raw system call?

It makes maintenance harder.

But I guess we can make an exception here, could you extract the system call stuff to a new epoll_create1 function, that for platforms that support it call libc::epoll_create1 and for others use the raw system interface?

Did it, made a new_epoll_fd() function.

Thomasdezeeuw

Almost their I think, I'm still not super happy about using raw system calls, but if we can minimise the usage to just Android I think it would be acceptable.

Thomasdezeeuw · 2022-07-11T09:08:27Z

src/sys/unix/selector/epoll.rs

+    let ep = syscall!(epoll_create1(flag))?;
+
+    // Try epoll_create1 syscall with an epoll_create fallback.
+    #[cfg(not(any(target_os = "illumos", target_os = "redox")))]


Could you change this to be android only with a comment referring to #1473 and explaining that Android API level 16 doesn't define epoll_create1. I really like to minimise the use of raw system calls.

Pushed a commit limiting this code to target_os = "android".

Thomasdezeeuw · 2022-07-17T19:43:02Z

I'm going to merge this as and will do a little cleanup and more docs into another pr.

link2xt mentioned this pull request Jul 2, 2022

error: undefined reference to 'epoll_create1' #1473

Closed

link2xt force-pushed the no-epoll_create1 branch 2 times, most recently from 40d581a to baee477 Compare July 2, 2022 23:20

Thomasdezeeuw requested changes Jul 3, 2022

View reviewed changes

link2xt force-pushed the no-epoll_create1 branch from baee477 to edf3483 Compare July 9, 2022 14:17

link2xt changed the title ~~Use epoll_create() instead of epoll_create1()~~ Use epoll_create() as a fallback for epoll_create1() Jul 9, 2022

link2xt requested a review from Thomasdezeeuw July 9, 2022 14:18

Use epoll_create() as a fallback for epoll_create1()

f37cc7f

link2xt force-pushed the no-epoll_create1 branch from edf3483 to f37cc7f Compare July 9, 2022 14:24

link2xt commented Jul 9, 2022

View reviewed changes

link2xt added 2 commits July 9, 2022 14:38

Use epoll_create1() function on Illumos

24a7e15

Illumos does not have epoll syscalls.

Redox does not have epoll syscalls too

545b6f4

Thomasdezeeuw requested changes Jul 9, 2022

View reviewed changes

link2xt added 2 commits July 9, 2022 16:07

Document race condidion in Poll::new()

46e56f1

Factor out new_epoll_fd() function

b450c41

Thomasdezeeuw reviewed Jul 11, 2022

View reviewed changes

Darksonn mentioned this pull request Jul 11, 2022

Runtime network support for target i686-unknown-linux-musl under linux kernal version 2.6.18 tokio-rs/tokio#4820

Closed

Limit epoll_create1 fallback to Android only

595d69e

Thomasdezeeuw approved these changes Jul 17, 2022

View reviewed changes

Thomasdezeeuw merged commit 8a96a4f into tokio-rs:master Jul 17, 2022

Thomasdezeeuw mentioned this pull request Jul 17, 2022

Cleanup epoll_create fallback #1594

Merged

link2xt deleted the no-epoll_create1 branch October 26, 2022 16:44

link2xt mentioned this pull request Mar 3, 2023

Implement fallback for sendmmsg and recvmmsg quinn-rs/quinn#1504

Merged

This was referenced Aug 4, 2024

nix build .#deltachat-rpc-server-armeabi-v7a-android fails with 1.142.3 release deltachat/deltachat-core-rust#5852

Closed

Build with Android NDK r24 is broken due to mio update in Tokio 1.39.0 tokio-rs/tokio#6748

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use epoll_create() as a fallback for epoll_create1() #1590

Use epoll_create() as a fallback for epoll_create1() #1590

link2xt commented Jul 2, 2022 •

edited

Loading

Thomasdezeeuw left a comment

link2xt commented Jul 3, 2022

Thomasdezeeuw commented Jul 3, 2022

link2xt commented Jul 3, 2022

Thomasdezeeuw commented Jul 3, 2022

notgull commented Jul 3, 2022

link2xt commented Jul 3, 2022

Noah-Kennedy commented Jul 3, 2022

Thomasdezeeuw commented Jul 4, 2022

link2xt commented Jul 4, 2022

Thomasdezeeuw commented Jul 4, 2022

link2xt commented Jul 9, 2022

notgull commented Jul 9, 2022

link2xt Jul 9, 2022

link2xt commented Jul 9, 2022

link2xt commented Jul 9, 2022

Thomasdezeeuw left a comment

Thomasdezeeuw Jul 9, 2022

link2xt Jul 9, 2022

link2xt Jul 9, 2022

Thomasdezeeuw Jul 10, 2022

link2xt Jul 10, 2022

Thomasdezeeuw left a comment

Thomasdezeeuw Jul 11, 2022

link2xt Jul 16, 2022

Thomasdezeeuw commented Jul 17, 2022

Use epoll_create() as a fallback for epoll_create1() #1590

Use epoll_create() as a fallback for epoll_create1() #1590

Conversation

link2xt commented Jul 2, 2022 • edited Loading

Thomasdezeeuw left a comment

Choose a reason for hiding this comment

link2xt commented Jul 3, 2022

Thomasdezeeuw commented Jul 3, 2022

link2xt commented Jul 3, 2022

Thomasdezeeuw commented Jul 3, 2022

notgull commented Jul 3, 2022

link2xt commented Jul 3, 2022

Noah-Kennedy commented Jul 3, 2022

Thomasdezeeuw commented Jul 4, 2022

link2xt commented Jul 4, 2022

Thomasdezeeuw commented Jul 4, 2022

link2xt commented Jul 9, 2022

notgull commented Jul 9, 2022

Choose a reason for hiding this comment

link2xt commented Jul 9, 2022

link2xt commented Jul 9, 2022

Thomasdezeeuw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Thomasdezeeuw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Thomasdezeeuw commented Jul 17, 2022

link2xt commented Jul 2, 2022 •

edited

Loading