Draft changes to the io_uring prototype #208
Conversation
for some reason, the linker on my linux machine fails to link the tests otherwise. investigate / fix before merging.
| @@ -1,18 +0,0 @@ | |||
| /* | |||
There was a problem hiding this comment.
This deletion seems probably wrong? Not sure where it came from
|
|
||
| public struct AsyncFileDescriptor: ~Copyable { | ||
| @usableFromInline var open: Bool = true | ||
| @usableFromInline let fileSlot: IORingFileSlot |
There was a problem hiding this comment.
Ideally this should be a borrow of the file slot, but that's hard to express right now
| mode, | ||
| options: options, | ||
| permissions: permissions, | ||
| intoSlot: fileSlot.borrow() |
There was a problem hiding this comment.
This is a hack, this should also be an actual borrow
| ) | ||
| } | ||
|
|
||
| internal init(_ fileSlot: consuming IORingFileSlot, ring: ManagedIORing) { |
There was a problem hiding this comment.
more "should be a borrow"
| } | ||
|
|
||
| @inlinable @inline(__always) | ||
| public consuming func close(isolation actor: isolated (any Actor)? = #isolation) async throws { |
There was a problem hiding this comment.
I want to review the heavy use of isolation parameters at some point, as well as inlinability
Sources/System/IORing.swift
Outdated
| self.isBorrow = isBorrow | ||
| } | ||
|
|
||
| func withResource() { |
Sources/System/IORing.swift
Outdated
| } | ||
|
|
||
| //TODO: this is a workaround for lifetime issues and should be removed | ||
| @usableFromInline func borrow() -> IOResource<T> { |
There was a problem hiding this comment.
This is awful, but it unblocks me for now
Sources/System/IORing.swift
Outdated
| let ringFlags: UInt32 | ||
| let ringDescriptor: Int32 | ||
|
|
||
| @usableFromInline let submissionMutex: Mutex<SQRing> |
There was a problem hiding this comment.
Another dubiously inline thing
Sources/System/IORing.swift
Outdated
| fatalError("failed to unregister files") | ||
| } | ||
|
|
||
| public func getFile() -> IORingFileSlot? { |
There was a problem hiding this comment.
Not clear to me that this is the interface we want, but need to think about it more
Sources/System/ManagedIORing.swift
Outdated
| } | ||
|
|
||
| public func submitAndWait(_ request: __owned IORequest, isolation actor: isolated (any Actor)? = #isolation) async -> IOCompletion { | ||
| var consumeOnceWorkaround: IORequest? = request |
There was a problem hiding this comment.
This is working around "can't statically guarantee the closure will only be called once"
Sources/System/IORequest.swift
Outdated
| case nop // nothing here | ||
| case openat( | ||
| atDirectory: FileDescriptor, | ||
| path: UnsafePointer<CChar>, |
There was a problem hiding this comment.
path should probably be a FilePath?
Sources/System/IORing.swift
Outdated
| return nil | ||
| } | ||
|
|
||
| public struct IORing: @unchecked Sendable, ~Copyable { |
There was a problem hiding this comment.
Does this need to be Sendable? I found it surprising that SQRing and CQRing are protected by Mutex. If I was writing a single threaded program, I wouldn't want that overhead.
There was a problem hiding this comment.
yeah, we decided to change that
| if ringDescriptor < 0 { | ||
| // TODO: error handling | ||
| } |
There was a problem hiding this comment.
should this move to line 221 before:
if params.features & IORING_FEAT_SINGLE_MMAP == 0
|| params.features & IORING_FEAT_NODROP == 0
{
close(ringDescriptor)
There was a problem hiding this comment.
I want to redo all the feature detection stuff but I'll take a look at this when I do
| let kernelHead: UnsafePointer<Atomic<UInt32>> | ||
| let kernelTail: UnsafePointer<Atomic<UInt32>> |
There was a problem hiding this comment.
should there be padding around these to prevent false sharing?
There was a problem hiding this comment.
Hmm possibly. Let me look over how they're used, but that's a good point to consider.
|
|
||
| //TODO: omitting signal mask for now | ||
| //Tell the kernel that we've submitted requests and/or are waiting for completions | ||
| internal func _enter( |
There was a problem hiding this comment.
AFAICT the implementation here for flushing can run into the problem previously encountered here:
See axboe/liburing#309 for background.
| ) { | ||
| return entry | ||
| } | ||
| // TODO: actually block here instead of spinning |
There was a problem hiding this comment.
Here you probably want to flush instead, to free up SQE space.
| self.rawValue = rawValue | ||
| } | ||
|
|
||
| public static let allocatedBuffer = Flags(rawValue: 1 << 0) |
There was a problem hiding this comment.
Would be great to have a link to e.g.
so one can see where these magic values comes from.
| submissionQueueEntries.count * MemoryLayout<io_uring_sqe>.size | ||
| ) | ||
| close(ringDescriptor) | ||
| } |
There was a problem hiding this comment.
I'm not opposed to the munmap calls being here, but I don't think that close should be. It's generally not a good idea to clean up resources in deinit, even in the ~Copyable case. Overwhelmingly the ring will be stored in a reference type of some kind, so the deinit-based cleanup just gets moved out one layer.
I think I'd pretty strongly prefer we put a close function here. I'm open to it being consuming, though that would also probably be inconvenient too. But generally we shouldn't be getting surprise closes.
This goes double given @CodaFi's note that we want to prevent classes of errors where we shut things down before the resources loaned to the kernel are actually cleaned up. Having an explicit close function gives us a hook on which we can hang that cleanup.
No description provided.