io-uring.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC 0/5] leverage completion cache for poll requests
@ 2021-09-17 19:38 Hao Xu
  2021-09-17 19:38 ` [PATCH 1/5] io_uring: return boolean value for io_alloc_async_data Hao Xu
                   ` (5 more replies)
  0 siblings, 6 replies; 13+ messages in thread
From: Hao Xu @ 2021-09-17 19:38 UTC (permalink / raw)
  To: Jens Axboe; +Cc: io-uring, Pavel Begunkov, Joseph Qi

1st is a non-related code clean
2nd ~ 4th are fixes.
5th is the the main logic, I tested it with an echo-server, both in
single-shot mode and multishot mode poll, not much difference compared
to code before with regard to req/s. But that may be resulted from my
poor network knowledge, feel free to test it and leave comments.

Hao Xu (5):
  io_uring: return boolean value for io_alloc_async_data
  io_uring: code clean for io_poll_complete()
  io_uring: fix race between poll completion and cancel_hash insertion
  io_uring: fix lacking of EPOLLONESHOT
  io_uring: leverage completion cache for poll requests

 fs/io_uring.c | 70 ++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 53 insertions(+), 17 deletions(-)

-- 
2.24.4


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

* [PATCH 1/5] io_uring: return boolean value for io_alloc_async_data
  2021-09-17 19:38 [RFC 0/5] leverage completion cache for poll requests Hao Xu
@ 2021-09-17 19:38 ` Hao Xu
  2021-09-17 19:38 ` [PATCH 2/5] io_uring: code clean for io_poll_complete() Hao Xu
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 13+ messages in thread
From: Hao Xu @ 2021-09-17 19:38 UTC (permalink / raw)
  To: Jens Axboe; +Cc: io-uring, Pavel Begunkov, Joseph Qi

boolean value is good enough for io_alloc_async_data.

Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
---
 fs/io_uring.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 91e4c89abf78..05c449ea3fd3 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -3309,7 +3309,7 @@ static void io_req_map_rw(struct io_kiocb *req, const struct iovec *iovec,
 	}
 }
 
