bpo-41001: Add os.eventfd() #20930

tiran · 2020-06-17T10:25:49Z

Signed-off-by: Christian Heimes christian@python.org

https://bugs.python.org/issue41001

Doc/library/os.rst

aeros · 2020-06-18T00:15:27Z

Thanks for working on this, Tiran. It seems like it would be worthwhile to add a brief What's New entry for the addition of os.eventfd().

aeros · 2020-06-18T03:22:56Z

Lib/test/test_os.py

@@ -3524,6 +3525,48 @@ def test_memfd_create(self):
        self.assertFalse(os.get_inheritable(fd2))


+@unittest.skipUnless(hasattr(os, 'eventfd'), 'requires os.eventfd')
+@support.requires_linux_version(2, 30)


I think the test class should be ran with Linux 2.27+ (to at least cover test_eventfd_initval) and then test_eventfd_semaphore (or any future tests that require use of the os.EFD_NONBLOCK flag) can require 2.30+. I'm not sure that it's entirely necessary or the number of devices it would actually be applicable to, but it seems like a better practice to have some amount of test coverage for 2.27 - 2.29 since the feature will be included in those versions.

Also, would it be worthwhile to add a test that makes use of select (maybe for poll and epoll as well, via mixin), which uses them to check if the file descriptor is ready for reading or writing? I suspect those will be typically used with os.eventfd(), so ensuring that behavior works as expected would make sense to me. I'm just not certain if it makes sense to include those in test_os, or if there might be a better location.

It's actually 2.6.30, not 2.30. I'll fix that with the next commit. Linux Kernel 2.6.30 was released in 2009. It's highly unlikely that somebody is running a Kernel version between 2.6.27 and 2.6.30.

It's actually 2.6.30, not 2.30. I'll fix that with the next commit.

Ah, good catch. I completely didn't notice the missing .6, despite recently reading over the man page for eventfd(). :-)

It's highly unlikely that somebody is running a Kernel version between 2.6.27 and 2.6.30.

Yeah, after looking over the dates for the releases, I'm now in agreement that it would be highly unlikely. If anything, I suspect most on the 2.6.x branch would be using 2.6.39 or 2.6.32.

