[RFC,1/7] blk-mq: Export blk_mq_hctx_has_pending() function
diff mbox series

Message ID 94a0d20e6304b909391abd9a425c71c376cad964.1563782844.git.baolin.wang@linaro.org
State New
Headers show
Series
  • Add MMC packed function
Related show

Commit Message

(Exiting) Baolin Wang July 22, 2019, 1:09 p.m. UTC
Some SD/MMC host controllers can support packed command or packed request,
that means we can package several requests to host controller at one time
to improve performence. And this patch set will introduce MMC packed function
to support this feature by following patches.

To support MMC packed function, the MMC layer need to know if there are
requests are pending now in hardware queue to help to combine requests
as much as possible. If we know there are requests pending in hardware
queue, then we should not package requests to host controller immediately,
instead we should collect more requests into MMC packed queue to be packed
to host controller with packed condition.

Thus export this function for MMC packed function.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 block/blk-mq.c         |    3 ++-
 include/linux/blk-mq.h |    1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

Comments

Ming Lei July 22, 2019, 2:19 p.m. UTC | #1
On Mon, Jul 22, 2019 at 09:09:36PM +0800, Baolin Wang wrote:
> Some SD/MMC host controllers can support packed command or packed request,
> that means we can package several requests to host controller at one time
> to improve performence. And this patch set will introduce MMC packed function
> to support this feature by following patches.
> 
> To support MMC packed function, the MMC layer need to know if there are
> requests are pending now in hardware queue to help to combine requests
> as much as possible. If we know there are requests pending in hardware
> queue, then we should not package requests to host controller immediately,
> instead we should collect more requests into MMC packed queue to be packed
> to host controller with packed condition.
> 
> Thus export this function for MMC packed function.
> 
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
>  block/blk-mq.c         |    3 ++-
>  include/linux/blk-mq.h |    1 +
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index b038ec6..5bd4ef9 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -63,12 +63,13 @@ static int blk_mq_poll_stats_bkt(const struct request *rq)
>   * Check if any of the ctx, dispatch list or elevator
>   * have pending work in this hardware queue.
>   */
> -static bool blk_mq_hctx_has_pending(struct blk_mq_hw_ctx *hctx)
> +bool blk_mq_hctx_has_pending(struct blk_mq_hw_ctx *hctx)
>  {
>  	return !list_empty_careful(&hctx->dispatch) ||
>  		sbitmap_any_bit_set(&hctx->ctx_map) ||
>  			blk_mq_sched_has_work(hctx);
>  }
> +EXPORT_SYMBOL_GPL(blk_mq_hctx_has_pending);

Just wondering why you don't use the 'last' field of 'struct blk_mq_queue_data',
which is passed to .queue_rq(), and supposed for implementing batch submission.
	

Thanks,
Ming
(Exiting) Baolin Wang July 23, 2019, 3:12 a.m. UTC | #2
Hi Ming,

On Mon, 22 Jul 2019 at 22:19, Ming Lei <ming.lei@redhat.com> wrote:
>
> On Mon, Jul 22, 2019 at 09:09:36PM +0800, Baolin Wang wrote:
> > Some SD/MMC host controllers can support packed command or packed request,
> > that means we can package several requests to host controller at one time
> > to improve performence. And this patch set will introduce MMC packed function
> > to support this feature by following patches.
> >
> > To support MMC packed function, the MMC layer need to know if there are
> > requests are pending now in hardware queue to help to combine requests
> > as much as possible. If we know there are requests pending in hardware
> > queue, then we should not package requests to host controller immediately,
> > instead we should collect more requests into MMC packed queue to be packed
> > to host controller with packed condition.
> >
> > Thus export this function for MMC packed function.
> >
> > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > ---
> >  block/blk-mq.c         |    3 ++-
> >  include/linux/blk-mq.h |    1 +
> >  2 files changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/block/blk-mq.c b/block/blk-mq.c
> > index b038ec6..5bd4ef9 100644
> > --- a/block/blk-mq.c
> > +++ b/block/blk-mq.c
> > @@ -63,12 +63,13 @@ static int blk_mq_poll_stats_bkt(const struct request *rq)
> >   * Check if any of the ctx, dispatch list or elevator
> >   * have pending work in this hardware queue.
> >   */
> > -static bool blk_mq_hctx_has_pending(struct blk_mq_hw_ctx *hctx)
> > +bool blk_mq_hctx_has_pending(struct blk_mq_hw_ctx *hctx)
> >  {
> >       return !list_empty_careful(&hctx->dispatch) ||
> >               sbitmap_any_bit_set(&hctx->ctx_map) ||
> >                       blk_mq_sched_has_work(hctx);
> >  }
> > +EXPORT_SYMBOL_GPL(blk_mq_hctx_has_pending);
>
> Just wondering why you don't use the 'last' field of 'struct blk_mq_queue_data',
> which is passed to .queue_rq(), and supposed for implementing batch submission.

