[PATCH liburing 0/2] add two interfaces for new timeout feature

[PATCH liburing 0/2] add two interfaces for new timeout feature

[Jiufei Xue]

This patch adds two interfaces for new timeout feature. They are 
safe for applications that split SQ and CQ handling in two threads.

Jiufei Xue (2):

io_uring_enter: add timeout support
test/timeout: add testcase for new timeout interface

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

[Jiufei Xue]

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.

Signed-off-by: Jiufei Xue <jiufei.xue@linux.alibaba.com>
---
 src/include/liburing.h          | 11 ++++--
 src/include/liburing/io_uring.h |  2 ++
 src/liburing.map                |  3 ++
 src/queue.c                     | 75 ++++++++++++++++++++++++++++++++++++++---
 src/setup.c                     |  1 +
 src/syscall.c                   |  4 +--
 src/syscall.h                   |  2 +-
 7 files changed, 88 insertions(+), 10 deletions(-)

diff --git a/src/include/liburing.h b/src/include/liburing.h
index 0505a4f..6176a63 100644
--- a/src/include/liburing.h
+++ b/src/include/liburing.h
@@ -56,6 +56,7 @@ struct io_uring {
 	struct io_uring_sq sq;
 	struct io_uring_cq cq;
 	unsigned flags;
+	unsigned features;
 	int ring_fd;
 };
 
@@ -93,6 +94,11 @@ extern int io_uring_wait_cqes(struct io_uring *ring,
 	struct __kernel_timespec *ts, sigset_t *sigmask);
 extern int io_uring_wait_cqe_timeout(struct io_uring *ring,
 	struct io_uring_cqe **cqe_ptr, struct __kernel_timespec *ts);
+extern int io_uring_wait_cqes2(struct io_uring *ring,
+	struct io_uring_cqe **cqe_ptr, unsigned wait_nr,
+	struct __kernel_timespec *ts, sigset_t *sigmask);
+extern int io_uring_wait_cqe_timeout2(struct io_uring *ring,
+	struct io_uring_cqe **cqe_ptr, struct __kernel_timespec *ts);
 extern int io_uring_submit(struct io_uring *ring);
 extern int io_uring_submit_and_wait(struct io_uring *ring, unsigned wait_nr);
 extern struct io_uring_sqe *io_uring_get_sqe(struct io_uring *ring);
@@ -120,7 +126,8 @@ extern int io_uring_unregister_personality(struct io_uring *ring, int id);
  */
 extern int __io_uring_get_cqe(struct io_uring *ring,
 			      struct io_uring_cqe **cqe_ptr, unsigned submit,
-			      unsigned wait_nr, sigset_t *sigmask);
+			      unsigned wait_nr, struct __kernel_timespec *ts,
+			      sigset_t *sigmask);
 
 #define LIBURING_UDATA_TIMEOUT	((__u64) -1)
 
@@ -491,7 +498,7 @@ static inline int io_uring_wait_cqe_nr(struct io_uring *ring,
 				      struct io_uring_cqe **cqe_ptr,
 				      unsigned wait_nr)
 {
-	return __io_uring_get_cqe(ring, cqe_ptr, 0, wait_nr, NULL);
+	return __io_uring_get_cqe(ring, cqe_ptr, 0, wait_nr, NULL, NULL);
 }
 
 /*
diff --git a/src/include/liburing/io_uring.h b/src/include/liburing/io_uring.h
index d39b45f..28f81e2 100644
--- a/src/include/liburing/io_uring.h
+++ b/src/include/liburing/io_uring.h
@@ -228,6 +228,7 @@ struct io_cqring_offsets {
  */
 #define IORING_ENTER_GETEVENTS	(1U << 0)
 #define IORING_ENTER_SQ_WAKEUP	(1U << 1)
