[PATCH for-next 0/6] defer tw fixes and other cleanups

[PATCH for-next 0/6] defer tw fixes and other cleanups

[Pavel Begunkov]

1-4 fix some defer tw problems, 5-6 are just dropped into the pile.

note: we intentionally break defer-taskrun test here.

Pavel Begunkov (6):
  io_uring: further limit non-owner defer-tw cq waiting
  io_uring: disallow defer-tw run w/ no submitters
  io_uring/iopoll: fix unexpected returns
  io_uring/iopoll: unify tw breaking logic
  io_uring: add fast path for io_run_local_work()
  io_uring: remove unused return from io_disarm_next

 io_uring/io_uring.c | 35 ++++++++++++++++++-----------------
 io_uring/io_uring.h |  6 ++++++
 io_uring/timeout.c  | 13 +++----------
 io_uring/timeout.h  |  2 +-
 4 files changed, 28 insertions(+), 28 deletions(-)

-- 
2.37.2

[PATCH 1/6] io_uring: further limit non-owner defer-tw cq waiting

[Pavel Begunkov]

In case of DEFER_TASK_WORK we try to restrict waiters to only one task,
which is also the only submitter; however, we don't do it reliably,
which might be very confusing and backfire in the future. E.g. we
currently allow multiple tasks in io_iopoll_check().

Fixes: dacbb30102689 ("io_uring: add IORING_SETUP_DEFER_TASKRUN")
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 io_uring/io_uring.c |  6 ++++++
 io_uring/io_uring.h | 11 +++++++++++
 2 files changed, 17 insertions(+)

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 0482087b7c64..dc6f64ecd926 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -1398,6 +1398,9 @@ static int io_iopoll_check(struct io_ring_ctx *ctx, long min)
 	int ret = 0;
 	unsigned long check_cq;
 
