All of lore.kernel.org
 help / color / mirror / Atom feed
From: Ming Lei <ming.lei@redhat.com>
To: Christoph Hellwig <hch@infradead.org>
Cc: Jens Axboe <axboe@kernel.dk>,
	linux-block@vger.kernel.org, Sagi Grimberg <sagi@grimberg.me>,
	Baolin Wang <baolin.wang7@gmail.com>
Subject: Re: [PATCH 3/9] blk-mq: don't predicate last flag in blk_mq_dispatch_rq_list
Date: Thu, 14 May 2020 10:19:29 +0800	[thread overview]
Message-ID: <20200514021929.GI2073570@T590> (raw)
In-Reply-To: <20200514020955.GH2073570@T590>

On Thu, May 14, 2020 at 10:09:55AM +0800, Ming Lei wrote:
> On Wed, May 13, 2020 at 05:27:53AM -0700, Christoph Hellwig wrote:
> > On Wed, May 13, 2020 at 05:54:37PM +0800, Ming Lei wrote:
> > > .commit_rqs() is supposed to handle partial dispatch when driver may not
> > > see .last of flag passed to .queue_rq().
> > > 
> > > We have added .commit_rqs() in case of partial dispatch and all consumers
> > > of bd->last have implemented .commit_rqs() callback, so it is perfect to
> > > pass real .last flag of the request list to .queue_rq() instead of faking
> > > it by trying to allocate driver tag for next request in the batching list.
> > 
> > The current case still seems like a nice optimization to avoid an extra
> > indirect function call.  So if you want to get rid of it I think it at
> > least needs a better rationale.
> 
> Forget to mention, trying to predicate the last request via allocating
> tag for next request can't avoid extra .commit_rqs() because this
> indirect call is always called when the rq list isn't done.
> 
> Also no matter .last is set or not, every implementation of .commit_rqs
> always grabs one lock, so looks this patch can get real win without any
> performance loss.
> 
> On the other side, .commit_rqs() can be avoided iff the last queued(successful)
> rq is marked as .last, and the cost is to keep current estimate on .last.
> However, why is .commit_rqs() required? Why doesn't .queue_rq() handle the batching
> submission before non-STS_OK is returned? And the inline handling can be quite
> efficient because one more spin lock acquire can be avoided usually. Then
> .commit_rqs() can be killed.

The only chance we need .commit_rqs() should be:

- requests are queued successfully, and the last queued rq isn't marked
  as last
- running out of budget or driver tag before queueing one new request

I think we need to document the interfaces(.commit_rqs & .queue_rq) clearly.

thanks,
Ming


  reply	other threads:[~2020-05-14  2:19 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-13  9:54 [PATCH 0/9] blk-mq: support batching dispatch from scheduler Ming Lei
2020-05-13  9:54 ` [PATCH 1/9] blk-mq: pass request queue into get/put budget callback Ming Lei
2020-05-13 10:06   ` Johannes Thumshirn
2020-05-13 12:24   ` Christoph Hellwig
2020-05-13 14:36   ` Doug Anderson
2020-05-13 22:48   ` Sagi Grimberg
2020-05-13  9:54 ` [PATCH 2/9] blk-mq: pass hctx to blk_mq_dispatch_rq_list Ming Lei
2020-05-13 12:26   ` Christoph Hellwig
2020-05-13 22:49   ` Sagi Grimberg
2020-05-13  9:54 ` [PATCH 3/9] blk-mq: don't predicate last flag in blk_mq_dispatch_rq_list Ming Lei
2020-05-13 12:27   ` Christoph Hellwig
2020-05-14  0:50     ` Ming Lei
2020-05-14  5:50       ` Christoph Hellwig
2020-05-14  2:09     ` Ming Lei
2020-05-14  2:19       ` Ming Lei [this message]
2020-05-14  3:21       ` Keith Busch
2020-05-14  8:28         ` Ming Lei
2020-05-13  9:54 ` [PATCH 4/9] blk-mq: move getting driver tag and bugget into one helper Ming Lei
2020-05-13 12:37   ` Christoph Hellwig
2020-05-13 22:54   ` Sagi Grimberg
2020-05-13  9:54 ` [PATCH 5/9] blk-mq: move .queue_rq code " Ming Lei
2020-05-13 12:38   ` Christoph Hellwig
2020-05-13  9:54 ` [PATCH 6/9] blk-mq: move code for handling partial dispatch " Ming Lei
2020-05-13 12:56   ` Christoph Hellwig
2020-05-13 13:01     ` Christoph Hellwig
2020-05-14  1:25       ` Ming Lei
2020-05-13  9:54 ` [PATCH 7/9] blk-mq: remove dead check from blk_mq_dispatch_rq_list Ming Lei
2020-05-13 12:57   ` Christoph Hellwig
2020-05-13 23:24   ` Sagi Grimberg
2020-05-13  9:54 ` [PATCH 8/9] blk-mq: pass obtained budget count to blk_mq_dispatch_rq_list Ming Lei
2020-05-13 13:26   ` Christoph Hellwig
2020-05-13  9:54 ` [PATCH 9/9] blk-mq: support batching dispatch in case of io scheduler Ming Lei
2020-05-23  7:45 ` [PATCH 0/9] blk-mq: support batching dispatch from scheduler Baolin Wang
2020-05-25  2:17   ` Ming Lei

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=20200514021929.GI2073570@T590 \
    --to=ming.lei@redhat.com \
    --cc=axboe@kernel.dk \
    --cc=baolin.wang7@gmail.com \
    --cc=hch@infradead.org \
    --cc=linux-block@vger.kernel.org \
    --cc=sagi@grimberg.me \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.