Re: [PATCH 2/3] io_uring: fix failed linkchain code logic

[Hao Xu <haoxu@linux.alibaba.com>]

在 2021/8/18 下午10:40, Pavel Begunkov 写道:
> On 8/18/21 1:22 PM, Hao Xu wrote:
>> 在 2021/8/18 下午6:20, Pavel Begunkov 写道:
>>> On 8/18/21 8:43 AM, Hao Xu wrote:
>>>> Given a linkchain like this:
>>>> req0(link_flag)-->req1(link_flag)-->...-->reqn(no link_flag)
>>>>
> [...]
>>>>        struct io_submit_link *link = &ctx->submit_state.link;
>>>> +    bool is_link = sqe->flags & (IOSQE_IO_LINK | IOSQE_IO_HARDLINK);
>>>> +    struct io_kiocb *head;
>>>>        int ret;
>>>>    +    /*
>>>> +     * we don't update link->last until we've done io_req_prep()
>>>> +     * since linked timeout uses old link->last
>>>> +     */
>>>> +    if (link->head)
>>>> +        link->last->link = req;
>>>> +    else if (is_link)
>>>> +        link->head = req;
>>>> +    head = link->head;
>>>
>>> It's a horrorsome amount of overhead. How about to set the fail flag
>>> if failed early and actually fail on io_queue_sqe(), as below. It's
>>> not tested and a couple more bits added, but hopefully gives the idea.
>> I get the idea, it's truely with less change. But why do you think the
>> above code bring in more overhead, since anyway we have to link the req
>> to the linkchain. I tested it with fio-direct-4k-read-with/without-sqpoll, didn't see performance regression.
> 
> Well, it's an exaggeration :) However, we were cutting the overhead,
> and there is no atomics or other heavy operations left in the hot path,
> just pure number of instructions a request should go through. That's
> just to clear the reason why I don't want extras on the path.
> 
> For the non-linked path, first it adds 2 ifs in front and removes one
> at the end. Then there is is_link, which is most likely to be saved
> on stack. And same with @head which I'd expect to be saved on stack.
Agree on most part except the @head, despite cache hit, a direct stack
variable head is better than a stack pointer link.
I'm excited to see io_uring is becoming faster and faster since we are
actively using it. Thanks for the great work, the recent refcount
optimization is amazing.
I'll send a v2 patchset.

Thanks,
Hao
> 
> If we have a way to avoid it, that's great, and it looks we can.
> 
> [...]
>>>    -    ret = io_req_prep(req, sqe);
>>> -    if (unlikely(ret))
>>> -        goto fail_req;
>>>          /* don't need @sqe from now on */
>>>        trace_io_uring_submit_sqe(ctx, req, req->opcode, req->user_data,
>>> @@ -6670,8 +6670,10 @@ static int io_submit_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req,
>>>            struct io_kiocb *head = link->head;
>>>    
>> maybe better to add an if(head & FAIL) here, since we don't need to
>> prep_async if we know it will be cancelled.
> 
> Such an early fail is marginal enough to not care about performance,
> but I agree that the check is needed as io_req_prep_async() won't be
> able to handle it. E.g. if it failed to grab a file.
>