[V4,2/6] blk-mq: pass hctx to blk_mq_dispatch_rq_list
diff mbox series

Message ID 20200602091502.1822499-3-ming.lei@redhat.com
State New
Headers show
Series
  • blk-mq: support batching dispatch from scheduler
Related show

Commit Message

Ming Lei June 2, 2020, 9:14 a.m. UTC
All requests in the 'list' of blk_mq_dispatch_rq_list belong to same
hctx, so it is better to pass hctx instead of request queue, because
blk-mq's dispatch target is hctx instead of request queue.

Cc: Sagi Grimberg <sagi@grimberg.me>
Cc: Baolin Wang <baolin.wang7@gmail.com>
Cc: Christoph Hellwig <hch@infradead.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Tested-by: Baolin Wang <baolin.wang7@gmail.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq-sched.c | 14 ++++++--------
 block/blk-mq.c       |  6 +++---
 block/blk-mq.h       |  2 +-
 3 files changed, 10 insertions(+), 12 deletions(-)

Comments

Damien Le Moal June 2, 2020, 9:25 a.m. UTC | #1
On 2020/06/02 18:15, Ming Lei wrote:
> All requests in the 'list' of blk_mq_dispatch_rq_list belong to same
> hctx, so it is better to pass hctx instead of request queue, because
> blk-mq's dispatch target is hctx instead of request queue.
> 
> Cc: Sagi Grimberg <sagi@grimberg.me>
> Cc: Baolin Wang <baolin.wang7@gmail.com>
> Cc: Christoph Hellwig <hch@infradead.org>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> Tested-by: Baolin Wang <baolin.wang7@gmail.com>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  block/blk-mq-sched.c | 14 ++++++--------
>  block/blk-mq.c       |  6 +++---
>  block/blk-mq.h       |  2 +-
>  3 files changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
> index a31e281e9d31..632c6f8b63f7 100644
> --- a/block/blk-mq-sched.c
> +++ b/block/blk-mq-sched.c
> @@ -96,10 +96,9 @@ static int blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
>  	struct elevator_queue *e = q->elevator;
>  	LIST_HEAD(rq_list);
>  	int ret = 0;
> +	struct request *rq;
>  
>  	do {
> -		struct request *rq;
> -
>  		if (e->type->ops.has_work && !e->type->ops.has_work(hctx))
>  			break;
>  
> @@ -131,7 +130,7 @@ static int blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
>  		 * in blk_mq_dispatch_rq_list().
>  		 */
>  		list_add(&rq->queuelist, &rq_list);
> -	} while (blk_mq_dispatch_rq_list(q, &rq_list, true));
> +	} while (blk_mq_dispatch_rq_list(rq->mq_hctx, &rq_list, true));

Why not use the hctx argument passed to the function instead of rq->mq_hctx ?

>  
>  	return ret;
>  }
> @@ -161,10 +160,9 @@ static int blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
>  	LIST_HEAD(rq_list);
>  	struct blk_mq_ctx *ctx = READ_ONCE(hctx->dispatch_from);
>  	int ret = 0;
> +	struct request *rq;
>  
>  	do {
> -		struct request *rq;
> -
>  		if (!list_empty_careful(&hctx->dispatch)) {
>  			ret = -EAGAIN;
>  			break;
> @@ -200,7 +198,7 @@ static int blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
>  		/* round robin for fair dispatch */
>  		ctx = blk_mq_next_ctx(hctx, rq->mq_ctx);
>  
> -	} while (blk_mq_dispatch_rq_list(q, &rq_list, true));
> +	} while (blk_mq_dispatch_rq_list(rq->mq_hctx, &rq_list, true));

Same here.

