| Message ID | 20200602091502.1822499-3-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | blk-mq: support batching dispatch from scheduler | expand |
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); >
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
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 > >
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
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);