lib: posix: Add support for eventfd #22863
Conversation
tsvehagen
requested review from
andrewboie,
andyross,
carlescufi,
galak,
MaureenHelm and
nashif
as code owners
|
It might make sense to have this somewhere in |
zephyrbot
added
area: API
tsvehagen
force-pushed
the
eventfd
branch
2 times, most recently
from
9fa3d54 to
6baa775
Compare
There was a problem hiding this comment.
I think we could leave this to lib/os as this is more of a generic feature than network specific one (even if it currently works only with network sockets).
I think your PR description is good and the last chapter could be added to commit message to give some insight how this is suppose to be used. So please add
The major use case for this is when using poll() and the sockets that you poll are dynamic. When a new
socket needs to be added to the poll, there must be some way to wake the thread and update the pollfds
before calling poll again. One way to solve it is to have a timeout set in the poll call and only update the
pollfds during a timeout but that is not a very nice solution. By instead including an eventfd in the pollfds, it
is possible to wake the polling thread by simply writing to the eventfd.
to the commit message.
I just wonder if it would be possible to add this support to some existing sample, now one needs to look at the unit test application to figure out how to use it. It would be easier if a sample usage would be found in some samples/net application. Not sure how feasible this is.
Updated commit message
Yea that is probably a good idea. I'll see if there is any suitable sample. |
There was a problem hiding this comment.
My main reason to reject is to have at least the depends on !ARCH_POSIX part.
I would be much happier if this was made compatible with the POSIX arch, but I will not block it based on that (even though I think it would be quite much nicer to not have features we cannot test on it)
include/sys/eventfd.h
Outdated
| * | ||
| * @return New eventfd file descriptor on success, -1 on error | ||
| */ | ||
| int eventfd(void); |
There was a problem hiding this comment.
I would think it would be better to not have functions in Zephyr with the same name as other functions in known APIs, almost matching them but not quite. Meaning, I'd think it better if it was named something else (say with a prefix or so) that indicated users it is not what they may think. But this is just my opinion.
There was a problem hiding this comment.
I would think it would be better to not have functions in Zephyr with the same name as other functions in known APIs, almost matching them but not quite.
Yes, this is a good idea in order to avoid confusion later.
There was a problem hiding this comment.
Any suggestions for name? Another option I guess would be to make the call behave the same way as in Linux and keeping the name?
There was a problem hiding this comment.
Any suggestions for name? Another option I guess would be to make the call behave the same way as in Linux and keeping the name?
I do not have any good suggestion for a name (or actually it would be prefix in this case), but as you said, the best option would be to make the API to work same way as in Linux in order to avoid conflicts.
There was a problem hiding this comment.
I don't have any suggestion either. I think it'd be better if @andyross commented in this regard as he has a better understanding of what is good to prefix things with.
A benefit of using different names, though, is that then the issue with the POSIX arch name conflicts are fixed without extra tricks.
There was a problem hiding this comment.
Another option I guess would be to make the call behave the same way as in Linux and keeping the name?
That'd solve this particular issue.
But note the other issue I mentioned below, that also applies to unit tests: As things stand today, you can neither compile this code targeting the POSIX arch, or build unit tests for it.
In general, I'm certainly inclined towards making small concessions in a design to allow for better test-ability. I'd say adding a prefix to these and the header filename is not a too tough one. So I'd ask for you to consider it as it solves both problems in one go.
There was a problem hiding this comment.
Note that this isn't actually in POSIX but a Linux specific call. So if the goal really is POSIX I don't think it will be possible. If by POSIX, we really want Linux, then it would be possible to run with the posix_cheats thing I guess.
There was a problem hiding this comment.
then it would be possible to run with the posix_cheats thing I guess.
@tsvehagen Note that posix_cheats.h is a header for the "POSIX arch" (meaning the posix part of the name comes from the arch name), to rename symbols out of the way to avoid conflicts with the underlaying libC/OS calls. Not just for the POSIX shim/API, even that most symbols renamed there are POSIX API related.
There was a problem hiding this comment.
Note that this isn't actually in POSIX but a Linux specific call.
Yes, but Linux is an OS based on POSIX principles. It doesn't follow the POSIX x.y.z standard straitjacket though, it goes far and beyond it (so, in *teen years, if Linux did it really well, its functionality might become a new volume of the POSIX standard). Let's call it "extended POSIX", shortened for "posix" in colloquial talk.
The point however is that the functionality proposed here is: a) based on, and intended to interact, with functionality provided by Zephyr POSIX subsystem; b) it's based on Linux functionality, and not Zephyr's NIH. Then the obvious choices would be: a) treat it as part of Zephyr POSIX subsys; b) follow Linux interface as closely as possibly, definitely not introduce avoidable discrepancies (like different function signatures).
This would also automatically address the issue which @aescolar brings: Zephyr POSIX subsys is officially not compatible with "native_posix" Zephyr target (due to the limitations of the latter). This is well-known issue: #13054 , which, when we checked with @aescolar last Novemeber, there was still no interest to resolve (@aescolar, feel free to correct me on exact formulation of the status).
So, my suggestion would be to move the functionality to lib/posix/. Or if there's strong desire to track extensions explicitly, to lib/posix_ext/.
I'd also like to bring attention that idea to add eventfd functionality isn't new, we used to have a feature request for that: #16376, which was closed due to lack of motion. Likewise, there's idea to implement epoll() (e.g. https://lists.zephyrproject.org/g/devel/topic/25004178), which also lacks critical mass so far.
So, thanks for coming forward with this, but my suggestion would be to look how to integrate these in/with existing Zephyr frameworks, instead of treating it as a thing in itself.
There was a problem hiding this comment.
I totally agree. I also read your comment #17706 (comment). Very well written 👍
What I meant to point out was just that there might still be some confusion as something that runs on for example native_posix on a Linux machine, which has a native eventfd, might not work on macOS for example.
So, thanks for coming forward with this, but my suggestion would be to look how to integrate these in/with existing Zephyr frameworks, instead of treating it as a thing in itself.
Sounds like a good idea. So to sum up I'll make the signature and behaviour like the eventfd in Linux and move it to lib/posix.
lib/os/Kconfig
Outdated
| @@ -21,4 +21,23 @@ config BASE64 | |||
| help | |||
| Enable base64 encoding and decoding functionality | |||
|
|
|||
| config EVENTFD | |||
| bool "Enable support for eventfd" | |||
| depends on NET_BUF | |||
There was a problem hiding this comment.
Note that how things are now in this PR, it cannot be used with the POSIX arch. So I'd ask either:
- To add here a
depends on !ARCH_POSIXto avoid users getting hit by it by surprise.
OR - Have the feature be compatible with it, by either renaming the conflicting types and symbols (
eventfd_t,eventfd_read,eventfd_write) in the code or adding them toposix_cheats.h, and renaming the header or having the POSIX arch underlying OS include paths and the zephyr app/os path not be added to each other library when compiling.
There was a problem hiding this comment.
I've added the symbols to posix_cheats.h and all tests now pass for native_posix.
There was a problem hiding this comment.
all tests now pass for native_posix
Uhm, how are you building and running that test?
lib/posix/eventfd.c would only be compiled-in if !CONFIG_EXTERNAL_LIBC. But CONFIG_EXTERNAL_LIBC will be set when using the POSIX arch (and POSIX_API won't be set either).
So if you build your test just targetting native_posix, you will be linking against the Linux eventfd* calls instead of the ones you are adding here.
Meaning, you wouldn't be testing lib/posix/eventfd.c
I don't have any other issue with the PR. But I would maybe blacklist the test in the posix arch targets in the test yaml? (unless your intention is to test that the API is equal to Linux, and that therefore the test passes also on Linux).
There was a problem hiding this comment.
Oh, I actually thought that was the idea :p. I mean to use the native calls when available... Correct that lib/posix/eventfd.c isn't tested on native_posix but it tests for same behaviour.
There was a problem hiding this comment.
Oh, I actually thought that was the idea :p
The idea as in "expecting somebody writing a Zephyr app, would want to have that app call the Linux eventfd"? (That would not produce sensible results from the application point of view. A Zephyr app running in native_posix should never be able to call the Linux API directly)
The only calls in native_posix that should be "native" from the app point of view are the ones to the libC utility functions (say strcmp), but none of the kernel calls (that's why posix_cheats.h was created, to rename those calls out of the way)
As it is now, I think this will confuse people into thinking this test is testing lib/posix/eventfd.c when run in native_posix, even that it does not.
So I'd blacklist the test for the posix arch, (and probably also add a depends on !ARCH_POSIX on the EVENTFD config)
There was a problem hiding this comment.
As it is now, I think this will confuse people into thinking this test is testing
lib/posix/eventfd.cwhen run in native_posix, even that it does not.
So I'd blacklist the test for the posix arch, (and probably also add a depends on !ARCH_POSIX on the EVENTFD config)
I have excluded posix arch from test and added the !ARCH_POSIX depends.
There was a problem hiding this comment.
Given that this doesn't fit into the current-in-progress release, I didn't rush into reviewing this, but skimming thru some notification emails, I'd like to sound some initial concerns:
int eventfd(void);
As mentioned in the related comment thread, this really should be consistent with well-known eventfd API (as implemented in Linux and/or Glibc), instead of implementing "kinda like, but not really" API, which would lead to great deal of confusion later on. The way I'd suggest to follow is a kind of TDD: write a simple sample which compiles and runs on Linux, then work towards making the same source (with as few #ifdef's as possiblem ideally 0) to compile and run in the same way on Zephyr. All the original samples in https://github.com/zephyrproject-rtos/zephyr/tree/master/samples/net/sockets follow this approach (sadly, later samples which break this convention were merged into that dir).
lib/os/eventfd.c
Outdated
| return -1; | ||
| } | ||
|
|
||
| *(eventfd_t *)buf = 1; |
There was a problem hiding this comment.
Can you please zero out up to 8 bytes of input buffer first, to make behavior compatible with Linux?
There was a problem hiding this comment.
Since eventfd_t is now u64_t it should behave the same.
lib/os/eventfd.c
Outdated
|
|
||
| *(eventfd_t *)buf = 1; | ||
|
|
||
| return 0; |
There was a problem hiding this comment.
I think it should always return sizeof(eventfd_t)
Good point. native_posix is good to emulate something completely orthogonal to underlying POSIX system. E.g. I believe original @aescolar's goal was to implement a BLE simulator using it. But doing stuff like POSIX_variant_1 -> Zephyr -> POSIX_variant_2 isn't easy or currently supported. But keeping all POSIX_variant_1-related stuff in one place/consistent will allow all of that to benefit, once a solution to layering problem above.
Ack, thanks! I also went thru the code, looks good, a few comments left. |
tsvehagen
force-pushed
the
eventfd
branch
2 times, most recently
from
50f2d4d to
9296df5
Compare
|
@tsvehagen: I'm sorry for possible delay here. Can please confirm the current status of this PR, are all current pending issues were addressed, is this ready for "final" review? (And what about Shippable CI?) In either case, please rebase on the latest master to avoid any bitrot. Thanks. |
I think this is ready yes. The only thing that has been discussed is to have some sample in I have rebased now. Not sure why shippable was failing. |
tsvehagen
changed the title
If it is not too much work, then a simple sample for this would be nice to have. |
|
@tsvehagen: Thanks, will review in detail next week. |
|
@tsvehagen: This looks really good, thanks! I especially appreciate your going for full Linux compatibility (uint64_t, semaphore flag). Regarding a sample app, I've coded (well, patched) one as part of my testing. It's a top commit in #24613, please cherry-pick it here (once it passed codestyle). |
|
Ok, top commit in #24613 should be ready for pickup. (I otherwise close that PR, to not keep noise in review queue.) |
This implements a file descriptor used for event notification that behaves like the eventfd in Linux. The eventfd supports nonblocking operation by setting the EFD_NONBLOCK flag and semaphore operation by settings the EFD_SEMAPHORE flag. The major use case for this is when using poll() and the sockets that you poll are dynamic. When a new socket needs to be added to the poll, there must be some way to wake the thread and update the pollfds before calling poll again. One way to solve it is to have a timeout set in the poll call and only update the pollfds during a timeout but that is not a very nice solution. By instead including an eventfd in the pollfds, it is possible to wake the polling thread by simply writing to the eventfd. Signed-off-by: Tobias Svehagen <tobias.svehagen@gmail.com>
Roughly based on eventfd example code from Linux manpages. Similarly to other POSIX-compatible sample, Makefile.posix is provided to build the code on a POSIX system (in this case, as eventfd() is Linux-specific, this has to be a Linux system). Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>
Ah nice, thanks. I've picked it up in this PR 👍 |
This implements a file descriptor used for event notification that
behaves like the eventfd in Linux.
The eventfd supports nonblocking operation by setting the EFD_NONBLOCK
flag and semaphore operation by settings the EFD_SEMAPHORE flag.
The major use case for this is when using poll() and the sockets that
you poll are dynamic. When a new socket needs to be added to the poll,
there must be some way to wake the thread and update the pollfds before
calling poll again. One way to solve it is to have a timeout set in the
poll call and only update the pollfds during a timeout but that is not
a very nice solution. By instead including an eventfd in the pollfds,
it is possible to wake the polling thread by simply writing to the
eventfd.