[PATCH for-next 0/5] random for-next patches

[PATCH for-next 0/5] random for-next patches

[Pavel Begunkov]

1/5 returns back an old lost optimisation
Others are small cleanups

Pavel Begunkov (5):
  io_uring: return back links tw run optimisation
  io_uring: don't export io_put_task()
  io_uring: simplify fallback execution
  io_uring: optimise ctx flags layout
  io_uring: refactor __io_req_complete_post

 include/linux/io_uring_types.h |  6 +++---
 io_uring/io_uring.c            | 34 ++++++++++++++++++++++------------
 io_uring/io_uring.h            | 10 ----------
 3 files changed, 25 insertions(+), 25 deletions(-)

-- 
2.38.1

[PATCH for-next 1/5] io_uring: return back links tw run optimisation

[Pavel Begunkov]

io_submit_flush_completions() may queue new requests for tw execution,
especially true for linked requests. Recheck the tw list for emptiness
after flushing completions.

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

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 718f56baecbd..5570422dc2fb 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -1344,8 +1344,11 @@ static int __io_run_local_work(struct io_ring_ctx *ctx, bool *locked)
 
 	if (!llist_empty(&ctx->work_llist))
 		goto again;
-	if (*locked)
+	if (*locked) {
 		io_submit_flush_completions(ctx);
+		if (!llist_empty(&ctx->work_llist))
+			goto again;
+	}
 	trace_io_uring_local_work_run(ctx, ret, loops);
 	return ret;
 }
-- 
2.38.1

Re: [PATCH for-next 1/5] io_uring: return back links tw run optimisation

[Jens Axboe]

On 1/16/23 9:48 AM, Pavel Begunkov wrote:
> io_submit_flush_completions() may queue new requests for tw execution,
> especially true for linked requests. Recheck the tw list for emptiness
> after flushing completions.

Did you check when it got lost? Would be nice to add a Fixes link?

-- 
Jens Axboe

Re: [PATCH for-next 1/5] io_uring: return back links tw run optimisation

[Pavel Begunkov]

On 1/16/23 18:43, Jens Axboe wrote:
> On 1/16/23 9:48 AM, Pavel Begunkov wrote:
>> io_submit_flush_completions() may queue new requests for tw execution,
>> especially true for linked requests. Recheck the tw list for emptiness
>> after flushing completions.
> 
> Did you check when it got lost? Would be nice to add a Fixes link?

fwiw, not fan of putting a "Fixes" tag on sth that is not a fix.

Looks like the optimisation was there for normal task_work, then
disappeared in f88262e60bb9c ("io_uring: lockless task list").
DEFERRED_TASKRUN came later and this patch handles exclusively
deferred tw. I probably need to send a patch for normal tw as well.

-- 
Pavel Begunkov

Re: [PATCH for-next 1/5] io_uring: return back links tw run optimisation

[Jens Axboe]

On 1/16/23 12:47 PM, Pavel Begunkov wrote:
> On 1/16/23 18:43, Jens Axboe wrote:
>> On 1/16/23 9:48 AM, Pavel Begunkov wrote:
>>> io_submit_flush_completions() may queue new requests for tw execution,
>>> especially true for linked requests. Recheck the tw list for emptiness
>>> after flushing completions.
>>
>> Did you check when it got lost? Would be nice to add a Fixes link?
> 
> fwiw, not fan of putting a "Fixes" tag on sth that is not a fix.

I'm not either as it isn't fully descriptive, but it is better than
not having that reference imho.

> Looks like the optimisation was there for normal task_work, then
> disappeared in f88262e60bb9c ("io_uring: lockless task list").
> DEFERRED_TASKRUN came later and this patch handles exclusively
> deferred tw. I probably need to send a patch for normal tw as well.

So maybe just use that commit? I can make a note in the message on
how it relates.

-- 
Jens Axboe

Re: [PATCH for-next 1/5] io_uring: return back links tw run optimisation

[Pavel Begunkov]