The 'last' field of 'struct blk_mq_queue_data' does not indicate the
last request in the hardware queue, since we want to collect more
requests from block layer as much as possible to be packed later.

And from blk_mq_do_dispatch_sched()--->blk_mq_dispatch_rq_list()--->
queue_rq(), I always get 'bd.last = true', which is not useful to
combine requests for MMC packed queue. Maybe I missed anything?

Thanks for your comments.
Ming Lei July 23, 2019, 3:31 a.m. UTC | #3
On Tue, Jul 23, 2019 at 11:12:57AM +0800, Baolin Wang wrote:
> Hi Ming,
> 
> On Mon, 22 Jul 2019 at 22:19, Ming Lei <ming.lei@redhat.com> wrote:
> >
> > On Mon, Jul 22, 2019 at 09:09:36PM +0800, Baolin Wang wrote:
> > > Some SD/MMC host controllers can support packed command or packed request,
> > > that means we can package several requests to host controller at one time
> > > to improve performence. And this patch set will introduce MMC packed function
> > > to support this feature by following patches.
> > >
> > > To support MMC packed function, the MMC layer need to know if there are
> > > requests are pending now in hardware queue to help to combine requests
> > > as much as possible. If we know there are requests pending in hardware
> > > queue, then we should not package requests to host controller immediately,
> > > instead we should collect more requests into MMC packed queue to be packed
> > > to host controller with packed condition.
> > >
> > > Thus export this function for MMC packed function.
> > >
> > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > > ---
> > >  block/blk-mq.c         |    3 ++-
> > >  include/linux/blk-mq.h |    1 +
> > >  2 files changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/block/blk-mq.c b/block/blk-mq.c
> > > index b038ec6..5bd4ef9 100644
> > > --- a/block/blk-mq.c
> > > +++ b/block/blk-mq.c
> > > @@ -63,12 +63,13 @@ static int blk_mq_poll_stats_bkt(const struct request *rq)
> > >   * Check if any of the ctx, dispatch list or elevator
> > >   * have pending work in this hardware queue.
> > >   */
> > > -static bool blk_mq_hctx_has_pending(struct blk_mq_hw_ctx *hctx)
> > > +bool blk_mq_hctx_has_pending(struct blk_mq_hw_ctx *hctx)
> > >  {
> > >       return !list_empty_careful(&hctx->dispatch) ||
> > >               sbitmap_any_bit_set(&hctx->ctx_map) ||
> > >                       blk_mq_sched_has_work(hctx);
> > >  }
> > > +EXPORT_SYMBOL_GPL(blk_mq_hctx_has_pending);
> >
> > Just wondering why you don't use the 'last' field of 'struct blk_mq_queue_data',
> > which is passed to .queue_rq(), and supposed for implementing batch submission.
> 
> The 'last' field of 'struct blk_mq_queue_data' does not indicate the
> last request in the hardware queue, since we want to collect more
> requests from block layer as much as possible to be packed later.
> 
> And from blk_mq_do_dispatch_sched()--->blk_mq_dispatch_rq_list()--->
> queue_rq(), I always get 'bd.last = true', which is not useful to
> combine requests for MMC packed queue. Maybe I missed anything?

That is one flaw of current implementation, and we may improve it,
so other drivers(virtio-io, ...) can benefit from it too.