>  
>  	WRITE_ONCE(hctx->dispatch_from, ctx);
>  	return ret;
> @@ -240,7 +238,7 @@ static int __blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)
>  	 */
>  	if (!list_empty(&rq_list)) {
>  		blk_mq_sched_mark_restart_hctx(hctx);
> -		if (blk_mq_dispatch_rq_list(q, &rq_list, false)) {
> +		if (blk_mq_dispatch_rq_list(hctx, &rq_list, false)) {
>  			if (has_sched_dispatch)
>  				ret = blk_mq_do_dispatch_sched(hctx);
>  			else
> @@ -253,7 +251,7 @@ static int __blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)
>  		ret = blk_mq_do_dispatch_ctx(hctx);
>  	} else {
>  		blk_mq_flush_busy_ctxs(hctx, &rq_list);
> -		blk_mq_dispatch_rq_list(q, &rq_list, false);
> +		blk_mq_dispatch_rq_list(hctx, &rq_list, false);
>  	}
>  
>  	return ret;
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index bcbf49bd7ebe..723bc39507fe 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -1236,10 +1236,10 @@ static void blk_mq_handle_zone_resource(struct request *rq,
>  /*
>   * Returns true if we did some work AND can potentially do more.
>   */
> -bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list,
> +bool blk_mq_dispatch_rq_list(struct blk_mq_hw_ctx *hctx, struct list_head *list,
>  			     bool got_budget)
>  {
> -	struct blk_mq_hw_ctx *hctx;
> +	struct request_queue *q = hctx->queue;
>  	struct request *rq, *nxt;
>  	bool no_tag = false;
>  	int errors, queued;
> @@ -1261,7 +1261,7 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list,
>  
>  		rq = list_first_entry(list, struct request, queuelist);
>  
> -		hctx = rq->mq_hctx;
> +		WARN_ON_ONCE(hctx != rq->mq_hctx);
>  		if (!got_budget && !blk_mq_get_dispatch_budget(q)) {
>  			blk_mq_put_driver_tag(rq);
>  			no_budget_avail = true;
> diff --git a/block/blk-mq.h b/block/blk-mq.h
> index 21d877105224..d2d737b16e0e 100644
> --- a/block/blk-mq.h
> +++ b/block/blk-mq.h
> @@ -40,7 +40,7 @@ struct blk_mq_ctx {
>  void blk_mq_exit_queue(struct request_queue *q);
>  int blk_mq_update_nr_requests(struct request_queue *q, unsigned int nr);
>  void blk_mq_wake_waiters(struct request_queue *q);
> -bool blk_mq_dispatch_rq_list(struct request_queue *, struct list_head *, bool);
> +bool blk_mq_dispatch_rq_list(struct blk_mq_hw_ctx *hctx, struct list_head *, bool);
>  void blk_mq_add_to_requeue_list(struct request *rq, bool at_head,
>  				bool kick_requeue_list);
>  void blk_mq_flush_busy_ctxs(struct blk_mq_hw_ctx *hctx, struct list_head *list);
>
Ming Lei June 2, 2020, 9:32 a.m. UTC | #2
On Tue, Jun 02, 2020 at 09:25:01AM +0000, Damien Le Moal wrote:
> On 2020/06/02 18:15, Ming Lei wrote:
> > All requests in the 'list' of blk_mq_dispatch_rq_list belong to same
> > hctx, so it is better to pass hctx instead of request queue, because
> > blk-mq's dispatch target is hctx instead of request queue.
> > 
> > Cc: Sagi Grimberg <sagi@grimberg.me>
> > Cc: Baolin Wang <baolin.wang7@gmail.com>
> > Cc: Christoph Hellwig <hch@infradead.org>
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
> > Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> > Tested-by: Baolin Wang <baolin.wang7@gmail.com>
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >  block/blk-mq-sched.c | 14 ++++++--------
> >  block/blk-mq.c       |  6 +++---
> >  block/blk-mq.h       |  2 +-
> >  3 files changed, 10 insertions(+), 12 deletions(-)
> > 
> > diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
> > index a31e281e9d31..632c6f8b63f7 100644
> > --- a/block/blk-mq-sched.c
> > +++ b/block/blk-mq-sched.c
> > @@ -96,10 +96,9 @@ static int blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
> >  	struct elevator_queue *e = q->elevator;
> >  	LIST_HEAD(rq_list);
> >  	int ret = 0;
> > +	struct request *rq;
> >  
> >  	do {
> > -		struct request *rq;
> > -
> >  		if (e->type->ops.has_work && !e->type->ops.has_work(hctx))
> >  			break;
> >  
> > @@ -131,7 +130,7 @@ static int blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
> >  		 * in blk_mq_dispatch_rq_list().
> >  		 */
> >  		list_add(&rq->queuelist, &rq_list);
> > -	} while (blk_mq_dispatch_rq_list(q, &rq_list, true));
> > +	} while (blk_mq_dispatch_rq_list(rq->mq_hctx, &rq_list, true));
> 
> Why not use the hctx argument passed to the function instead of rq->mq_hctx ?

e->type->ops.dispatch_request(hctx) may return one request which's
.mq_hctx isn't same with the 'hctx' argument, so far bfq and deadline
may do that.


Thanks,
Ming
Damien Le Moal June 2, 2020, 9:39 a.m. UTC | #3
On 2020/06/02 18:32, Ming Lei wrote:
> On Tue, Jun 02, 2020 at 09:25:01AM +0000, Damien Le Moal wrote:
>> On 2020/06/02 18:15, Ming Lei wrote:
>>> All requests in the 'list' of blk_mq_dispatch_rq_list belong to same
>>> hctx, so it is better to pass hctx instead of request queue, because
>>> blk-mq's dispatch target is hctx instead of request queue.
>>>
>>> Cc: Sagi Grimberg <sagi@grimberg.me>
>>> Cc: Baolin Wang <baolin.wang7@gmail.com>
>>> Cc: Christoph Hellwig <hch@infradead.org>
>>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>>> Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
>>> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>>> Tested-by: Baolin Wang <baolin.wang7@gmail.com>
>>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
>>> ---
>>>  block/blk-mq-sched.c | 14 ++++++--------
>>>  block/blk-mq.c       |  6 +++---
>>>  block/blk-mq.h       |  2 +-
>>>  3 files changed, 10 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
>>> index a31e281e9d31..632c6f8b63f7 100644
>>> --- a/block/blk-mq-sched.c
>>> +++ b/block/blk-mq-sched.c
>>> @@ -96,10 +96,9 @@ static int blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
>>>  	struct elevator_queue *e = q->elevator;
>>>  	LIST_HEAD(rq_list);
>>>  	int ret = 0;
>>> +	struct request *rq;
>>>  
>>>  	do {
>>> -		struct request *rq;
>>> -
>>>  		if (e->type->ops.has_work && !e->type->ops.has_work(hctx))
>>>  			break;
>>>  
>>> @@ -131,7 +130,7 @@ static int blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
>>>  		 * in blk_mq_dispatch_rq_list().
>>>  		 */
>>>  		list_add(&rq->queuelist, &rq_list);
>>> -	} while (blk_mq_dispatch_rq_list(q, &rq_list, true));
>>> +	} while (blk_mq_dispatch_rq_list(rq->mq_hctx, &rq_list, true));
>>
>> Why not use the hctx argument passed to the function instead of rq->mq_hctx ?
> 
> e->type->ops.dispatch_request(hctx) may return one request which's
> .mq_hctx isn't same with the 'hctx' argument, so far bfq and deadline
> may do that.

Ah, OK. But then all requests in rq_list may have different hctx. So is it wise
to pass hctx as an argument to blk_mq_dispatch_rq_list() ? The loop in that
function will still need to look at each rq hctx (hctx = rq->mq_hctx) for the
budget. So the hctx argument may not be needed at all, no ? Am I missing something ?

> 
> 
> Thanks,
> Ming
> 
>
Ming Lei June 2, 2020, 1:49 p.m. UTC | #4
On Tue, Jun 02, 2020 at 09:39:18AM +0000, Damien Le Moal wrote:
> On 2020/06/02 18:32, Ming Lei wrote:
> > On Tue, Jun 02, 2020 at 09:25:01AM +0000, Damien Le Moal wrote:
> >> On 2020/06/02 18:15, Ming Lei wrote:
> >>> All requests in the 'list' of blk_mq_dispatch_rq_list belong to same
> >>> hctx, so it is better to pass hctx instead of request queue, because
> >>> blk-mq's dispatch target is hctx instead of request queue.
> >>>
> >>> Cc: Sagi Grimberg <sagi@grimberg.me>
> >>> Cc: Baolin Wang <baolin.wang7@gmail.com>
> >>> Cc: Christoph Hellwig <hch@infradead.org>
> >>> Reviewed-by: Christoph Hellwig <hch@lst.de>
> >>> Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
> >>> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> >>> Tested-by: Baolin Wang <baolin.wang7@gmail.com>
> >>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> >>> ---
> >>>  block/blk-mq-sched.c | 14 ++++++--------
> >>>  block/blk-mq.c       |  6 +++---
> >>>  block/blk-mq.h       |  2 +-
> >>>  3 files changed, 10 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
> >>> index a31e281e9d31..632c6f8b63f7 100644
> >>> --- a/block/blk-mq-sched.c
> >>> +++ b/block/blk-mq-sched.c
> >>> @@ -96,10 +96,9 @@ static int blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
> >>>  	struct elevator_queue *e = q->elevator;
> >>>  	LIST_HEAD(rq_list);
> >>>  	int ret = 0;
> >>> +	struct request *rq;
> >>>  
> >>>  	do {
> >>> -		struct request *rq;
> >>> -
> >>>  		if (e->type->ops.has_work && !e->type->ops.has_work(hctx))
> >>>  			break;
> >>>  
> >>> @@ -131,7 +130,7 @@ static int blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
> >>>  		 * in blk_mq_dispatch_rq_list().
> >>>  		 */
> >>>  		list_add(&rq->queuelist, &rq_list);
> >>> -	} while (blk_mq_dispatch_rq_list(q, &rq_list, true));
> >>> +	} while (blk_mq_dispatch_rq_list(rq->mq_hctx, &rq_list, true));
> >>
> >> Why not use the hctx argument passed to the function instead of rq->mq_hctx ?
> > 
> > e->type->ops.dispatch_request(hctx) may return one request which's
> > .mq_hctx isn't same with the 'hctx' argument, so far bfq and deadline
> > may do that.
> 
> Ah, OK. But then all requests in rq_list may have different hctx. So is it wise
> to pass hctx as an argument to blk_mq_dispatch_rq_list() ? The loop in that

&rq_list is one single request list.

> function will still need to look at each rq hctx (hctx = rq->mq_hctx) for the
> budget. So the hctx argument may not be needed at all, no ? Am I missing something ?

The final patch will add batching dispatch support, and more requests in
same hctx will be added to this list, at that time, the hctx argument
becomes reasonable.


Thanks,
Ming

Patch
diff mbox series

diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index a31e281e9d31..632c6f8b63f7 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -96,10 +96,9 @@  static int blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
 	struct elevator_queue *e = q->elevator;
 	LIST_HEAD(rq_list);
 	int ret = 0;
+	struct request *rq;
 
 	do {
-		struct request *rq;
-
 		if (e->type->ops.has_work && !e->type->ops.has_work(hctx))
 			break;
 
@@ -131,7 +130,7 @@  static int blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
 		 * in blk_mq_dispatch_rq_list().
 		 */
 		list_add(&rq->queuelist, &rq_list);
-	} while (blk_mq_dispatch_rq_list(q, &rq_list, true));
+	} while (blk_mq_dispatch_rq_list(rq->mq_hctx, &rq_list, true));
 
 	return ret;
 }
