search

ringbahn / iou

Rust interface to io_uring
Apache License 2.0
328 stars 22 forks source link

Cqe iterator #22

Closed withoutboats closed 4 years ago

withoutboats commented 4 years ago

This adds a new API for processing CQEs as a psuedo-Iterator. Now, the APIs which return multiple CQEs (ending with for_cqes) return a CompletionQueueEvents type, which has a next_cqe and for_each method, which allow the user to advance through the ready CQEs on the queue.

This API is designed to synchronize with the kernel as few times as possible while processing all available CQEs. This is good if the code you perform for each CQE is very fast, but it can lead to a backed-up CQ if that code takes significant time. For example, this API would be good for a use which wakes tasks when IO events complete (e.g. in the reactor of a Rust Futures API), but not so much for something which actually completes the tasks synchronously on the same thread the events are completed.

This commit also collapses some repetitious unsafe code into singular underlying peek and wait functions, so that we should hopefully not see divergences and bugs.

Closes #14. There's substantial changes to unsafe code here, so I'd like other reviews and will review myself again next week before merging.

withoutboats commented 4 years ago

cc @michael-grunder

michael-grunder commented 4 years ago

I updated my project to use the for_each iterators and everything seems to be working perfectly. 🎉

Edit: Also no complaints from ASAN or Valgrind.

Thanks again, this is great!

withoutboats commented 4 years ago

Awesome, glad to hear it :)

withoutboats commented 4 years ago

quininer (author of linux-io-uring) pointed out that CQEs are just 2 words of data. Instead of making our CompletionQueueEvent a reference into the completion queue, we can just copy the data out and return it. This avoids the lifetime issues and lets us make CompletionQueueEvents a real iterator.

I won't be able to revisit this code until after the new years, but I'll experiment with that approach.

withoutboats commented 4 years ago

Closing in favor of #47