All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH for-next 0/5] 5.15 cleanups and optimisations
@ 2021-08-14 16:26 Pavel Begunkov
  2021-08-14 16:26 ` [PATCH 1/5] io_uring: optimise iowq refcounting Pavel Begunkov
                   ` (4 more replies)
  0 siblings, 5 replies; 13+ messages in thread
From: Pavel Begunkov @ 2021-08-14 16:26 UTC (permalink / raw)
  To: Jens Axboe, io-uring

Some improvements after killing refcounting, and other cleanups.

With 2/2 with will be only tracking reqs with
file->f_op == &io_uring_fops, which is nice.

Pavel Begunkov (5):
  io_uring: optimise iowq refcounting
  io_uring: don't inflight-track linked timeouts
  io_uring: optimise initial ltimeout refcounting
  io_uring: kill not necessary resubmit switch
  io_uring: deduplicate cancellation code

 fs/io_uring.c | 82 ++++++++++++++++++++++++---------------------------
 1 file changed, 38 insertions(+), 44 deletions(-)

-- 
2.32.0


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

* [PATCH 1/5] io_uring: optimise iowq refcounting
  2021-08-14 16:26 [PATCH for-next 0/5] 5.15 cleanups and optimisations Pavel Begunkov
@ 2021-08-14 16:26 ` Pavel Begunkov
  2021-08-14 19:13   ` Jens Axboe
  2021-08-14 16:26 ` [PATCH 2/5] io_uring: don't inflight-track linked timeouts Pavel Begunkov
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 13+ messages in thread
From: Pavel Begunkov @ 2021-08-14 16:26 UTC (permalink / raw)
  To: Jens Axboe, io-uring

If a requests is forwarded into io-wq, there is a good chance it hasn't
been refcounted yet and we can save one req_ref_get() by setting the
refcount number to the right value directly.

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

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 51c4166f68b5..0d9a443d4987 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1115,14 +1115,19 @@ static inline void req_ref_get(struct io_kiocb *req)
 	atomic_inc(&req->refs);
 }
 
-static inline void io_req_refcount(struct io_kiocb *req)
+static inline void __io_req_refcount(struct io_kiocb *req, int nr)
 {
 	if (!(req->flags & REQ_F_REFCOUNT)) {
 		req->flags |= REQ_F_REFCOUNT;
-		atomic_set(&req->refs, 1);
+		atomic_set(&req->refs, nr);
 	}
 }
 
+static inline void io_req_refcount(struct io_kiocb *req)
+{
+	__io_req_refcount(req, 1);
+}
+
 static inline void io_req_set_rsrc_node(struct io_kiocb *req)
 {
 	struct io_ring_ctx *ctx = req->ctx;
@@ -6311,9 +6316,11 @@ static void io_wq_submit_work(struct io_wq_work *work)
 	struct io_kiocb *timeout;
 	int ret = 0;
 
-	io_req_refcount(req);
-	/* will be dropped by ->io_free_work() after returning to io-wq */
-	req_ref_get(req);
+	/* one will be dropped by ->io_free_work() after returning to io-wq */
+	if (!(req->flags & REQ_F_REFCOUNT))
+		__io_req_refcount(req, 2);
+	else
+		req_ref_get(req);
 
 	timeout = io_prep_linked_timeout(req);
 	if (timeout)
-- 
2.32.0


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

* [PATCH 2/5] io_uring: don't inflight-track linked timeouts
  2021-08-14 16:26 [PATCH for-next 0/5] 5.15 cleanups and optimisations Pavel Begunkov
  2021-08-14 16:26 ` [PATCH 1/5] io_uring: optimise iowq refcounting Pavel Begunkov
@ 2021-08-14 16:26 ` Pavel Begunkov
  2021-08-14 16:26 ` [PATCH 3/5] io_uring: optimise initial ltimeout refcounting Pavel Begunkov
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 13+ messages in thread
From: Pavel Begunkov @ 2021-08-14 16:26 UTC (permalink / raw)
  To: Jens Axboe, io-uring

