subr_smp: Clean up topology analysis, add additional layers

[cem]

Summary

Separate concerns into individual routines with a single loop each. Handle
missing topology layers more gracefully (infer a single unit).

Analyze some additional optional layers which may be present on e.g. AMD Zen
systems (groups, aka dies, per package; and cachegroups, aka CCXes, per
group).

Display that additional information when it is relevent (non-one).

Test Plan

This patch depends on the (uncommitted) work done in
https://reviews.freebsd.org/D11801 to actually produce a result on a Zen
system.

Works fine on my Sandybridge laptop (boot -v):

Package ID shift: 4
L3 cache ID shift: 4
L2 cache ID shift: 1
L1 cache ID shift: 1
Core ID shift: 1
FreeBSD/SMP: Multiprocessor System Detected: 4 CPUs
FreeBSD/SMP: 1 package(s) x 2 core(s) x 2 hardware threads

(This matches prior behavior.)

[cem]

Fix two issues discovered in testing:

• Only look at last-level caches (assumed L3 is the LLC on all amd64) for cache groups
• For the infered single unit cases, pass down the root node untouched rather than automatically advancing to some other topology node

[markj]

sys/kern/subr_smp.c

1060 ↗

(On Diff #32035)

This comment is somewhat stale now.

1101 ↗

(On Diff #32035)

The amount of similarity between all of these functions bugs me. Have you tried using a "level" enum parameter to identify the current level of recursion? I think that'd let you eliminate most of the duplication without having too many special cases.

[cem]

sys/kern/subr_smp.c

1060 ↗

(On Diff #32035)

Fixed locally, will upload with the next patch.

1101 ↗

(On Diff #32035)

The conditionals vary from level to level, and treatment of "empty" levels vary. I think a macro could do it, but not a single function with a level parameter.

[cem]

Fix comment, and reduce explicit code duplication with a shared topology level
analysis macro.

[markj]

Looks ok to me, but I'm not very familiar with this code. I haven't hit any problems with it on a dual-package Xeon system.

sys/kern/subr_smp.c

1101 ↗

(On Diff #32035)

I slightly prefer something like this, if only because it avoids bloating the kernel text with code that gets executed once: https://people.freebsd.org/~markj/topo_analyze.c
I think the macro is fine though.

[mjoras]

mjoras@icarium ~> sysctl hw.model
hw.model: AMD Ryzen Threadripper 1950X 16-Core Processor

Original detection:

kernel: FreeBSD/SMP: Multiprocessor System Detected: 32 CPUs
kernel: FreeBSD/SMP: 1 package(s) x 16 core(s) x 2 hardware threads

New detection:

kernel: FreeBSD/SMP: Multiprocessor System Detected: 32 CPUs
kernel: FreeBSD/SMP: 1 package(s) x 2 groups x 2 cache groups x 4 core(s) x 2 hardware threads

Looks correct to me. I'm not really familiar with this code but the it seems sound enough, reading through the other review. I mildly prefer markj's suggested approach to the macros.

[kib]

sys/kern/subr_smp.c

1101 ↗

(On Diff #32035)

I also prefer your approach both for the text bloat and because large expanding macros are ugly to read. Or you might use small callback predicates functions instead of inlining the switch.

[cem]

Switch from the macro approach to a single table-driven recursive function.

I prefer having the conditional data codified in a separate table rather than
embedded in switch statements in the recursive function.

[markj]

sys/kern/subr_smp.c

1069 ↗

(On Diff #32268)

"count" would be a more consistent and idiomatic name.

sys/sys/smp.h

138 ↗

(On Diff #32268)

"levels" seems like an odd name - these fields are counting the number of entities at each level.

[cem]

sys/kern/subr_smp.c

1069 ↗

(On Diff #32268)

Ok.

sys/sys/smp.h

138 ↗

(On Diff #32268)

This is one of those "I don't care what color the bikeshed is." If you've got a better name, I'm happy to change it. "entities" or "units?"

[cem]

counter -> count

[markj]

sys/sys/smp.h

138 ↗

(On Diff #32268)

"entities" is fine with me.