[PATCH for-next 0/9] poll & rsrc quiesce improvements

[PATCH for-next 0/9] poll & rsrc quiesce improvements

[Pavel Begunkov]

A bunch of random patches cleaning up poll and doing some
preparation for future work.

Pavel Begunkov (9):
  io_uring: kill io_poll_issue's PF_EXITING check
  io_uring: carve io_poll_check_events fast path
  io_uring: remove ctx variable in io_poll_check_events
  io_uring: imporve poll warning handling
  io_uring: combine poll tw handlers
  io_uring: don't raw spin unlock to match cq_lock
  io_uring: improve rsrc quiesce refs checks
  io_uring: don't reinstall quiesce node for each tw
  io_uring: reshuffle issue_flags

 include/linux/io_uring.h | 11 ++---
 io_uring/io_uring.c      |  4 +-
 io_uring/io_uring.h      |  5 ++
 io_uring/poll.c          | 98 ++++++++++++++++++----------------------
 io_uring/rsrc.c          | 53 ++++++++++------------
 5 files changed, 79 insertions(+), 92 deletions(-)

-- 
2.38.1

[PATCH for-next 1/9] io_uring: kill io_poll_issue's PF_EXITING check

[Pavel Begunkov]

We don't need to worry about checking PF_EXITING in io_poll_issue().
task works using the function should take care of it and never try to
resubmit / retry if the task is dying.

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

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index adecdf65b130..15d285d8ce0f 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -1808,8 +1808,6 @@ static int io_issue_sqe(struct io_kiocb *req, unsigned int issue_flags)
 int io_poll_issue(struct io_kiocb *req, bool *locked)
 {
 	io_tw_lock(req->ctx, locked);
-	if (unlikely(req->task->flags & PF_EXITING))
-		return -EFAULT;
 	return io_issue_sqe(req, IO_URING_F_NONBLOCK|IO_URING_F_MULTISHOT|
 				 IO_URING_F_COMPLETE_DEFER);
 }
-- 
2.38.1

[PATCH for-next 2/9] io_uring: carve io_poll_check_events fast path

[Pavel Begunkov]

The fast path in io_poll_check_events() is when we have only one
(i.e. master) reference. Move all verification, cancellations
checks, edge case handling and so on under a common if.

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

