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

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

On 2020/8/4 下午12:50, Jens Axboe wrote:
> 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...
> 

As suggested by Stefan, applications can pass a flag, say IORING_SETUP_GETEVENTS_TIMEOUT
to initialize the ring to indicate they want to use the new feature. 
Function io_uring_wait_cqes() need to submit the timeout sqe neither the kernel is not
supported nor applications do not want to use the new feature.

So we do not need to add a new API.

Thanks,
Jiufei