Re: [PATCH 1/3] io_uring: split req init from submit

[Pavel Begunkov <asml.silence@gmail.com>]

On 8/1/23 16:17, Keith Busch wrote:
> On Tue, Aug 01, 2023 at 03:13:59PM +0100, Pavel Begunkov wrote:
>> On 7/31/23 22:00, Jens Axboe wrote:
>>> On 7/31/23 6:53?AM, Pavel Begunkov wrote:
>>>> On 7/28/23 21:14, Keith Busch wrote:
>>>>> From: Keith Busch <kbusch@kernel.org>
>>>>>
>>>>> Split the req initialization and link handling from the submit. This
>>>>> simplifies the submit path since everything that can fail is separate
>>>>> from it, and makes it easier to create batched submissions later.
>>>>
>>>> Keith, I don't think this prep patch does us any good, I'd rather
>>>> shove the link assembling code further out of the common path. I like
>>>> the first version more (see [1]). I'd suggest to merge it, and do
>>>> cleaning up after.
>>>>
>>>> I'll also say that IMHO the overhead is well justified. It's not only
>>>> about having multiple nvmes, the problem slows down cases mixing storage
>>>> with net and the rest of IO in a single ring.
>>>>
>>>> [1] https://lore.kernel.org/io-uring/20230504162427.1099469-1-kbusch@meta.com/
>>>
>>> The downside of that one, to me, is that it just serializes all of it
>>> and we end up looping over the submission list twice.
>>
>> Right, and there is nothing can be done if we want to know about all
>> requests in advance, at least without changing uapi and/or adding
>> userspace hints.
>>
>>> With alloc+init
>>> split, at least we get some locality wins by grouping the setup side of
>>> the requests.
>>
>> I don't think I follow, what grouping do you mean? As far as I see, v1
>> and v2 are essentially same with the difference of whether you have a
>> helper for setting up links or not, see io_setup_link() from v2. In both
>> cases it's executed in the same sequence:
>>
>> 1) init (generic init + opcode init + link setup) each request and put
>>     into a temporary list.
>> 2) go go over the list and submit them one by one
>>
>> And after inlining they should look pretty close.
> 
> The main difference in this one compared to the original version is that
> everything in the 2nd loop is just for the final dispatch. Anything that
> can fail, fallback, or defer to async happens in the first loop. I'm not
> sure that makes a difference in runtime, but having the 2nd loop handle
> only fast-path requests was what I set out to do for this version.

For performance it doesn't matter, it's a very slow path and we should
not be hitting it. And it only smears single req submission over multiple
places, for instance it won't be legal to use io_submit_sqe() without
those extra checks. Those are all minor points, but I don't think it's
anyhow better than v1 in this aspect.

-- 
Pavel Begunkov