Page MenuHomeFreeBSD

Expose threads-per-core and physical core count information
ClosedPublic

Authored by cem on Nov 24 2018, 6:15 AM.

Details

Summary

With new sysctls (to the best of our ability do detect them).

(The hw.physicalcpu name matches MacOS' sysctl of the same concept.)

Restructured smp.4 slightly for clarity while documenting.

Made the topo_root global and MI, although only x86 fills it in for now, in
order to use its information for the sysctls.

Test Plan

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

cem created this revision.Nov 24 2018, 6:15 AM
markj added inline comments.Dec 5 2018, 5:18 PM
sys/kern/subr_smp.c
105 ↗(On Diff #51037)

"smt_threads_online" sounds like a total count of hardware threads. If you're open to other suggestions, "threads_per_core" seems more intuitive to me.

109 ↗(On Diff #51037)

It seems odd to me to name a single sysctl this way because MacOS does it. kern.smp.cores is consistent with FreeBSD naming, and OneFS already has it IIRC. hw.physicalcpu could be made an alias of kern.smp.cores, but I think the latter should exist.

187 ↗(On Diff #51037)

Style: missing period.

196 ↗(On Diff #51037)

Under what circumstances can this cause mp_ncpus and smp_cores to have different values?

cem marked 4 inline comments as done.Dec 6 2018, 3:28 AM
cem added inline comments.
sys/kern/subr_smp.c
105 ↗(On Diff #51037)

Sure, I'd be happy to change this to threads_per_core.

109 ↗(On Diff #51037)

Yeah, I forgot OneFS had something like this. I'd be happy to make the default kern.smp.cores, and just add hw.physicalcpu as an OS-X compat shim/alias.

187 ↗(On Diff #51037)

Will fix

196 ↗(On Diff #51037)

Well, mp_ncpus includes hyperthreads, while [the goal] smp_cores doesn't. So on a HTT or SMT system, these will differ by the HTT/SMT factor.

In practice, there is a bug here in that the CORE count needs to be multiplied by higher level topology factors. E.g.,

FreeBSD/SMP: Multiprocessor System Detected: 32 CPUs
FreeBSD/SMP: 1 package(s) x 2 groups x 2 cache groups x 4 core(s) x 2 hardware threads
#                                                       ^^^^^^^^^
...
kern.smp.cpus: 32
hw.ncpu: 32
kern.smp.smt_threads_online: 2
# ^^^ so far so good
...
hw.physicalcpu: 4
# ^^^ wrong; that's cores per cache group.
# Needs to be multiplied by 2 cache groups x 2 groups x 1 package to get
# the correct value, 16.
markj added inline comments.Dec 6 2018, 4:18 PM
sys/kern/subr_smp.c
109 ↗(On Diff #51037)

Why add a compat shim for this one sysctl?

196 ↗(On Diff #51037)

I see. Taking a step back, though, the code above will only compile on i386 and amd64, since no other platforms provide topo_analyze(). This might be a signal that smp_ncores should be set in machdep code. What other platforms besides powerpc support SMT?

cem marked 2 inline comments as done.Dec 6 2018, 5:14 PM
cem added inline comments.
sys/kern/subr_smp.c
109 ↗(On Diff #51037)

As in, why add the alias sysctl at all? Or did I use the wrong terminology in calling a 2nd sysctl a compat shim? I'm picturing just a 2nd sysctl attached to the same variable.

As far as why — muscle memory / portability from MacOS. If someone writes a program that uses that sysctl there, it's one less porting barrier to FreeBSD. Ditto administrative use. I'll admit both of these are fairly weak reasons, and I'd be fine dropping the compatibility name.

196 ↗(On Diff #51037)

topo_analyze() is in kern/subr_smp.c — it's not MD code. It should compile everywhere. That's why I selected it, vs adding more MD code.

No other archs populate the topology tree that topo_analyze uses at this time, yes. They could, or they could just set the variables in MD code directly as a smaller step.

Re: other platforms besides x86 and ppc: Doesn't sparc have SMT? And apparently there are MIPS SMT implementations.

It might make sense to separate this into two commits

  • One that moves topo_root and cpu_topo_probed into MI code, so that the MI topo code is actually usable in MI code (no functional change);
  • And independent of that, this logical change which adds the sysctls and populates the variables, perhaps in MD code, without using topo_analyze.
cem edited the test plan for this revision. (Show Details)Dec 6 2018, 5:15 PM
markj added inline comments.Dec 7 2018, 9:20 PM
sys/kern/subr_smp.c
109 ↗(On Diff #51037)

My personal bar for something like that would to have at least one concrete example where having had that sysctl on FreeBSD would have helped someone. In the absence of that, the addition of a compat sysctl is speculative and adds that much more bloat to the kernel.

196 ↗(On Diff #51037)

Got it, I got confused by topo_probe().

I would suggest trying to populate smp_cores in MD code, the same way smp_cpus is initialized.

cem updated this revision to Diff 52238.Dec 22 2018, 7:00 AM
cem marked 8 inline comments as done.
  • Drop topo_node stuff from this revision
  • Initialize mp_ncores, smt_threads_per_core in MD code where appropriate (x86, ppc)

I didn't see any SMT identification in our existing sparc or MIPS code -- we
may not support any of those models with that.

I don't believe arm(64) supports SMT, but please correct me if I am mistaken.

Builds on amd64, but kicked off a tinderbox to confirm.

+Justin for ppc changes.

cem updated this revision to Diff 52251.Dec 22 2018, 10:10 PM

Fix trivial 'oops': accidentally did not initialize nhyper in x86 MD code

PowerPC bits look fine to me.

markj accepted this revision.Jan 3 2019, 5:28 PM
markj added inline comments.
sys/x86/x86/mp_x86.c
607 ↗(On Diff #52251)

nthreads would be a better name. "Hyperthreading" is Intel's marketing name.

This revision is now accepted and ready to land.Jan 3 2019, 5:28 PM
cem added inline comments.Jan 4 2019, 6:16 AM
sys/x86/x86/mp_x86.c
607 ↗(On Diff #52251)

It's not counting all threads, though. Just CPUs that advertise the cpu_hyperthread flag. nhyper matches that name and use. I think nthreads would be less clear. n_logical_threads gets closer but becomes wordy.

This revision was automatically updated to reflect the committed changes.