+#define IORING_ENTER_GETEVENTS_TIMEOUT	(1U << 2)
 
 /*
  * Passed in for io_uring_setup(2). Copied back with updated info on success
@@ -255,6 +256,7 @@ struct io_uring_params {
 #define IORING_FEAT_CUR_PERSONALITY	(1U << 4)
 #define IORING_FEAT_FAST_POLL		(1U << 5)
 #define IORING_FEAT_POLL_32BITS 	(1U << 6)
+#define IORING_FEAT_GETEVENTS_TIMEOUT	(1U << 7)
 
 /*
  * io_uring_register(2) opcodes and arguments
diff --git a/src/liburing.map b/src/liburing.map
index 38bd558..15c5f9d 100644
--- a/src/liburing.map
+++ b/src/liburing.map
@@ -56,4 +56,7 @@ LIBURING_0.6 {
 } LIBURING_0.5;
 
 LIBURING_0.7 {
+	global:
+		io_uring_wait_cqe_timeout2;
+		io_uring_wait_cqes2;
 } LIBURING_0.6;
diff --git a/src/queue.c b/src/queue.c
index be80d7a..cdd6a15 100644
--- a/src/queue.c
+++ b/src/queue.c
@@ -65,11 +65,16 @@ static int __io_uring_peek_cqe(struct io_uring *ring,
 }
 
 int __io_uring_get_cqe(struct io_uring *ring, struct io_uring_cqe **cqe_ptr,
-		       unsigned submit, unsigned wait_nr, sigset_t *sigmask)
+		       unsigned submit, unsigned wait_nr, struct __kernel_timespec *ts,
+		       sigset_t *sigmask)
 {
 	struct io_uring_cqe *cqe = NULL;
 	const int to_wait = wait_nr;
 	int ret = 0, err;
+	struct {
+		sigset_t *sigmask;
+		struct __kernel_timespec *ts;
+	} arg, *argp;
 
 	do {
 		bool cq_overflow_flush = false;
@@ -87,13 +92,26 @@ int __io_uring_get_cqe(struct io_uring *ring, struct io_uring_cqe **cqe_ptr,
 		}
 		if (wait_nr && cqe)
 			wait_nr--;
-		if (wait_nr || cq_overflow_flush)
+		if (wait_nr || cq_overflow_flush) {
 			flags = IORING_ENTER_GETEVENTS;
+			if (ring->features & IORING_FEAT_GETEVENTS_TIMEOUT)
+				flags |= IORING_ENTER_GETEVENTS_TIMEOUT;
+		}
 		if (submit)
 			sq_ring_needs_enter(ring, submit, &flags);
-		if (wait_nr || submit || cq_overflow_flush)
+		if (wait_nr || submit || cq_overflow_flush) {
+			if (ring->features & IORING_FEAT_GETEVENTS_TIMEOUT) {
+				argp = &arg;
+				memset(argp, 0, sizeof(arg));
+				if (sigmask)
+					argp->sigmask = sigmask;
+				if (ts)
+					argp->ts = ts;
+			} else
+				argp = (void *)sigmask;
 			ret = __sys_io_uring_enter(ring->ring_fd, submit,
-						   wait_nr, flags, sigmask);
+						   wait_nr, flags, (void *)argp);
+		}
 		if (ret < 0) {
 			err = -errno;
 		} else if (ret == (int)submit) {
@@ -230,7 +248,7 @@ int io_uring_wait_cqes(struct io_uring *ring, struct io_uring_cqe **cqe_ptr,
 		to_submit = __io_uring_flush_sq(ring);
 	}
 
-	return __io_uring_get_cqe(ring, cqe_ptr, to_submit, wait_nr, sigmask);
+	return __io_uring_get_cqe(ring, cqe_ptr, to_submit, wait_nr, ts, sigmask);
 }
 
 /*
@@ -245,6 +263,53 @@ int io_uring_wait_cqe_timeout(struct io_uring *ring,
 }
 
 /*
+ * If feature IORING_FEAT_GETEVENTS_TIMEOUT supported, this interface is
+ * safe for applications that split SQ and CQ handling in two threads.
+ * Applications need to call io_uring_submit() to submit the requests
+ * first.
+ */
+int io_uring_wait_cqes2(struct io_uring *ring, struct io_uring_cqe **cqe_ptr,
+			unsigned wait_nr, struct __kernel_timespec *ts,
+			sigset_t *sigmask)
+{
+	unsigned to_submit = 0;
+
+	if (ts && !(ring->features & IORING_FEAT_GETEVENTS_TIMEOUT)) {
+		struct io_uring_sqe *sqe;
+		int ret;
+
+		/*
+		 * If the SQ ring is full, we may need to submit IO first
+		 */
+		sqe = io_uring_get_sqe(ring);
+		if (!sqe) {
+			ret = io_uring_submit(ring);
+			if (ret < 0)
+				return ret;
+			sqe = io_uring_get_sqe(ring);
+			if (!sqe)
+				return -EAGAIN;
+		}
+		io_uring_prep_timeout(sqe, ts, wait_nr, 0);
+		sqe->user_data = LIBURING_UDATA_TIMEOUT;
+		to_submit = __io_uring_flush_sq(ring);
+	}
+
+	return __io_uring_get_cqe(ring, cqe_ptr, to_submit, wait_nr, ts, sigmask);
+}
+
+/*
+ * See io_uring_wait_cqes2() - this function is the same, it just always uses
+ * '1' as the wait_nr.
+ */
+int io_uring_wait_cqe_timeout2(struct io_uring *ring,
+			       struct io_uring_cqe **cqe_ptr,
+			       struct __kernel_timespec *ts)
+{
+       return io_uring_wait_cqes2(ring, cqe_ptr, 1, ts, NULL);
+}
+
+/*
  * Submit sqes acquired from io_uring_get_sqe() to the kernel.
  *
  * Returns number of sqes submitted
diff --git a/src/setup.c b/src/setup.c
index 2b17b94..b258cf1 100644
--- a/src/setup.c
+++ b/src/setup.c
@@ -96,6 +96,7 @@ int io_uring_queue_mmap(int fd, struct io_uring_params *p, struct io_uring *ring
 	if (!ret) {
 		ring->flags = p->flags;
 		ring->ring_fd = fd;
+		ring->features = p->features;
 	}
 	return ret;
 }
diff --git a/src/syscall.c b/src/syscall.c
index c41e099..926700b 100644
--- a/src/syscall.c
+++ b/src/syscall.c
@@ -48,8 +48,8 @@ int __sys_io_uring_setup(unsigned entries, struct io_uring_params *p)
 }
 
 int __sys_io_uring_enter(int fd, unsigned to_submit, unsigned min_complete,
-			 unsigned flags, sigset_t *sig)
+			 unsigned flags, void *argp)
 {
 	return syscall(__NR_io_uring_enter, fd, to_submit, min_complete,
-			flags, sig, _NSIG / 8);
+			flags, argp, _NSIG / 8);
 }
diff --git a/src/syscall.h b/src/syscall.h
index 7e299d4..b135d42 100644
--- a/src/syscall.h
+++ b/src/syscall.h
@@ -7,7 +7,7 @@
  */
 extern int __sys_io_uring_setup(unsigned entries, struct io_uring_params *p);
 extern int __sys_io_uring_enter(int fd, unsigned to_submit,
-	unsigned min_complete, unsigned flags, sigset_t *sig);
+	unsigned min_complete, unsigned flags, void *argp);
 extern int __sys_io_uring_register(int fd, unsigned int opcode, const void *arg,
 	unsigned int nr_args);
 
