public inbox for [email protected]
 help / color / mirror / Atom feed
From: Jens Axboe <[email protected]>
To: Jiufei Xue <[email protected]>
Cc: [email protected], Stefan Metzmacher <[email protected]>
Subject: Re: [PATCH liburing 1/2] io_uring_enter: add timeout support
Date: Mon, 3 Aug 2020 23:19:03 -0600	[thread overview]
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>

On 8/3/20 11:04 PM, Jiufei Xue wrote:
> 
> 
> 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.

We should not add a private setup flag for this, as the kernel doesn't
really care, and hence the flag wouldn't even exist on the kernel side.
This is all liburing internals. We could have a function in liburing ala
io_uring_set_thread_shared() or something and mark it appropriately, and
then have the existing API do the right thing. We could even have it
return 0/-errno so that applications could handle the case where the
kernel doesn't support it. Should probably name it after what it is,
though, so io_uring_set_cqwait_timeout() or something like that.

We also need to consider what should happen if the app has asked for
this behavior, and wait_cqe() is called with pending submissions. Do we
return an error for this case, or just assume that's what the
applications wants? There's really two cases here:

1) liburing wants to use the newer timeout mechanism, but the
   application hasn't told us if it cares or not.

2) Application explicitly asked for the new behavior, as it relies on
   it. This is a trivial case, as we should not be looking at the SQ
   side at all for this one.

For #1, probably safest to just flush existing submissions and still use
the new API. If the app didn't ask for thread safe SQ/CQ, then it should
not care. And this is existing behavior, after all.

So maybe it'll work out OK. I guess the only remaining hurdle is the
change of struct io_uring, which we'll need to handle carefully.

-- 
Jens Axboe


  reply	other threads:[~2020-08-04  5:19 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
2020-08-04  5:19                           ` Jens Axboe [this message]
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 \
    [email protected] \
    [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