On 1/16/23 21:04, Jens Axboe wrote:
> On 1/16/23 12:47 PM, Pavel Begunkov wrote:
>> On 1/16/23 18:43, Jens Axboe wrote:
>>> On 1/16/23 9:48 AM, Pavel Begunkov wrote:
>>>> io_submit_flush_completions() may queue new requests for tw execution,
>>>> especially true for linked requests. Recheck the tw list for emptiness
>>>> after flushing completions.
>>>
>>> Did you check when it got lost? Would be nice to add a Fixes link?
>>
>> fwiw, not fan of putting a "Fixes" tag on sth that is not a fix.
> 
> I'm not either as it isn't fully descriptive, but it is better than
> not having that reference imho.

Agree, but it's also not great that it might be tried to be
backported. Maybe adding a link would be nicer?

Link: https://lore.kernel.org/r/20220622134028.2013417-4-dylany@fb.com


>> Looks like the optimisation was there for normal task_work, then
>> disappeared in f88262e60bb9c ("io_uring: lockless task list").
>> DEFERRED_TASKRUN came later and this patch handles exclusively
>> deferred tw. I probably need to send a patch for normal tw as well.
> 
> So maybe just use that commit? I can make a note in the message on
> how it relates.

-- 
Pavel Begunkov

Re: [PATCH for-next 1/5] io_uring: return back links tw run optimisation

[Jens Axboe]

On 1/16/23 2:14 PM, Pavel Begunkov wrote:
> On 1/16/23 21:04, Jens Axboe wrote:
>> On 1/16/23 12:47 PM, Pavel Begunkov wrote:
>>> On 1/16/23 18:43, Jens Axboe wrote:
>>>> On 1/16/23 9:48 AM, Pavel Begunkov wrote:
>>>>> io_submit_flush_completions() may queue new requests for tw execution,
>>>>> especially true for linked requests. Recheck the tw list for emptiness
>>>>> after flushing completions.
>>>>
>>>> Did you check when it got lost? Would be nice to add a Fixes link?
>>>
>>> fwiw, not fan of putting a "Fixes" tag on sth that is not a fix.
>>
>> I'm not either as it isn't fully descriptive, but it is better than
>> not having that reference imho.
> 
> Agree, but it's also not great that it might be tried to be
> backported. Maybe adding a link would be nicer?
> 
> Link: https://lore.kernel.org/r/20220622134028.2013417-4-dylany@fb.com

Only the auto-select bot would pick it, but I'm guessing it'll fail
and that'll be the end of that. Normal stable additions need a
cc stable as well, the fixes is not enough to trigger that.

-- 
Jens Axboe

Re: [PATCH for-next 1/5] io_uring: return back links tw run optimisation

[Pavel Begunkov]

On 1/16/23 21:04, Jens Axboe wrote:
> On 1/16/23 12:47 PM, Pavel Begunkov wrote:
>> On 1/16/23 18:43, Jens Axboe wrote:
>>> On 1/16/23 9:48 AM, Pavel Begunkov wrote:
>>>> io_submit_flush_completions() may queue new requests for tw execution,
>>>> especially true for linked requests. Recheck the tw list for emptiness
>>>> after flushing completions.
>>>
>>> Did you check when it got lost? Would be nice to add a Fixes link?
>>
>> fwiw, not fan of putting a "Fixes" tag on sth that is not a fix.
> 
> I'm not either as it isn't fully descriptive, but it is better than
> not having that reference imho.
> 
>> Looks like the optimisation was there for normal task_work, then
>> disappeared in f88262e60bb9c ("io_uring: lockless task list").
>> DEFERRED_TASKRUN came later and this patch handles exclusively
>> deferred tw. I probably need to send a patch for normal tw as well.
> 
> So maybe just use that commit? I can make a note in the message on
> how it relates.

Yes please, thanks

-- 
Pavel Begunkov

Re: [PATCH for-next 1/5] io_uring: return back links tw run optimisation

[Jens Axboe]

