client/fingerprint: correctly fingerprint E/P cores of Apple Silicon chips #16672

shoenig · 2023-03-27T21:25:46Z

This PR adds detection of asymetric core types (Power (P) & Efficiency (E))
when running on M1/M2 Apple Silicon CPUs. This functionality is provided
by shoenig/go-m1cpu which makes use of the Apple IOKit framework to read
undocumented registers containing CPU performance data. Currently working
on getting that functionality merged upstream into gopsutil, but gopsutil
would still not support detecting P vs E cores like this PR does.

Also refactors the CPUFingerprinter code to handle the mixed core
types, now setting power vs efficiency cpu attributes.

For now the scheduler is still unaware of mixed core types - on Apple
platforms tasks cannot reserve cores anyway so it doesn't matter, but
at least now the total CPU shares available will be correct.

Future work should include adding support for detecting P/E cores on
the latest and upcoming Intel chips, where computation of total cpu shares
is currently incorrect. For that, we should also include updating the
scheduler to be core-type aware, so that tasks making use of resources.cores
on Linux platforms can be assigned the correct number of CPU shares for the core
type(s) they have been assigned.

node attributes before

cpu.arch                  = arm64
cpu.modelname             = Apple M2 Pro
cpu.numcores              = 12
cpu.reservablecores       = 0
cpu.totalcompute          = 1000

node attributes after

cpu.arch                  = arm64
cpu.frequency.efficiency  = 2424
cpu.frequency.power       = 3504
cpu.modelname             = Apple M2 Pro
cpu.numcores.efficiency   = 4
cpu.numcores.power        = 8
cpu.reservablecores       = 0
cpu.totalcompute          = 37728

Fixes #12029
Fixes #11046
Fixes #9408

…chips This PR adds detection of asymetric core types (Power & Efficiency) (P/E) when running on M1/M2 Apple Silicon CPUs. This functionality is provided by shoenig/go-m1cpu which makes use of the Apple IOKit framework to read undocumented registers containing CPU performance data. Currently working on getting that functionality merged upstream into gopsutil, but gopsutil would still not support detecting P vs E cores like this PR does. Also refactors the CPUFingerprinter code to handle the mixed core types, now setting power vs efficiency cpu attributes. For now the scheduler is still unaware of mixed core types - on Apple platforms tasks cannot reserve cores anyway so it doesn't matter, but at least now the total CPU shares available will be correct. Future work should include adding support for detecting P/E cores on the latest and upcoming Intel chips, where computation of total cpu shares is currently incorrect. For that, we should also include updating the scheduler to be core-type aware, so that tasks of resources.cores on Linux platforms can be assigned the correct number of CPU shares for the core type(s) they have been assigned. node attributes before cpu.arch = arm64 cpu.modelname = Apple M2 Pro cpu.numcores = 12 cpu.reservablecores = 0 cpu.totalcompute = 1000 node attributes after cpu.arch = arm64 cpu.frequency.efficiency = 2424 cpu.frequency.power = 3504 cpu.modelname = Apple M2 Pro cpu.numcores.efficiency = 4 cpu.numcores.power = 8 cpu.reservablecores = 0 cpu.totalcompute = 37728

lgfa29

Mostly questions, but LGTM. Just missing a CHANGELOG entry 🙂

Smoke test on my machine:

Attributes
cpu.arch                  = arm64
cpu.frequency.efficiency  = 2064
cpu.frequency.power       = 3228
cpu.modelname             = Apple M1 Max
cpu.numcores.efficiency   = 2
cpu.numcores.power        = 8
cpu.reservablecores       = 0
cpu.totalcompute          = 29952

lgfa29 · 2023-03-28T00:53:17Z

helper/stats/cpu.go

