From: Jens Axboe <axboe@kernel.dk>
To: Christoph Hellwig <hch@lst.de>
Cc: linux-fsdevel@vger.kernel.org, linux-aio@kvack.org,
linux-block@vger.kernel.org, jmoyer@redhat.com, avi@scylladb.com
Subject: Re: [PATCH 09/13] io_uring: add submission polling
Date: Mon, 28 Jan 2019 10:05:37 -0700 [thread overview]
Message-ID: <9cf014d9-cf16-fa49-83f0-0e9ddb57cc81@kernel.dk> (raw)
In-Reply-To: <20190128150926.GB10110@lst.de>
On 1/28/19 8:09 AM, Christoph Hellwig wrote:
> On Wed, Jan 23, 2019 at 08:35:22AM -0700, Jens Axboe wrote:
>> Proof of concept.
>
> Is that still true?
I guess I can remove it now, dates back to when it was initially just
a test. But it should be solid, I'll kill that part.
>> 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?
I think so. Question is what kind of granularity we need for this. I
think we can go pretty coarse and keep it in msec, using a short to
pass this in like we do for the thread CPU. That gives us 0..65535 msec,
which should be plenty of range.
>> 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?
Probably not, I'll kill it.
>> + /*
>> + * 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?
I think that'd make it awkward, since it's out-of-line. If the app is doing
things it shouldn't in this case, its own io_uring_enter() would most likely
fail occasionally with -EBUSY anyway.
>> 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.
I like that change, I'll add IORING_ENTER_SQ_WAKEUP instead of using
'to_submit' for this. We can't validate the number anyway.
>> + 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.
It is working around a bug in shutting down a thread that is affinitized
to a single CPU, I just didn't want to deal with hunting that down right
now.
>> 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.
Removed.
--
Jens Axboe
next prev parent reply other threads:[~2019-01-28 17:05 UTC|newest]
Thread overview: 62+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-23 15:35 [PATCHSET v7] io_uring IO interface Jens Axboe
2019-01-23 15:35 ` [PATCH 01/18] fs: add an iopoll method to struct file_operations Jens Axboe
2019-01-28 14:25 ` Christoph Hellwig
2019-01-28 16:13 ` Jens Axboe
2019-01-23 15:35 ` [PATCH 02/18] block: wire up block device iopoll method Jens Axboe
2019-01-23 15:35 ` [PATCH 03/18] block: add bio_set_polled() helper Jens Axboe
2019-01-28 14:26 ` Christoph Hellwig
2019-01-23 15:35 ` [PATCH 04/18] iomap: wire up the iopoll method Jens Axboe
2019-01-23 15:35 ` [PATCH 05/18] Add io_uring IO interface Jens Axboe
2019-01-28 14:57 ` Christoph Hellwig
2019-01-28 16:26 ` Jens Axboe
2019-01-28 16:34 ` Christoph Hellwig
2019-01-28 19:32 ` Jens Axboe
2019-01-28 18:25 ` Jens Axboe
2019-01-29 6:30 ` Christoph Hellwig
2019-01-29 11:58 ` Arnd Bergmann
2019-01-29 15:20 ` Jens Axboe
2019-01-29 16:18 ` Arnd Bergmann
2019-01-29 16:19 ` Jens Axboe
2019-01-29 16:26 ` Arnd Bergmann
2019-01-29 16:28 ` Jens Axboe
2019-01-29 16:46 ` Arnd Bergmann
2019-01-29 0:47 ` Andy Lutomirski
2019-01-29 1:20 ` Jens Axboe
2019-01-29 6:45 ` Christoph Hellwig
2019-01-29 12:05 ` Arnd Bergmann
2019-01-31 5:11 ` Andy Lutomirski
2019-01-31 16:37 ` Jens Axboe
2019-01-23 15:35 ` [PATCH 06/18] io_uring: add fsync support Jens Axboe
2019-01-23 15:35 ` [PATCH 07/13] io_uring: add support for pre-mapped user IO buffers Jens Axboe
2019-01-23 15:35 ` [PATCH 07/18] io_uring: support for IO polling Jens Axboe
2019-01-28 15:02 ` Christoph Hellwig
2019-01-28 16:46 ` Jens Axboe
2019-01-29 6:27 ` Christoph Hellwig
2019-01-29 13:20 ` Jens Axboe
2019-01-23 15:35 ` [PATCH 08/18] fs: add fget_many() and fput_many() Jens Axboe
2019-01-28 14:29 ` Christoph Hellwig
2019-01-28 16:48 ` Jens Axboe
2019-01-23 15:35 ` [PATCH 08/13] io_uring: add file set registration Jens Axboe
2019-01-23 15:35 ` [PATCH 09/13] io_uring: add submission polling Jens Axboe
2019-01-28 15:09 ` Christoph Hellwig
2019-01-28 17:05 ` Jens Axboe [this message]
2019-01-29 6:29 ` Christoph Hellwig
2019-01-29 13:21 ` Jens Axboe
2019-01-28 21:13 ` Jeff Moyer
2019-01-28 21:28 ` Jens Axboe
2019-01-23 15:35 ` [PATCH 09/18] io_uring: use fget/fput_many() for file references Jens Axboe
2019-01-23 15:35 ` [PATCH 10/13] io_uring: add io_kiocb ref count Jens Axboe
2019-01-23 15:35 ` [PATCH 10/18] io_uring: batch io_kiocb allocation Jens Axboe
2019-01-23 15:35 ` [PATCH 11/18] block: implement bio helper to add iter bvec pages to bio Jens Axboe
2019-01-28 14:31 ` Christoph Hellwig
2019-01-28 16:54 ` Jens Axboe
2019-01-23 15:35 ` [PATCH 11/13] io_uring: add support for IORING_OP_POLL Jens Axboe
2019-01-23 15:35 ` [PATCH 12/18] io_uring: add support for pre-mapped user IO buffers Jens Axboe
2019-01-23 15:35 ` [PATCH 12/13] io_uring: allow workqueue item to handle multiple buffered requests Jens Axboe
2019-01-23 15:35 ` [PATCH 13/18] io_uring: add file set registration Jens Axboe
2019-01-23 15:35 ` [PATCH 13/13] io_uring: add io_uring_event cache hit information Jens Axboe
2019-01-23 15:35 ` [PATCH 14/18] io_uring: add submission polling Jens Axboe
2019-01-23 15:35 ` [PATCH 15/18] io_uring: add io_kiocb ref count Jens Axboe
2019-01-23 15:35 ` [PATCH 16/18] io_uring: add support for IORING_OP_POLL Jens Axboe
2019-01-23 15:35 ` [PATCH 17/18] io_uring: allow workqueue item to handle multiple buffered requests Jens Axboe
2019-01-23 15:35 ` [PATCH 18/18] io_uring: add io_uring_event cache hit information Jens Axboe
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9cf014d9-cf16-fa49-83f0-0e9ddb57cc81@kernel.dk \
--to=axboe@kernel.dk \
--cc=avi@scylladb.com \
--cc=hch@lst.de \
--cc=jmoyer@redhat.com \
--cc=linux-aio@kvack.org \
--cc=linux-block@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).