-static inline int io_alloc_async_data(struct io_kiocb *req)
+static inline bool io_alloc_async_data(struct io_kiocb *req)
 {
 	WARN_ON_ONCE(!io_op_defs[req->opcode].async_size);
 	req->async_data = kmalloc(io_op_defs[req->opcode].async_size, GFP_KERNEL);
-- 
2.24.4


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

* [PATCH 2/5] io_uring: code clean for io_poll_complete()
  2021-09-17 19:38 [RFC 0/5] leverage completion cache for poll requests Hao Xu
  2021-09-17 19:38 ` [PATCH 1/5] io_uring: return boolean value for io_alloc_async_data Hao Xu
@ 2021-09-17 19:38 ` Hao Xu
  2021-09-17 19:38 ` [PATCH 3/5] io_uring: fix race between poll completion and cancel_hash insertion Hao Xu
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 13+ messages in thread
From: Hao Xu @ 2021-09-17 19:38 UTC (permalink / raw)
  To: Jens Axboe; +Cc: io-uring, Pavel Begunkov, Joseph Qi

No need to return boolean value, since no callers consume it.

Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
---
 fs/io_uring.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 05c449ea3fd3..6ea7d4003499 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -5350,14 +5350,14 @@ static bool __io_poll_complete(struct io_kiocb *req, __poll_t mask)
 	return !(flags & IORING_CQE_F_MORE);
 }
 
-static inline bool io_poll_complete(struct io_kiocb *req, __poll_t mask)
+static inline void io_poll_complete(struct io_kiocb *req, __poll_t mask)
 	__must_hold(&req->ctx->completion_lock)
 {
 	bool done;
 
 	done = __io_poll_complete(req, mask);
 	io_commit_cqring(req->ctx);
-	return done;
+	return;
 }
 
 static void io_poll_task_func(struct io_kiocb *req, bool *locked)
-- 
2.24.4


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

* [PATCH 3/5] io_uring: fix race between poll completion and cancel_hash insertion
  2021-09-17 19:38 [RFC 0/5] leverage completion cache for poll requests Hao Xu
  2021-09-17 19:38 ` [PATCH 1/5] io_uring: return boolean value for io_alloc_async_data Hao Xu
  2021-09-17 19:38 ` [PATCH 2/5] io_uring: code clean for io_poll_complete() Hao Xu
@ 2021-09-17 19:38 ` Hao Xu
  2021-09-17 19:38 ` [PATCH 4/5] io_uring: fix lacking of EPOLLONESHOT Hao Xu
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 13+ messages in thread
From: Hao Xu @ 2021-09-17 19:38 UTC (permalink / raw)
  To: Jens Axboe; +Cc: io-uring, Pavel Begunkov, Joseph Qi

If poll arming and poll completion runs parallelly, there maybe races.
For instance, run io_poll_add in iowq and io_poll_task_func in original
context, then:
             iowq                          original context
  io_poll_add
    vfs_poll
     (interruption happens
      tw queued to original
      context)                              io_poll_task_func
                                              generate cqe
                                              del from cancel_hash[]
    if !poll.done
      insert to cancel_hash[]

The entry left in cancel_hash[], similar case for fast poll.
Fix it by set poll.done = true when del from cancel_hash[].

Fixes: 5082620fb2ca ("io_uring: terminate multishot poll for CQ ring overflow")
Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
---
 fs/io_uring.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 6ea7d4003499..02425022c3b0 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -5340,10 +5340,8 @@ static bool __io_poll_complete(struct io_kiocb *req, __poll_t mask)
 	}
 	if (req->poll.events & EPOLLONESHOT)
 		flags = 0;
-	if (!io_cqring_fill_event(ctx, req->user_data, error, flags)) {
-		req->poll.done = true;
+	if (!io_cqring_fill_event(ctx, req->user_data, error, flags))
 		flags = 0;
-	}
 	if (flags & IORING_CQE_F_MORE)
 		ctx->cq_extra++;
 
@@ -5374,6 +5372,7 @@ static void io_poll_task_func(struct io_kiocb *req, bool *locked)
 		if (done) {
 			io_poll_remove_double(req);
 			hash_del(&req->hash_node);
+			req->poll.done = true;
 		} else {
 			req->result = 0;
 			add_wait_queue(req->poll.head, &req->poll.wait);
@@ -5511,6 +5510,7 @@ static void io_async_task_func(struct io_kiocb *req, bool *locked)
 
 	hash_del(&req->hash_node);
 	io_poll_remove_double(req);
+	apoll->poll.done = true;
 	spin_unlock(&ctx->completion_lock);
 
 	if (!READ_ONCE(apoll->poll.canceled))
-- 
2.24.4


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

* [PATCH 4/5] io_uring: fix lacking of EPOLLONESHOT
  2021-09-17 19:38 [RFC 0/5] leverage completion cache for poll requests Hao Xu
                   ` (2 preceding siblings ...)
  2021-09-17 19:38 ` [PATCH 3/5] io_uring: fix race between poll completion and cancel_hash insertion Hao Xu
@ 2021-09-17 19:38 ` Hao Xu
  2021-09-17 19:38 ` [PATCH 5/5] io_uring: leverage completion cache for poll requests Hao Xu
  2021-09-17 20:28 ` [RFC 0/5] " Pavel Begunkov
  5 siblings, 0 replies; 13+ messages in thread
From: Hao Xu @ 2021-09-17 19:38 UTC (permalink / raw)
  To: Jens Axboe; +Cc: io-uring, Pavel Begunkov, Joseph Qi

We should set EPOLLONESHOT if cqring_fill_event() returns false since
io_poll_add() decides to put req or not by it.

Fixes: 5082620fb2ca ("io_uring: terminate multishot poll for CQ ring overflow")
Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
---
 fs/io_uring.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 02425022c3b0..b1d6c3a1d3cd 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -5340,8 +5340,10 @@ static bool __io_poll_complete(struct io_kiocb *req, __poll_t mask)
 	}
 	if (req->poll.events & EPOLLONESHOT)
 		flags = 0;