On 1/16/23 2:15 PM, Pavel Begunkov wrote:
> On 1/16/23 21:04, Jens Axboe wrote:
>> On 1/16/23 12:47 PM, Pavel Begunkov wrote:
>>> On 1/16/23 18:43, Jens Axboe wrote:
>>>> On 1/16/23 9:48 AM, Pavel Begunkov wrote:
>>>>> io_submit_flush_completions() may queue new requests for tw execution,
>>>>> especially true for linked requests. Recheck the tw list for emptiness
>>>>> after flushing completions.
>>>>
>>>> Did you check when it got lost? Would be nice to add a Fixes link?
>>>
>>> fwiw, not fan of putting a "Fixes" tag on sth that is not a fix.
>>
>> I'm not either as it isn't fully descriptive, but it is better than
>> not having that reference imho.
>>
>>> Looks like the optimisation was there for normal task_work, then
>>> disappeared in f88262e60bb9c ("io_uring: lockless task list").
>>> DEFERRED_TASKRUN came later and this patch handles exclusively
>>> deferred tw. I probably need to send a patch for normal tw as well.
>>
>> So maybe just use that commit? I can make a note in the message on
>> how it relates.
> 
> Yes please, thanks

Done:

https://git.kernel.dk/cgit/linux-block/commit/?h=for-6.3/io_uring&id=b48f4ef033089cf03c28bb09ae054dbfdf11635a

-- 
Jens Axboe

[PATCH for-next 2/5] io_uring: don't export io_put_task()

[Pavel Begunkov]

io_put_task() is only used in uring.c so enclose it there together with
__io_put_task().

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

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 5570422dc2fb..1b72ff558c17 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -715,7 +715,7 @@ static void io_cqring_overflow_flush(struct io_ring_ctx *ctx)
 		io_cqring_do_overflow_flush(ctx);
 }
 
-void __io_put_task(struct task_struct *task, int nr)
+static void __io_put_task(struct task_struct *task, int nr)
 {
 	struct io_uring_task *tctx = task->io_uring;
 
@@ -725,6 +725,15 @@ void __io_put_task(struct task_struct *task, int nr)
 	put_task_struct_many(task, nr);
 }
 
+/* must to be called somewhat shortly after putting a request */
+static inline void io_put_task(struct task_struct *task, int nr)
+{
+	if (likely(task == current))
+		task->io_uring->cached_refs += nr;
+	else
+		__io_put_task(task, nr);
+}
+
 void io_task_refs_refill(struct io_uring_task *tctx)
 {
 	unsigned int refill = -tctx->cached_refs + IO_TCTX_REFS_CACHE_NR;
diff --git a/io_uring/io_uring.h b/io_uring/io_uring.h
index 5113e0ddb01d..c68edf9872a5 100644
--- a/io_uring/io_uring.h
+++ b/io_uring/io_uring.h
@@ -70,7 +70,6 @@ void io_wq_submit_work(struct io_wq_work *work);
 
 void io_free_req(struct io_kiocb *req);
 void io_queue_next(struct io_kiocb *req);
-void __io_put_task(struct task_struct *task, int nr);
 void io_task_refs_refill(struct io_uring_task *tctx);
 bool __io_alloc_req_refill(struct io_ring_ctx *ctx);
 
@@ -319,15 +318,6 @@ static inline void io_commit_cqring_flush(struct io_ring_ctx *ctx)
 		__io_commit_cqring_flush(ctx);
 }
 
-/* must to be called somewhat shortly after putting a request */
-static inline void io_put_task(struct task_struct *task, int nr)
-{
-	if (likely(task == current))
-		task->io_uring->cached_refs += nr;
-	else
-		__io_put_task(task, nr);
-}
-
 static inline void io_get_task_refs(int nr)
 {
 	struct io_uring_task *tctx = current->io_uring;
-- 
2.38.1

[PATCH for-next 3/5] io_uring: simplify fallback execution

[Pavel Begunkov]

Lock the ring with uring_lock in io_fallback_req_func(), which should
make it a bit safer and easier. With that we also don't need refs
pinning as io_ring_exit_work() will wait until uring_lock is freed.

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

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 1b72ff558c17..e690c884dc95 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -245,17 +245,15 @@ static __cold void io_fallback_req_func(struct work_struct *work)
 						fallback_work.work);
 	struct llist_node *node = llist_del_all(&ctx->fallback_llist);
 	struct io_kiocb *req, *tmp;
-	bool locked = false;
+	bool locked = true;
 
-	percpu_ref_get(&ctx->refs);
+	mutex_lock(&ctx->uring_lock);
 	llist_for_each_entry_safe(req, tmp, node, io_task_work.node)
 		req->io_task_work.func(req, &locked);
