public inbox for [email protected]
 help / color / mirror / Atom feed
From: Kanchan Joshi <[email protected]>
To: Jens Axboe <[email protected]>
Cc: io-uring <[email protected]>, Kanchan Joshi <[email protected]>
Subject: Re: [PATCH] io_uring/uring_cmd: push IRQ based completions through task_work
Date: Tue, 21 Mar 2023 10:08:45 +0530	[thread overview]
Message-ID: <CA+1E3rKsrpp80qRuRM1K=cv6vTivhNO8J4bV_hXC96QrsB=dhw@mail.gmail.com> (raw)
In-Reply-To: <CA+1E3r+ANR2dk=KqAOiQ300B+QdfEt2HHCtze1qcz5P60SuSow@mail.gmail.com>

On Tue, Mar 21, 2023 at 10:02 AM Kanchan Joshi <[email protected]> wrote:
>
> On Tue, Mar 21, 2023 at 2:12 AM Jens Axboe <[email protected]> wrote:
> >
> > On 3/20/23 2:03?PM, Jens Axboe wrote:
> > > On 3/20/23 9:06?AM, Kanchan Joshi wrote:
> > >> On Sun, Mar 19, 2023 at 8:51?PM Jens Axboe <[email protected]> wrote:
> > >>>
> > >>> This is similar to what we do on the non-passthrough read/write side,
> > >>> and helps take advantage of the completion batching we can do when we
> > >>> post CQEs via task_work. On top of that, this avoids a uring_lock
> > >>> grab/drop for every completion.
> > >>>
> > >>> In the normal peak IRQ based testing, this increases performance in
> > >>> my testing from ~75M to ~77M IOPS, or an increase of 2-3%.
> > >>>
> > >>> Signed-off-by: Jens Axboe <[email protected]>
> > >>>
> > >>> ---
> > >>>
> > >>> diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
> > >>> index 2e4c483075d3..b4fba5f0ab0d 100644
> > >>> --- a/io_uring/uring_cmd.c
> > >>> +++ b/io_uring/uring_cmd.c
> > >>> @@ -45,18 +45,21 @@ static inline void io_req_set_cqe32_extra(struct io_kiocb *req,
> > >>>  void io_uring_cmd_done(struct io_uring_cmd *ioucmd, ssize_t ret, ssize_t res2)
> > >>>  {
> > >>>         struct io_kiocb *req = cmd_to_io_kiocb(ioucmd);
> > >>> +       struct io_ring_ctx *ctx = req->ctx;
> > >>>
> > >>>         if (ret < 0)
> > >>>                 req_set_fail(req);
> > >>>
> > >>>         io_req_set_res(req, ret, 0);
> > >>> -       if (req->ctx->flags & IORING_SETUP_CQE32)
> > >>> +       if (ctx->flags & IORING_SETUP_CQE32)
> > >>>                 io_req_set_cqe32_extra(req, res2, 0);
> > >>> -       if (req->ctx->flags & IORING_SETUP_IOPOLL)
> > >>> +       if (ctx->flags & IORING_SETUP_IOPOLL) {
> > >>>                 /* order with io_iopoll_req_issued() checking ->iopoll_complete */
> > >>>                 smp_store_release(&req->iopoll_completed, 1);
> > >>> -       else
> > >>> -               io_req_complete_post(req, 0);
> > >>> +               return;
> > >>> +       }
> > >>> +       req->io_task_work.func = io_req_task_complete;
> > >>> +       io_req_task_work_add(req);
> > >>>  }
> > >>
> > >> Since io_uring_cmd_done itself would be executing in task-work often
> > >> (always in case of nvme), can this be further optimized by doing
> > >> directly what this new task-work (that is being set up here) would
> > >> have done?
> > >> Something like below on top of your patch -
> > >>
> > >> diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
> > >> index e1929f6e5a24..7a764e04f309 100644
> > >> --- a/io_uring/uring_cmd.c
> > >> +++ b/io_uring/uring_cmd.c
> > >> @@ -58,8 +58,12 @@ void io_uring_cmd_done(struct io_uring_cmd *ioucmd,
> > >> ssize_t ret, ssize_t res2)
> > >>                 smp_store_release(&req->iopoll_completed, 1);
> > >>                 return;
> > >>         }
> > >> -       req->io_task_work.func = io_req_task_complete;
> > >> -       io_req_task_work_add(req);
> > >> +       if (in_task()) {
> > >> +               io_req_complete_defer(req);
> > >> +       } else {
> > >> +               req->io_task_work.func = io_req_task_complete;
> > >> +               io_req_task_work_add(req);
> > >> +       }
> > >>  }
> > >>  EXPORT_SYMBOL_GPL(io_uring_cmd_done);
> > >
> > > Good point, though I do think we should rework to pass in the flags
> > > instead. I'll take a look.
> >
> > Something like this, totally untested... And this may be more
> > interesting than it would appear, because the current:
> >
> >         io_req_complete_post(req, 0);
> >
> > in io_uring_cmd_done() is passing in that it has the CQ ring locked, but
> > that does not look like it's guaranteed? So this is more of a
> > correctness thing first and foremost, more so than an optimization.
> >
> > Hmm?
>
> When zero is passed to io_req_complete_post, it calls
> __io_req_complete_post() which takes CQ lock as the first thing.
> So the correct thing will happen. Am I missing something?

And because this CQ lock was there, optimization is able to improve the numbers.

  reply	other threads:[~2023-03-21  4:39 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-03-19 15:18 [PATCH] io_uring/uring_cmd: push IRQ based completions through task_work Jens Axboe
2023-03-20 15:06 ` Kanchan Joshi
2023-03-20 20:03   ` Jens Axboe
2023-03-20 20:42     ` Jens Axboe
2023-03-21  4:32       ` Kanchan Joshi
2023-03-21  4:38         ` Kanchan Joshi [this message]
2023-03-27 11:16       ` Pavel Begunkov
2023-03-20 23:35   ` Ming Lei
2023-03-21  1:39     ` Jens Axboe
2023-03-21  1:54       ` Ming Lei
2023-03-21  1:56         ` Jens Axboe

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CA+1E3rKsrpp80qRuRM1K=cv6vTivhNO8J4bV_hXC96QrsB=dhw@mail.gmail.com' \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox