Re: [PATCH 09/13] io_uring: add submission polling
flat view
From: Christoph Hellwig <hch@lst.de>
Date: 2019-01-28 15:09:30
Also in:
linux-fsdevel
On Wed, Jan 23, 2019 at 08:35:22AM -0700, Jens Axboe wrote:
Proof of concept.
Is that still true?
1) Maybe have smarter backoff. Busy loop for X time, then go to monitor/mwait, finally the schedule we have now after an idle second. Might not be worth the complexity. 2) Probably want the application to pass in the appropriate grace period, not hard code it at 1 second.
2) actually sounds really useful. Should we look into it ASAP?
quoted hunk ↗ jump to hunk
struct { /* CQ ring */@@ -264,6 +267,9 @@ static void __io_cqring_add_event(struct io_ring_ctx *ctx, u64 ki_user_data, if (waitqueue_active(&ctx->wait)) wake_up(&ctx->wait); + if ((ctx->flags & IORING_SETUP_SQPOLL) && + waitqueue_active(&ctx->sqo_wait))
waitqueue_active is really cheap and sqo_wait should not otherwise by active. Do we really need the flags check here?
quoted hunk ↗ jump to hunk
+ /* + * Normal IO, just pretend everything completed. + * We don't have to poll completions for that. + */ + if (ctx->flags & IORING_SETUP_IOPOLL) { + /* + * App should not use IORING_ENTER_GETEVENTS + * with thread polling, but if it does, then + * ensure we are mutually exclusive.
Should we just return an error early on in this case instead?
quoted hunk ↗ jump to hunk
if (to_submit) { + if (ctx->flags & IORING_SETUP_SQPOLL) { + wake_up(&ctx->sqo_wait); + ret = to_submit;
Do these semantics really make sense? Maybe we should have an IORING_ENTER_WAKE_SQ instead of overloading the to_submit argument? Especially as we don't really care about returning the number passed in.
+ if (ctx->sqo_thread) {
+ kthread_park(ctx->sqo_thread);Can you explain why we need the whole kthread_park game? It is only intended to deal with pausing a thread, and if need it to shut down a thread we have a bug somewhere.
quoted hunk ↗ jump to hunk
static void io_sq_offload_stop(struct io_ring_ctx *ctx) { + if (ctx->sqo_thread) { + kthread_park(ctx->sqo_thread); + kthread_stop(ctx->sqo_thread); + ctx->sqo_thread = NULL;
Also there isn't really much of a point in setting pointers to NULL just before freeing the containing structure. In the best case this now papers over bugs that poisoning or kasan would otherwise find.