+	if (!io_allowed_run_tw(ctx))
+		return -EEXIST;
+
 	check_cq = READ_ONCE(ctx->check_cq);
 	if (unlikely(check_cq)) {
 		if (check_cq & BIT(IO_CHECK_CQ_OVERFLOW_BIT))
@@ -2386,6 +2389,9 @@ static int io_cqring_wait(struct io_ring_ctx *ctx, int min_events,
 	ktime_t timeout = KTIME_MAX;
 	int ret;
 
+	if (!io_allowed_run_tw(ctx))
+		return -EEXIST;
+
 	do {
 		/* always run at least 1 task work to process local work */
 		ret = io_run_task_work_ctx(ctx);
diff --git a/io_uring/io_uring.h b/io_uring/io_uring.h
index 9d89425292b7..4eea0836170e 100644
--- a/io_uring/io_uring.h
+++ b/io_uring/io_uring.h
@@ -329,4 +329,15 @@ static inline struct io_kiocb *io_alloc_req(struct io_ring_ctx *ctx)
 	return container_of(node, struct io_kiocb, comp_list);
 }
 
+static inline bool io_allowed_run_tw(struct io_ring_ctx *ctx)
+{
+	if (!(ctx->flags & IORING_SETUP_DEFER_TASKRUN))
+		return true;
+	if (unlikely(ctx->submitter_task != current)) {
+		/* maybe this is before any submissions */
+		return !ctx->submitter_task;
+	}
+	return true;
+}
+
 #endif
-- 
2.37.2

Re: [PATCH 1/6] io_uring: further limit non-owner defer-tw cq waiting

[Dylan Yudaken]

On Thu, 2022-09-08 at 16:56 +0100, Pavel Begunkov wrote:
> In case of DEFER_TASK_WORK we try to restrict waiters to only one
> task,
> which is also the only submitter; however, we don't do it reliably,
> which might be very confusing and backfire in the future. E.g. we
> currently allow multiple tasks in io_iopoll_check().
> 
> Fixes: dacbb30102689 ("io_uring: add IORING_SETUP_DEFER_TASKRUN")
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
>  io_uring/io_uring.c |  6 ++++++
>  io_uring/io_uring.h | 11 +++++++++++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
> index 0482087b7c64..dc6f64ecd926 100644
> --- a/io_uring/io_uring.c
> +++ b/io_uring/io_uring.c
> @@ -1398,6 +1398,9 @@ static int io_iopoll_check(struct io_ring_ctx
> *ctx, long min)
>         int ret = 0;
>         unsigned long check_cq;
>  
> +       if (!io_allowed_run_tw(ctx))
> +               return -EEXIST;
> +
>         check_cq = READ_ONCE(ctx->check_cq);
>         if (unlikely(check_cq)) {
>                 if (check_cq & BIT(IO_CHECK_CQ_OVERFLOW_BIT))
> @@ -2386,6 +2389,9 @@ static int io_cqring_wait(struct io_ring_ctx
> *ctx, int min_events,
>         ktime_t timeout = KTIME_MAX;
>         int ret;
>  
> +       if (!io_allowed_run_tw(ctx))
> +               return -EEXIST;
> +
>         do {
>                 /* always run at least 1 task work to process local
> work */
>                 ret = io_run_task_work_ctx(ctx);
> diff --git a/io_uring/io_uring.h b/io_uring/io_uring.h
> index 9d89425292b7..4eea0836170e 100644
> --- a/io_uring/io_uring.h
> +++ b/io_uring/io_uring.h
> @@ -329,4 +329,15 @@ static inline struct io_kiocb
> *io_alloc_req(struct io_ring_ctx *ctx)
>         return container_of(node, struct io_kiocb, comp_list);
>  }
>  
> +static inline bool io_allowed_run_tw(struct io_ring_ctx *ctx)
> +{
> +       if (!(ctx->flags & IORING_SETUP_DEFER_TASKRUN))
> +               return true;
> +       if (unlikely(ctx->submitter_task != current)) {
> +               /* maybe this is before any submissions */
> +               return !ctx->submitter_task;
> +       }
> +       return true;
> +}
> +
>  #endif

Reviewed-by: Dylan Yudaken <dylany@fb.com>

[PATCH 2/6] io_uring: disallow defer-tw run w/ no submitters

[Pavel Begunkov]

We try to restrict CQ waiters when IORING_SETUP_DEFER_TASKRUN is set,
but if nothing has been submitted yet it'll allow any waiter, which
violates the contract.

Fixes: dacbb30102689 ("io_uring: add IORING_SETUP_DEFER_TASKRUN")
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 io_uring/io_uring.c | 7 +------
 io_uring/io_uring.h | 9 ++-------
 2 files changed, 3 insertions(+), 13 deletions(-)

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index dc6f64ecd926..7f60d384e917 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -1169,13 +1169,8 @@ int __io_run_local_work(struct io_ring_ctx *ctx, bool locked)
 	int ret;
 	unsigned int loops = 1;
 
-	if (unlikely(ctx->submitter_task != current)) {
-		/* maybe this is before any submissions */
-		if (!ctx->submitter_task)
-			return 0;
-
+	if (unlikely(ctx->submitter_task != current))
 		return -EEXIST;
-	}
 
 	node = io_llist_xchg(&ctx->work_llist, &fake);
 	ret = 0;
diff --git a/io_uring/io_uring.h b/io_uring/io_uring.h
index 4eea0836170e..d38173b9ac19 100644
--- a/io_uring/io_uring.h
+++ b/io_uring/io_uring.h
@@ -331,13 +331,8 @@ static inline struct io_kiocb *io_alloc_req(struct io_ring_ctx *ctx)
 
 static inline bool io_allowed_run_tw(struct io_ring_ctx *ctx)
 {
-	if (!(ctx->flags & IORING_SETUP_DEFER_TASKRUN))
-		return true;
-	if (unlikely(ctx->submitter_task != current)) {
-		/* maybe this is before any submissions */
-		return !ctx->submitter_task;
-	}
-	return true;
+	return likely(!(ctx->flags & IORING_SETUP_DEFER_TASKRUN) ||
+		      ctx->submitter_task == current);
 }
 
 #endif
-- 
2.37.2

Re: [PATCH 2/6] io_uring: disallow defer-tw run w/ no submitters

[Dylan Yudaken]

On Thu, 2022-09-08 at 16:56 +0100, Pavel Begunkov wrote:
> We try to restrict CQ waiters when IORING_SETUP_DEFER_TASKRUN is set,
> but if nothing has been submitted yet it'll allow any waiter, which
> violates the contract.
> 
> Fixes: dacbb30102689 ("io_uring: add IORING_SETUP_DEFER_TASKRUN")
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
>  io_uring/io_uring.c | 7 +------
>  io_uring/io_uring.h | 9 ++-------
>  2 files changed, 3 insertions(+), 13 deletions(-)
> 

Reviewed-by: Dylan Yudaken <dylany@fb.com>

[PATCH 3/6] io_uring/iopoll: fix unexpected returns

[Pavel Begunkov]

We may propagate a positive return value of io_run_task_work() out of
io_iopoll_check(), which breaks our tests. io_run_task_work() doesn't
return anything useful for us, ignore the return value.

Fixes: dacbb30102689 ("io_uring: add IORING_SETUP_DEFER_TASKRUN")
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 io_uring/io_uring.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 7f60d384e917..8233a375e8c9 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -1435,12 +1435,9 @@ static int io_iopoll_check(struct io_ring_ctx *ctx, long min)
 				u32 tail = ctx->cached_cq_tail;
 
 				mutex_unlock(&ctx->uring_lock);
-				ret = io_run_task_work();
+				io_run_task_work();
 				mutex_lock(&ctx->uring_lock);
 
-				if (ret < 0)
-					break;
-
 				/* some requests don't go through iopoll_list */
 				if (tail != ctx->cached_cq_tail ||
 				    wq_list_empty(&ctx->iopoll_list))
-- 
2.37.2

Re: [PATCH 3/6] io_uring/iopoll: fix unexpected returns

[Dylan Yudaken]

On Thu, 2022-09-08 at 16:56 +0100, Pavel Begunkov wrote:
> We may propagate a positive return value of io_run_task_work() out of
> io_iopoll_check(), which breaks our tests. io_run_task_work() doesn't
> return anything useful for us, ignore the return value.
> 
> Fixes: dacbb30102689 ("io_uring: add IORING_SETUP_DEFER_TASKRUN")
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
>  io_uring/io_uring.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 

Reviewed-by: Dylan Yudaken <dylany@fb.com>

[PATCH 4/6] io_uring/iopoll: unify tw breaking logic

[Pavel Begunkov]

Let's keep checks for whether to break the iopoll loop or not same for
normal and defer tw, this includes ->cached_cq_tail checks guarding
against polling more than asked for.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 io_uring/io_uring.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 8233a375e8c9..2fb5f1e78fb2 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -1428,21 +1428,21 @@ static int io_iopoll_check(struct io_ring_ctx *ctx, long min)
 		 */
 		if (wq_list_empty(&ctx->iopoll_list) ||
 		    io_task_work_pending(ctx)) {
+			u32 tail = ctx->cached_cq_tail;
+
 			if (!llist_empty(&ctx->work_llist))
 				__io_run_local_work(ctx, true);
+
 			if (task_work_pending(current) ||
 			    wq_list_empty(&ctx->iopoll_list)) {
-				u32 tail = ctx->cached_cq_tail;
-
 				mutex_unlock(&ctx->uring_lock);
 				io_run_task_work();
 				mutex_lock(&ctx->uring_lock);
-
-				/* some requests don't go through iopoll_list */
-				if (tail != ctx->cached_cq_tail ||
-				    wq_list_empty(&ctx->iopoll_list))
-					break;
 			}
+			/* some requests don't go through iopoll_list */
+			if (tail != ctx->cached_cq_tail ||
+			    wq_list_empty(&ctx->iopoll_list))
+				break;
 		}
 		ret = io_do_iopoll(ctx, !min);
 		if (ret < 0)
-- 
2.37.2

[PATCH 5/6] io_uring: add fast path for io_run_local_work()

[Pavel Begunkov]

We'll grab uring_lock and call __io_run_local_work() with several
atomics inside even if there are no task works. Skip it if ->work_llist
is empty.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 io_uring/io_uring.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 2fb5f1e78fb2..85b795e4dc6a 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -1208,6 +1208,9 @@ int io_run_local_work(struct io_ring_ctx *ctx)
 	bool locked;
 	int ret;
 
+	if (llist_empty(&ctx->work_llist))
+		return 0;
+
 	locked = mutex_trylock(&ctx->uring_lock);
 	ret = __io_run_local_work(ctx, locked);
 	if (locked)
-- 
2.37.2

Re: [PATCH 5/6] io_uring: add fast path for io_run_local_work()

[Dylan Yudaken]

On Thu, 2022-09-08 at 16:56 +0100, Pavel Begunkov wrote:
> We'll grab uring_lock and call __io_run_local_work() with several
> atomics inside even if there are no task works. Skip it if -
> >work_llist
> is empty.
> 
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
>  io_uring/io_uring.c | 3 +++
>  1 file changed, 3 insertions(+)

Reviewed-by: Dylan Yudaken <dylany@fb.com>

[PATCH 6/6] io_uring: remove unused return from io_disarm_next

[Pavel Begunkov]

We removed conditional io_commit_cqring_flush() guarding against
spurious eventfd and the io_disarm_next()'s return value is not used
anymore, just void it.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 io_uring/timeout.c | 13 +++----------
 io_uring/timeout.h |  2 +-
 2 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/io_uring/timeout.c b/io_uring/timeout.c
index 78ea2c64b70e..e8a8c2099480 100644
--- a/io_uring/timeout.c
+++ b/io_uring/timeout.c
@@ -149,11 +149,10 @@ static inline void io_remove_next_linked(struct io_kiocb *req)
 	nxt->link = NULL;
 }
 
-bool io_disarm_next(struct io_kiocb *req)
+void io_disarm_next(struct io_kiocb *req)
 	__must_hold(&req->ctx->completion_lock)
 {
 	struct io_kiocb *link = NULL;
-	bool posted = false;
 
 	if (req->flags & REQ_F_ARM_LTIMEOUT) {
 		link = req->link;
@@ -161,7 +160,6 @@ bool io_disarm_next(struct io_kiocb *req)
 		if (link && link->opcode == IORING_OP_LINK_TIMEOUT) {
 			io_remove_next_linked(req);
 			io_req_tw_post_queue(link, -ECANCELED, 0);
-			posted = true;
 		}
 	} else if (req->flags & REQ_F_LINK_TIMEOUT) {
 		struct io_ring_ctx *ctx = req->ctx;
@@ -169,17 +167,12 @@ bool io_disarm_next(struct io_kiocb *req)
 		spin_lock_irq(&ctx->timeout_lock);
 		link = io_disarm_linked_timeout(req);
 		spin_unlock_irq(&ctx->timeout_lock);
-		if (link) {
-			posted = true;
+		if (link)
 			io_req_tw_post_queue(link, -ECANCELED, 0);
-		}
 	}
 	if (unlikely((req->flags & REQ_F_FAIL) &&
-		     !(req->flags & REQ_F_HARDLINK))) {
-		posted |= (req->link != NULL);
+		     !(req->flags & REQ_F_HARDLINK)))
 		io_fail_links(req);
-	}
-	return posted;
 }
 
 struct io_kiocb *__io_disarm_linked_timeout(struct io_kiocb *req,
diff --git a/io_uring/timeout.h b/io_uring/timeout.h
index 858c62644897..a6939f18313e 100644
--- a/io_uring/timeout.h
+++ b/io_uring/timeout.h
@@ -27,7 +27,7 @@ int io_timeout_cancel(struct io_ring_ctx *ctx, struct io_cancel_data *cd);
 __cold bool io_kill_timeouts(struct io_ring_ctx *ctx, struct task_struct *tsk,
 			     bool cancel_all);
 void io_queue_linked_timeout(struct io_kiocb *req);
-bool io_disarm_next(struct io_kiocb *req);
+void io_disarm_next(struct io_kiocb *req);
 
 int io_timeout_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe);
 int io_link_timeout_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe);