-	if (!io_cqring_fill_event(ctx, req->user_data, error, flags))
+	if (!io_cqring_fill_event(ctx, req->user_data, error, flags)) {
+		req->poll.events |= EPOLLONESHOT;
 		flags = 0;
+	}
 	if (flags & IORING_CQE_F_MORE)
 		ctx->cq_extra++;
 
-- 
2.24.4


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

* [PATCH 5/5] io_uring: leverage completion cache for poll requests
  2021-09-17 19:38 [RFC 0/5] leverage completion cache for poll requests Hao Xu
                   ` (3 preceding siblings ...)
  2021-09-17 19:38 ` [PATCH 4/5] io_uring: fix lacking of EPOLLONESHOT Hao Xu
@ 2021-09-17 19:38 ` Hao Xu
  2021-09-17 19:51   ` Hao Xu
  2021-09-17 20:39   ` Pavel Begunkov
  2021-09-17 20:28 ` [RFC 0/5] " Pavel Begunkov
  5 siblings, 2 replies; 13+ messages in thread
From: Hao Xu @ 2021-09-17 19:38 UTC (permalink / raw)
  To: Jens Axboe; +Cc: io-uring, Pavel Begunkov, Joseph Qi

Leverage completion cache to handle completions of poll requests in a
batch.
Good thing is we save compl_nr - 1 completion_lock and
io_cqring_ev_posted.
Bad thing is compl_nr extra ifs in flush_completion.

Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
---
 fs/io_uring.c | 64 +++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 49 insertions(+), 15 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index b1d6c3a1d3cd..0f72cb0bf79a 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1099,6 +1099,8 @@ static int io_install_fixed_file(struct io_kiocb *req, struct file *file,
 				 unsigned int issue_flags, u32 slot_index);
 static enum hrtimer_restart io_link_timeout_fn(struct hrtimer *timer);
 
+static bool io_complete_poll(struct io_kiocb *req);
+
 static struct kmem_cache *req_cachep;
 
 static const struct file_operations io_uring_fops;
@@ -2333,6 +2335,11 @@ static void __io_submit_flush_completions(struct io_ring_ctx *ctx)
 	for (i = 0; i < nr; i++) {
 		struct io_kiocb *req = state->compl_reqs[i];
 
+		if (req->opcode == IORING_OP_POLL_ADD) {
+			if (!io_complete_poll(req))
+				state->compl_reqs[i] = NULL;
+			continue;
+		}
 		__io_cqring_fill_event(ctx, req->user_data, req->result,
 					req->compl.cflags);
 	}
@@ -2344,7 +2351,7 @@ static void __io_submit_flush_completions(struct io_ring_ctx *ctx)
 	for (i = 0; i < nr; i++) {
 		struct io_kiocb *req = state->compl_reqs[i];
 
-		if (req_ref_put_and_test(req))
+		if (req && req_ref_put_and_test(req))
 			io_req_free_batch(&rb, req, &ctx->submit_state);
 	}
 
@@ -5360,6 +5367,23 @@ static inline void io_poll_complete(struct io_kiocb *req, __poll_t mask)
 	return;
 }
 