-- 
1.8.3.1

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

[Jens Axboe]

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?

> diff --git a/src/include/liburing.h b/src/include/liburing.h
> index 0505a4f..6176a63 100644
> --- a/src/include/liburing.h
> +++ b/src/include/liburing.h
> @@ -56,6 +56,7 @@ struct io_uring {
>  	struct io_uring_sq sq;
>  	struct io_uring_cq cq;
>  	unsigned flags;
> +	unsigned features;
>  	int ring_fd;
>  };

This breaks the API, as it changes the size of the ring...

-- 
Jens Axboe

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

[Jiufei Xue]

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.

>> diff --git a/src/include/liburing.h b/src/include/liburing.h
>> index 0505a4f..6176a63 100644
>> --- a/src/include/liburing.h
>> +++ b/src/include/liburing.h
>> @@ -56,6 +56,7 @@ struct io_uring {
>>  	struct io_uring_sq sq;
>>  	struct io_uring_cq cq;
>>  	unsigned flags;
>> +	unsigned features;
>>  	int ring_fd;
>>  };
> 
> This breaks the API, as it changes the size of the ring...
> 
Oh, yes, I haven't considering that before. So could I add this feature
bit to io_uring.flags. Any suggestion?

Thanks,
JIufei. 

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

[Jens Axboe]

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.

