Question about num_possible_cpus() and cpu_possible_mask

Question about num_possible_cpus() and cpu_possible_mask

[Michael Kelley]

Question:  Is there any intention to guarantee that the cpu_possible_mask is
"dense", in that all bit positions 0 thru (nr_cpu_ids - 1) are set, with no
"holes"? If that were true, then num_possible_cpus() would be equal to
nr_cpu_ids.

x86 always sets up cpu_possible_mask as dense, as does ARM64 with ACPI.
But it appears there are errors cases on ARM64 with DeviceTree where this
is not the case. I haven't looked at other architectures.

There's evidence both ways:
1) A somewhat recent report[1] on SPARC where cpu_possible_mask
   isn't dense, and there's code assuming that it is dense. This report
   got me thinking about the question.
  
2) setup_nr_cpu_ids() in kernel/smp.c is coded to *not* assume it is dense

3) But there are several places throughout the kernel that do something like
   the following, which assumes they are dense:

	array = kcalloc(num_possible_cpus(), sizeof(<some struct>), GFP_KERNEL);
	....
	index into "array" with smp_processor_id()

On balance, I'm assuming that there's no requirement for cpu_possible_mask
to be dense, and code like #3 above is technically wrong. It should be
using nr_cpu_ids instead of num_possible_cpus(), which is also faster.
We get away with it 99.99% of the time because all (or almost all?)
architectures populate cpu_possible_mask as dense.

There are 6 places in Hyper-V specific code that do #3. And it works because
Hyper-V code only runs on x86 and ARM64 where cpu_possible_mask is
always dense. But in the interest of correctness and robustness against
future changes, I'm planning to fix the Hyper-V code.

There are also a few other places throughout the kernel with the same
problem, and I may look at fixing those as well.

Or maybe my assumptions are off-base. Any thoughts or guidance before
I start submitting patches?

Thanks,

Michael

[1] https://lore.kernel.org/lkml/20240621033005.6mccm7waduelb4m5@oppo.com/

Re: Question about num_possible_cpus() and cpu_possible_mask

[Peter Zijlstra]

On Wed, Sep 25, 2024 at 04:04:33AM +0000, Michael Kelley wrote:
> Question:  Is there any intention to guarantee that the cpu_possible_mask is
> "dense", in that all bit positions 0 thru (nr_cpu_ids - 1) are set, with no
> "holes"? If that were true, then num_possible_cpus() would be equal to
> nr_cpu_ids.

I think we've historically had machines where there were holes in. And I
think we're wanting to have holes in for modern hybrid x86 that have HT,
although I'm not entirely sure where those patches are atm.

Thomas, didn't we have a patch that renumbers CPUs for hybrid crud sich
that HT is always the low bit and we end up with holes because the atoms
don't have HT on?

Or was that on my plate and it got lost in the giant todo pile?

> x86 always sets up cpu_possible_mask as dense, as does ARM64 with ACPI.
> But it appears there are errors cases on ARM64 with DeviceTree where this
> is not the case. I haven't looked at other architectures.
> 
> There's evidence both ways:
> 1) A somewhat recent report[1] on SPARC where cpu_possible_mask
>    isn't dense, and there's code assuming that it is dense. This report
>    got me thinking about the question.
>   
> 2) setup_nr_cpu_ids() in kernel/smp.c is coded to *not* assume it is dense
> 
> 3) But there are several places throughout the kernel that do something like
>    the following, which assumes they are dense:
> 
> 	array = kcalloc(num_possible_cpus(), sizeof(<some struct>), GFP_KERNEL);
> 	....
> 	index into "array" with smp_processor_id()

I would consider this pattern broken.

> On balance, I'm assuming that there's no requirement for cpu_possible_mask
> to be dense, and code like #3 above is technically wrong. It should be
> using nr_cpu_ids instead of num_possible_cpus(), which is also faster.
> We get away with it 99.99% of the time because all (or almost all?)
> architectures populate cpu_possible_mask as dense.
> 
> There are 6 places in Hyper-V specific code that do #3. And it works because
> Hyper-V code only runs on x86 and ARM64 where cpu_possible_mask is
> always dense. But in the interest of correctness and robustness against
> future changes, I'm planning to fix the Hyper-V code.
> 
> There are also a few other places throughout the kernel with the same
> problem, and I may look at fixing those as well.
> 
> Or maybe my assumptions are off-base. Any thoughts or guidance before
> I start submitting patches?

You're on the right track, should not assume the mask is dense.

Re: Question about num_possible_cpus() and cpu_possible_mask

[Thomas Gleixner]

On Mon, Sep 30 2024 at 09:56, Peter Zijlstra wrote:
> On Wed, Sep 25, 2024 at 04:04:33AM +0000, Michael Kelley wrote:
>> Question:  Is there any intention to guarantee that the cpu_possible_mask is
>> "dense", in that all bit positions 0 thru (nr_cpu_ids - 1) are set, with no
>> "holes"? If that were true, then num_possible_cpus() would be equal to
>> nr_cpu_ids.
>
> I think we've historically had machines where there were holes in. And I
> think we're wanting to have holes in for modern hybrid x86 that have HT,
> although I'm not entirely sure where those patches are atm.
>
> Thomas, didn't we have a patch that renumbers CPUs for hybrid crud sich
> that HT is always the low bit and we end up with holes because the atoms
> don't have HT on?
>
> Or was that on my plate and it got lost in the giant todo pile?