(Based on https://upload.wikimedia.org/wikipedia/en/timeline/40658da696ee210d2be5f221ac12e43b.png.)

aeros · 2020-06-18T04:25:21Z

Doc/library/os.rst

@@ -3195,6 +3195,27 @@ features:
   .. versionadded:: 3.8


+.. function:: eventfd(initval[, flags=os.EFD_CLOEXEC])
+
+   Create an event file descriptor


I think this could be expanded upon a bit, specifically by:

Explicitly mentioning that the file descriptor is returned. I think it helps to make usage a bit more clear, and is done for the above memfd_create().

Briefly summarize the use for it as a lightweight notifier/waiter for events. Compared to a pipe, it is more resource efficient as an event notifier.

E.g.

Suggested change

Create an event file descriptor

Create and return an "eventfd" file descriptor. It can be used as a lightweight

event notifier.

.. note::

Compared to a pipe that is exclusively used for event notification,

`eventfd()` can be used as a more resource efficient alternative that

only uses a single file descriptor and has far less overhead.

While I agree with making the low-level OS functions that are thin wrappers to existing OS APIs fairly minimal in terms of documentation details (especially if they're already well documented), I think the existing one could provide a bit more information to the user that explains why they might want to use it.

For the full details one can view the man page, but IMO, we at least should summarize the general purpose. Otherwise, I suspect it will significantly deter discoverability of the feature.

tiran · 2020-06-18T07:57:56Z

I have updated the documentation, added a select() test case, and included eventfd_read + eventfd_write helper functions. They are just too useful to omit them.

pitrou

This is nice, though I suspect eventfd_read does a system call even when uncontended?

pitrou · 2020-06-18T17:20:16Z

Doc/library/os.rst

+   :func:`eventfd_write` increments the event counter.
+
+   *initval* is the initial value of the event counter. It must be an
+   unsigned integer between 0 and 2\ :sup:`64`\ -\ 2.


According to this man page, the initialization value is an unsigned int. In any case, I don't think it's useful to document the maximum value.

I have updated the documentation. I think it's useful to document that the initial value is limited to UINT32_MAX while the maximum counter is UINT64_MAX - 1.

pitrou · 2020-06-18T17:25:42Z

Doc/library/os.rst

+   Provide semaphore-like semantics for reads from a :func:`eventfd` file
+   descriptor. On read the internal counter is decremented by one.
+
+   .. availability:: Linux 2.6.30 or newer


What's the rationale for adding an availability markup for this data member but not the others?

All other items have same availability as eventfd, semaphore came a bit later.

Doc/library/os.rst

Lib/test/test_os.py

Modules/posixmodule.c

bedevere-bot · 2020-06-18T17:32:39Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

tiran · 2020-06-19T09:59:57Z

I have made the requested changes; please review again

bedevere-bot · 2020-06-19T10:00:00Z

Thanks for making the requested changes!

@pitrou: please review the changes made to this pull request.

aeros

LGTM (other than a minor suggested change for the whatsnew entry).

As mentioned in a previous review comment, I would prefer to have some mention in the docs of the specific use case for eventfd as a lightweight event notifier/waiter (essentially a brief summary of the "Notes" section from the man page) so that users are aware of its general functionality, even if they may not be already familiar with the system call. Unlike in the earlier years of Python, it's becoming increasingly common for developers to have started with Python before C (which has been my own experience). So, I think it's often quite helpful to at least have a summary of the purpose of the function, instead of assuming the reader is already familiar with it.

But IMO, it's fine to merge as is, that can potentially be considered as a future enhancement to the documentation. The core implementation of the wrapper, helpers, and the tests looks good to me.

(Note that I'm not particularly familiar with the details of implementing a CPython wrapper for a system call, I mainly used the existing members that work with FDs for comparison.)

Doc/whatsnew/3.10.rst

giampaolo · 2020-06-23T23:17:35Z

Lib/test/test_os.py

+try:
+    import select
+except ImportError:
+    select = None


It seems all other modules / tests don't guard against select not being available. It looks like all platforms should support it (at least select.select).

aeros · 2020-06-25T02:13:31Z

@tiran This is more of a question than it is feedback, but is there a particular reason why we don't link to a specific man page for additional information here? I see that it's not done for other similar members that are effectively just wrappers for system calls, but it seems like it would be quite useful for readers in this case (particularly since our documentation of it is just a brief summary). This could very easily be done using the :manpage: inline sphinx role:

:manpage:`eventfd(2)`

Lib/test/test_os.py

Modules/posixmodule.c

pablogsal · 2020-08-31T11:48:24Z

I would suggest also to add a test using poll/epoll if available, but I don't feel strongly about it

The ``eventfd_read`` and ``eventfd_write`` functions are convenient wrappers that perform proper read/write and conversion from/to uint64_t. Signed-off-by: Christian Heimes <christian@python.org>

Signed-off-by: Christian Heimes <christian@python.org>

- initval is a uint32_t - improve documentation - add whatsnew Signed-off-by: Christian Heimes <christian@python.org>

Signed-off-by: Christian Heimes <christian@python.org>

Co-authored-by: Kyle Stanley <aeros167@gmail.com>

bedevere-bot · 2020-11-13T18:48:56Z

@tiran: Please replace # with GH- in the commit message next time. Thanks!

Co-authored-by: Kyle Stanley <aeros167@gmail.com>

tiran added the type-feature A feature request or enhancement label Jun 17, 2020

the-knights-who-say-ni added the CLA signed label Jun 17, 2020

bedevere-bot added the awaiting core review label Jun 17, 2020

giampaolo reviewed Jun 17, 2020

View reviewed changes

Doc/library/os.rst Show resolved Hide resolved

tiran force-pushed the bpo-41001-eventfd branch 2 times, most recently from 2891141 to 307cf27 Compare June 17, 2020 17:28

aeros reviewed Jun 18, 2020

View reviewed changes

aeros requested a review from pitrou June 18, 2020 04:31

pitrou requested changes Jun 18, 2020

View reviewed changes

bedevere-bot removed the awaiting core review label Jun 18, 2020

bedevere-bot added the awaiting changes label Jun 18, 2020

tiran force-pushed the bpo-41001-eventfd branch from e95a084 to 4910300 Compare June 19, 2020 09:59

bedevere-bot added awaiting change review and removed awaiting changes labels Jun 19, 2020

bedevere-bot requested a review from pitrou June 19, 2020 10:00

tiran force-pushed the bpo-41001-eventfd branch 2 times, most recently from 1c504ff to 5c9d03e Compare June 23, 2020 13:35

aeros approved these changes Jun 23, 2020

View reviewed changes

Doc/whatsnew/3.10.rst Outdated Show resolved Hide resolved

bedevere-bot added awaiting merge and removed awaiting change review labels Jun 23, 2020

giampaolo reviewed Jun 23, 2020

View reviewed changes

tiran force-pushed the bpo-41001-eventfd branch 2 times, most recently from 50747e8 to 54c49fb Compare June 24, 2020 04:04

pablogsal reviewed Aug 31, 2020

View reviewed changes

Lib/test/test_os.py Show resolved Hide resolved

pablogsal reviewed Aug 31, 2020

View reviewed changes

Modules/posixmodule.c Outdated Show resolved Hide resolved

tiran force-pushed the bpo-41001-eventfd branch from 54c49fb to bc99dd7 Compare November 13, 2020 10:13

tiran requested a review from markshannon as a code owner November 13, 2020 10:13

tiran force-pushed the bpo-41001-eventfd branch 2 times, most recently from fcaa02c to 0cf9151 Compare November 13, 2020 10:26

tiran and others added 7 commits November 13, 2020 11:27

bpo-41001: Add os.eventfd()

019a9fb

Add eventfd_read/write, fix value type

2732046

The ``eventfd_read`` and ``eventfd_write`` functions are convenient wrappers that perform proper read/write and conversion from/to uint64_t. Signed-off-by: Christian Heimes <christian@python.org>

Add eventfd select test case

c0e9b4b

Signed-off-by: Christian Heimes <christian@python.org>

Adress more code reviews

9c0dff5

- initval is a uint32_t - improve documentation - add whatsnew Signed-off-by: Christian Heimes <christian@python.org>

Always import select

4efa39c

Signed-off-by: Christian Heimes <christian@python.org>

Update Doc/whatsnew/3.10.rst

f3da4ed

Co-authored-by: Kyle Stanley <aeros167@gmail.com>

Add example and mention man page

9c59140

tiran force-pushed the bpo-41001-eventfd branch from 0cf9151 to 9c59140 Compare November 13, 2020 10:28

tiran merged commit cd9fed6 into python:master Nov 13, 2020

tiran deleted the bpo-41001-eventfd branch November 13, 2020 18:48

bedevere-bot removed the awaiting merge label Nov 13, 2020

adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021

bpo-41001: Add os.eventfd() (python#20930)

eb59e4b

Co-authored-by: Kyle Stanley <aeros167@gmail.com>

m-tmatma mentioned this pull request Aug 22, 2023

Add wrapper for timerfd_create, timerfd_settime, and timerfd_gettime to os module #108277

Closed

-   Create an event file descriptor
+   Create and return an "eventfd" file descriptor. It can be used as a lightweight
+   event notifier.
+   .. note::
+      Compared to a pipe that is exclusively used for event notification,
+      `eventfd()` can be used as a more resource efficient alternative that
+      only uses a single file descriptor and has far less overhead.

Uh oh!

bpo-41001: Add os.eventfd() #20930

bpo-41001: Add os.eventfd() #20930

Uh oh!

Conversation

tiran commented Jun 17, 2020 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

aeros commented Jun 18, 2020

Uh oh!

aeros Jun 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tiran Jun 18, 2020

Choose a reason for hiding this comment

Uh oh!

aeros Jun 18, 2020

Choose a reason for hiding this comment

Uh oh!

aeros Jun 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tiran commented Jun 18, 2020

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

pitrou Jun 18, 2020

Choose a reason for hiding this comment

Uh oh!

tiran Jun 19, 2020

Choose a reason for hiding this comment

Uh oh!

pitrou Jun 18, 2020

Choose a reason for hiding this comment

Uh oh!

tiran Jun 19, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Jun 18, 2020

Uh oh!

tiran commented Jun 19, 2020

Uh oh!

bedevere-bot commented Jun 19, 2020

Uh oh!

aeros left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

giampaolo Jun 23, 2020

Choose a reason for hiding this comment

Uh oh!

aeros commented Jun 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pablogsal commented Aug 31, 2020

Uh oh!

bedevere-bot commented Nov 13, 2020

Uh oh!

Uh oh!

tiran commented Jun 17, 2020 •

edited by bedevere-bot

Loading

aeros Jun 18, 2020 •

edited

Loading

aeros Jun 18, 2020 •

edited

Loading

aeros left a comment •

edited

Loading

aeros commented Jun 25, 2020 •

edited

Loading