-
-	if (locked) {
-		io_submit_flush_completions(ctx);
-		mutex_unlock(&ctx->uring_lock);
-	}
-	percpu_ref_put(&ctx->refs);
+	if (WARN_ON_ONCE(!locked))
+		return;
+	io_submit_flush_completions(ctx);
+	mutex_unlock(&ctx->uring_lock);
 }
 
 static int io_alloc_hash_table(struct io_hash_table *table, unsigned bits)
-- 
2.38.1

[PATCH for-next 4/5] io_uring: optimise ctx flags layout

[Pavel Begunkov]

There may be different cost for reeading just one byte or more, so it's
benificial to keep ctx flag bits that we access together in a single
byte. That affected code generation of __io_cq_unlock_post_flush() and
removed one memory load.

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

diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h
index cc0cf0705b8f..0efe4d784358 100644
--- a/include/linux/io_uring_types.h
+++ b/include/linux/io_uring_types.h
@@ -196,17 +196,17 @@ struct io_ring_ctx {
 	/* const or read-mostly hot data */
 	struct {
 		unsigned int		flags;
-		unsigned int		compat: 1;
 		unsigned int		drain_next: 1;
 		unsigned int		restricted: 1;
 		unsigned int		off_timeout_used: 1;
 		unsigned int		drain_active: 1;
-		unsigned int		drain_disabled: 1;
 		unsigned int		has_evfd: 1;
-		unsigned int		syscall_iopoll: 1;
 		/* all CQEs should be posted only by the submitter task */
 		unsigned int		task_complete: 1;
+		unsigned int		syscall_iopoll: 1;
 		unsigned int		poll_activated: 1;
+		unsigned int		drain_disabled: 1;
+		unsigned int		compat: 1;
 
 		enum task_work_notify_mode	notify_method;
 		struct io_rings			*rings;
-- 
2.38.1

[PATCH for-next 5/5] io_uring: refactor __io_req_complete_post

[Pavel Begunkov]

Keep parts of __io_req_complete_post() relying on req->flags together so
the value can be cached.

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

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index e690c884dc95..27d9abd24a83 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -974,14 +974,14 @@ static void __io_req_complete_post(struct io_kiocb *req)
 				req->link = NULL;
 			}
 		}
+		io_put_kbuf_comp(req);
+		io_dismantle_req(req);
 		io_req_put_rsrc(req);
 		/*
 		 * Selected buffer deallocation in io_clean_op() assumes that
 		 * we don't hold ->completion_lock. Clean them here to avoid
 		 * deadlocks.
 		 */
-		io_put_kbuf_comp(req);
-		io_dismantle_req(req);
 		io_put_task(req->task, 1);
 		wq_list_add_head(&req->comp_list, &ctx->locked_free_list);
 		ctx->locked_free_nr++;
-- 
2.38.1

Re: [PATCH for-next 0/5] random for-next patches

[Jens Axboe]

On Mon, 16 Jan 2023 16:48:56 +0000, Pavel Begunkov wrote:
> 1/5 returns back an old lost optimisation
> Others are small cleanups
> 
> Pavel Begunkov (5):
>   io_uring: return back links tw run optimisation
>   io_uring: don't export io_put_task()
>   io_uring: simplify fallback execution
>   io_uring: optimise ctx flags layout
>   io_uring: refactor __io_req_complete_post
> 
> [...]

Applied, thanks!

[1/5] io_uring: return back links tw run optimisation
      commit: b48f4ef033089cf03c28bb09ae054dbfdf11635a
[2/5] io_uring: don't export io_put_task()
      commit: 41cc377f69cc1702d989c33eccbacd845d463c72
[3/5] io_uring: simplify fallback execution
      commit: ae96a39a7537ab49b9fb497e7c5e860ffc6fde72
[4/5] io_uring: optimise ctx flags layout
      commit: 4a26869e3c95ee20d03b178e413a619928a84d26
[5/5] io_uring: refactor __io_req_complete_post
      commit: 2c5c148670c650381bce849e164757ab6a2729be

Best regards,
-- 
Jens Axboe