Re: [RFC PATCH bitmap-for-next 4/4] blk_mq: Fix cpumask_check() warning in blk_mq_hctx_next_cpu()

[Valentin Schneider]

On 06/10/22 06:50, Yury Norov wrote:
> On Thu, Oct 06, 2022 at 01:21:12PM +0100, Valentin Schneider wrote:
>> blk_mq_hctx_next_cpu() implements a form of cpumask_next_and_wrap() using
>> cpumask_next_and_cpu() and blk_mq_first_mapped_cpu():
>>
>> [    5.398453] WARNING: CPU: 3 PID: 162 at include/linux/cpumask.h:110 __blk_mq_delay_run_hw_queue+0x16b/0x180
>> [    5.399317] Modules linked in:
>> [    5.399646] CPU: 3 PID: 162 Comm: ssh-keygen Tainted: G                 N 6.0.0-rc4-00004-g93003cb24006 #55
>> [    5.400135] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.14.0-0-g155821a1990b-prebuilt.qemu.org 04/01/2014
>> [    5.405430] Call Trace:
>> [    5.406152]  <TASK>
>> [    5.406452]  blk_mq_sched_insert_requests+0x67/0x150
>> [    5.406759]  blk_mq_flush_plug_list+0xd0/0x280
>> [    5.406987]  ? bit_wait+0x60/0x60
>> [    5.407317]  __blk_flush_plug+0xdb/0x120
>> [    5.407561]  ? bit_wait+0x60/0x60
>> [    5.407765]  io_schedule_prepare+0x38/0x40
>> [...]
>>
>> This triggers a warning when next_cpu == nr_cpu_ids - 1, so rewrite it
>> using cpumask_next_and_wrap() directly. The backwards-going goto can be
>> removed, as the cpumask_next*() operation already ANDs hctx->cpumask and
>> cpu_online_mask, which implies checking for an online CPU.
>>
>> No change in behaviour intended.
>>
>> Suggested-by: Yury Norov <yury.norov@xxxxxxxxx>
>> Signed-off-by: Valentin Schneider <vschneid@xxxxxxxxxx>
>> ---
>>  block/blk-mq.c | 39 +++++++++++++--------------------------
>>  1 file changed, 13 insertions(+), 26 deletions(-)
>>
>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>> index c96c8c4f751b..1520794dd9ea 100644
>> --- a/block/blk-mq.c
>> +++ b/block/blk-mq.c
>> @@ -2038,42 +2038,29 @@ static inline int blk_mq_first_mapped_cpu(struct blk_mq_hw_ctx *hctx)
>>   */
>>  static int blk_mq_hctx_next_cpu(struct blk_mq_hw_ctx *hctx)
>>  {
>> -	bool tried = false;
>>      int next_cpu = hctx->next_cpu;
>>
>>      if (hctx->queue->nr_hw_queues == 1)
>>              return WORK_CPU_UNBOUND;
>>
>> -	if (--hctx->next_cpu_batch <= 0) {
>> -select_cpu:
>> -		next_cpu = cpumask_next_and(next_cpu, hctx->cpumask,
>> -				cpu_online_mask);
>> -		if (next_cpu >= nr_cpu_ids)
>> -			next_cpu = blk_mq_first_mapped_cpu(hctx);
>> +	if (--hctx->next_cpu_batch > 0 && cpu_online(next_cpu))
>> +		return next_cpu;
>> +
>> +	next_cpu = cpumask_next_and_wrap(next_cpu, hctx->cpumask, cpu_online_mask, next_cpu, false);
>
> Last two parameters are simply useless. In fact, in many cases they
> are useless for cpumask_next_wrap(). I'm working on simplifying the
> cpumask_next_wrap() so that it would take just 2 parameters - pivot
> point and cpumask.
>
> Regarding 'next' version - we already have find_next_and_bit_wrap(),
> and I think cpumask_next_and_wrap() should use it.
>

Oh, I had missed those, that makes more sense indeed.

> For the context: those last parameters are needed to exclude part of
> cpumask from traversing, and to implement for-loop. Now that we have
> for_each_cpu_wrap() based on for_each_set_bit_wrap(), it's possible
> to remove them. I'm working on it.

Sounds good.