+static bool io_complete_poll(struct io_kiocb *req)
+{
+	bool done;
+
+	done = __io_poll_complete(req, req->result);
+	if (done) {
+		io_poll_remove_double(req);
+		hash_del(&req->hash_node);
+		req->poll.done = true;
+	} else {
+		req->result = 0;
+		add_wait_queue(req->poll.head, &req->poll.wait);
+	}
+
+	return done;
+}
+
 static void io_poll_task_func(struct io_kiocb *req, bool *locked)
 {
 	struct io_ring_ctx *ctx = req->ctx;
@@ -5367,18 +5391,10 @@ static void io_poll_task_func(struct io_kiocb *req, bool *locked)
 
 	if (io_poll_rewait(req, &req->poll)) {
 		spin_unlock(&ctx->completion_lock);
-	} else {
+	} else if (!*locked) {
 		bool done;
 
-		done = __io_poll_complete(req, req->result);
-		if (done) {
-			io_poll_remove_double(req);
-			hash_del(&req->hash_node);
-			req->poll.done = true;
-		} else {
-			req->result = 0;
-			add_wait_queue(req->poll.head, &req->poll.wait);
-		}
+		done = io_complete_poll(req);
 		io_commit_cqring(ctx);
 		spin_unlock(&ctx->completion_lock);
 		io_cqring_ev_posted(ctx);
@@ -5388,6 +5404,13 @@ static void io_poll_task_func(struct io_kiocb *req, bool *locked)
 			if (nxt)
 				io_req_task_submit(nxt, locked);
 		}
+	} else {
+		struct io_submit_state *state = &ctx->submit_state;
+
+		spin_unlock(&ctx->completion_lock);
+		state->compl_reqs[state->compl_nr++] = req;
+		if (state->compl_nr == ARRAY_SIZE(state->compl_reqs))
+			io_submit_flush_completions(ctx);
 	}
 }
 
@@ -5833,6 +5856,7 @@ static int io_poll_add(struct io_kiocb *req, unsigned int issue_flags)
 	struct io_ring_ctx *ctx = req->ctx;
 	struct io_poll_table ipt;
 	__poll_t mask;
+	bool locked = current == req->task;
 
 	ipt.pt._qproc = io_poll_queue_proc;
 
@@ -5841,14 +5865,24 @@ static int io_poll_add(struct io_kiocb *req, unsigned int issue_flags)
 
 	if (mask) { /* no async, we'd stolen it */
 		ipt.error = 0;
-		io_poll_complete(req, mask);
+		if (!locked)
+			io_poll_complete(req, mask);
 	}
 	spin_unlock(&ctx->completion_lock);
 
 	if (mask) {
-		io_cqring_ev_posted(ctx);
-		if (poll->events & EPOLLONESHOT)
-			io_put_req(req);
+		if (!locked) {
+			io_cqring_ev_posted(ctx);
+			if (poll->events & EPOLLONESHOT)
+				io_put_req(req);
+		} else {
+			struct io_submit_state *state = &ctx->submit_state;
+
+			req->result = mask;
+			state->compl_reqs[state->compl_nr++] = req;
+			if (state->compl_nr == ARRAY_SIZE(state->compl_reqs))
+				io_submit_flush_completions(ctx);
+		}
 	}
 	return ipt.error;
 }
-- 
2.24.4


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

