Re: [PATCH 09/13] io_uring: add submission polling

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