Re: [PATCH 1/7] cpumask: fix checking valid cpu range

[Yury Norov <yury.norov@gmail.com>]

On Wed, Sep 28, 2022 at 01:18:20PM +0100, Valentin Schneider wrote:
> On 19/09/22 14:05, Yury Norov wrote:
> > The range of valid CPUs is [0, nr_cpu_ids). Some cpumask functions are
> > passed with a shifted CPU index, and for them, the valid range is
> > [-1, nr_cpu_ids-1). Currently for those functions, we check the index
> > against [-1, nr_cpu_ids), which is wrong.
> >
> > Signed-off-by: Yury Norov <yury.norov@gmail.com>
> > ---
> >  include/linux/cpumask.h | 19 ++++++++-----------
> >  1 file changed, 8 insertions(+), 11 deletions(-)
> >
> > diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
> > index e4f9136a4a63..a1cd4eb1a3d6 100644
> > --- a/include/linux/cpumask.h
> > +++ b/include/linux/cpumask.h
> > @@ -174,9 +174,8 @@ static inline unsigned int cpumask_last(const struct cpumask *srcp)
> >  static inline
> >  unsigned int cpumask_next(int n, const struct cpumask *srcp)
> >  {
> > -	/* -1 is a legal arg here. */
> > -	if (n != -1)
> > -		cpumask_check(n);
> > +	/* n is a prior cpu */
> > +	cpumask_check(n + 1);
> >       return find_next_bit(cpumask_bits(srcp), nr_cpumask_bits, n + 1);
> 
> I'm confused, this makes passing nr_cpu_ids-1 to cpumask_next*() trigger a
> warning. The documentation does states:
> 
> * @n: the cpu prior to the place to search (ie. return will be > @n)
> 
> So n is a valid CPU number (with -1 being the exception for scan
> initialization), this shouldn't exclude nr_cpu_ids-1.

For a regular cpumask function, like cpumask_any_but(), the valid range is
[0, nr_cpu_ids).

'Special' functions shift by 1 when call underlying find API:

  static inline
  unsigned int cpumask_next(int n, const struct cpumask *srcp)
  {
          /* n is a prior cpu */
          cpumask_check(n + 1);
          return find_next_bit(cpumask_bits(srcp), nr_cpumask_bits, n + 1);
  }

So, for them the valid range [0, nr_cpu_ids) must be shifted in other
direction: [-1, nr_cpu_ids-1). 

> IMO passing nr_cpu_ids-1 should be treated the same as passing the
> last set bit in a bitmap: no warning, and returns the bitmap
> size.

This is how cpumask_check() works for normal functions. For
cpumask_next() passing nr_cpu_ids-1 is the same as passing nr_cpu_ids
for cpumask_any_but(), and it should trigger warning in both cases.
(Or should not, but it's a different story.)

> calling code which seems like unnecessary boiler plate
> 
> For instance, I trigger the cpumask_check() warning there:
> 
> 3d2dcab932d0:block/blk-mq.c @l2047
>         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);
>                 hctx->next_cpu_batch = BLK_MQ_CPU_WORK_BATCH;
>         }
> 
> next_cpu is a valid CPU number, shifting it doesn't seem to make sense, and
> we do want it to reach nr_cpu_ids-1.

next_cpu is a valid CPU number for all, but not for cpumask_next().
The warning is valid. If we are at the very last cpu, what for we look
for next?

The snippet above should be fixed like this:

          if (--hctx->next_cpu_batch <= 0) {
  select_cpu:
                  if (next_cpu == nr_cpu_ids - 1)
                          next_cpu = nr_cpu_ids;
                  else
                          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);
                  hctx->next_cpu_batch = BLK_MQ_CPU_WORK_BATCH;
          }

The original motivation for this special shifted semantics was to
avoid passing '+1' in cpumask_next() everywhere where it's used to
iterate over cpumask. This is especially ugly because it brings negative
semantics in such a simple thing like an index, and makes people confused.
It was a bad decision, but now it's so broadly used that we have to live
with it.

The strategy to mitigate this is to minimize using of that 'special'
functions. They all are cpumask_next()-like. In this series I reworked
for_each_cpu() to not use cpumask_next().

Often, cpumask_next() is a part of opencoded for_each_cpu(), and this
is relatively easy to fix. In case of blk_mq_hctx_next_cpu() that you
mentioned above, cpumask_next_and() usage looks unavoidable, and
there's nothing to do with that, except that being careful.

It didn't trigger the warning in my test setup, so I didn't fix it.
Feel free to submit a patch, if you observe the warning for yourself.

Maybe we should consider nr_cpu_ids as a special valid index for
cpumask_check(), a sign of the end of an array. This would help to
silence many warnings, like this one. For now I'm leaning towards that
it's more a hack than a meaningful change. 

Thanks,
Yury