Re: [PATCH liburing 1/2] io_uring_enter: add timeout support

[Jens Axboe <axboe@kernel.dk>]

On 8/3/20 7:29 PM, Jiufei Xue wrote:
> 
> Hi Jens,
> On 2020/8/4 上午12:41, Jens Axboe wrote:
>> On 8/2/20 9:16 PM, Jiufei Xue wrote:
>>> Hi Jens,
>>>
>>> On 2020/7/31 上午11:57, Jens Axboe wrote:
>>>> Then why not just make the sqe-less timeout path flush existing requests,
>>>> if it needs to? Seems a lot simpler than adding odd x2 variants, which
>>>> won't really be clear.
>>>>
>>> Flushing the requests will access and modify the head of submit queue, that
>>> may race with the submit thread. I think the reap thread should not touch
>>> the submit queue when IORING_FEAT_GETEVENTS_TIMEOUT is supported.
>>
>> Ahhh, that's the clue I was missing, yes that's a good point!
>>
>>>> Chances are, if it's called with sq entries pending, the caller likely
>>>> wants those submitted. Either the caller was aware and relying on that
>>>> behavior, or the caller is simply buggy and has a case where it doesn't
>>>> submit IO before waiting for completions.
>>>>
>>>
>>> That is not true when the SQ/CQ handling are split in two different threads.
>>> The reaping thread is not aware of the submit queue. It should only wait for
>>> completion of the requests, such as below:
>>>
>>> submitting_thread:                   reaping_thread:
>>>
>>> io_uring_get_sqe()
>>> io_uring_prep_nop()     
>>>                                  io_uring_wait_cqe_timeout2()
>>> io_uring_submit()
>>>                                  woken if requests are completed or timeout
>>>
>>>
>>> And if the SQ/CQ handling are in the same thread, applications should use the
>>> old API if they do not want to submit the request themselves.
>>>
>>> io_uring_get_sqe
>>> io_uring_prep_nop
>>> io_uring_wait_cqe_timeout
>>
>> Thanks, yes it's all clear to me now. I do wonder if we can't come up with
>> something better than postfixing the functions with a 2, that seems kind of
>> ugly and doesn't really convey to anyone what the difference is.
>>
>> Any suggestions for better naming?
>>
> how about io_uring_wait_cqe_timeout_nolock()? That means applications can use
> the new APIs without synchronization.

But even applications that don't share the ring across submit/complete
threads will want to use the new interface, if supported by the kernel.
Yes, if they share, they must use it - but even if they don't, it's
likely going to be a more logical interface for them.

So I don't think that _nolock() really conveys that very well, but at
the same time I don't have any great suggestions.

io_uring_wait_cqe_timeout_direct()? Or we could go simpler and just call
it io_uring_wait_cqe_timeout_r(), which is a familiar theme from libc
that is applied to thread safe implementations.

I'll ponder this a bit...

-- 
Jens Axboe