Re: [PATCH V2 RESEND 3/5] blk-mq: stop to handle IO before hctx's all CPUs become offline

[John Garry <john.garry@xxxxxxxxxx>]

On 07/10/2019 16:04, Ming Lei wrote:
> On Mon, Oct 07, 2019 at 11:23:22AM +0100, John Garry wrote:
> > On 06/10/2019 03:45, Ming Lei wrote:
> > > +	}
> > > +}
> > > +
> > > +static int blk_mq_hctx_notify_online(unsigned int cpu, struct hlist_node *node)
> > > +{
> > > +	struct blk_mq_hw_ctx *hctx = hlist_entry_safe(node,
> > > +			struct blk_mq_hw_ctx, cpuhp_online);
> > > +	unsigned prev_cpu = -1;
> > > +
> > > +	while (true) {
> > > +		unsigned next_cpu = cpumask_next_and(prev_cpu, hctx->cpumask,
> > > +				cpu_online_mask);
> > > +
> > > +		if (next_cpu >= nr_cpu_ids)
> > > +			break;
> > > +
> > > +		/* return if there is other online CPU on this hctx */
> > > +		if (next_cpu != cpu)
> > > +			return 0;
> > > +
> > > +		prev_cpu = next_cpu;
> > > +	}
> > > +
> > > +	set_bit(BLK_MQ_S_INTERNAL_STOPPED, &hctx->state);
> > > +	blk_mq_drain_inflight_rqs(hctx);
> > > +
> >
> > Does this do the same:
> >
> > {
> > 	struct blk_mq_hw_ctx *hctx = hlist_entry_safe(node,
> > 			struct blk_mq_hw_ctx, cpuhp_online);
> > 	cpumask_var_t tmp;
> >
> > 	cpumask_and(tmp, hctx->cpumask, cpu_online_mask);
> >
> > 	/* test if there is any other cpu online in the hctx cpu mask */
> > 	if (cpumask_any_but(tmp, cpu) < nr_cpu_ids)
> > 		return 0;
> >
> > 	set_bit(BLK_MQ_S_INTERNAL_STOPPED, &hctx->state);
> > 	blk_mq_drain_inflight_rqs(hctx);
> >
> > 	return 0;
> > }
> >
> > If so, it's more readable and concise.
>
> Yes, but we have to allocate space for 'tmp', that is what this patch
> tries to avoid,

Yeah, I forgot about the extra complications of the cpumask offstack 
stuff; but it does seem rarely used...

There is this:

{
	struct blk_mq_hw_ctx *hctx = hlist_entry_safe(node,
			struct blk_mq_hw_ctx, cpuhp_online);

	if ((cpumask_next_and(-1, hctx->cpumask, cpu_online_mask) ==
	     cpu) &&
	     (cpumask_next_and(cpu, hctx->cpumask, cpu_online_mask) >=
              nr_cpu_ids)) {
		set_bit(BLK_MQ_S_INTERNAL_STOPPED, &hctx->state);
		blk_mq_drain_inflight_rqs(hctx);
	}

	return 0;
}

... which looks effectively the same as yours, except a bit more 
readable (ignoring the fixable spilling of lines) to me.

Thanks,
John

> given the logic isn't too complicated.
> > BTW, You could have added my Tested-by tags...
>
> OK, I will add it in V3.
>
>
> Thanks,
> Ming
>
> .