Tracking linked timeouts as infligh was needed to make sure that io-wq
is not destroyed by io_uring_cancel_generic() racing with
io_async_cancel_one() accessing it. Now, cancellations issued by linked
timeouts are done in the task context, so it's already synchronised.

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

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 0d9a443d4987..d572a831cf85 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -5699,8 +5699,6 @@ static int io_timeout_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe,
 
 	data->mode = io_translate_timeout_mode(flags);
 	hrtimer_init(&data->timer, CLOCK_MONOTONIC, data->mode);
-	if (is_timeout_link)
-		io_req_track_inflight(req);
 	return 0;
 }
 
-- 
2.32.0


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

* [PATCH 3/5] io_uring: optimise initial ltimeout refcounting
  2021-08-14 16:26 [PATCH for-next 0/5] 5.15 cleanups and optimisations Pavel Begunkov
  2021-08-14 16:26 ` [PATCH 1/5] io_uring: optimise iowq refcounting Pavel Begunkov
  2021-08-14 16:26 ` [PATCH 2/5] io_uring: don't inflight-track linked timeouts Pavel Begunkov
@ 2021-08-14 16:26 ` Pavel Begunkov
  2021-08-14 16:26 ` [PATCH 4/5] io_uring: kill not necessary resubmit switch Pavel Begunkov
  2021-08-14 16:26 ` [PATCH 5/5] io_uring: deduplicate cancellations Pavel Begunkov
  4 siblings, 0 replies; 13+ messages in thread
From: Pavel Begunkov @ 2021-08-14 16:26 UTC (permalink / raw)
  To: Jens Axboe, io-uring

Linked timeouts are never refcounted when it comes to the first call to
__io_prep_linked_timeout(), so save an io_ref_get() and set the desired
value directly.

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

diff --git a/fs/io_uring.c b/fs/io_uring.c
index d572a831cf85..37b5516b63c8 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1310,8 +1310,7 @@ static struct io_kiocb *__io_prep_linked_timeout(struct io_kiocb *req)
 
 	/* linked timeouts should have two refs once prep'ed */
 	io_req_refcount(req);
-	io_req_refcount(nxt);
-	req_ref_get(nxt);
+	__io_req_refcount(nxt, 2);
 
 	nxt->timeout.head = req;
 	nxt->flags |= REQ_F_LTIMEOUT_ACTIVE;
-- 
2.32.0


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

* [PATCH 4/5] io_uring: kill not necessary resubmit switch
  2021-08-14 16:26 [PATCH for-next 0/5] 5.15 cleanups and optimisations Pavel Begunkov
                   ` (2 preceding siblings ...)
  2021-08-14 16:26 ` [PATCH 3/5] io_uring: optimise initial ltimeout refcounting Pavel Begunkov
@ 2021-08-14 16:26 ` Pavel Begunkov
  2021-08-14 16:26 ` [PATCH 5/5] io_uring: deduplicate cancellations Pavel Begunkov
  4 siblings, 0 replies; 13+ messages in thread
From: Pavel Begunkov @ 2021-08-14 16:26 UTC (permalink / raw)
  To: Jens Axboe, io-uring

773af69121ecc ("io_uring: always reissue from task_work context") makes
all resubmission to be made from task_work, so we don't need that hack
with resubmit/not-resubmit switch anymore.

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

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 37b5516b63c8..c16d172ca37f 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -2291,7 +2291,7 @@ static inline bool io_run_task_work(void)
  * Find and free completed poll iocbs
  */
 static void io_iopoll_complete(struct io_ring_ctx *ctx, unsigned int *nr_events,
-			       struct list_head *done, bool resubmit)
+			       struct list_head *done)
 {
 	struct req_batch rb;
 	struct io_kiocb *req;
@@ -2306,7 +2306,7 @@ static void io_iopoll_complete(struct io_ring_ctx *ctx, unsigned int *nr_events,
 		req = list_first_entry(done, struct io_kiocb, inflight_entry);
 		list_del(&req->inflight_entry);
 
-		if (READ_ONCE(req->result) == -EAGAIN && resubmit &&
+		if (READ_ONCE(req->result) == -EAGAIN &&
 		    !(req->flags & REQ_F_DONT_REISSUE)) {
 			req->iopoll_completed = 0;
 			io_req_task_queue_reissue(req);
@@ -2329,7 +2329,7 @@ static void io_iopoll_complete(struct io_ring_ctx *ctx, unsigned int *nr_events,
 }
 
 static int io_do_iopoll(struct io_ring_ctx *ctx, unsigned int *nr_events,
-			long min, bool resubmit)
+			long min)
 {
 	struct io_kiocb *req, *tmp;
 	LIST_HEAD(done);
@@ -2369,7 +2369,7 @@ static int io_do_iopoll(struct io_ring_ctx *ctx, unsigned int *nr_events,
 	}
 
 	if (!list_empty(&done))
-		io_iopoll_complete(ctx, nr_events, &done, resubmit);
+		io_iopoll_complete(ctx, nr_events, &done);
 
 	return 0;
 }
@@ -2387,7 +2387,7 @@ static void io_iopoll_try_reap_events(struct io_ring_ctx *ctx)
 	while (!list_empty(&ctx->iopoll_list)) {
 		unsigned int nr_events = 0;
 
-		io_do_iopoll(ctx, &nr_events, 0, false);
+		io_do_iopoll(ctx, &nr_events, 0);
 
 		/* let it sleep and repeat later if can't complete a request */
 		if (nr_events == 0)
@@ -2449,7 +2449,7 @@ static int io_iopoll_check(struct io_ring_ctx *ctx, long min)
 			    list_empty(&ctx->iopoll_list))
 				break;
 		}
-		ret = io_do_iopoll(ctx, &nr_events, min, true);
+		ret = io_do_iopoll(ctx, &nr_events, min);
 	} while (!ret && nr_events < min && !need_resched());
 out:
 	mutex_unlock(&ctx->uring_lock);
@@ -6855,7 +6855,7 @@ static int __io_sq_thread(struct io_ring_ctx *ctx, bool cap_entries)
 
 		mutex_lock(&ctx->uring_lock);
 		if (!list_empty(&ctx->iopoll_list))
-			io_do_iopoll(ctx, &nr_events, 0, true);
+			io_do_iopoll(ctx, &nr_events, 0);
 
 		/*
 		 * Don't submit if refs are dying, good for io_uring_register(),
-- 
2.32.0


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

* [PATCH 5/5] io_uring: deduplicate cancellations
  2021-08-14 16:26 [PATCH for-next 0/5] 5.15 cleanups and optimisations Pavel Begunkov
                   ` (3 preceding siblings ...)
  2021-08-14 16:26 ` [PATCH 4/5] io_uring: kill not necessary resubmit switch Pavel Begunkov
@ 2021-08-14 16:26 ` Pavel Begunkov
  4 siblings, 0 replies; 13+ messages in thread
