Re: [PATCH for-next] io_uring: fix CQE reordering

[Jens Axboe <axboe@kernel.dk>]

On 9/23/22 8:43 AM, Pavel Begunkov wrote:
> On 9/23/22 15:35, Jens Axboe wrote:
>> On 9/23/22 8:26 AM, Pavel Begunkov wrote:
>>> On 9/23/22 15:19, Jens Axboe wrote:
>>>> On 9/23/22 7:53 AM, Pavel Begunkov wrote:
>>>>> Overflowing CQEs may result in reordeing, which is buggy in case of
>>>>> links, F_MORE and so.
>>>>>
>>>>> Reported-by: Dylan Yudaken <dylany@fb.com>
>>>>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>>>>> ---
>>>>>    io_uring/io_uring.c | 12 ++++++++++--
>>>>>    io_uring/io_uring.h | 12 +++++++++---
>>>>>    2 files changed, 19 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
>>>>> index f359e24b46c3..62d1f55fde55 100644
>>>>> --- a/io_uring/io_uring.c
>>>>> +++ b/io_uring/io_uring.c
>>>>> @@ -609,7 +609,7 @@ static bool __io_cqring_overflow_flush(struct io_ring_ctx *ctx, bool force)
>>>>>          io_cq_lock(ctx);
>>>>>        while (!list_empty(&ctx->cq_overflow_list)) {
>>>>> -        struct io_uring_cqe *cqe = io_get_cqe(ctx);
>>>>> +        struct io_uring_cqe *cqe = io_get_cqe_overflow(ctx, true);
>>>>>            struct io_overflow_cqe *ocqe;
>>>>>              if (!cqe && !force)
>>>>> @@ -736,12 +736,19 @@ bool io_req_cqe_overflow(struct io_kiocb *req)
>>>>>     * control dependency is enough as we're using WRITE_ONCE to
>>>>>     * fill the cq entry
>>>>>     */
>>>>> -struct io_uring_cqe *__io_get_cqe(struct io_ring_ctx *ctx)
>>>>> +struct io_uring_cqe *__io_get_cqe(struct io_ring_ctx *ctx, bool overflow)
>>>>>    {
>>>>>        struct io_rings *rings = ctx->rings;
>>>>>        unsigned int off = ctx->cached_cq_tail & (ctx->cq_entries - 1);
>>>>>        unsigned int free, queued, len;
>>>>>    +    /*
>>>>> +     * Posting into the CQ when there are pending overflowed CQEs may break
>>>>> +     * ordering guarantees, which will affect links, F_MORE users and more.
>>>>> +     * Force overflow the completion.
>>>>> +     */
>>>>> +    if (!overflow && (ctx->check_cq & BIT(IO_CHECK_CQ_OVERFLOW_BIT)))
>>>>> +        return NULL;
>>>>
>>>> Rather than pass this bool around for the hot path, why not add a helper
>>>> for the case where 'overflow' isn't known? That can leave the regular
>>>> io_get_cqe() avoiding this altogether.
>>>
>>> Was choosing from two ugly-ish solutions, but io_get_cqe() should be
>>> inline and shouldn't really matter, but that's only the case in theory
>>> though. If someone cleans up the CQE32 part and puts it into a separate
>>> non-inline function, it'll be actually inlined.
>>
>> Yes, in theory the current one will be fine as it's known at compile
>> time. In theory... Didn't check if practice agrees with that, would
>> prefer if we didn't leave this to the compiler. Fiddling some other
>> bits, will check in a bit if I have a better idea.
> 
> When inline constants are propagated to the moment they're needed,
> no sane compiler will do otherwise, that's one of the most basic
> optimisations. Don't think it's sane not relying on that.

Yeah it's probably fine as-is, I'd expect it to as well for sure.-- 
Jens Axboe