Re: [PATCH 3/3] io_uring/msg_ring: improve handling of target CQE posting

[Pavel Begunkov <asml.silence@gmail.com>]

On 3/28/24 18:52, Jens Axboe wrote:
> Use the exported helper for queueing task_work, rather than rolling our
> own.
> 
> This improves peak performance of message passing by about 5x in some
> basic testing, with 2 threads just sending messages to each other.
> Before this change, it was capped at around 700K/sec, with the change
> it's at over 4M/sec.
> 
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
>   io_uring/msg_ring.c | 27 ++++++++++-----------------
>   1 file changed, 10 insertions(+), 17 deletions(-)
> 
> diff --git a/io_uring/msg_ring.c b/io_uring/msg_ring.c
> index d1f66a40b4b4..e12a9e8a910a 100644
> --- a/io_uring/msg_ring.c
> +++ b/io_uring/msg_ring.c
> @@ -11,9 +11,9 @@
>   #include "io_uring.h"
>   #include "rsrc.h"
>   #include "filetable.h"
> +#include "refs.h"
>   #include "msg_ring.h"
>   
> -
>   /* All valid masks for MSG_RING */
>   #define IORING_MSG_RING_MASK		(IORING_MSG_RING_CQE_SKIP | \
>   					IORING_MSG_RING_FLAGS_PASS)
> @@ -21,7 +21,6 @@
>   struct io_msg {
>   	struct file			*file;
>   	struct file			*src_file;
> -	struct callback_head		tw;
>   	u64 user_data;
>   	u32 len;
>   	u32 cmd;
> @@ -73,26 +72,20 @@ static inline bool io_msg_need_remote(struct io_ring_ctx *target_ctx)
>   	return current != target_ctx->submitter_task;
>   }
>   
> -static int io_msg_exec_remote(struct io_kiocb *req, task_work_func_t func)
> +static int io_msg_exec_remote(struct io_kiocb *req, io_req_tw_func_t func)
>   {
>   	struct io_ring_ctx *ctx = req->file->private_data;
> -	struct io_msg *msg = io_kiocb_to_cmd(req, struct io_msg);
>   	struct task_struct *task = READ_ONCE(ctx->submitter_task);
>   
> -	if (unlikely(!task))
> -		return -EOWNERDEAD;
> -
> -	init_task_work(&msg->tw, func);
> -	if (task_work_add(ctx->submitter_task, &msg->tw, TWA_SIGNAL))
> -		return -EOWNERDEAD;
> -
> +	__io_req_set_refcount(req, 2);

I'd argue it's better avoid any more req refcount users, I'd be more
happy it it dies out completely at some point.

Why it's even needed here? You pass it via tw to post a CQE/etc and
then pass it back via another tw hop to complete IIRC, the ownership
is clear. At least it worth a comment.


> +	req->io_task_work.func = func;
> +	io_req_task_work_add_remote(req, task, ctx, IOU_F_TWQ_LAZY_WAKE);
>   	return IOU_ISSUE_SKIP_COMPLETE;
>   }
>   
> -static void io_msg_tw_complete(struct callback_head *head)
> +static void io_msg_tw_complete(struct io_kiocb *req, struct io_tw_state *ts)
>   {
> -	struct io_msg *msg = container_of(head, struct io_msg, tw);
> -	struct io_kiocb *req = cmd_to_io_kiocb(msg);
> +	struct io_msg *msg = io_kiocb_to_cmd(req, struct io_msg);
>   	struct io_ring_ctx *target_ctx = req->file->private_data;
>   	int ret = 0;
>   
> @@ -120,6 +113,7 @@ static void io_msg_tw_complete(struct callback_head *head)
>   
>   	if (ret < 0)
>   		req_set_fail(req);
> +	req_ref_put_and_test(req);
>   	io_req_queue_tw_complete(req, ret);
>   }
>   
> @@ -205,16 +199,15 @@ static int io_msg_install_complete(struct io_kiocb *req, unsigned int issue_flag
>   	return ret;
>   }
>   
> -static void io_msg_tw_fd_complete(struct callback_head *head)
> +static void io_msg_tw_fd_complete(struct io_kiocb *req, struct io_tw_state *ts)
>   {
> -	struct io_msg *msg = container_of(head, struct io_msg, tw);
> -	struct io_kiocb *req = cmd_to_io_kiocb(msg);
>   	int ret = -EOWNERDEAD;
>   
>   	if (!(current->flags & PF_EXITING))
>   		ret = io_msg_install_complete(req, IO_URING_F_UNLOCKED);
>   	if (ret < 0)
>   		req_set_fail(req);
> +	req_ref_put_and_test(req);
>   	io_req_queue_tw_complete(req, ret);
>   }
>   

-- 
Pavel Begunkov