Skip to content

[IO_URING] Add support for pollAdd operation - #273

Open
FranzBusch wants to merge 19 commits into
mainfrom
fb-io-uring-poll-add
Open

[IO_URING] Add support for pollAdd operation#273
FranzBusch wants to merge 19 commits into
mainfrom
fb-io-uring-poll-add

Conversation

@FranzBusch

Copy link
Copy Markdown
Member

Motivation

To observe events on file descriptors IO_URING supports the pollAdd operation. This is useful when you want to observe a file descriptor becoming ready to read or write

Modifications

This PR adds a new IORing.Request.PollEvents option set to model the poll masks. Furthermore, it adds a new static func pollAdd to the IORing.Request.

Result

We can now use IO_URING to poll for events on file descriptors.

@FranzBusch

Copy link
Copy Markdown
Member Author

@swift-ci please test

@FranzBusch
FranzBusch force-pushed the fb-io-uring-poll-add branch 2 times, most recently from c64fc9c to e69d5c1 Compare December 8, 2025 11:05
@FranzBusch

Copy link
Copy Markdown
Member Author

@swift-ci please test

Catfish-Man
Catfish-Man previously approved these changes Dec 9, 2025

@Catfish-Man Catfish-Man left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a few comments but I don't think any of them are necessarily blockers

Comment thread Sources/System/IORing/PollEvents.swift Outdated
///
/// - ``PollEvents``: The events that can be monitored.
/// - ``IORing/Request/cancel(_:matching:)``: Cancelling poll operations.
@inlinable public static func pollAdd(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typically Swift naming style would be verb-first but if there's a good reason to have it this way it's probably fine

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was trying to match what the io_uring operation was called. I wasn't sure how much we tried to change the naming to fit our Swift naming guidelines.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guessed that was probably what you were doing. I remember this coming up during the initial proposal review and iirc folks leaned "don't try to make the names friendlier" so that looking up docs will work better. I'm still torn on it but I see the logic.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know what you prefer and I am happy to change if needed. I am open to both.

@glessard glessard Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pollAdd is somewhat greppable with io_uring docs, but not directly since the names are usually something like io_uring_prep_poll_add or IORING_OP_POLL_ADD. This is the name for the request, so maybe just poll would be better: poll(fd, events: PollEvents, isMultiShot: Bool, context: UInt64).

A future addition would be removal of events, and the constant used for that in the C library is IORING_POLL_UPDATE_EVENTS. I don't think we would want pollUpdate or pollRemove, maybe just update.

Comment thread Sources/System/IORing/IORequest.swift Outdated
Comment thread Sources/System/IORing/PollEvents.swift
@FranzBusch

Copy link
Copy Markdown
Member Author

@swift-ci please test

1 similar comment
@glessard

Copy link
Copy Markdown
Contributor

@swift-ci please test

Comment thread Sources/System/IORing/IORequest.swift Outdated
@FranzBusch

Copy link
Copy Markdown
Member Author

@swift-ci please test

@glessard glessard left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be inclined to approve this addition if that were our process.

Comment thread Sources/System/IORing/IORequest.swift Outdated
Comment thread Sources/System/IORing/IORequest.swift Outdated
@FranzBusch

Copy link
Copy Markdown
Member Author

@swift-ci please test

FranzBusch and others added 5 commits July 27, 2026 16:26
To observe events on file descriptors IO_URING supports the `pollAdd` operation. This is useful when you want to observe a file descriptor becoming ready to read or write

This PR adds a new `IORing.Request.PollEvents` option set to model the poll masks. Furthermore, it adds a new `static func pollAdd` to the `IORing.Request`.

We can now use IO_URING to poll for events on file descriptors.
Co-authored-by: Martin <sebastian.toivonen@gmail.com>
@glessard
glessard force-pushed the fb-io-uring-poll-add branch from 9dfe6c6 to 5a94258 Compare July 28, 2026 21:51
@glessard

Copy link
Copy Markdown
Contributor

@swift-ci please test

case pollOut = 0x0004
case pollErr = 0x0008
case pollHup = 0x0010
case pollNval = 0x0020

@glessard glessard Jul 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added cases for POLLHUP and POLLNVAL. These names are inscrutable in any situation. Should they be pollHup and pollNval, or pollHangup and pollInvalid, or hangUp and invalidDescriptor. For now I continued along with the earlier work in this PR and used what I consider to be the bad names.

Missing are POLLPRI and POLLRDHUP, which would be relevant for sockets.

Better names would be readable, writable, error, hangUp, invalidDescriptor (+ priorityData, and peerClosed)

@glessard
glessard force-pushed the fb-io-uring-poll-add branch from 5a94258 to d4373e3 Compare July 28, 2026 23:46
@glessard

Copy link
Copy Markdown
Contributor

@swift-ci please test

@glessard

glessard commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Public symbols added in this PR:

struct IORing.Request.PollEvents: OptionSet, Hashable, Codable, CaseIterable {
  init(rawValue: UInt32)
  static var allCases: [PollEvents]
  static var pollIn   // 0x0001
  static var pollOut  // 0x0004
  static var pollErr  // 0x0008
  static var pollHup  // 0x0010
  static var pollNval // 0x0020
}

extension IORing.Request {
  static func pollAdd(_:pollEvents:isMultiShot:context:)
}

Let's figure out whether these are the names we want.

I think the presence of poll on everything is a distraction. PollEvents is fine, but the 5 flags could have better names. I'm also not convinced by pollAdd as a case name on Request.

I believe having some accessors on Completion to obtain information would be useful. Something coarse would be a Result<PollEvents, Errno>-returning accessor. Any Completion improvements can be separate, but we can note that the usage examples (tests) don't look ergonomic.

@glessard
glessard dismissed Catfish-Man’s stale review July 29, 2026 00:49

Many more additions made, requested a new review.

@glessard
glessard requested a review from jrflat July 29, 2026 00:58
@FranzBusch

Copy link
Copy Markdown
Member Author

I think the presence of poll on everything is a distraction. PollEvents is fine, but the 5 flags could have better names.

I think this is a good suggestion and just renaming them to .in, .out, .error, .hub, etc would be better. I am not sure if we should give them completely new names such as readable or writable. Users of this API are most likely looking at man pages for liburing and there is a lot of value sticking close to that documentation.

I'm also not convinced by pollAdd as a case name on Request.

Similarly here I personally think that sticking as close to the libuiring and ops code naming is beneficial when it comes to discovery.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants