public inbox for [email protected]
 help / color / mirror / Atom feed
* [PATCH 0/2] implement timeout update
@ 2020-11-29 17:12 Pavel Begunkov
  2020-11-29 17:12 ` [PATCH 1/2] io_uring: restructure io_timeout_cancel() Pavel Begunkov
  2020-11-29 17:12 ` [PATCH 2/2] io_uring: add timeout update Pavel Begunkov
  0 siblings, 2 replies; 6+ messages in thread
From: Pavel Begunkov @ 2020-11-29 17:12 UTC (permalink / raw)
  To: Jens Axboe, io-uring

Timeout update is a IORING_OP_TIMEOUT_REMOVE request with timeout_flags
containing a new IORING_TIMEOUT_UPDATE flag. Even though naming may be
confusing, but update and remove are very similar both code and
functionality wise, so doesn't seem necessary to add a new opcode.

Updates don't support offsets, but I don't see a need either. Can
be implemented in the future by passing it in sqe->len.

Pavel Begunkov (2):
  io_uring: restructure io_timeout_cancel()
  io_uring: add timeout update

 fs/io_uring.c                 | 90 +++++++++++++++++++++++++++--------
 include/uapi/linux/io_uring.h |  1 +
 2 files changed, 71 insertions(+), 20 deletions(-)

-- 
2.24.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/2] io_uring: restructure io_timeout_cancel()
  2020-11-29 17:12 [PATCH 0/2] implement timeout update Pavel Begunkov
@ 2020-11-29 17:12 ` Pavel Begunkov
  2020-11-29 17:12 ` [PATCH 2/2] io_uring: add timeout update Pavel Begunkov
  1 sibling, 0 replies; 6+ messages in thread
From: Pavel Begunkov @ 2020-11-29 17:12 UTC (permalink / raw)
  To: Jens Axboe, io-uring

Add io_timeout_extract() helper, which searches and disarms timeouts,
but doesn't complete them. No functional changes.

Signed-off-by: Pavel Begunkov <[email protected]>
---
 fs/io_uring.c | 42 +++++++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 19 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 12e641c61708..bffcbec6c9be 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -5639,24 +5639,10 @@ static enum hrtimer_restart io_timeout_fn(struct hrtimer *timer)
 	return HRTIMER_NORESTART;
 }
 
-static int __io_timeout_cancel(struct io_kiocb *req)
-{
-	struct io_timeout_data *io = req->async_data;
-	int ret;
-
-	ret = hrtimer_try_to_cancel(&io->timer);
-	if (ret == -1)
-		return -EALREADY;
-	list_del_init(&req->timeout.list);
-
-	req_set_fail_links(req);
-	io_cqring_fill_event(req, -ECANCELED);
-	io_put_req_deferred(req, 1);
-	return 0;
-}
-
-static int io_timeout_cancel(struct io_ring_ctx *ctx, __u64 user_data)
+static struct io_kiocb *io_timeout_extract(struct io_ring_ctx *ctx,
+					   __u64 user_data)
 {
+	struct io_timeout_data *io;
 	struct io_kiocb *req;
 	int ret = -ENOENT;
 
@@ -5668,9 +5654,27 @@ static int io_timeout_cancel(struct io_ring_ctx *ctx, __u64 user_data)
 	}
 
 	if (ret == -ENOENT)
-		return ret;
+		return ERR_PTR(ret);
+
+	io = req->async_data;
+	ret = hrtimer_try_to_cancel(&io->timer);
+	if (ret == -1)
+		return ERR_PTR(-EALREADY);
+	list_del_init(&req->timeout.list);
+	return req;
+}
 
-	return __io_timeout_cancel(req);
+static int io_timeout_cancel(struct io_ring_ctx *ctx, __u64 user_data)
+{
+	struct io_kiocb *req = io_timeout_extract(ctx, user_data);
+
+	if (IS_ERR(req))
+		return PTR_ERR(req);
+
+	req_set_fail_links(req);
+	io_cqring_fill_event(req, -ECANCELED);
+	io_put_req_deferred(req, 1);
+	return 0;
 }
 
 static int io_timeout_remove_prep(struct io_kiocb *req,
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 2/2] io_uring: add timeout update
  2020-11-29 17:12 [PATCH 0/2] implement timeout update Pavel Begunkov
  2020-11-29 17:12 ` [PATCH 1/2] io_uring: restructure io_timeout_cancel() Pavel Begunkov
@ 2020-11-29 17:12 ` Pavel Begunkov
  2020-11-30 18:15   ` Jens Axboe
  1 sibling, 1 reply; 6+ messages in thread
From: Pavel Begunkov @ 2020-11-29 17:12 UTC (permalink / raw)
  To: Jens Axboe, io-uring

Support timeout updates through IORING_OP_TIMEOUT_REMOVE with passed in
IORING_TIMEOUT_UPDATE. Updates doesn't support offset timeout mode.
Oirignal timeout.off will be ignored as well.

Signed-off-by: Pavel Begunkov <[email protected]>
---
 fs/io_uring.c                 | 52 +++++++++++++++++++++++++++++++++--
 include/uapi/linux/io_uring.h |  1 +
 2 files changed, 50 insertions(+), 3 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index bffcbec6c9be..63d0d546e661 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -453,6 +453,10 @@ struct io_timeout {
 struct io_timeout_rem {
 	struct file			*file;
 	u64				addr;
+
+	/* timeout update */
+	struct timespec64		ts;
+	u32				flags;
 };
 
 struct io_rw {
@@ -5677,17 +5681,51 @@ static int io_timeout_cancel(struct io_ring_ctx *ctx, __u64 user_data)
 	return 0;
 }
 
+static int io_timeout_update(struct io_ring_ctx *ctx, __u64 user_data,
+			     struct timespec64 *ts, enum hrtimer_mode mode)
+{
+	struct io_kiocb *req = io_timeout_extract(ctx, user_data);
+	struct io_timeout_data *data;
+
+	if (IS_ERR(req))
+		return PTR_ERR(req);
+
+	req->timeout.off = 0; /* noseq */
+	data = req->async_data;
+	list_add_tail(&req->timeout.list, &ctx->timeout_list);
+	hrtimer_init(&data->timer, CLOCK_MONOTONIC, mode);
+	data->timer.function = io_timeout_fn;
+	hrtimer_start(&data->timer, timespec64_to_ktime(*ts), mode);
+	return 0;
+}
+
 static int io_timeout_remove_prep(struct io_kiocb *req,
 				  const struct io_uring_sqe *sqe)
 {
+	struct io_timeout_rem *tr = &req->timeout_rem;
+	int ret;
+
 	if (unlikely(req->ctx->flags & IORING_SETUP_IOPOLL))
 		return -EINVAL;
 	if (unlikely(req->flags & (REQ_F_FIXED_FILE | REQ_F_BUFFER_SELECT)))
 		return -EINVAL;
-	if (sqe->ioprio || sqe->buf_index || sqe->len || sqe->timeout_flags)
+	if (sqe->ioprio || sqe->buf_index || sqe->len)
 		return -EINVAL;
 
-	req->timeout_rem.addr = READ_ONCE(sqe->addr);
+	tr->addr = READ_ONCE(sqe->addr);
+	tr->flags = READ_ONCE(sqe->timeout_flags);
+	if (tr->flags) {
+		if (!(tr->flags & IORING_TIMEOUT_UPDATE))
+			return -EINVAL;
+		if (tr->flags & ~(IORING_TIMEOUT_UPDATE|IORING_TIMEOUT_ABS))
+			return -EINVAL;
+
+		ret = __io_sq_thread_acquire_mm(req->ctx);
+		if (ret)
+			return ret;
+		if (get_timespec64(&tr->ts, u64_to_user_ptr(sqe->addr2)))
+			return -EFAULT;
+	}
 	return 0;
 }
 
@@ -5696,11 +5734,19 @@ static int io_timeout_remove_prep(struct io_kiocb *req,
  */
 static int io_timeout_remove(struct io_kiocb *req)
 {
+	struct io_timeout_rem *tr = &req->timeout_rem;
 	struct io_ring_ctx *ctx = req->ctx;
 	int ret;
 
 	spin_lock_irq(&ctx->completion_lock);
-	ret = io_timeout_cancel(ctx, req->timeout_rem.addr);
+	if (req->timeout_rem.flags & IORING_TIMEOUT_UPDATE) {
+		enum hrtimer_mode mode = (tr->flags & IORING_TIMEOUT_ABS)
+					? HRTIMER_MODE_ABS : HRTIMER_MODE_REL;
+
+		ret = io_timeout_update(ctx, tr->addr, &tr->ts, mode);
+	} else {
+		ret = io_timeout_cancel(ctx, tr->addr);
+	}
 
 	io_cqring_fill_event(req, ret);
 	io_commit_cqring(ctx);
diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
index 6bb8229de892..12a6443ea60d 100644
--- a/include/uapi/linux/io_uring.h
+++ b/include/uapi/linux/io_uring.h
@@ -151,6 +151,7 @@ enum {
  * sqe->timeout_flags
  */
 #define IORING_TIMEOUT_ABS	(1U << 0)
+#define IORING_TIMEOUT_UPDATE	(1U << 31)
 
 /*
  * sqe->splice_flags
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] io_uring: add timeout update
  2020-11-29 17:12 ` [PATCH 2/2] io_uring: add timeout update Pavel Begunkov
@ 2020-11-30 18:15   ` Jens Axboe
  2020-11-30 18:27     ` Pavel Begunkov
  0 siblings, 1 reply; 6+ messages in thread
From: Jens Axboe @ 2020-11-30 18:15 UTC (permalink / raw)
  To: Pavel Begunkov, io-uring

On 11/29/20 10:12 AM, Pavel Begunkov wrote:
> +	tr->flags = READ_ONCE(sqe->timeout_flags);
> +	if (tr->flags) {
> +		if (!(tr->flags & IORING_TIMEOUT_UPDATE))
> +			return -EINVAL;
> +		if (tr->flags & ~(IORING_TIMEOUT_UPDATE|IORING_TIMEOUT_ABS))
> +			return -EINVAL;

These flag comparisons are a bit obtuse - perhaps warrants a comment?

> +		ret = __io_sq_thread_acquire_mm(req->ctx);
> +		if (ret)
> +			return ret;

Why is this done manually?

> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
> index 6bb8229de892..12a6443ea60d 100644
> --- a/include/uapi/linux/io_uring.h
> +++ b/include/uapi/linux/io_uring.h
> @@ -151,6 +151,7 @@ enum {
>   * sqe->timeout_flags
>   */
>  #define IORING_TIMEOUT_ABS	(1U << 0)
> +#define IORING_TIMEOUT_UPDATE	(1U << 31)

Why bit 31?

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] io_uring: add timeout update
  2020-11-30 18:15   ` Jens Axboe
@ 2020-11-30 18:27     ` Pavel Begunkov
  2020-11-30 18:40       ` Jens Axboe
  0 siblings, 1 reply; 6+ messages in thread
From: Pavel Begunkov @ 2020-11-30 18:27 UTC (permalink / raw)
  To: Jens Axboe, io-uring

On 30/11/2020 18:15, Jens Axboe wrote:
> On 11/29/20 10:12 AM, Pavel Begunkov wrote:
>> +	tr->flags = READ_ONCE(sqe->timeout_flags);
>> +	if (tr->flags) {
>> +		if (!(tr->flags & IORING_TIMEOUT_UPDATE))
>> +			return -EINVAL;
>> +		if (tr->flags & ~(IORING_TIMEOUT_UPDATE|IORING_TIMEOUT_ABS))
>> +			return -EINVAL;
> 
> These flag comparisons are a bit obtuse - perhaps warrants a comment?

Ok, the one below should be more readable.

if (tr->flags & IORING_TIMEOUT_UPDATE) {
	if (flags & ~ALLOWED_UPDATE_FLAGS)
		return -EINVAL;
	...
} else if (tr->flags) {
	/* timeout removal doesn't support flags */
	return -EINVAL;
}

> 
>> +		ret = __io_sq_thread_acquire_mm(req->ctx);
>> +		if (ret)
>> +			return ret;
> 
> Why is this done manually?

mm is only needed in *prep(), so don't want IO_WQ_WORK_MM to put it
into req->work since it also affects timeout remove reqs.

> 
>> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
>> index 6bb8229de892..12a6443ea60d 100644
>> --- a/include/uapi/linux/io_uring.h
>> +++ b/include/uapi/linux/io_uring.h
>> @@ -151,6 +151,7 @@ enum {
>>   * sqe->timeout_flags
>>   */
>>  #define IORING_TIMEOUT_ABS	(1U << 0)
>> +#define IORING_TIMEOUT_UPDATE	(1U << 31)
> 
> Why bit 31?

Left bits for other potential timeout modes, don't know which though.
Can return it to bit 1.


-- 
Pavel Begunkov

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] io_uring: add timeout update
  2020-11-30 18:27     ` Pavel Begunkov
@ 2020-11-30 18:40       ` Jens Axboe
  0 siblings, 0 replies; 6+ messages in thread
From: Jens Axboe @ 2020-11-30 18:40 UTC (permalink / raw)
  To: Pavel Begunkov, io-uring

On 11/30/20 11:27 AM, Pavel Begunkov wrote:
> On 30/11/2020 18:15, Jens Axboe wrote:
>> On 11/29/20 10:12 AM, Pavel Begunkov wrote:
>>> +	tr->flags = READ_ONCE(sqe->timeout_flags);
>>> +	if (tr->flags) {
>>> +		if (!(tr->flags & IORING_TIMEOUT_UPDATE))
>>> +			return -EINVAL;
>>> +		if (tr->flags & ~(IORING_TIMEOUT_UPDATE|IORING_TIMEOUT_ABS))
>>> +			return -EINVAL;
>>
>> These flag comparisons are a bit obtuse - perhaps warrants a comment?
> 
> Ok, the one below should be more readable.
> 
> if (tr->flags & IORING_TIMEOUT_UPDATE) {
> 	if (flags & ~ALLOWED_UPDATE_FLAGS)
> 		return -EINVAL;
> 	...
> } else if (tr->flags) {
> 	/* timeout removal doesn't support flags */
> 	return -EINVAL;
> }

Yeah, that's actually readable.

>>> +		ret = __io_sq_thread_acquire_mm(req->ctx);
>>> +		if (ret)
>>> +			return ret;
>>
>> Why is this done manually?
> 
> mm is only needed in *prep(), so don't want IO_WQ_WORK_MM to put it
> into req->work since it also affects timeout remove reqs.

Don't think it's worth it, I'd just mark it needing it uncondtionally
instead. Reason being the obvious of not having stuff like this buried
deep in an op hander, but also that for non SQPOLL, we'll do these
inline always and it's a no-op. For SQPOLL we'd need to grab it, but
I'd rather pay that cost than have this in there.

>>> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
>>> index 6bb8229de892..12a6443ea60d 100644
>>> --- a/include/uapi/linux/io_uring.h
>>> +++ b/include/uapi/linux/io_uring.h
>>> @@ -151,6 +151,7 @@ enum {
>>>   * sqe->timeout_flags
>>>   */
>>>  #define IORING_TIMEOUT_ABS	(1U << 0)
>>> +#define IORING_TIMEOUT_UPDATE	(1U << 31)
>>
>> Why bit 31?
> 
> Left bits for other potential timeout modes, don't know which though.
> Can return it to bit 1.

Let's just use 1, there's no clear separation in there anyway.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-11-30 18:41 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-11-29 17:12 [PATCH 0/2] implement timeout update Pavel Begunkov
2020-11-29 17:12 ` [PATCH 1/2] io_uring: restructure io_timeout_cancel() Pavel Begunkov
2020-11-29 17:12 ` [PATCH 2/2] io_uring: add timeout update Pavel Begunkov
2020-11-30 18:15   ` Jens Axboe
2020-11-30 18:27     ` Pavel Begunkov
2020-11-30 18:40       ` Jens Axboe

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox