IO-Uring Archive on lore.kernel.org
 help / color / Atom feed
From: Jens Axboe <axboe@kernel.dk>
To: Jiufei Xue <jiufei.xue@linux.alibaba.com>
Cc: io-uring@vger.kernel.org
Subject: Re: [PATCH liburing 1/2] io_uring_enter: add timeout support
Date: Thu, 30 Jul 2020 21:57:48 -0600
Message-ID: <253b4df7-a35b-4d49-8cdc-c6fa24446bf9@kernel.dk> (raw)
In-Reply-To: <ec69d835-ddca-88bc-a97e-8f0d4d621bda@linux.alibaba.com>

On 7/30/20 9:16 PM, Jiufei Xue wrote:
> 
> 
> On 2020/7/31 上午10:56, Jens Axboe wrote:
>> On 7/30/20 8:12 PM, Jiufei Xue wrote:
>>>
>>>
>>> On 2020/7/30 下午11:28, Jens Axboe wrote:
>>>> On 7/29/20 8:32 PM, Jiufei Xue wrote:
>>>>> Hi Jens,
>>>>>
>>>>> On 2020/7/30 上午1:51, Jens Axboe wrote:
>>>>>> On 7/29/20 4:10 AM, Jiufei Xue wrote:
>>>>>>> Kernel can handle timeout when feature IORING_FEAT_GETEVENTS_TIMEOUT
>>>>>>> supported. Add two new interfaces: io_uring_wait_cqes2(),
>>>>>>> io_uring_wait_cqe_timeout2() for applications to use this feature.
>>>>>>
>>>>>> Why add new new interfaces, when the old ones already pass in the
>>>>>> timeout? Surely they could just use this new feature, instead of the
>>>>>> internal timeout, if it's available?
>>>>>>
>>>>> Applications use the old one may not call io_uring_submit() because
>>>>> io_uring_wait_cqes() will do it. So I considered to add a new one.
>>>>
>>>> Not sure I see how that's a problem - previously, you could not do that
>>>> either, if you were doing separate submit/complete threads. So this
>>>> doesn't really add any new restrictions. The app can check for the
>>>> feature flag to see if it's safe to do so now.
>>>> Yes, new applications can check for the feature flag. What about the existing
>>>
>>> apps? The existing applications which do not separate submit/complete
>>> threads may use io_uring_wait_cqes() or io_uring_wait_cqe_timeout() without
>>> submiting the requests. No one will do that now when the feature is supported.
>>
>> Right, and I feel like I'm missing something here, what's the issue with
>> that? As far as the application is concerned, a different mechanism may be
>> used to achieve the timeout, but it should work in the same way.
>>
>> So please explain this as clearly as you can, as I'm probably missing
>> something...
>> I am sorry for the confusion. Here is an example below: 
> 
> io_uring_get_sqe
> io_uring_prep_nop
> io_uring_wait_cqe_timeout
> 
> If an existing application call io_uring_wait_cqe_timeout() after preparing
> some sqes, the older APIs will submit the requests.
> 
> However, If we reuse the existing APIs and found the feature is supported,
> we will not submit the requests.
> 
> I think we should not change the behaviors for existing APIs.

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.

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.

Hence I really don't think that's a big deal, and even arguably the right
thing to do.

-- 
Jens Axboe


  reply index

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 [this message]
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
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=253b4df7-a35b-4d49-8cdc-c6fa24446bf9@kernel.dk \
    --to=axboe@kernel.dk \
    --cc=io-uring@vger.kernel.org \
    --cc=jiufei.xue@linux.alibaba.com \
    /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

IO-Uring Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/io-uring/0 io-uring/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 io-uring io-uring/ https://lore.kernel.org/io-uring \
		io-uring@vger.kernel.org
	public-inbox-index io-uring

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.io-uring


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git