* Re: [PATCH 5/5] io_uring: leverage completion cache for poll requests
  2021-09-17 19:38 ` [PATCH 5/5] io_uring: leverage completion cache for poll requests Hao Xu
@ 2021-09-17 19:51   ` Hao Xu
  2021-09-17 20:39   ` Pavel Begunkov
  1 sibling, 0 replies; 13+ messages in thread
From: Hao Xu @ 2021-09-17 19:51 UTC (permalink / raw)
  To: Jens Axboe; +Cc: io-uring, Pavel Begunkov, Joseph Qi

在 2021/9/18 上午3:38, Hao Xu 写道:
> Leverage completion cache to handle completions of poll requests in a
> batch.
> Good thing is we save compl_nr - 1 completion_lock and
> io_cqring_ev_posted.
> Bad thing is compl_nr extra ifs in flush_completion.
> 
> Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
Sorry, please ignore this one, didn't notice it failed to pass
poll-mshot-update test
> ---
>   fs/io_uring.c | 64 +++++++++++++++++++++++++++++++++++++++------------
>   1 file changed, 49 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index b1d6c3a1d3cd..0f72cb0bf79a 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -1099,6 +1099,8 @@ static int io_install_fixed_file(struct io_kiocb *req, struct file *file,
>   				 unsigned int issue_flags, u32 slot_index);
>   static enum hrtimer_restart io_link_timeout_fn(struct hrtimer *timer);
>   
> +static bool io_complete_poll(struct io_kiocb *req);
> +
>   static struct kmem_cache *req_cachep;
>   
>   static const struct file_operations io_uring_fops;
> @@ -2333,6 +2335,11 @@ static void __io_submit_flush_completions(struct io_ring_ctx *ctx)
>   	for (i = 0; i < nr; i++) {
>   		struct io_kiocb *req = state->compl_reqs[i];
>   
> +		if (req->opcode == IORING_OP_POLL_ADD) {
> +			if (!io_complete_poll(req))
> +				state->compl_reqs[i] = NULL;
> +			continue;
> +		}
>   		__io_cqring_fill_event(ctx, req->user_data, req->result,
>   					req->compl.cflags);
>   	}
> @@ -2344,7 +2351,7 @@ static void __io_submit_flush_completions(struct io_ring_ctx *ctx)
>   	for (i = 0; i < nr; i++) {
>   		struct io_kiocb *req = state->compl_reqs[i];
>   
> -		if (req_ref_put_and_test(req))
> +		if (req && req_ref_put_and_test(req))
>   			io_req_free_batch(&rb, req, &ctx->submit_state);
>   	}
>   
> @@ -5360,6 +5367,23 @@ static inline void io_poll_complete(struct io_kiocb *req, __poll_t mask)
>   	return;
>   }
>   
> +static bool io_complete_poll(struct io_kiocb *req)
> +{
> +	bool done;
> +
> +	done = __io_poll_complete(req, req->result);
> +	if (done) {
> +		io_poll_remove_double(req);
> +		hash_del(&req->hash_node);
> +		req->poll.done = true;
> +	} else {
> +		req->result = 0;
> +		add_wait_queue(req->poll.head, &req->poll.wait);
> +	}
> +
> +	return done;
> +}
> +
>   static void io_poll_task_func(struct io_kiocb *req, bool *locked)
>   {
>   	struct io_ring_ctx *ctx = req->ctx;
> @@ -5367,18 +5391,10 @@ static void io_poll_task_func(struct io_kiocb *req, bool *locked)
>   
>   	if (io_poll_rewait(req, &req->poll)) {
>   		spin_unlock(&ctx->completion_lock);
> -	} else {
> +	} else if (!*locked) {
>   		bool done;
>   
> -		done = __io_poll_complete(req, req->result);
> -		if (done) {
> -			io_poll_remove_double(req);
> -			hash_del(&req->hash_node);
> -			req->poll.done = true;
> -		} else {
> -			req->result = 0;
> -			add_wait_queue(req->poll.head, &req->poll.wait);
> -		}
> +		done = io_complete_poll(req);
>   		io_commit_cqring(ctx);
>   		spin_unlock(&ctx->completion_lock);
>   		io_cqring_ev_posted(ctx);
> @@ -5388,6 +5404,13 @@ static void io_poll_task_func(struct io_kiocb *req, bool *locked)
>   			if (nxt)
>   				io_req_task_submit(nxt, locked);
>   		}
> +	} else {
> +		struct io_submit_state *state = &ctx->submit_state;
> +
> +		spin_unlock(&ctx->completion_lock);
> +		state->compl_reqs[state->compl_nr++] = req;
> +		if (state->compl_nr == ARRAY_SIZE(state->compl_reqs))
> +			io_submit_flush_completions(ctx);
>   	}
>   }
>   
> @@ -5833,6 +5856,7 @@ static int io_poll_add(struct io_kiocb *req, unsigned int issue_flags)
>   	struct io_ring_ctx *ctx = req->ctx;
>   	struct io_poll_table ipt;
>   	__poll_t mask;
> +	bool locked = current == req->task;
>   
>   	ipt.pt._qproc = io_poll_queue_proc;
>   
> @@ -5841,14 +5865,24 @@ static int io_poll_add(struct io_kiocb *req, unsigned int issue_flags)
>   
>   	if (mask) { /* no async, we'd stolen it */
>   		ipt.error = 0;
> -		io_poll_complete(req, mask);
> +		if (!locked)
> +			io_poll_complete(req, mask);
>   	}
>   	spin_unlock(&ctx->completion_lock);
>   
>   	if (mask) {
> -		io_cqring_ev_posted(ctx);
> -		if (poll->events & EPOLLONESHOT)
> -			io_put_req(req);
> +		if (!locked) {
> +			io_cqring_ev_posted(ctx);
> +			if (poll->events & EPOLLONESHOT)
> +				io_put_req(req);
> +		} else {
> +			struct io_submit_state *state = &ctx->submit_state;
> +
> +			req->result = mask;
> +			state->compl_reqs[state->compl_nr++] = req;
> +			if (state->compl_nr == ARRAY_SIZE(state->compl_reqs))
> +				io_submit_flush_completions(ctx);
> +		}
>   	}
>   	return ipt.error;
>   }
> 


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

* Re: [RFC 0/5] leverage completion cache for poll requests
  2021-09-17 19:38 [RFC 0/5] leverage completion cache for poll requests Hao Xu
                   ` (4 preceding siblings ...)
  2021-09-17 19:38 ` [PATCH 5/5] io_uring: leverage completion cache for poll requests Hao Xu
@ 2021-09-17 20:28 ` Pavel Begunkov
  5 siblings, 0 replies; 13+ messages in thread
