On 3/29/24 13:32, Jens Axboe wrote:
On 3/29/24 6:54 AM, Pavel Begunkov wrote:
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@xxxxxxxxx>
---
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.
It's not, it was more documentation than anything else. But I agree that
we should just avoid it, I'll kill it.
Great, it was confusing and I don't think it's even correct. In case
it comes with refcounting enabled you'd get only 1 ref instead of
desired 2. See how io_wq_submit_work() does it. Probably it's better
to kill the "__" set refs helper.
--
Pavel Begunkov
