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
