Page MenuHomeFreeBSD

subr_smp: Clean up topology analysis, add additional layers

Authored by cem on Aug 14 2017, 6:33 AM.
Referenced Files
Unknown Object (File)
Tue, Nov 21, 1:43 PM
Unknown Object (File)
Mon, Nov 13, 11:21 AM
Unknown Object (File)
Oct 25 2023, 5:34 PM
Unknown Object (File)
Oct 20 2023, 7:58 AM
Unknown Object (File)
Oct 20 2023, 3:19 AM
Unknown Object (File)
Oct 17 2023, 7:08 AM
Unknown Object (File)
Oct 14 2023, 5:49 AM
Unknown Object (File)
Oct 11 2023, 7:40 PM



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

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

Test Plan

This patch depends on the (uncommitted) work done in to actually produce a result on a Zen

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.)

Diff Detail

rS FreeBSD src repository - subversion
Lint Not Applicable
Tests Not Applicable

Event Timeline

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
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 marked an inline comment as done.Aug 18 2017, 8:35 PM
cem added inline comments.
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 marked 2 inline comments as done.

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

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.

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:
I think the macro is fine though.

This revision is now accepted and ready to land.Aug 18 2017, 9:42 PM
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.

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 edited edge metadata.
cem marked 2 inline comments as done.

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.

This revision now requires review to proceed.Aug 20 2017, 5:22 PM
markj added inline comments.
1069 ↗(On Diff #32268)

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

138 ↗(On Diff #32268)

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

This revision is now accepted and ready to land.Aug 21 2017, 11:37 PM
1069 ↗(On Diff #32268)


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 edited edge metadata.

counter -> count

This revision now requires review to proceed.Aug 21 2017, 11:42 PM
138 ↗(On Diff #32268)

"entities" is fine with me.

This revision was automatically updated to reflect the committed changes.