On Sat, Mar 04, 2023 at 11:19:54AM -0800, Linus Torvalds wrote:

On Fri, Mar 3, 2023 at 9:51 PM Yury Norov [off-list ref] wrote:

quoted

And the following code will be broken:

cpumask_t m1, m2;

cpumask_setall(m1); // m1 is ffff ffff ffff ffff because it uses
                    // compile-time optimized nr_cpumask_bits

for_each_cpu(cpu, m1) // 32 iterations because it relied on nr_cpu_ids
        cpumask_set_cpu(cpu, m2); // m2 is ffff ffff XXXX XXXX

So  honestly, it looks like you picked an example of something very
unusual to then make everything else slower.

What about bootable sticks?
 

Rather than commit aa47a7c215e7, we should just have fixed 'setall()'
and 'for_each_cpu()' to use nr_cpu_ids, and then the rest would
continue to use nr_cpumask_bits.

No, that wouldn't work for all.

That particular code sequence is arguably broken to begin with.
setall() should really only be used as a mask, most definitely not as
some kind of "all possible cpus".

Sorry, don't understand this.

The latter is "cpu_possible_mask", which is very different indeed (and
often what you want is "cpu_online_mask")

Don't understand this possible vs online argument, but OK. What about this?

        if (cpumask_setall_is_fixed)
                cpumask_setall(mask);
        else {
                for_each_online_cpu(cpu)
                        cpumask_set_cpu(cpu, mask);
             }

        // You forgot to 'fix' cpumask_equal()
        BUG_ON(!cpumask_equal(cpu_online_mask, mask));

Or this:

        cpumask_clear(mask);
        for_each_cpu_not(cpu, mask)
                cpumask_set_cpu(cpu, mask);
        BUG_ON(!cpumask_full(mask));

The root of the problem is that half of cpumask API relies (relied) on
compile-time nr_cpumask_bits, and the other - on boot-time nr_cpu_ids.

So, if you consistently propagate your 'fix', you'll get rid of
nr_cpumask_bits entirely with all that associate overhead.

That's what I actually did. And even tried to minimize the
overhead the best way I can think of.

But I'd certainly be ok with using nr_cpu_ids for setall, partly
exactly because it's so rare. It would probably be better to remove it
entirely, but whatever.

              Linus