-- 
2.37.2

Re: [PATCH for-next 0/6] defer tw fixes and other cleanups

[Jens Axboe]

On Thu, 8 Sep 2022 16:56:51 +0100, Pavel Begunkov wrote:
> 1-4 fix some defer tw problems, 5-6 are just dropped into the pile.
> 
> note: we intentionally break defer-taskrun test here.
> 
> Pavel Begunkov (6):
>   io_uring: further limit non-owner defer-tw cq waiting
>   io_uring: disallow defer-tw run w/ no submitters
>   io_uring/iopoll: fix unexpected returns
>   io_uring/iopoll: unify tw breaking logic
>   io_uring: add fast path for io_run_local_work()
>   io_uring: remove unused return from io_disarm_next
> 
> [...]

Applied, thanks!

[1/6] io_uring: further limit non-owner defer-tw cq waiting
      commit: 3ecccd114f6030f882e0dbc80d4958a80e0acb1c
[2/6] io_uring: disallow defer-tw run w/ no submitters
      commit: 598ee40dc2602dea491b72d003c505e4e5ce4e36
[3/6] io_uring/iopoll: fix unexpected returns
      commit: 6ccbce854d5d30d328cf4735869c40a13113c89a
[4/6] io_uring/iopoll: unify tw breaking logic
      commit: c701863860cb6bc822ab0c96a351f8cd21c82b26
[5/6] io_uring: add fast path for io_run_local_work()
      commit: 2592425110ebfdc1f8b5d89bb4460d5b028840a6
[6/6] io_uring: remove unused return from io_disarm_next
      commit: 62073e8ffd01ca543eab391e0af9a3194a6e9df3

Best regards,
-- 
Jens Axboe