Re: [PATCH for-next 1/2] io_uring/cmd: add cmd lazy tw wake helper

[Kanchan Joshi <joshi.k@samsung.com>]

[-- Attachment #1: Type: text/plain, Size: 5770 bytes --]

On Tue, May 16, 2023 at 07:52:23PM +0100, Pavel Begunkov wrote:
>On 5/16/23 11:00, Kanchan Joshi wrote:
>>On Mon, May 15, 2023 at 01:54:42PM +0100, Pavel Begunkov wrote:
>>>We want to use IOU_F_TWQ_LAZY_WAKE in commands. First, introduce a new
>>>cmd tw helper accepting TWQ flags, and then add
>>>io_uring_cmd_do_in_task_laz() that will pass IOU_F_TWQ_LAZY_WAKE and
>>>imply the "lazy" semantics, i.e. it posts no more than 1 CQE and
>>>delaying execution of this tw should not prevent forward progress.
>>>
>>>Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>>>---
>>>include/linux/io_uring.h | 18 ++++++++++++++++--
>>>io_uring/uring_cmd.c     | 16 ++++++++++++----
>>>2 files changed, 28 insertions(+), 6 deletions(-)
>>>
>>>diff --git a/include/linux/io_uring.h b/include/linux/io_uring.h
>>>index 7fe31b2cd02f..bb9c666bd584 100644
>>>--- a/include/linux/io_uring.h
>>>+++ b/include/linux/io_uring.h
>>>@@ -46,13 +46,23 @@ int io_uring_cmd_import_fixed(u64 ubuf, unsigned long len, int rw,
>>>                  struct iov_iter *iter, void *ioucmd);
>>>void io_uring_cmd_done(struct io_uring_cmd *cmd, ssize_t ret, ssize_t res2,
>>>            unsigned issue_flags);
>>>-void io_uring_cmd_complete_in_task(struct io_uring_cmd *ioucmd,
>>>-            void (*task_work_cb)(struct io_uring_cmd *, unsigned));
>>>struct sock *io_uring_get_socket(struct file *file);
>>>void __io_uring_cancel(bool cancel_all);
>>>void __io_uring_free(struct task_struct *tsk);
>>>void io_uring_unreg_ringfd(void);
>>>const char *io_uring_get_opcode(u8 opcode);
>>>+void __io_uring_cmd_do_in_task(struct io_uring_cmd *ioucmd,
>>>+                void (*task_work_cb)(struct io_uring_cmd *, unsigned),
>>>+                unsigned flags);
>>>+/* users should follow semantics of IOU_F_TWQ_LAZY_WAKE */
>>
>>Should this also translate to some warn_on anywhere?
>
>Would love to but don't see how. We can only check it doesn't
>produce more than 1 CQE, but that would need
>
>nr_cqes_before = cqes_ready();
>tw_item->run();
>WARN_ON(cqes_ready() >= nr_cqes_before + 1);
>
>but that's just too ugly
>
>
>>>+void io_uring_cmd_do_in_task_lazy(struct io_uring_cmd *ioucmd,
>>>+            void (*task_work_cb)(struct io_uring_cmd *, unsigned));
>>>+
>>>+static inline void io_uring_cmd_complete_in_task(struct io_uring_cmd *ioucmd,
>>>+            void (*task_work_cb)(struct io_uring_cmd *, unsigned))
>>>+{
>>>+    __io_uring_cmd_do_in_task(ioucmd, task_work_cb, 0);
>>>+}
>>>
>>>static inline void io_uring_files_cancel(void)
>>>{
>>>@@ -85,6 +95,10 @@ static inline void io_uring_cmd_complete_in_task(struct io_uring_cmd *ioucmd,
>>>            void (*task_work_cb)(struct io_uring_cmd *, unsigned))
>>>{
>>>}
>>>+static inline void io_uring_cmd_do_in_task_lazy(struct io_uring_cmd *ioucmd,
>>>+            void (*task_work_cb)(struct io_uring_cmd *, unsigned))
>>>+{
>>>+}
>>>static inline struct sock *io_uring_get_socket(struct file *file)
>>>{
>>>    return NULL;
>>>diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
>>>index 5e32db48696d..476c7877ce58 100644
>>>--- a/io_uring/uring_cmd.c
>>>+++ b/io_uring/uring_cmd.c
>>>@@ -20,16 +20,24 @@ static void io_uring_cmd_work(struct io_kiocb *req, struct io_tw_state *ts)
>>>    ioucmd->task_work_cb(ioucmd, issue_flags);
>>>}
>>>
>>>-void io_uring_cmd_complete_in_task(struct io_uring_cmd *ioucmd,
>>>-            void (*task_work_cb)(struct io_uring_cmd *, unsigned))
>>>+void __io_uring_cmd_do_in_task(struct io_uring_cmd *ioucmd,
>>>+            void (*task_work_cb)(struct io_uring_cmd *, unsigned),
>>>+            unsigned flags)
>>>{
>>>    struct io_kiocb *req = cmd_to_io_kiocb(ioucmd);
>>>
>>>    ioucmd->task_work_cb = task_work_cb;
>>>    req->io_task_work.func = io_uring_cmd_work;
>>>-    io_req_task_work_add(req);
>>>+    __io_req_task_work_add(req, flags);
>>>+}
>>>+EXPORT_SYMBOL_GPL(__io_uring_cmd_do_in_task);
>
>--- a/include/linux/io_uring.h
>+++ b/include/linux/io_uring.h
>
>+static inline void io_uring_cmd_complete_in_task(struct io_uring_cmd *ioucmd,
>+			void (*task_work_cb)(struct io_uring_cmd *, unsigned))
>+{
>+	__io_uring_cmd_do_in_task(ioucmd, task_work_cb, 0);
>+}
>
>That should fail for nvme unless exported.

But it does not. Give it a try.

>>Any reason to export this? No one is using this at the moment.
>>>+void io_uring_cmd_do_in_task_lazy(struct io_uring_cmd *ioucmd,
>>>+            void (*task_work_cb)(struct io_uring_cmd *, unsigned))
>>>+{
>>>+    __io_uring_cmd_do_in_task(ioucmd, task_work_cb, IOU_F_TWQ_LAZY_WAKE);
>>>}
>>>-EXPORT_SYMBOL_GPL(io_uring_cmd_complete_in_task);
>>>+EXPORT_SYMBOL_GPL(io_uring_cmd_do_in_task_lazy);
>>
>>Seems you did not want callers to pass the the new flag (LAZY_WAKE) and
>>therefore this helper.
>
>Yep, I wouldn't mind exposing just *LAZY_WAKE but don't want
>to let it use whatever flags there might be in the future.
>
>Initially I wanted to just make io_uring_cmd_complete_in_task and
>io_uring_cmd_do_in_task_lazy static inline, but that would need
>some code shuffling to make it clean.
>
>>And if you did not want callers to know about this flag (internal
>>details of io_uring), it would be better to have two exported helpers
>>io_uring_cmd_do_in_task_lazy() and io_uring_cmd_complete_in_task().
>>Both will use the internal helper __io_uring_cmd_do_in_task with
>>different flag.
>
>That's how it should be in this patch

Nah, in this patch __io_uring_cmd_do_in_task is exported helper. And
io_uring_cmd_complete_in_task has been changed too (explicit export to
header based one). Seems like bit more shuffling than what is necessary.

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]