public inbox for [email protected]
 help / color / mirror / Atom feed
From: Jiufei Xue <[email protected]>
To: Jens Axboe <[email protected]>
Cc: [email protected], Stefan Metzmacher <[email protected]>
Subject: Re: [PATCH liburing 1/2] io_uring_enter: add timeout support
Date: Tue, 4 Aug 2020 13:04:24 +0800	[thread overview]
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>



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

  reply	other threads:[~2020-08-04  5:04 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-29 10:10 [PATCH liburing 0/2] add two interfaces for new timeout feature Jiufei Xue
2020-07-29 10:10 ` [PATCH liburing 1/2] io_uring_enter: add timeout support Jiufei Xue
2020-07-29 17:51   ` Jens Axboe
2020-07-30  2:32     ` Jiufei Xue
2020-07-30 15:28       ` Jens Axboe
2020-07-31  2:12         ` Jiufei Xue
2020-07-31  2:56           ` Jens Axboe
2020-07-31  3:16             ` Jiufei Xue
2020-07-31  3:57               ` Jens Axboe
2020-08-03  3:16                 ` Jiufei Xue
2020-08-03 16:41                   ` Jens Axboe
2020-08-03 19:16                     ` Stefan Metzmacher
2020-08-04  1:29                     ` Jiufei Xue
2020-08-04  4:50                       ` Jens Axboe
2020-08-04  5:04                         ` Jiufei Xue [this message]
2020-08-04  5:19                           ` Jens Axboe
2020-07-29 10:10 ` [PATCH liburing 2/2] test/timeout: add testcase for new timeout interface Jiufei Xue

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=b5804943-103f-0dda-2bea-ac5d46ed4b56@linux.alibaba.com \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox