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

From: Kanchan Joshi <joshi.k@samsung.com>
To: Pavel Begunkov <asml.silence@gmail.com>
Cc: linux-nvme@lists.infradead.org, linux-block@vger.kernel.org,
	io-uring@vger.kernel.org, axboe@kernel.dk, kbusch@kernel.org,
	hch@lst.de, sagi@grimberg.me
Subject: Re: [PATCH for-next 1/2] io_uring/cmd: add cmd lazy tw wake helper
Date: Tue, 16 May 2023 15:30:11 +0530	[thread overview]
Message-ID: <20230516100000.GA26860@green245> (raw)
In-Reply-To: <5b9f6716006df7e817f18bd555aee2f8f9c8b0c3.1684154817.git.asml.silence@gmail.com>

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

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?
>+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);

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.
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.

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