Messages in this thread | | | Subject | Re: [PATCH 5/7] blk-mq-sched: add framework for MQ capable IO schedulers | From | Bart Van Assche <> | Date | Tue, 13 Dec 2016 15:29:18 +0100 |
| |
On 12/08/2016 09:13 PM, Jens Axboe wrote: > +static inline void blk_mq_sched_put_request(struct request *rq) > +{ > + struct request_queue *q = rq->q; > + struct elevator_queue *e = q->elevator; > + > + if (e && e->type->mq_ops.put_request) > + e->type->mq_ops.put_request(rq); > + else > + blk_mq_free_request(rq); > +}
blk_mq_free_request() always triggers a call of blk_queue_exit(). dd_put_request() only triggers a call of blk_queue_exit() if it is not a shadow request. Is that on purpose?
> +static inline struct request * > +blk_mq_sched_get_request(struct request_queue *q, unsigned int op, > + struct blk_mq_alloc_data *data) > +{ > + struct elevator_queue *e = q->elevator; > + struct blk_mq_hw_ctx *hctx; > + struct blk_mq_ctx *ctx; > + struct request *rq; > + > + blk_queue_enter_live(q); > + ctx = blk_mq_get_ctx(q); > + hctx = blk_mq_map_queue(q, ctx->cpu); > + > + blk_mq_set_alloc_data(data, q, 0, ctx, hctx); > + > + if (e && e->type->mq_ops.get_request) > + rq = e->type->mq_ops.get_request(q, op, data); > + else > + rq = __blk_mq_alloc_request(data, op); > + > + if (rq) > + data->hctx->queued++; > + > + return rq; > + > +}
Some but not all callers of blk_mq_sched_get_request() call blk_queue_exit() if this function returns NULL. Please consider to move the blk_queue_exit() call from the blk_mq_alloc_request() error path into this function. I think that will make it a lot easier to verify whether or not the blk_queue_enter() / blk_queue_exit() calls are balanced properly.
Additionally, since blk_queue_enter() / blk_queue_exit() calls by blk_mq_sched_get_request() and blk_mq_sched_put_request() must be balanced and since the latter function only calls blk_queue_exit() for non-shadow requests, shouldn't blk_mq_sched_get_request() call blk_queue_enter_live() only if __blk_mq_alloc_request() is called?
Thanks,
Bart.
| |