Thanks,
Ming
(Exiting) Baolin Wang July 23, 2019, 7:15 a.m. UTC | #4
On 23/07/2019, Ming Lei <ming.lei@redhat.com> wrote:
> On Tue, Jul 23, 2019 at 11:12:57AM +0800, Baolin Wang wrote:
>> Hi Ming,
>>
>> On Mon, 22 Jul 2019 at 22:19, Ming Lei <ming.lei@redhat.com> wrote:
>> >
>> > On Mon, Jul 22, 2019 at 09:09:36PM +0800, Baolin Wang wrote:
>> > > Some SD/MMC host controllers can support packed command or packed
>> > > request,
>> > > that means we can package several requests to host controller at one
>> > > time
>> > > to improve performence. And this patch set will introduce MMC packed
>> > > function
>> > > to support this feature by following patches.
>> > >
>> > > To support MMC packed function, the MMC layer need to know if there
>> > > are
>> > > requests are pending now in hardware queue to help to combine
>> > > requests
>> > > as much as possible. If we know there are requests pending in
>> > > hardware
>> > > queue, then we should not package requests to host controller
>> > > immediately,
>> > > instead we should collect more requests into MMC packed queue to be
>> > > packed
>> > > to host controller with packed condition.
>> > >
>> > > Thus export this function for MMC packed function.
>> > >
>> > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> > > ---
>> > >  block/blk-mq.c         |    3 ++-
>> > >  include/linux/blk-mq.h |    1 +
>> > >  2 files changed, 3 insertions(+), 1 deletion(-)
>> > >
>> > > diff --git a/block/blk-mq.c b/block/blk-mq.c
>> > > index b038ec6..5bd4ef9 100644
>> > > --- a/block/blk-mq.c
>> > > +++ b/block/blk-mq.c
>> > > @@ -63,12 +63,13 @@ static int blk_mq_poll_stats_bkt(const struct
>> > > request *rq)
>> > >   * Check if any of the ctx, dispatch list or elevator
>> > >   * have pending work in this hardware queue.
>> > >   */
>> > > -static bool blk_mq_hctx_has_pending(struct blk_mq_hw_ctx *hctx)
>> > > +bool blk_mq_hctx_has_pending(struct blk_mq_hw_ctx *hctx)
>> > >  {
>> > >       return !list_empty_careful(&hctx->dispatch) ||
>> > >               sbitmap_any_bit_set(&hctx->ctx_map) ||
>> > >                       blk_mq_sched_has_work(hctx);
>> > >  }
>> > > +EXPORT_SYMBOL_GPL(blk_mq_hctx_has_pending);
>> >
>> > Just wondering why you don't use the 'last' field of 'struct
>> > blk_mq_queue_data',
>> > which is passed to .queue_rq(), and supposed for implementing batch
>> > submission.
>>
>> The 'last' field of 'struct blk_mq_queue_data' does not indicate the
>> last request in the hardware queue, since we want to collect more
>> requests from block layer as much as possible to be packed later.
>>
>> And from blk_mq_do_dispatch_sched()--->blk_mq_dispatch_rq_list()--->
>> queue_rq(), I always get 'bd.last = true', which is not useful to
>> combine requests for MMC packed queue. Maybe I missed anything?
>
> That is one flaw of current implementation, and we may improve it,
> so other drivers(virtio-io, ...) can benefit from it too.
>

OK. I am not sure can I add a new flag to indicate if there are
requests are pending in the hardware queue? That will help MMC driver
to combine more requests.

Or do you have any other good suggestion? Thanks.

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 5bd4ef9..cb240f4 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1257,6 +1257,8 @@ bool blk_mq_dispatch_rq_list(struct
request_queue *q, struct list_head *list,
                        bd.last = !blk_mq_get_driver_tag(nxt);
                }

+               bd.rq_pending = blk_mq_hctx_has_pending(hctx);
+
                ret = q->mq_ops->queue_rq(hctx, &bd);
                if (ret == BLK_STS_RESOURCE || ret == BLK_STS_DEV_RESOURCE) {
                        /*
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 15a2b7b..9b06fe0 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -118,6 +118,7 @@ struct blk_mq_tag_set {
 struct blk_mq_queue_data {
        struct request *rq;
        bool last;
+       bool rq_pending;
 };

Patch
diff mbox series

diff --git a/block/blk-mq.c b/block/blk-mq.c
index b038ec6..5bd4ef9 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -63,12 +63,13 @@  static int blk_mq_poll_stats_bkt(const struct request *rq)
  * Check if any of the ctx, dispatch list or elevator
  * have pending work in this hardware queue.
  */
-static bool blk_mq_hctx_has_pending(struct blk_mq_hw_ctx *hctx)
+bool blk_mq_hctx_has_pending(struct blk_mq_hw_ctx *hctx)
 {
 	return !list_empty_careful(&hctx->dispatch) ||
 		sbitmap_any_bit_set(&hctx->ctx_map) ||
 			blk_mq_sched_has_work(hctx);
 }
+EXPORT_SYMBOL_GPL(blk_mq_hctx_has_pending);
 
 /*
  * Mark this ctx as having pending work in this hardware queue
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 3fa1fa5..15a2b7b 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -334,6 +334,7 @@  int blk_mq_freeze_queue_wait_timeout(struct request_queue *q,
 void blk_mq_quiesce_queue_nowait(struct request_queue *q);
 
 unsigned int blk_mq_rq_cpu(struct request *rq);
+bool blk_mq_hctx_has_pending(struct blk_mq_hw_ctx *hctx);
 
 /*
  * Driver command data is immediately after the request. So subtract request