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