| Message ID | 20240328185413.759531-4-axboe@kernel.dk (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Cleanup and improve MSG_RING performance | expand |
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); > } >
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.
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.
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.
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); + 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); }
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(-)