On Thu, Jan 07, 2021 at 12:18:15PM +0100, Jan Kara wrote:

On Thu 07-01-21 14:19:18, Ming Lei wrote:

quoted

On Wed, Jan 06, 2021 at 11:24:28AM +0100, Jan Kara wrote:

quoted

+/* 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