@@ -161,10 +160,9 @@  static int blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
 	LIST_HEAD(rq_list);
 	struct blk_mq_ctx *ctx = READ_ONCE(hctx->dispatch_from);
 	int ret = 0;
+	struct request *rq;
 
 	do {
-		struct request *rq;
-
 		if (!list_empty_careful(&hctx->dispatch)) {
 			ret = -EAGAIN;
 			break;
@@ -200,7 +198,7 @@  static int blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
 		/* round robin for fair dispatch */
 		ctx = blk_mq_next_ctx(hctx, rq->mq_ctx);
 
-	} while (blk_mq_dispatch_rq_list(q, &rq_list, true));
+	} while (blk_mq_dispatch_rq_list(rq->mq_hctx, &rq_list, true));
 
 	WRITE_ONCE(hctx->dispatch_from, ctx);
 	return ret;
@@ -240,7 +238,7 @@  static int __blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)
 	 */
 	if (!list_empty(&rq_list)) {
 		blk_mq_sched_mark_restart_hctx(hctx);
-		if (blk_mq_dispatch_rq_list(q, &rq_list, false)) {
+		if (blk_mq_dispatch_rq_list(hctx, &rq_list, false)) {
 			if (has_sched_dispatch)
 				ret = blk_mq_do_dispatch_sched(hctx);
 			else
@@ -253,7 +251,7 @@  static int __blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)
 		ret = blk_mq_do_dispatch_ctx(hctx);
 	} else {
 		blk_mq_flush_busy_ctxs(hctx, &rq_list);
-		blk_mq_dispatch_rq_list(q, &rq_list, false);
+		blk_mq_dispatch_rq_list(hctx, &rq_list, false);
 	}
 
 	return ret;
diff --git a/block/blk-mq.c b/block/blk-mq.c
index bcbf49bd7ebe..723bc39507fe 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1236,10 +1236,10 @@  static void blk_mq_handle_zone_resource(struct request *rq,
 /*
  * Returns true if we did some work AND can potentially do more.
  */
-bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list,
+bool blk_mq_dispatch_rq_list(struct blk_mq_hw_ctx *hctx, struct list_head *list,
 			     bool got_budget)
 {
-	struct blk_mq_hw_ctx *hctx;
+	struct request_queue *q = hctx->queue;
 	struct request *rq, *nxt;
 	bool no_tag = false;
 	int errors, queued;
@@ -1261,7 +1261,7 @@  bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list,
 
 		rq = list_first_entry(list, struct request, queuelist);
 
-		hctx = rq->mq_hctx;
+		WARN_ON_ONCE(hctx != rq->mq_hctx);
 		if (!got_budget && !blk_mq_get_dispatch_budget(q)) {
 			blk_mq_put_driver_tag(rq);
 			no_budget_avail = true;
diff --git a/block/blk-mq.h b/block/blk-mq.h
index 21d877105224..d2d737b16e0e 100644
--- a/block/blk-mq.h
+++ b/block/blk-mq.h
@@ -40,7 +40,7 @@  struct blk_mq_ctx {
 void blk_mq_exit_queue(struct request_queue *q);
 int blk_mq_update_nr_requests(struct request_queue *q, unsigned int nr);
 void blk_mq_wake_waiters(struct request_queue *q);
-bool blk_mq_dispatch_rq_list(struct request_queue *, struct list_head *, bool);
+bool blk_mq_dispatch_rq_list(struct blk_mq_hw_ctx *hctx, struct list_head *, bool);
 void blk_mq_add_to_requeue_list(struct request *rq, bool at_head,
 				bool kick_requeue_list);
 void blk_mq_flush_busy_ctxs(struct blk_mq_hw_ctx *hctx, struct list_head *list);