sys/pcpu.h: Implement IS_BSP() for all arches, provide BSP_CPUID()

[olce]

Summary

BSP_CPUID() is defined in a separate machine header (<machine/_smp.h>)
because defining it in <machine/smp.h> and then including the latter
from <sys/pcpu.h> causes circular header dependencies in
machine-dependent headers (for at least amd64 and powerpc64).

[olce]

• Improve formatting in sys/powerpc/include/_smp.h

[markj]

Where is the include loop exactly? I can't see it. IMO we should try to fix that instead of introducing a new header.

Why do we need IS_BSP() on other platforms?

sys/arm/include/_smp.h

5

The copyright statement is supposed to come first, see https://docs.freebsd.org/en/articles/committers-guide/#pref-license

[olce]

In D60377#1385060, @markj wrote:

Where is the include loop exactly? I can't see it. IMO we should try to fix that instead of introducing a new header.

This is exactly what I usually do, but here I saw this stack:

--- genoffset_test.o ---                                                                                                                                                                                                                                                                                                                                                            
In file included from /usr/src-CURRENT-2/sys/kern/genoffset.c:34:                                                                                                                                                                                                                                                                                                                   
In file included from /usr/src-CURRENT-2/sys/sys/proc.h:64:                                                                                                                                                                                                                                                                                                                         
In file included from /usr/src-CURRENT-2/sys/sys/pcpu.h:48:                                                                                                                                                                                                                                                                                                                         
In file included from ./machine/pcpu.h:38:                                                                                                                                                                                                                                                                                                                                          
In file included from ./machine/smp.h:18:                                                                                                                                                                                                                                                                                                                                           
In file included from ./x86/x86_smp.h:16:                                                                                                                                                                                                                                                                                                                                           
In file included from ./x86/apicvar.h:31:                                                                                                                                                                                                                                                                                                                                           
In file included from /usr/src-CURRENT-2/sys/sys/bus.h:168:                                                                                                                                                                                                                                                                                                                         
In file included from /usr/src-CURRENT-2/sys/sys/systm.h:100:                                                                                                                                                                                                                                                                                                                       
/usr/src-CURRENT-2/sys/sys/kpilite.h:37:33: error: use of undeclared identifier 'curthread'                                                                                                                                                                                                                                                                                         
   37 |         KASSERT((struct thread *)td == curthread, ("sched_pin called on non curthread"));                                                                                                                                                                                                                                                                                   
      |                                        ^                                                                                                                                                                                                                                                                                                                                    
/usr/src-CURRENT-2/sys/sys/kpilite.h:46:33: error: use of undeclared identifier 'curthread'                                                                                                                                                                                                                                                                                         
   46 |         KASSERT((struct thread *)td == curthread, ("sched_unpin called on non curthread"));                                                                                                                                                                                                                                                                                 
      |                                        ^

showing a circular dependency involving proc.h, which is why I wasn't particular eager to try to resolve it initially.

Now that you report you don't see it, I realize why: It's because I have another (unpublished) commit that adds including <sys/bus.h> in '<x86/apicvar.h>, which I did because the latter needs the definitions of enum intr_trigger` and enum intr_polarity.

Let's see how easy it is to make <x86/apicvar.h> standalone without including <sys/bus.h>, which hopefully will be enough to break the cycle according to your report.

Why do we need IS_BSP() on other platforms?

To stop relying on the wrong assumption that CPUID 0 is the BSP when specifically wanting to run on the BSP, e.g., in ACPI for shutdown/suspend.

sys/arm/include/_smp.h

5

I just copied my Foundation contract's template, and having the copyright second is "acceptable" as we both know. But yes, I'll change it if these headers finally stay.

[kib]

sys/powerpc/include/_smp.h

14

The '_name.h' headers are supposed to be absolutely minimal.
In particular, bringing in whole kassert.h is somewhat too much.

You either could rely on external inclusion of kassert.h or do something like

#ifdef _SYS_KASSERT_H_
<your def>
#else
#define BSP_CPUID() (powerpc_bsp_cpuid)
#endif

[olce]

In D60377#1385284, @olce wrote:

Let's see how easy it is to make <x86/apicvar.h> standalone without including <sys/bus.h>, which hopefully will be enough to break the cycle according to your report.

That part is relatively easy (leading to the creation of a <sys/_bus.h> though), but unfortunately it does not solve the main problem, which is that if BSP_CPUID() is put in <machine/smp.h> (as it logically should) and <sys/pcpu.h> includes the latter as a dependency of its IS_BSP() definition, all sorts of problems arise.

There's a circular dependency again with curthread, fixable by moving its definition in <sys/pcpu.h>, but then there are collisions between some of the definitions of <x86/x86_smp.h> and those of several files, including acpi_pxm.c (struct cpu_info and cpu_add()), sys/x86/cpufreq/est.c (struct cpu_info), sys/contrib/openzfs/module/os/freebsd/zfs/dmu_os.c (dr_data)...

So the <machine/_smp.h> approach looks much more palatable. That header is reduced to the minimum, and could be included virtually everywhere (including all files relying on <sys/pcpu.h>, including <sys/systm.h>) without inducing collisions, whereas <machine/smp.h> remains reserved to specific cases as it causes much more pollution.

Do you see some workable alternative to this? (well, the simplest alternative is certainly just not try to make headers standalone and relying on inclusion order in this case, but I'd like to move away from it and have the impression that doing so is the general direction we want to go into)

sys/powerpc/include/_smp.h

14

The '_name.h' headers are supposed to be absolutely minimal.

I agree.

In particular, bringing in whole kassert.h is somewhat too much.

It's relatively minimal already (_types.h, cdefs.h), and we should strive to keep it like that.

If you fear that this may render evolutions of kassert.h more difficult, that inclusion can be simply removed as a quick way out at that moment (although we should preferably avoid that).

You either could rely on external inclusion of kassert.h or do something like (snip)

Out of the two, I prefer to rely on prior kassert.h inclusion (else you sometimes get the assertion, sometimes not, and you're not sure when).

But I'd really prefer that headers are made standalone, as much as possible.

[olce]

• Change order of copyright and SPDX tag.

[olce]

Sorry, withdrawing temporarily again, it looks like there is only a single PowerPC platform that does not have the BSP == CPUID 0 invariant, so going to propose a fix for that, which should lead to a lot of simplification of this revision.