From: Pavel Begunkov @ 2021-08-14 16:26 UTC (permalink / raw)
  To: Jens Axboe, io-uring

IORING_OP_ASYNC_CANCEL and IORING_OP_LINK_TIMEOUT have enough of
overlap, so extract a helper for request cancellation and use in both.
Also, removes some amount of ugliness because of success_ret.

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

diff --git a/fs/io_uring.c b/fs/io_uring.c
index c16d172ca37f..5560620968c9 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -5790,32 +5790,24 @@ static int io_async_cancel_one(struct io_uring_task *tctx, u64 user_data,
 	return ret;
 }
 
-static void io_async_find_and_cancel(struct io_ring_ctx *ctx,
-				     struct io_kiocb *req, __u64 sqe_addr,
-				     int success_ret)
+static int io_try_cancel_userdata(struct io_kiocb *req, u64 sqe_addr)
+	__acquires(&req->ctx->completion_lock)
 {
+	struct io_ring_ctx *ctx = req->ctx;
 	int ret;
 
+	WARN_ON_ONCE(req->task != current);
+
 	ret = io_async_cancel_one(req->task->io_uring, sqe_addr, ctx);
 	spin_lock(&ctx->completion_lock);
 	if (ret != -ENOENT)
-		goto done;
+		return ret;
 	spin_lock_irq(&ctx->timeout_lock);
 	ret = io_timeout_cancel(ctx, sqe_addr);
 	spin_unlock_irq(&ctx->timeout_lock);
 	if (ret != -ENOENT)
-		goto done;
-	ret = io_poll_cancel(ctx, sqe_addr, false);
-done:
-	if (!ret)
-		ret = success_ret;
-	io_cqring_fill_event(ctx, req->user_data, ret, 0);
-	io_commit_cqring(ctx);
-	spin_unlock(&ctx->completion_lock);
-	io_cqring_ev_posted(ctx);
-
-	if (ret < 0)
-		req_set_fail(req);
+		return ret;
+	return io_poll_cancel(ctx, sqe_addr, false);
 }
 
 static int io_async_cancel_prep(struct io_kiocb *req,
@@ -5839,17 +5831,7 @@ static int io_async_cancel(struct io_kiocb *req, unsigned int issue_flags)
 	struct io_tctx_node *node;
 	int ret;
 
-	/* tasks should wait for their io-wq threads, so safe w/o sync */
-	ret = io_async_cancel_one(req->task->io_uring, sqe_addr, ctx);
-	spin_lock(&ctx->completion_lock);
-	if (ret != -ENOENT)
-		goto done;
-	spin_lock_irq(&ctx->timeout_lock);
-	ret = io_timeout_cancel(ctx, sqe_addr);
-	spin_unlock_irq(&ctx->timeout_lock);
-	if (ret != -ENOENT)
-		goto done;
-	ret = io_poll_cancel(ctx, sqe_addr, false);
+	ret = io_try_cancel_userdata(req, sqe_addr);
 	if (ret != -ENOENT)
 		goto done;
 	spin_unlock(&ctx->completion_lock);
@@ -6416,9 +6398,17 @@ static void io_req_task_link_timeout(struct io_kiocb *req)
 {
 	struct io_kiocb *prev = req->timeout.prev;
 	struct io_ring_ctx *ctx = req->ctx;
+	int ret;
 
 	if (prev) {
-		io_async_find_and_cancel(ctx, req, prev->user_data, -ETIME);
+		ret = io_try_cancel_userdata(req, prev->user_data);
+		if (!ret)
+			ret = -ETIME;
+		io_cqring_fill_event(ctx, req->user_data, ret, 0);
+		io_commit_cqring(ctx);
+		spin_unlock(&ctx->completion_lock);
+		io_cqring_ev_posted(ctx);
+
 		io_put_req(prev);
 		io_put_req(req);
 	} else {
-- 
2.32.0


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

* Re: [PATCH 1/5] io_uring: optimise iowq refcounting
  2021-08-14 16:26 ` [PATCH 1/5] io_uring: optimise iowq refcounting Pavel Begunkov
@ 2021-08-14 19:13   ` Jens Axboe
  2021-08-14 19:31     ` Pavel Begunkov
  0 siblings, 1 reply; 13+ messages in thread
From: Jens Axboe @ 2021-08-14 19:13 UTC (permalink / raw)
  To: Pavel Begunkov, io-uring

On 8/14/21 10:26 AM, Pavel Begunkov wrote:
> If a requests is forwarded into io-wq, there is a good chance it hasn't
> been refcounted yet and we can save one req_ref_get() by setting the
> refcount number to the right value directly.

Not sure this really matters, but can't hurt either. But...

> @@ -1115,14 +1115,19 @@ static inline void req_ref_get(struct io_kiocb *req)
>  	atomic_inc(&req->refs);
>  }
>  
> -static inline void io_req_refcount(struct io_kiocb *req)
> +static inline void __io_req_refcount(struct io_kiocb *req, int nr)
>  {
>  	if (!(req->flags & REQ_F_REFCOUNT)) {
>  		req->flags |= REQ_F_REFCOUNT;
> -		atomic_set(&req->refs, 1);
> +		atomic_set(&req->refs, nr);
>  	}
>  }
>  
> +static inline void io_req_refcount(struct io_kiocb *req)
> +{
> +	__io_req_refcount(req, 1);
> +}
> +

I really think these should be io_req_set_refcount() or something like
that, making it clear that we're actively setting/manipulating the ref
count.

-- 
Jens Axboe


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

* Re: [PATCH 1/5] io_uring: optimise iowq refcounting
  2021-08-14 19:13   ` Jens Axboe
@ 2021-08-14 19:31     ` Pavel Begunkov
  2021-08-14 19:36       ` Pavel Begunkov
  2021-08-14 19:37       ` Jens Axboe
  0 siblings, 2 replies; 13+ messages in thread
From: Pavel Begunkov @ 2021-08-14 19:31 UTC (permalink / raw)
  To: Jens Axboe, io-uring

On 8/14/21 8:13 PM, Jens Axboe wrote:
> On 8/14/21 10:26 AM, Pavel Begunkov wrote:
>> If a requests is forwarded into io-wq, there is a good chance it hasn't
>> been refcounted yet and we can save one req_ref_get() by setting the
>> refcount number to the right value directly.
> 
> Not sure this really matters, but can't hurt either. But...

The refcount patches made this one atomic worse, and I just prefer
to not regress, even if slightly

>> @@ -1115,14 +1115,19 @@ static inline void req_ref_get(struct io_kiocb *req)
>>  	atomic_inc(&req->refs);
>>  }
>>  
>> -static inline void io_req_refcount(struct io_kiocb *req)
>> +static inline void __io_req_refcount(struct io_kiocb *req, int nr)
>>  {
>>  	if (!(req->flags & REQ_F_REFCOUNT)) {
>>  		req->flags |= REQ_F_REFCOUNT;
>> -		atomic_set(&req->refs, 1);
>> +		atomic_set(&req->refs, nr);
>>  	}
>>  }
>>  
>> +static inline void io_req_refcount(struct io_kiocb *req)
>> +{
>> +	__io_req_refcount(req, 1);
>> +}
>> +
> 
> I really think these should be io_req_set_refcount() or something like
> that, making it clear that we're actively setting/manipulating the ref
> count.

Agree. A separate patch, maybe?

-- 
Pavel Begunkov

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

* Re: [PATCH 1/5] io_uring: optimise iowq refcounting
  2021-08-14 19:31     ` Pavel Begunkov
@ 2021-08-14 19:36       ` Pavel Begunkov
  2021-08-14 19:38         ` Jens Axboe
  2021-08-14 19:37       ` Jens Axboe
  1 sibling, 1 reply; 13+ messages in thread
From: Pavel Begunkov @ 2021-08-14 19:36 UTC (permalink / raw)
  To: Jens Axboe, io-uring

On 8/14/21 8:31 PM, Pavel Begunkov wrote:
> On 8/14/21 8:13 PM, Jens Axboe wrote:
>> On 8/14/21 10:26 AM, Pavel Begunkov wrote:
>>> If a requests is forwarded into io-wq, there is a good chance it hasn't
>>> been refcounted yet and we can save one req_ref_get() by setting the
>>> refcount number to the right value directly.
>>
>> Not sure this really matters, but can't hurt either. But...
> 
> The refcount patches made this one atomic worse, and I just prefer
> to not regress, even if slightly
> 
>>> @@ -1115,14 +1115,19 @@ static inline void req_ref_get(struct io_kiocb *req)
>>>  	atomic_inc(&req->refs);
>>>  }
>>>  
>>> -static inline void io_req_refcount(struct io_kiocb *req)
>>> +static inline void __io_req_refcount(struct io_kiocb *req, int nr)
>>>  {
>>>  	if (!(req->flags & REQ_F_REFCOUNT)) {
>>>  		req->flags |= REQ_F_REFCOUNT;
>>> -		atomic_set(&req->refs, 1);
>>> +		atomic_set(&req->refs, nr);
>>>  	}
>>>  }
>>>  
>>> +static inline void io_req_refcount(struct io_kiocb *req)
>>> +{
>>> +	__io_req_refcount(req, 1);
>>> +}
>>> +
>>
>> I really think these should be io_req_set_refcount() or something like
>> that, making it clear that we're actively setting/manipulating the ref
>> count.
> 
> Agree. A separate patch, maybe?

I mean it just would be a bit easier for me, instead of rebasing
this series and not yet sent patches.

-- 
Pavel Begunkov

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

* Re: [PATCH 1/5] io_uring: optimise iowq refcounting
  2021-08-14 19:31     ` Pavel Begunkov
  2021-08-14 19:36       ` Pavel Begunkov
@ 2021-08-14 19:37       ` Jens Axboe
  1 sibling, 0 replies; 13+ messages in thread
From: Jens Axboe @ 2021-08-14 19:37 UTC (permalink / raw)
  To: Pavel Begunkov, io-uring

On 8/14/21 1:31 PM, Pavel Begunkov wrote:
> On 8/14/21 8:13 PM, Jens Axboe wrote:
>> On 8/14/21 10:26 AM, Pavel Begunkov wrote:
>>> If a requests is forwarded into io-wq, there is a good chance it hasn't
>>> been refcounted yet and we can save one req_ref_get() by setting the
>>> refcount number to the right value directly.
>>
>> Not sure this really matters, but can't hurt either. But...
> 
> The refcount patches made this one atomic worse, and I just prefer
> to not regress, even if slightly

Not really against it, but doubt it's measurable if you end up hitting
the io-wq slower path anyway. But as I said, can't really hurt, so not
aginst it.

>>> @@ -1115,14 +1115,19 @@ static inline void req_ref_get(struct io_kiocb *req)
>>>  	atomic_inc(&req->refs);
>>>  }
>>>  
>>> -static inline void io_req_refcount(struct io_kiocb *req)
>>> +static inline void __io_req_refcount(struct io_kiocb *req, int nr)
>>>  {
>>>  	if (!(req->flags & REQ_F_REFCOUNT)) {
>>>  		req->flags |= REQ_F_REFCOUNT;
>>> -		atomic_set(&req->refs, 1);
>>> +		atomic_set(&req->refs, nr);
>>>  	}
>>>  }
>>>  
>>> +static inline void io_req_refcount(struct io_kiocb *req)
>>> +{
>>> +	__io_req_refcount(req, 1);
>>> +}
>>> +
>>
>> I really think these should be io_req_set_refcount() or something like
>> that, making it clear that we're actively setting/manipulating the ref
>> count.
> 
> Agree. A separate patch, maybe?

Maybe just fold it into this one, as it's splitting out a helper anyway.
Or do it as a prep patch before this one, up to you.

-- 
Jens Axboe


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

* Re: [PATCH 1/5] io_uring: optimise iowq refcounting
  2021-08-14 19:36       ` Pavel Begunkov
@ 2021-08-14 19:38         ` Jens Axboe
  2021-08-15  9:41           ` Pavel Begunkov
  0 siblings, 1 reply; 13+ messages in thread
From: Jens Axboe @ 2021-08-14 19:38 UTC (permalink / raw)
  To: Pavel Begunkov, io-uring

On 8/14/21 1:36 PM, Pavel Begunkov wrote:
> On 8/14/21 8:31 PM, Pavel Begunkov wrote:
>> On 8/14/21 8:13 PM, Jens Axboe wrote:
>>> On 8/14/21 10:26 AM, Pavel Begunkov wrote:
>>>> If a requests is forwarded into io-wq, there is a good chance it hasn't
>>>> been refcounted yet and we can save one req_ref_get() by setting the
>>>> refcount number to the right value directly.
>>>
>>> Not sure this really matters, but can't hurt either. But...
>>
>> The refcount patches made this one atomic worse, and I just prefer
>> to not regress, even if slightly
>>
>>>> @@ -1115,14 +1115,19 @@ static inline void req_ref_get(struct io_kiocb *req)
>>>>  	atomic_inc(&req->refs);
>>>>  }
>>>>  
>>>> -static inline void io_req_refcount(struct io_kiocb *req)
>>>> +static inline void __io_req_refcount(struct io_kiocb *req, int nr)
>>>>  {
>>>>  	if (!(req->flags & REQ_F_REFCOUNT)) {
>>>>  		req->flags |= REQ_F_REFCOUNT;
>>>> -		atomic_set(&req->refs, 1);
>>>> +		atomic_set(&req->refs, nr);
>>>>  	}
>>>>  }
>>>>  
>>>> +static inline void io_req_refcount(struct io_kiocb *req)
>>>> +{
>>>> +	__io_req_refcount(req, 1);
>>>> +}
>>>> +
>>>
>>> I really think these should be io_req_set_refcount() or something like
>>> that, making it clear that we're actively setting/manipulating the ref
>>> count.
>>
>> Agree. A separate patch, maybe?
> 
> I mean it just would be a bit easier for me, instead of rebasing
> this series and not yet sent patches.

I think it should come before this series at least, or be folded into the
first patch. So probably no way around the rebase, sorry...
-- 
Jens Axboe


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

* Re: [PATCH 1/5] io_uring: optimise iowq refcounting
  2021-08-14 19:38         ` Jens Axboe
@ 2021-08-15  9:41           ` Pavel Begunkov
  2021-08-15 15:01             ` Jens Axboe
  0 siblings, 1 reply; 13+ messages in thread
From: Pavel Begunkov @ 2021-08-15  9:41 UTC (permalink / raw)
  To: Jens Axboe, io-uring

On 8/14/21 8:38 PM, Jens Axboe wrote:
> On 8/14/21 1:36 PM, Pavel Begunkov wrote:
>> On 8/14/21 8:31 PM, Pavel Begunkov wrote:
>>> On 8/14/21 8:13 PM, Jens Axboe wrote:
>>>> On 8/14/21 10:26 AM, Pavel Begunkov wrote:
>>>>> If a requests is forwarded into io-wq, there is a good chance it hasn't
>>>>> been refcounted yet and we can save one req_ref_get() by setting the
>>>>> refcount number to the right value directly.
>>>>
>>>> Not sure this really matters, but can't hurt either. But...
>>>
>>> The refcount patches made this one atomic worse, and I just prefer
>>> to not regress, even if slightly
>>>
>>>>> @@ -1115,14 +1115,19 @@ static inline void req_ref_get(struct io_kiocb *req)
>>>>>  	atomic_inc(&req->refs);
>>>>>  }
>>>>>  
>>>>> -static inline void io_req_refcount(struct io_kiocb *req)
>>>>> +static inline void __io_req_refcount(struct io_kiocb *req, int nr)
>>>>>  {
>>>>>  	if (!(req->flags & REQ_F_REFCOUNT)) {
>>>>>  		req->flags |= REQ_F_REFCOUNT;
>>>>> -		atomic_set(&req->refs, 1);
>>>>> +		atomic_set(&req->refs, nr);
>>>>>  	}
>>>>>  }
>>>>>  
>>>>> +static inline void io_req_refcount(struct io_kiocb *req)
>>>>> +{
>>>>> +	__io_req_refcount(req, 1);
>>>>> +}
>>>>> +
>>>>
>>>> I really think these should be io_req_set_refcount() or something like
>>>> that, making it clear that we're actively setting/manipulating the ref
>>>> count.
>>>
>>> Agree. A separate patch, maybe?
>>
>> I mean it just would be a bit easier for me, instead of rebasing
>> this series and not yet sent patches.
> 
> I think it should come before this series at least, or be folded into the
> first patch. So probably no way around the rebase, sorry...

Don't see the point, but anyway, just resent it

-- 
Pavel Begunkov

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

* Re: [PATCH 1/5] io_uring: optimise iowq refcounting
  2021-08-15  9:41           ` Pavel Begunkov
@ 2021-08-15 15:01             ` Jens Axboe
  0 siblings, 0 replies; 13+ messages in thread
From: Jens Axboe @ 2021-08-15 15:01 UTC (permalink / raw)
  To: Pavel Begunkov, io-uring

On 8/15/21 3:41 AM, Pavel Begunkov wrote:
> On 8/14/21 8:38 PM, Jens Axboe wrote:
>> On 8/14/21 1:36 PM, Pavel Begunkov wrote:
>>> On 8/14/21 8:31 PM, Pavel Begunkov wrote:
>>>> On 8/14/21 8:13 PM, Jens Axboe wrote:
>>>>> On 8/14/21 10:26 AM, Pavel Begunkov wrote:
>>>>>> If a requests is forwarded into io-wq, there is a good chance it hasn't
>>>>>> been refcounted yet and we can save one req_ref_get() by setting the
>>>>>> refcount number to the right value directly.
>>>>>
>>>>> Not sure this really matters, but can't hurt either. But...
>>>>
>>>> The refcount patches made this one atomic worse, and I just prefer
>>>> to not regress, even if slightly
>>>>
>>>>>> @@ -1115,14 +1115,19 @@ static inline void req_ref_get(struct io_kiocb *req)
>>>>>>  	atomic_inc(&req->refs);
>>>>>>  }
>>>>>>  
>>>>>> -static inline void io_req_refcount(struct io_kiocb *req)
>>>>>> +static inline void __io_req_refcount(struct io_kiocb *req, int nr)
>>>>>>  {
>>>>>>  	if (!(req->flags & REQ_F_REFCOUNT)) {
>>>>>>  		req->flags |= REQ_F_REFCOUNT;
>>>>>> -		atomic_set(&req->refs, 1);
>>>>>> +		atomic_set(&req->refs, nr);
>>>>>>  	}
>>>>>>  }
>>>>>>  
>>>>>> +static inline void io_req_refcount(struct io_kiocb *req)
>>>>>> +{
>>>>>> +	__io_req_refcount(req, 1);
>>>>>> +}
>>>>>> +
>>>>>
>>>>> I really think these should be io_req_set_refcount() or something like
>>>>> that, making it clear that we're actively setting/manipulating the ref
>>>>> count.
>>>>
>>>> Agree. A separate patch, maybe?
>>>
>>> I mean it just would be a bit easier for me, instead of rebasing
>>> this series and not yet sent patches.
>>
>> I think it should come before this series at least, or be folded into the
>> first patch. So probably no way around the rebase, sorry...
> 
> Don't see the point, but anyway, just resent it

That's the usual approach, first a prep patch to clean it up, then change
on top. The opposite might be easier since the other patches already exist,
but it's backwards in terms of ordering imho.

-- 
Jens Axboe


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

end of thread, other threads:[~2021-08-15 15:01 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-14 16:26 [PATCH for-next 0/5] 5.15 cleanups and optimisations Pavel Begunkov
2021-08-14 16:26 ` [PATCH 1/5] io_uring: optimise iowq refcounting Pavel Begunkov
2021-08-14 19:13   ` Jens Axboe
2021-08-14 19:31     ` Pavel Begunkov
2021-08-14 19:36       ` Pavel Begunkov
2021-08-14 19:38         ` Jens Axboe
2021-08-15  9:41           ` Pavel Begunkov
2021-08-15 15:01             ` Jens Axboe
2021-08-14 19:37       ` Jens Axboe
2021-08-14 16:26 ` [PATCH 2/5] io_uring: don't inflight-track linked timeouts Pavel Begunkov
2021-08-14 16:26 ` [PATCH 3/5] io_uring: optimise initial ltimeout refcounting Pavel Begunkov
2021-08-14 16:26 ` [PATCH 4/5] io_uring: kill not necessary resubmit switch Pavel Begunkov
2021-08-14 16:26 ` [PATCH 5/5] io_uring: deduplicate cancellations Pavel Begunkov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.