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

[Jiufei Xue <jiufei.xue@linux.alibaba.com>]

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.

Thanks,
Jiufei