>>> diff --git a/src/include/liburing.h b/src/include/liburing.h
>>> index 0505a4f..6176a63 100644
>>> --- a/src/include/liburing.h
>>> +++ b/src/include/liburing.h
>>> @@ -56,6 +56,7 @@ struct io_uring {
>>>  	struct io_uring_sq sq;
>>>  	struct io_uring_cq cq;
>>>  	unsigned flags;
>>> +	unsigned features;
>>>  	int ring_fd;
>>>  };
>>
>> This breaks the API, as it changes the size of the ring...
>>
> Oh, yes, I haven't considering that before. So could I add this feature
> bit to io_uring.flags. Any suggestion?

Either that, or we add this (and add pad that we can use later) and just
say that for the next release you have to re-compile against the lib.
That will break existing applications, unless they are recompiled... But
it might not be a bad idea to do so, just so we can pad io_uring out a
little bit to provide for future flexibility.

-- 
Jens Axboe

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

[Jiufei Xue]

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.


>>>> diff --git a/src/include/liburing.h b/src/include/liburing.h
>>>> index 0505a4f..6176a63 100644
>>>> --- a/src/include/liburing.h
>>>> +++ b/src/include/liburing.h
>>>> @@ -56,6 +56,7 @@ struct io_uring {
>>>>  	struct io_uring_sq sq;
>>>>  	struct io_uring_cq cq;
>>>>  	unsigned flags;
>>>> +	unsigned features;
>>>>  	int ring_fd;
>>>>  };
>>>
>>> This breaks the API, as it changes the size of the ring...
>>>
>> Oh, yes, I haven't considering that before. So could I add this feature
>> bit to io_uring.flags. Any suggestion?
> 
> Either that, or we add this (and add pad that we can use later) and just
> say that for the next release you have to re-compile against the lib.
> That will break existing applications, unless they are recompiled... But
> it might not be a bad idea to do so, just so we can pad io_uring out a
> little bit to provide for future flexibility.
>
Agree about that. So should we increase the major version after adding the
feature flag and some pad?

Thanks,
Jiufei

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

[Jens Axboe]

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...

>>> Oh, yes, I haven't considering that before. So could I add this feature
>>> bit to io_uring.flags. Any suggestion?
>>
>> Either that, or we add this (and add pad that we can use later) and just
>> say that for the next release you have to re-compile against the lib.
>> That will break existing applications, unless they are recompiled... But
>> it might not be a bad idea to do so, just so we can pad io_uring out a
>> little bit to provide for future flexibility.
>>
> Agree about that. So should we increase the major version after adding the
> feature flag and some pad?

I think so, also a good time to think about other cases where that might be
useful.

But I think we need to flesh out the API first, as that might impact things.

-- 
Jens Axboe

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

[Jiufei Xue]

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.

Thanks,
Jiufei

>>>> Oh, yes, I haven't considering that before. So could I add this feature
>>>> bit to io_uring.flags. Any suggestion?
>>>
>>> Either that, or we add this (and add pad that we can use later) and just
>>> say that for the next release you have to re-compile against the lib.
>>> That will break existing applications, unless they are recompiled... But
>>> it might not be a bad idea to do so, just so we can pad io_uring out a
>>> little bit to provide for future flexibility.
>>>
>> Agree about that. So should we increase the major version after adding the
>> feature flag and some pad?
> 
> I think so, also a good time to think about other cases where that might be
> useful.
> 
> But I think we need to flesh out the API first, as that might impact things.
> 

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

[Jens Axboe]

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

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

[Jiufei Xue]

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.

> 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

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

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

[Jens Axboe]

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?

-- 
Jens Axboe

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

[Stefan Metzmacher]

[-- Attachment #1.1: Type: text/plain, Size: 2155 bytes --]

Am 03.08.20 um 18:41 schrieb Jens Axboe:
> 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?

Isn't a bit in ring->flags enough? Instead of a new function?

Also the struct passed to the kernel should be a named one instead of
an anonymous struct defined in two places. Maybe a wrapping union would
be good...

metze



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Jiufei Xue]

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

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

[Jens Axboe]

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...

-- 
Jens Axboe

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

[Jiufei Xue]

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

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

[Jens Axboe]

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

[PATCH liburing 2/2] test/timeout: add testcase for new timeout interface

[Jiufei Xue]

Signed-off-by: Jiufei Xue <jiufei.xue@linux.alibaba.com>
---
 test/Makefile  |  1 +
 test/timeout.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 94 insertions(+)

