Messages in this thread | | | Date | Sat, 4 Mar 2023 12:51:30 -0800 | From | Yury Norov <> | Subject | Re: [PATCH v3 2/2] vfs: avoid duplicating creds in faccessat if possible |
| |
On Sat, Mar 04, 2023 at 11:19:54AM -0800, Linus Torvalds wrote: > On Fri, Mar 3, 2023 at 9:51 PM Yury Norov <yury.norov@gmail.com> wrote: > > > > 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
| |