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@xxxxxxxxxxx>
> >>> Cc: Baolin Wang <baolin.wang7@xxxxxxxxx>
> >>> Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>
> >>> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
> >>> Reviewed-by: Sagi Grimberg <sagi@xxxxxxxxxxx>
> >>> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@xxxxxxx>
> >>> Tested-by: Baolin Wang <baolin.wang7@xxxxxxxxx>
> >>> Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxx>
> >>> ---
> >>> 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