From: Pavel Begunkov @ 2021-09-17 20:28 UTC (permalink / raw)
  To: Hao Xu, Jens Axboe; +Cc: io-uring, Joseph Qi

On 9/17/21 8:38 PM, Hao Xu wrote:
> 1st is a non-related code clean
> 2nd ~ 4th are fixes.

Looks only 3 and 4 are fixes. It's usually a good idea to send
fixes separately or place them first in the series, so can be
picked for-current / for-next. However, 1-2 are very simple,
maybe easier to leave as is.


> 5th is the the main logic, I tested it with an echo-server, both in
> single-shot mode and multishot mode poll, not much difference compared
> to code before with regard to req/s. But that may be resulted from my
> poor network knowledge, feel free to test it and leave comments.
> 
> Hao Xu (5):
>   io_uring: return boolean value for io_alloc_async_data
>   io_uring: code clean for io_poll_complete()
>   io_uring: fix race between poll completion and cancel_hash insertion
>   io_uring: fix lacking of EPOLLONESHOT
>   io_uring: leverage completion cache for poll requests
> 
>  fs/io_uring.c | 70 ++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 53 insertions(+), 17 deletions(-)
> 

-- 
Pavel Begunkov

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

* Re: [PATCH 5/5] io_uring: leverage completion cache for poll requests
  2021-09-17 19:38 ` [PATCH 5/5] io_uring: leverage completion cache for poll requests Hao Xu
  2021-09-17 19:51   ` Hao Xu
@ 2021-09-17 20:39   ` Pavel Begunkov
  2021-09-18  6:11     ` Hao Xu
  1 sibling, 1 reply; 13+ messages in thread
From: Pavel Begunkov @ 2021-09-17 20:39 UTC (permalink / raw)
  To: Hao Xu, Jens Axboe; +Cc: io-uring, Joseph Qi

On 9/17/21 8:38 PM, Hao Xu wrote:
> Leverage completion cache to handle completions of poll requests in a
> batch.
> Good thing is we save compl_nr - 1 completion_lock and
> io_cqring_ev_posted.
> Bad thing is compl_nr extra ifs in flush_completion.

It does something really wrong breaking all abstractions, we can't go
with this. Can we have one of those below?
- __io_req_complete(issue_flags), forwarding issue_flags from above
- or maybe io_req_task_complete(), if we're in tw

In any case, I'd recommend sending it separately from fixes.

-- 
Pavel Begunkov

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

