Re: [PATCH 2/2] blk-mq: Improve performance of non-mq IO schedulers with multiple HW queues

[Ming Lei <ming.lei@redhat.com>]

On Thu, Jan 07, 2021 at 12:18:15PM +0100, Jan Kara wrote:
> On Thu 07-01-21 14:19:18, Ming Lei wrote:
> > On Wed, Jan 06, 2021 at 11:24:28AM +0100, Jan Kara wrote:
> > > +/* Check if there are requests queued in hctx lists. */
> > > +static bool blk_mq_hctx_has_queued_rq(struct blk_mq_hw_ctx *hctx)
> > > +{
> > > +	return !list_empty_careful(&hctx->dispatch) ||
> > > +		sbitmap_any_bit_set(&hctx->ctx_map);
> > > +}
> > > +
> > 
> > blk_mq_hctx_mark_pending() is only called in case of none scheduler, so
> > looks not necessary to check hctx->ctx_map in blk_mq_hctx_has_queued_rq()
> > which is supposed to be used when real io scheduler is attached to MQ queue.
> 
> Yes, I know. I just wanted to make the code less fragile... In particular I
> was somewhat uneasy that we'd rely on the implicit behavior that
> blk_mq_get_sqsched_hctx() can return non-NULL only if sbitmap_any_bit_set()
> is not needed. But maybe we could structure the code like:

BTW, I mentioned the point because sbitmap_any_bit_set(hctx->ctx_map) may take
some CPU cycle in case that nr_cpu_ids is big.

> 
> 	sq_hctx = NULL;
> 	if (blk_mq_has_sqsched(q))
> 		sq_hctx = blk_mq_get_sq_hctx(q);
> 	queue_for_each_hw_ctx(q, hctx, i) {
> 		...
> 		if (!sq_hctx || sq_hctx == hctx ||
> 		    !list_empty_careful(&hctx->dispatch))
> 			... run ...
> 	}
> 
> Because then it is kind of obvious that sq_hctx is set only if there's IO
> scheduler for the queue and thus ctx_map is unused. What do you think?

IMO, the above is more readable and efficient.

Thanks,
Ming