-		}
+			var err error
+			if cpuPowerCoreCount, err = cpu.Counts(true); err != nil {
+				initErr = errors.Join(initErr, fmt.Errorf("failed to detect number of CPU cores: %v", err))


Suggested change

initErr = errors.Join(initErr, fmt.Errorf("failed to detect number of CPU cores: %v", err))

initErr = errors.Join(initErr, fmt.Errorf("failed to detect number of CPU cores: %w", err))

I think this let us unwrap the inner error? I'm still getting used to the new errors stuff 😅

lgfa29 · 2023-03-28T00:53:48Z

helper/stats/cpu.go

+			// detecting little cores on non-apple platforms
+			cpuTotalTicks = uint64(cpuPowerCoreCount) * cpuPowerCoreMHz
+
+			initErr = err


Is this overwriting the accumulated errors with just the last one?

this bit isn't in a loop - it's just setting the global initErr inside a sync.Do so that the cpu detection only happens once

But we write to initErr twice inside the default clause right?

nomad/helper/stats/cpu.go

Lines 60 to 62 in 2b28c83

if cpuInfoStats, err = cpu.InfoWithContext(ctx); err != nil {

initErr = errors.Join(initErr, fmt.Errorf("Unable to obtain CPU information: %w", err))

}

nomad/helper/stats/cpu.go

Lines 52 to 54 in 2b28c83

if cpuPowerCoreCount, err = cpu.Counts(true); err != nil {

initErr = errors.Join(initErr, fmt.Errorf("failed to detect number of CPU cores: %w", err))

}

If both fail I expect initErr to have two erros, but this initErr = err is making it so only the last error is returned.

lgfa29 · 2023-03-28T00:54:53Z

helper/stats/cpu.go

+
+			var cpuInfo []cpu.InfoStat
+			if cpuInfo, err = cpu.InfoWithContext(ctx); err != nil {
+				errors.Join(err, fmt.Errorf("failed to detect CPU information: %v", err))


Is the joined error being ignore? Or does it mutate err?

// Join returns an error that wraps the given errors.

So, I think we need to capture the returned error.

yeah @jrasell is right - this line actually does nothing because the result is returned from Join ... however this line is unnecessary anyway since the assignment to initErr already contains a contextual message

lgfa29 · 2023-03-28T00:59:15Z

client/fingerprint/cpu.go

+	logger hclog.Logger
+
+	// accumulates result in these resource structs
+	resources     *structs.Resources


Should we keep the COMPAT note in case do we clean this up in the future?

nomad/client/fingerprint/cpu.go

Lines 37 to 38 in c071791

// COMPAT(0.10): Remove in 0.10

resp.Resources = &structs.Resources{

jrasell

LGTM, only minor comments and as Luiz mentioned, missing changelog.

jrasell · 2023-03-28T07:32:50Z

client/fingerprint/cpu_default.go

@@ -3,6 +3,6 @@

 package fingerprint

-func (f *CPUFingerprint) deriveReservableCores(req *FingerprintRequest) ([]uint16, error) {
-	return nil, nil
+func (f *CPUFingerprint) deriveReservableCores(string) []uint16 {


nit: ignore the unused parameter

Suggested change

func (f *CPUFingerprint) deriveReservableCores(string) []uint16 {

func (f *CPUFingerprint) deriveReservableCores(_ string) []uint16 {

jrasell · 2023-03-28T07:38:09Z

helper/stats/cpu.go

+
+			var cpuInfo []cpu.InfoStat
+			if cpuInfo, err = cpu.InfoWithContext(ctx); err != nil {
+				errors.Join(err, fmt.Errorf("failed to detect CPU information: %v", err))


// Join returns an error that wraps the given errors.

So, I think we need to capture the returned error.

jrasell · 2023-03-28T07:38:46Z

helper/stats/cpu.go

-		// node to fall into a unique computed node class
-		cpuMhzPerCore = math.Floor(cpuMhzPerCore)
-		cpuTotalTicks = math.Floor(float64(cpuNumCores) * cpuMhzPerCore)
+			for _, cpu := range cpuInfo {


Can we rename the cpu variable, as this clashes with an import?

vercel bot deployed to Preview – nomad-storybook-and-ui March 27, 2023 21:30 View deployment

shoenig force-pushed the cpu-detect-apple-silicon branch from bac2ed4 to c95b13d Compare March 27, 2023 21:56

vercel bot deployed to Preview – nomad-storybook-and-ui March 27, 2023 22:00 View deployment

shoenig force-pushed the cpu-detect-apple-silicon branch from c95b13d to 3b83232 Compare March 27, 2023 22:07

vercel bot deployed to Preview – nomad-storybook-and-ui March 27, 2023 22:11 View deployment

shoenig force-pushed the cpu-detect-apple-silicon branch from 3b83232 to a5c121a Compare March 27, 2023 22:47

vercel bot deployed to Preview – nomad-storybook-and-ui March 27, 2023 22:51 View deployment

shoenig marked this pull request as ready for review March 27, 2023 23:16

shoenig requested review from lgfa29 and jrasell March 27, 2023 23:16

lgfa29 approved these changes Mar 28, 2023

View reviewed changes

jrasell approved these changes Mar 28, 2023

View reviewed changes

fingerprint/cpu: follow up cr items

2b28c83

vercel bot deployed to Preview – nomad-storybook-and-ui March 28, 2023 13:14 View deployment

shoenig added the backport/1.5.x backport to 1.5.x release line label Mar 28, 2023

shoenig merged commit fd900d0 into hashicorp:main Mar 28, 2023

hc-github-team-nomad-core mentioned this pull request Mar 28, 2023

Backport of client/fingerprint: correctly fingerprint E/P cores of Apple Silicon chips into release/1.5.x #16690

Merged

ZhongRuoyu mentioned this pull request Apr 4, 2023

Error when trying to build nomad master on apple M2 shoenig/go-m1cpu#5

Closed

tgross mentioned this pull request Jun 21, 2023

Use cpu_total_compute configuration for CPU usage too #17628

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client/fingerprint: correctly fingerprint E/P cores of Apple Silicon chips #16672

client/fingerprint: correctly fingerprint E/P cores of Apple Silicon chips #16672

shoenig commented Mar 27, 2023 •

edited

Loading

lgfa29 left a comment

lgfa29 Mar 28, 2023

shoenig Mar 28, 2023

lgfa29 Mar 28, 2023

shoenig Mar 28, 2023

lgfa29 Mar 28, 2023

lgfa29 Mar 28, 2023

jrasell Mar 28, 2023

shoenig Mar 28, 2023

lgfa29 Mar 28, 2023

shoenig Mar 28, 2023

jrasell left a comment

jrasell Mar 28, 2023

jrasell Mar 28, 2023

jrasell Mar 28, 2023

shoenig Mar 28, 2023

	initErr = errors.Join(initErr, fmt.Errorf("failed to detect number of CPU cores: %v", err))
	initErr = errors.Join(initErr, fmt.Errorf("failed to detect number of CPU cores: %w", err))

	if cpuInfoStats, err = cpu.InfoWithContext(ctx); err != nil {
	initErr = errors.Join(initErr, fmt.Errorf("Unable to obtain CPU information: %w", err))
	}

	if cpuPowerCoreCount, err = cpu.Counts(true); err != nil {
	initErr = errors.Join(initErr, fmt.Errorf("failed to detect number of CPU cores: %w", err))
	}

	// COMPAT(0.10): Remove in 0.10
	resp.Resources = &structs.Resources{

	func (f *CPUFingerprint) deriveReservableCores(string) []uint16 {
	func (f *CPUFingerprint) deriveReservableCores(_ string) []uint16 {

client/fingerprint: correctly fingerprint E/P cores of Apple Silicon chips #16672

client/fingerprint: correctly fingerprint E/P cores of Apple Silicon chips #16672

Conversation

shoenig commented Mar 27, 2023 • edited Loading

lgfa29 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrasell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shoenig commented Mar 27, 2023 •

edited

Loading