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

[Jens Axboe <axboe@kernel.dk>]

On 3/29/24 9:46 AM, Pavel Begunkov wrote:
> 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@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.
>>
>> 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.

Yeah, I think there's a bit of room for cleanups on the refs side. But
thankfully it's not very prevalent in the code base.

-- 
Jens Axboe