diff --git a/test/Makefile b/test/Makefile
index a693d6f..8892ee9 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -89,6 +89,7 @@ submit-reuse: XCFLAGS = -lpthread
 poll-v-poll: XCFLAGS = -lpthread
 across-fork: XCFLAGS = -lpthread
 ce593a6c480a-test: XCFLAGS = -lpthread
+timeout: XCFLAGS = -lpthread
 
 install: $(all_targets) runtests.sh runtests-loop.sh
 	$(INSTALL) -D -d -m 755 $(datadir)/liburing-test/
diff --git a/test/timeout.c b/test/timeout.c
index 7e9f11d..5002ff4 100644
--- a/test/timeout.c
+++ b/test/timeout.c
@@ -10,6 +10,7 @@
 #include <string.h>
 #include <fcntl.h>
 #include <sys/time.h>
+#include <pthread.h>
 
 #include "liburing.h"
 #include "../src/syscall.h"
@@ -18,6 +19,11 @@
 static int not_supported;
 static int no_modify;
 
+struct thread_data {
+	struct io_uring *ring;
+	volatile int do_exit;
+};
+
 static void msec_to_ts(struct __kernel_timespec *ts, unsigned int msec)
 {
 	ts->tv_sec = msec / 1000;
@@ -232,6 +238,87 @@ err:
 	return 1;
 }
 
+static void *test_reap_thread_fn(void *__data)
+{
+	struct thread_data *data = __data;
+	struct io_uring *ring = (struct io_uring *)data->ring;
+	struct io_uring_cqe *cqe;
+	struct __kernel_timespec ts;
+	int ret, i = 0;
+
+	msec_to_ts(&ts, TIMEOUT_MSEC);
+	while (!data->do_exit) {
+		ret = io_uring_wait_cqes2(ring, &cqe, 2, &ts, NULL);
+		if (ret == -ETIME) {
+			if (i == 2)
+				break;
+			else
+				continue;
+		} else if (ret < 0) {
+			fprintf(stderr, "%s: wait timeout failed: %d\n", __FUNCTION__, ret);
+			goto err;
+		}
+		ret = cqe->res;
+		if (ret < 0) {
+			fprintf(stderr, "res: %d\n", ret);
+			goto err;
+		}
+
+		io_uring_cqe_seen(ring, cqe);
+		i++;
+	}
+
+	if (i != 2) {
+		fprintf(stderr, "got %d completions\n", i);
+		ret = 1;
+		goto err;
+	}
+	return NULL;
+
+err:
+	return (void *)(intptr_t)ret;
+}
+
+static int test_single_timeout_wait_new(struct io_uring *ring)
+{
+	struct thread_data data;
+	struct io_uring_sqe *sqe;
+	pthread_t reap_thread;
+	int ret;
+	void *retval;
+
+	if (!(ring->features & IORING_FEAT_GETEVENTS_TIMEOUT)) {
+		fprintf(stdout, "feature IORING_FEAT_GETEVENTS_TIMEOUT not supported.\n");
+		return 0;
+	}
+
+	data.ring = ring;
+	data.do_exit = 0;
+
+	sqe = io_uring_get_sqe(ring);
+	io_uring_prep_nop(sqe);
+	io_uring_sqe_set_data(sqe, (void *) 1);
+
+	sqe = io_uring_get_sqe(ring);
+	io_uring_prep_nop(sqe);
+	io_uring_sqe_set_data(sqe, (void *) 1);
+
+	pthread_create(&reap_thread, NULL, test_reap_thread_fn, &data);
+
+	ret = io_uring_submit(ring);
+	if (ret <= 0) {
+		fprintf(stderr, "%s: sqe submit failed: %d\n", __FUNCTION__, ret);
+		goto err;
+	}
+
+	sleep(1);
+	data.do_exit = 1;
+	pthread_join(reap_thread, &retval);
+	return (int)(intptr_t)retval;
+err:
+	return 1;
+}
+
 /*
  * Test single timeout waking us up
  */
@@ -1054,6 +1141,12 @@ int main(int argc, char *argv[])
 		return ret;
 	}
 
+	ret = test_single_timeout_wait_new(&ring);
+	if (ret) {
+		fprintf(stderr, "test_single_timeout_wait_new failed\n");
+		return ret;
+	}
+
 	/*
 	 * this test must go last, it kills the ring
 	 */
-- 
1.8.3.1