| Message ID | 6328acdbb5e60efc762b18003382de077e6e1367.1673887636.git.asml.silence@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | random for-next patches | expand |
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?
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.
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.
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.
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
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.
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
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; }
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(-)