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

[Pavel Begunkov]

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

> 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

-- 
Pavel Begunkov