* Re: [PATCH 5/5] io_uring: leverage completion cache for poll requests
  2021-09-17 20:39   ` Pavel Begunkov
@ 2021-09-18  6:11     ` Hao Xu
  2021-09-19 12:04       ` Pavel Begunkov
  0 siblings, 1 reply; 13+ messages in thread
From: Hao Xu @ 2021-09-18  6:11 UTC (permalink / raw)
  To: Pavel Begunkov, Jens Axboe; +Cc: io-uring, Joseph Qi

在 2021/9/18 上午4:39, Pavel Begunkov 写道:
> On 9/17/21 8:38 PM, Hao Xu wrote:
>> Leverage completion cache to handle completions of poll requests in a
>> batch.
>> Good thing is we save compl_nr - 1 completion_lock and
>> io_cqring_ev_posted.
>> Bad thing is compl_nr extra ifs in flush_completion.
> 
> It does something really wrong breaking all abstractions, we can't go
> with this. Can we have one of those below?
> - __io_req_complete(issue_flags), forwarding issue_flags from above
> - or maybe io_req_task_complete(), if we're in tw
Make sense. we may need to remove io_clean_op logic in
io_req_complete_state(), multishot reqs shouldn't do it, and it's ok for
normal reqs since we do it later in __io_submit_flush_completions->
io_req_free_batch->io_dismantle_req->io_clean_op, correct me if I'm
wrong.
I'll send another version later.

> 
> In any case, I'd recommend sending it separately from fixes.
> 


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

* Re: [PATCH 5/5] io_uring: leverage completion cache for poll requests
  2021-09-18  6:11     ` Hao Xu
@ 2021-09-19 12:04       ` Pavel Begunkov
  2021-09-19 15:52         ` Hao Xu
  0 siblings, 1 reply; 13+ messages in thread
From: Pavel Begunkov @ 2021-09-19 12:04 UTC (permalink / raw)
  To: Hao Xu, Jens Axboe; +Cc: io-uring, Joseph Qi

On 9/18/21 7:11 AM, Hao Xu wrote:
> 在 2021/9/18 上午4:39, Pavel Begunkov 写道:
>> On 9/17/21 8:38 PM, Hao Xu wrote:
>>> Leverage completion cache to handle completions of poll requests in a
>>> batch.
>>> Good thing is we save compl_nr - 1 completion_lock and
>>> io_cqring_ev_posted.
>>> Bad thing is compl_nr extra ifs in flush_completion.
>>
>> It does something really wrong breaking all abstractions, we can't go
>> with this. Can we have one of those below?
>> - __io_req_complete(issue_flags), forwarding issue_flags from above
>> - or maybe io_req_task_complete(), if we're in tw
> Make sense. we may need to remove io_clean_op logic in

> io_req_complete_state(), multishot reqs shouldn't do it, and it's ok for
> normal reqs since we do it later in __io_submit_flush_completions->
> io_req_free_batch->io_dismantle_req->io_clean_op, correct me if I'm
> wrong.

req->compl.cflags is in the first 64B, i.e. aliased with req->rw and
others. We need to clean everything left in there before using the
space, that's what io_clean_op() there is for

-- 
Pavel Begunkov

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

* Re: [PATCH 5/5] io_uring: leverage completion cache for poll requests
  2021-09-19 12:04       ` Pavel Begunkov
@ 2021-09-19 15:52         ` Hao Xu
  2021-09-19 16:10           ` Hao Xu
  0 siblings, 1 reply; 13+ messages in thread
From: Hao Xu @ 2021-09-19 15:52 UTC (permalink / raw)
  To: Pavel Begunkov, Jens Axboe; +Cc: io-uring, Joseph Qi

在 2021/9/19 下午8:04, Pavel Begunkov 写道:
> On 9/18/21 7:11 AM, Hao Xu wrote:
>> 在 2021/9/18 上午4:39, Pavel Begunkov 写道:
>>> On 9/17/21 8:38 PM, Hao Xu wrote:
>>>> Leverage completion cache to handle completions of poll requests in a
>>>> batch.
>>>> Good thing is we save compl_nr - 1 completion_lock and
>>>> io_cqring_ev_posted.
>>>> Bad thing is compl_nr extra ifs in flush_completion.
>>>
>>> It does something really wrong breaking all abstractions, we can't go
>>> with this. Can we have one of those below?
>>> - __io_req_complete(issue_flags), forwarding issue_flags from above
>>> - or maybe io_req_task_complete(), if we're in tw
>> Make sense. we may need to remove io_clean_op logic in
> 
>> io_req_complete_state(), multishot reqs shouldn't do it, and it's ok for
>> normal reqs since we do it later in __io_submit_flush_completions->
>> io_req_free_batch->io_dismantle_req->io_clean_op, correct me if I'm
>> wrong.
> 
> req->compl.cflags is in the first 64B, i.e. aliased with req->rw and
> others. We need to clean everything left in there before using the
> space, that's what io_clean_op() there is for
True, and that's why we souldn't do it for multishot reqs, since they
are not completed yet, and we won't reuse the req resource until its
final completion.
> 


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

* Re: [PATCH 5/5] io_uring: leverage completion cache for poll requests
  2021-09-19 15:52         ` Hao Xu