diff --git a/io_uring/poll.c b/io_uring/poll.c
index 599ba28c89b2..8987e13d302e 100644
--- a/io_uring/poll.c
+++ b/io_uring/poll.c
@@ -247,27 +247,30 @@ static int io_poll_check_events(struct io_kiocb *req, bool *locked)
 	do {
 		v = atomic_read(&req->poll_refs);
 
-		/* tw handler should be the owner, and so have some references */
-		if (WARN_ON_ONCE(!(v & IO_POLL_REF_MASK)))
-			return IOU_POLL_DONE;
-		if (v & IO_POLL_CANCEL_FLAG)
-			return -ECANCELED;
-		/*
-		 * cqe.res contains only events of the first wake up
-		 * and all others are be lost. Redo vfs_poll() to get
-		 * up to date state.
-		 */
-		if ((v & IO_POLL_REF_MASK) != 1)
-			req->cqe.res = 0;
-		if (v & IO_POLL_RETRY_FLAG) {
-			req->cqe.res = 0;
+		if (unlikely(v != 1)) {
+			/* tw should be the owner and so have some refs */
+			if (WARN_ON_ONCE(!(v & IO_POLL_REF_MASK)))
+				return IOU_POLL_DONE;
+			if (v & IO_POLL_CANCEL_FLAG)
+				return -ECANCELED;
 			/*
-			 * We won't find new events that came in between
-			 * vfs_poll and the ref put unless we clear the flag
-			 * in advance.
+			 * cqe.res contains only events of the first wake up
+			 * and all others are to be lost. Redo vfs_poll() to get
+			 * up to date state.
 			 */
-			atomic_andnot(IO_POLL_RETRY_FLAG, &req->poll_refs);
-			v &= ~IO_POLL_RETRY_FLAG;
+			if ((v & IO_POLL_REF_MASK) != 1)
+				req->cqe.res = 0;
+
+			if (v & IO_POLL_RETRY_FLAG) {
+				req->cqe.res = 0;
+				/*
+				 * We won't find new events that came in between
+				 * vfs_poll and the ref put unless we clear the
+				 * flag in advance.
+				 */
+				atomic_andnot(IO_POLL_RETRY_FLAG, &req->poll_refs);
+				v &= ~IO_POLL_RETRY_FLAG;
+			}
 		}
 
 		/* the mask was stashed in __io_poll_execute */
-- 
2.38.1

[PATCH for-next 3/9] io_uring: remove ctx variable in io_poll_check_events

[Pavel Begunkov]

ctx is only used by io_poll_check_events() for multishot poll CQE
posting, don't save it on stack in advance.

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

diff --git a/io_uring/poll.c b/io_uring/poll.c
index 8987e13d302e..ada0017e3d88 100644
--- a/io_uring/poll.c
+++ b/io_uring/poll.c
@@ -237,7 +237,6 @@ enum {
  */
 static int io_poll_check_events(struct io_kiocb *req, bool *locked)
 {
-	struct io_ring_ctx *ctx = req->ctx;
 	int v, ret;
 
 	/* req->task == current here, checking PF_EXITING is safe */
@@ -289,7 +288,7 @@ static int io_poll_check_events(struct io_kiocb *req, bool *locked)
 			__poll_t mask = mangle_poll(req->cqe.res &
 						    req->apoll_events);
 
-			if (!io_aux_cqe(ctx, *locked, req->cqe.user_data,
+			if (!io_aux_cqe(req->ctx, *locked, req->cqe.user_data,
 					mask, IORING_CQE_F_MORE, false)) {
 				io_req_set_res(req, mask, 0);
 				return IOU_POLL_REMOVE_POLL_USE_RES;
-- 
2.38.1

[PATCH for-next 4/9] io_uring: imporve poll warning handling

[Pavel Begunkov]

Don't try to complete requests if their refs are broken and we've got
a warning, it's much better to drop them and potentially leaking than
double freeing.

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

diff --git a/io_uring/poll.c b/io_uring/poll.c
index ada0017e3d88..8f16d2a48ff8 100644
--- a/io_uring/poll.c
+++ b/io_uring/poll.c
@@ -249,7 +249,7 @@ static int io_poll_check_events(struct io_kiocb *req, bool *locked)
 		if (unlikely(v != 1)) {
 			/* tw should be the owner and so have some refs */
 			if (WARN_ON_ONCE(!(v & IO_POLL_REF_MASK)))
-				return IOU_POLL_DONE;
+				return IOU_POLL_NO_ACTION;
 			if (v & IO_POLL_CANCEL_FLAG)
 				return -ECANCELED;
 			/*
-- 
2.38.1

[PATCH for-next 5/9] io_uring: combine poll tw handlers

[Pavel Begunkov]

Merge apoll and regular poll tw handlers, it will help with inlining.

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

diff --git a/io_uring/poll.c b/io_uring/poll.c
index 8f16d2a48ff8..ee7da6150ec4 100644
--- a/io_uring/poll.c
+++ b/io_uring/poll.c
@@ -321,50 +321,38 @@ static void io_poll_task_func(struct io_kiocb *req, bool *locked)
 	ret = io_poll_check_events(req, locked);
 	if (ret == IOU_POLL_NO_ACTION)
 		return;
-
-	if (ret == IOU_POLL_DONE) {
-		struct io_poll *poll = io_kiocb_to_cmd(req, struct io_poll);
-		req->cqe.res = mangle_poll(req->cqe.res & poll->events);
-	} else if (ret != IOU_POLL_REMOVE_POLL_USE_RES) {
-		req->cqe.res = ret;
-		req_set_fail(req);
-	}
-
 	io_poll_remove_entries(req);
 	io_poll_tw_hash_eject(req, locked);
 
-	io_req_set_res(req, req->cqe.res, 0);
-	io_req_task_complete(req, locked);
-}
+	if (req->opcode == IORING_OP_POLL_ADD) {
+		if (ret == IOU_POLL_DONE) {
+			struct io_poll *poll;
 
-static void io_apoll_task_func(struct io_kiocb *req, bool *locked)
-{
-	int ret;
-
-	ret = io_poll_check_events(req, locked);
-	if (ret == IOU_POLL_NO_ACTION)
-		return;
-
-	io_tw_lock(req->ctx, locked);
-	io_poll_remove_entries(req);
-	io_poll_tw_hash_eject(req, locked);
+			poll = io_kiocb_to_cmd(req, struct io_poll);
+			req->cqe.res = mangle_poll(req->cqe.res & poll->events);
+		} else if (ret != IOU_POLL_REMOVE_POLL_USE_RES) {
+			req->cqe.res = ret;
+			req_set_fail(req);
+		}
 
-	if (ret == IOU_POLL_REMOVE_POLL_USE_RES)
+		io_req_set_res(req, req->cqe.res, 0);
 		io_req_task_complete(req, locked);
-	else if (ret == IOU_POLL_DONE)
-		io_req_task_submit(req, locked);
-	else
-		io_req_defer_failed(req, ret);
+	} else {
+		io_tw_lock(req->ctx, locked);
+
+		if (ret == IOU_POLL_REMOVE_POLL_USE_RES)
+			io_req_task_complete(req, locked);
+		else if (ret == IOU_POLL_DONE)
+			io_req_task_submit(req, locked);
+		else
+			io_req_defer_failed(req, ret);
+	}
 }
 
 static void __io_poll_execute(struct io_kiocb *req, int mask)
 {
 	io_req_set_res(req, mask, 0);
-
-	if (req->opcode == IORING_OP_POLL_ADD)
-		req->io_task_work.func = io_poll_task_func;
-	else
-		req->io_task_work.func = io_apoll_task_func;
+	req->io_task_work.func = io_poll_task_func;
 
 	trace_io_uring_task_add(req, mask);
 	io_req_task_work_add(req);
-- 
2.38.1

[PATCH for-next 6/9] io_uring: don't raw spin unlock to match cq_lock

[Pavel Begunkov]

There is one newly added place when we lock ring with io_cq_lock() but
unlocking is hand coded calling spin_unlock directly. It's ugly and
troublesome in the long run.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 io_uring/io_uring.c | 2 +-
 io_uring/io_uring.h | 5 +++++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 15d285d8ce0f..c30765579a8e 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -860,7 +860,7 @@ bool io_aux_cqe(struct io_ring_ctx *ctx, bool defer, u64 user_data, s32 res, u32
 		io_cq_lock(ctx);
 		__io_flush_post_cqes(ctx);
 		/* no need to flush - flush is deferred */
-		spin_unlock(&ctx->completion_lock);
+		io_cq_unlock(ctx);
 	}
 
 	/* For defered completions this is not as strict as it is otherwise,
diff --git a/io_uring/io_uring.h b/io_uring/io_uring.h
index 062899b1fe86..2277c05f52a6 100644
--- a/io_uring/io_uring.h
+++ b/io_uring/io_uring.h
@@ -93,6 +93,11 @@ static inline void io_cq_lock(struct io_ring_ctx *ctx)
 	spin_lock(&ctx->completion_lock);
 }
 
+static inline void io_cq_unlock(struct io_ring_ctx *ctx)
+{
+	spin_unlock(&ctx->completion_lock);
+}
+
 void io_cq_unlock_post(struct io_ring_ctx *ctx);
 
 static inline struct io_uring_cqe *io_get_cqe_overflow(struct io_ring_ctx *ctx,
-- 
2.38.1

[PATCH for-next 7/9] io_uring: improve rsrc quiesce refs checks

[Pavel Begunkov]

Do a little bit of refactoring of io_rsrc_ref_quiesce(), flat the data
refs checks and so get rid of a conditional weird unlock-else-break
construct.

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

diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c
index 133608200769..b36d32534165 100644
--- a/io_uring/rsrc.c
+++ b/io_uring/rsrc.c
@@ -330,17 +330,14 @@ __cold static int io_rsrc_ref_quiesce(struct io_rsrc_data *data,
 		ret = wait_for_completion_interruptible(&data->done);
 		if (!ret) {
 			mutex_lock(&ctx->uring_lock);
-			if (atomic_read(&data->refs) > 0) {
-				/*
-				 * it has been revived by another thread while
-				 * we were unlocked
-				 */
-				mutex_unlock(&ctx->uring_lock);
-			} else {
+			if (atomic_read(&data->refs) <= 0)
 				break;
-			}
+			/*
+			 * it has been revived by another thread while
+			 * we were unlocked
+			 */
+			mutex_unlock(&ctx->uring_lock);
 		}
-
 reinit:
 		atomic_inc(&data->refs);
 		/* wait for all works potentially completing data->done */
-- 
2.38.1

[PATCH for-next 8/9] io_uring: don't reinstall quiesce node for each tw

[Pavel Begunkov]

There is no need to reinit data and install a new rsrc node every time
we get a task_work, it's detrimental, just execute it and conitnue
waiting.

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

diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c
index b36d32534165..d25309400a45 100644
--- a/io_uring/rsrc.c
+++ b/io_uring/rsrc.c
@@ -309,22 +309,27 @@ __cold static int io_rsrc_ref_quiesce(struct io_rsrc_data *data,
 	/* As we may drop ->uring_lock, other task may have started quiesce */
 	if (data->quiesce)
 		return -ENXIO;
+	ret = io_rsrc_node_switch_start(ctx);
+	if (ret)
+		return ret;
+	io_rsrc_node_switch(ctx, data);
+
+	/* kill initial ref, already quiesced if zero */
+	if (atomic_dec_and_test(&data->refs))
+		return 0;
 
 	data->quiesce = true;
+	mutex_unlock(&ctx->uring_lock);
 	do {
-		ret = io_rsrc_node_switch_start(ctx);
-		if (ret)
-			break;
-		io_rsrc_node_switch(ctx, data);
-
-		/* kill initial ref, already quiesced if zero */
-		if (atomic_dec_and_test(&data->refs))
-			break;
-		mutex_unlock(&ctx->uring_lock);
-
 		ret = io_run_task_work_sig(ctx);
-		if (ret < 0)
-			goto reinit;
+		if (ret < 0) {
+			atomic_inc(&data->refs);
+			/* wait for all works potentially completing data->done */
+			flush_delayed_work(&ctx->rsrc_put_work);
+			reinit_completion(&data->done);
+			mutex_lock(&ctx->uring_lock);
+			break;
+		}
 
 		flush_delayed_work(&ctx->rsrc_put_work);
 		ret = wait_for_completion_interruptible(&data->done);
@@ -338,14 +343,7 @@ __cold static int io_rsrc_ref_quiesce(struct io_rsrc_data *data,
 			 */
 			mutex_unlock(&ctx->uring_lock);
 		}
-reinit:
-		atomic_inc(&data->refs);
-		/* wait for all works potentially completing data->done */
-		flush_delayed_work(&ctx->rsrc_put_work);
-		reinit_completion(&data->done);
-
-		mutex_lock(&ctx->uring_lock);
-	} while (ret >= 0);
+	} while (1);
 	data->quiesce = false;
 
 	return ret;
-- 
2.38.1

[PATCH for-next 9/9] io_uring: reshuffle issue_flags

[Pavel Begunkov]

Reshuffle issue flags to keep normal flags separate from the uring_cmd
ctx-setup like flags. Shift the second type to the second byte so it's
easier to add new ones in the future.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 include/linux/io_uring.h | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/include/linux/io_uring.h b/include/linux/io_uring.h
index 0ded9e271523..29e519752da4 100644
--- a/include/linux/io_uring.h
+++ b/include/linux/io_uring.h
@@ -9,16 +9,15 @@
 enum io_uring_cmd_flags {
 	IO_URING_F_COMPLETE_DEFER	= 1,
 	IO_URING_F_UNLOCKED		= 2,
+	/* the request is executed from poll, it should not be freed */
+	IO_URING_F_MULTISHOT		= 4,
 	/* int's last bit, sign checks are usually faster than a bit test */
 	IO_URING_F_NONBLOCK		= INT_MIN,
 
 	/* ctx state flags, for URING_CMD */
-	IO_URING_F_SQE128		= 4,
-	IO_URING_F_CQE32		= 8,
-	IO_URING_F_IOPOLL		= 16,
-
-	/* the request is executed from poll, it should not be freed */
-	IO_URING_F_MULTISHOT		= 32,
+	IO_URING_F_SQE128		= (1 << 8),
+	IO_URING_F_CQE32		= (1 << 9),
+	IO_URING_F_IOPOLL		= (1 << 10),
 };
 
 struct io_uring_cmd {
-- 
2.38.1

Re: [PATCH for-next 0/9] poll & rsrc quiesce improvements

[Jens Axboe]

On Wed, 30 Nov 2022 15:21:50 +0000, Pavel Begunkov wrote:
> A bunch of random patches cleaning up poll and doing some
> preparation for future work.
> 
> Pavel Begunkov (9):
>   io_uring: kill io_poll_issue's PF_EXITING check
>   io_uring: carve io_poll_check_events fast path
>   io_uring: remove ctx variable in io_poll_check_events
>   io_uring: imporve poll warning handling
>   io_uring: combine poll tw handlers
>   io_uring: don't raw spin unlock to match cq_lock
>   io_uring: improve rsrc quiesce refs checks
>   io_uring: don't reinstall quiesce node for each tw
>   io_uring: reshuffle issue_flags
> 
> [...]

Applied, thanks!

[1/9] io_uring: kill io_poll_issue's PF_EXITING check
      commit: f6f7f903e78dddcb1e1552b896e0e3e9c14c17ae
[2/9] io_uring: carve io_poll_check_events fast path
      commit: 9805fa2d94993e16efd0e1adbd2b54d8d1fe2f9f
[3/9] io_uring: remove ctx variable in io_poll_check_events
      commit: 047b6aef0966f9863e1940b57c256ebbb465a6b5
[4/9] io_uring: imporve poll warning handling
      commit: c3bfb57ea7011e0c04e4b7f28cb357a551b1efb9
[5/9] io_uring: combine poll tw handlers
      commit: 443e57550670234f1bd34983b3c577edcf2eeef5
[6/9] io_uring: don't raw spin unlock to match cq_lock
      commit: 618d653a345a477aaae307a0455900eb8789e952
[7/9] io_uring: improve rsrc quiesce refs checks
      commit: 0ced756f6412123b01cd72e5741d9dd6ae5f1dd5
[8/9] io_uring: don't reinstall quiesce node for each tw
      commit: 77e3202a21967e7de5b4412c0534f2e34e175227
[9/9] io_uring: reshuffle issue_flags
      commit: 7500194a630b11236761df35fef300009d7d3f6f

Best regards,
-- 
Jens Axboe