We talked about it some time ago, but that went nowhere.

Thanks,

        tglx

RE: Question about num_possible_cpus() and cpu_possible_mask

[Michael Kelley]

From: Peter Zijlstra <peterz@infradead.org> Sent: Monday, September 30, 2024 12:56 AM
> 
> On Wed, Sep 25, 2024 at 04:04:33AM +0000, Michael Kelley wrote:
> > Question:  Is there any intention to guarantee that the cpu_possible_mask is
> > "dense", in that all bit positions 0 thru (nr_cpu_ids - 1) are set, with no
> > "holes"? If that were true, then num_possible_cpus() would be equal to
> > nr_cpu_ids.
> 
> I think we've historically had machines where there were holes in. And I
> think we're wanting to have holes in for modern hybrid x86 that have HT,
> although I'm not entirely sure where those patches are atm.
> 
> Thomas, didn't we have a patch that renumbers CPUs for hybrid crud sich
> that HT is always the low bit and we end up with holes because the atoms
> don't have HT on?
> 
> Or was that on my plate and it got lost in the giant todo pile?
> 
> > x86 always sets up cpu_possible_mask as dense, as does ARM64 with ACPI.
> > But it appears there are errors cases on ARM64 with DeviceTree where this
> > is not the case. I haven't looked at other architectures.
> >
> > There's evidence both ways:
> > 1) A somewhat recent report[1] on SPARC where cpu_possible_mask
> >    isn't dense, and there's code assuming that it is dense. This report
> >    got me thinking about the question.
> >
> > 2) setup_nr_cpu_ids() in kernel/smp.c is coded to *not* assume it is dense
> >
> > 3) But there are several places throughout the kernel that do something like
> >    the following, which assumes they are dense:
> >
> > 	array = kcalloc(num_possible_cpus(), sizeof(<some struct>), GFP_KERNEL);
> > 	....
> > 	index into "array" with smp_processor_id()
> 
> I would consider this pattern broken.
> 
> > On balance, I'm assuming that there's no requirement for cpu_possible_mask
> > to be dense, and code like #3 above is technically wrong. It should be
> > using nr_cpu_ids instead of num_possible_cpus(), which is also faster.
> > We get away with it 99.99% of the time because all (or almost all?)
> > architectures populate cpu_possible_mask as dense.
> >
> > There are 6 places in Hyper-V specific code that do #3. And it works because
> > Hyper-V code only runs on x86 and ARM64 where cpu_possible_mask is
> > always dense. But in the interest of correctness and robustness against
> > future changes, I'm planning to fix the Hyper-V code.
> >
> > There are also a few other places throughout the kernel with the same
> > problem, and I may look at fixing those as well.
> >
> > Or maybe my assumptions are off-base. Any thoughts or guidance before
> > I start submitting patches?
> 
> You're on the right track, should not assume the mask is dense.

Thanks Peter and Mark for your confirmation. I'll work on some patches.

Michael

Re: Question about num_possible_cpus() and cpu_possible_mask

[Mark Rutland]

On Wed, Sep 25, 2024 at 04:04:33AM +0000, Michael Kelley wrote:
> Question:  Is there any intention to guarantee that the cpu_possible_mask is
> "dense", in that all bit positions 0 thru (nr_cpu_ids - 1) are set, with no
> "holes"? If that were true, then num_possible_cpus() would be equal to
> nr_cpu_ids.
> 
> x86 always sets up cpu_possible_mask as dense, as does ARM64 with ACPI.
> But it appears there are errors cases on ARM64 with DeviceTree where this
> is not the case. I haven't looked at other architectures.
> 
> There's evidence both ways:
> 1) A somewhat recent report[1] on SPARC where cpu_possible_mask
>    isn't dense, and there's code assuming that it is dense. This report
>    got me thinking about the question.
>   
> 2) setup_nr_cpu_ids() in kernel/smp.c is coded to *not* assume it is dense
> 
> 3) But there are several places throughout the kernel that do something like
>    the following, which assumes they are dense:
> 
> 	array = kcalloc(num_possible_cpus(), sizeof(<some struct>), GFP_KERNEL);
> 	....
> 	index into "array" with smp_processor_id()
> 
> On balance, I'm assuming that there's no requirement for cpu_possible_mask
> to be dense, and code like #3 above is technically wrong. It should be
> using nr_cpu_ids instead of num_possible_cpus(), which is also faster.
> We get away with it 99.99% of the time because all (or almost all?)
> architectures populate cpu_possible_mask as dense.
> 
> There are 6 places in Hyper-V specific code that do #3. And it works because
> Hyper-V code only runs on x86 and ARM64 where cpu_possible_mask is
> always dense.

Maybe that happens be be true under Hyper-V, but in general
cpu_possible_mask is not always dense on arm64, and we've had the change
core code to handle that in the past, e.g.

  bc75e99983df1efd ("rcu: Correctly handle sparse possible cpu")

> But in the interest of correctness and robustness against
> future changes, I'm planning to fix the Hyper-V code.

To me, that sounds like the right thing to do.

Mark.