@ 2021-09-19 16:10           ` Hao Xu
  0 siblings, 0 replies; 13+ messages in thread
From: Hao Xu @ 2021-09-19 16:10 UTC (permalink / raw)
  To: Pavel Begunkov, Jens Axboe; +Cc: io-uring, Joseph Qi

在 2021/9/19 下午11:52, Hao Xu 写道:
> 在 2021/9/19 下午8:04, Pavel Begunkov 写道:
>> On 9/18/21 7:11 AM, Hao Xu wrote:
>>> 在 2021/9/18 上午4:39, Pavel Begunkov 写道:
>>>> On 9/17/21 8:38 PM, Hao Xu wrote:
>>>>> Leverage completion cache to handle completions of poll requests in a
>>>>> batch.
>>>>> Good thing is we save compl_nr - 1 completion_lock and
>>>>> io_cqring_ev_posted.
>>>>> Bad thing is compl_nr extra ifs in flush_completion.
>>>>
>>>> It does something really wrong breaking all abstractions, we can't go
>>>> with this. Can we have one of those below?
>>>> - __io_req_complete(issue_flags), forwarding issue_flags from above
>>>> - or maybe io_req_task_complete(), if we're in tw
>>> Make sense. we may need to remove io_clean_op logic in
>>
>>> io_req_complete_state(), multishot reqs shouldn't do it, and it's ok for
>>> normal reqs since we do it later in __io_submit_flush_completions->
>>> io_req_free_batch->io_dismantle_req->io_clean_op, correct me if I'm
>>> wrong.
>>
>> req->compl.cflags is in the first 64B, i.e. aliased with req->rw and
>> others. We need to clean everything left in there before using the
>> space, that's what io_clean_op() there is for
> True, and that's why we souldn't do it for multishot reqs, since they
> are not completed yet, and we won't reuse the req resource until its
> final completion.
I see what you are saying now..yes, we shouldn't remove that
io_clean_op there.
>>


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

end of thread, other threads:[~2021-09-19 16:10 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-17 19:38 [RFC 0/5] leverage completion cache for poll requests Hao Xu
2021-09-17 19:38 ` [PATCH 1/5] io_uring: return boolean value for io_alloc_async_data Hao Xu
2021-09-17 19:38 ` [PATCH 2/5] io_uring: code clean for io_poll_complete() Hao Xu
2021-09-17 19:38 ` [PATCH 3/5] io_uring: fix race between poll completion and cancel_hash insertion Hao Xu
2021-09-17 19:38 ` [PATCH 4/5] io_uring: fix lacking of EPOLLONESHOT Hao Xu
2021-09-17 19:38 ` [PATCH 5/5] io_uring: leverage completion cache for poll requests Hao Xu
2021-09-17 19:51   ` Hao Xu
2021-09-17 20:39   ` Pavel Begunkov
2021-09-18  6:11     ` Hao Xu
2021-09-19 12:04       ` Pavel Begunkov
2021-09-19 15:52         ` Hao Xu
2021-09-19 16:10           ` Hao Xu
2021-09-17 20:28 ` [RFC 0/5] " Pavel Begunkov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).