Re: [PATCH RFC 5.13 2/2] io_uring: submit sqes in the original context when waking up sqthread

From: Pavel Begunkov <asml.silence@gmail.com>
To: Jens Axboe <axboe@kernel.dk>, Hao Xu <haoxu@linux.alibaba.com>
Cc: io-uring@vger.kernel.org, Joseph Qi <joseph.qi@linux.alibaba.com>
Subject: Re: [PATCH RFC 5.13 2/2] io_uring: submit sqes in the original context when waking up sqthread
Date: Wed, 28 Apr 2021 15:50:43 +0100	[thread overview]
Message-ID: <093a196a-1925-4f0d-aa2f-0cc1d46484c8@gmail.com> (raw)
In-Reply-To: <d8316547-311d-7995-7faa-4008d577c74c@kernel.dk>

On 4/28/21 3:39 PM, Jens Axboe wrote:
> On 4/28/21 8:34 AM, Pavel Begunkov wrote:
>> On 4/28/21 2:32 PM, Hao Xu wrote:
>>> sqes are submitted by sqthread when it is leveraged, which means there
>>> is IO latency when waking up sqthread. To wipe it out, submit limited
>>> number of sqes in the original task context.
>>> Tests result below:
>>
>> Frankly, it can be a nest of corner cases if not now then in the future,
>> leading to a high maintenance burden. Hence, if we consider the change,
>> I'd rather want to limit the userspace exposure, so it can be removed
>> if needed.
>>
>> A noticeable change of behaviour here, as Hao recently asked, is that
>> the ring can be passed to a task from a completely another thread group,
>> and so the feature would execute from that context, not from the
>> original/sqpoll one.
>>
>> Not sure IORING_ENTER_SQ_DEPUTY knob is needed, but at least can be
>> ignored if the previous point is addressed.
> 
> I mostly agree on that. The problem I see is that for most use cases,
> the "submit from task itself if we need to enter the kernel" is
> perfectly fine, and would probably be preferable. But there are also
> uses cases that absolutely do not want to spend any extra cycles doing
> submit, they are isolating the submission to sqpoll exclusively and that
> is part of the win there. Based on that, I don't think it can be an
> automatic kind of feature.

Reasonable. 

> I do think the naming is kind of horrible. IORING_ENTER_SQ_SUBMIT_IDLE
> would likely be better, or maybe even more verbose as
> IORING_ENTER_SQ_SUBMIT_ON_IDLE.
> 
> On top of that, I don't think an extra submit flag is a huge deal, I
> don't imagine we'll end up with a ton of them. In fact, two have been
> added related to sqpoll since the inception, out of the 3 total added
> flags.

I don't care about the flag itself, nor about performance as it's
nicely under the SQPOLL check, but I rather want to leave a way to
ignore the feature if we would (ever) need to disable it, either
with flag or without it.

> This is all independent of implementation detail and needed fixes to the
> patch.
> 

-- 
Pavel Begunkov