Handling arm64: topology and online information #2744

iwankgb · 2020-11-29T22:09:34Z

Fixes #2743 (to some extent). Fixes #2783 too.

According to kernel docs CONFIG_HOTPLUG_CPU is the only way of disabling or enabling cpu hotplug. It should therefore be safe to assume, that if /sys/devices/system/cpu/cpuXXX/online file does not exist then hotplug is disabled.

The above might not be true on some systems, apparently. Raspberry Pi 4 (Cortex-A72) seems to be one such system.

Besides this:

GetMachineMemoryByType() and getUniqueCPUPropertyCount() have been moved to utils/sysfs where they belong (these functions read from sysfs).
all the edited files have been reformatted automatically to make linter happy.

iwankgb · 2020-12-28T19:27:20Z

@bobbypage it seems that this fix could make some folks in Kubernetes community merrier: kubernetes/kubernetes#95039. Might be worth being reviewed.

utils/sysfs/sysfs_arm64.go

bobbypage · 2020-12-30T08:18:51Z

Thanks, LGTM for changes here.

Only question/concern: the changes here basically make the assumption that arm64 does need to check cpu/cpuXXX/online at all; cpus are always online for arm64. Is this a reasonable assumption?

I don't know too much about cpu hotplug / arm64 support, but just wondering based on #2744 (comment), is something weird with Raspberry Pi 4 specifically? Just thinking that maybe there's some other ARM64 machines that do support cpu hotplug and might need the online check? LMK what you think, and if this is actually a real concern.

bobbypage · 2020-12-30T08:21:49Z

See also related in #2677 regarding if kernel complied with CONFIG_HOTPLUG_CPU=n

iwankgb · 2020-12-30T13:10:18Z

I looked at Graviton2 EC2 instance yesterday and it behaves as expected: online file is there. Perhaps better way to fix it would be to assume that lack of online file means lack of hotplug capabilities, irrespective of architecture.

bobbypage · 2020-12-31T08:39:42Z

Yeah, that's kinda what I was thinking, perhaps that's a better fix? WDYT?

iwankgb · 2020-12-31T09:27:02Z

It will require pretty extensive testing on various architectures and configurations. We should at least test:

Hotplug enabled on x86 and arm
Hotplug disabled on x86 and arm
Hyperthreading disabled on x86
hyperthreading enabled on x86
RPi that is weird an unusual

I will try to conduct arm testing on Graviton2 and perhaps some other arm that I will be able to get hold of. X86 should not be an issue.

My original intention was to avoid changes to x86 code - this is why I decided to limit changes to arm only.

iwankgb · 2021-01-04T22:05:10Z

utils/sysfs/sysfs.go

+// getOnlineStateID looks for line similar to `206: online` from /sys/devices/system/cpu/hotplug/states
+// and extracts number from the beginning.
+// See: https://www.kernel.org/doc/html/latest/core-api/cpu_hotplug.html#testing-of-hotplug-states
+func getOnlineStateID(cpuPath string) (int, error) {


I think that it would make sense to cache result of this function: it can't change when system is running, I believe. Do you think that using package-level variable to cache the output makes sense?

if it can't change, I guess doesn't hurt to cache it. Re using package level variable, I think it's fine, e.g. see this existing example: https://github.com/google/cadvisor/blob/master/machine/machine.go#L53

iwankgb · 2021-01-04T22:06:04Z

utils/sysfs/sysfs.go

+// isCPUOnline verifies if a CPU in online. Takes path to /sys/devices/system/cpu/cpu*/ directory as a parameter.
+func isCPUOnline(cpuPath string) bool {
+	if isx86() && isZeroCPU(cpuPath) {
+		// It *is* possible to enable CPU hotplug for CPU0 but it requires setting a boot flag.


I believe it is safe, but I can't promise it. Do you think we should check kernel boot params?

is there any reason we need special logic for cpu0?

or actually just looking at my 32 core ubuntu GCE VM:

root@ubuntu-big:~# find /sys/devices/system/cpu/ | grep "online" /sys/devices/system/cpu/cpu21/online /sys/devices/system/cpu/cpu11/online /sys/devices/system/cpu/cpu9/online /sys/devices/system/cpu/cpu7/online /sys/devices/system/cpu/cpu28/online /sys/devices/system/cpu/cpu18/online /sys/devices/system/cpu/cpu5/online /sys/devices/system/cpu/cpu26/online /sys/devices/system/cpu/cpu16/online /sys/devices/system/cpu/cpu3/online /sys/devices/system/cpu/cpu24/online /sys/devices/system/cpu/online /sys/devices/system/cpu/cpu14/online /sys/devices/system/cpu/cpu1/online /sys/devices/system/cpu/cpu22/online /sys/devices/system/cpu/cpu12/online /sys/devices/system/cpu/cpu30/online /sys/devices/system/cpu/cpu20/online /sys/devices/system/cpu/cpu10/online /sys/devices/system/cpu/cpu8/online /sys/devices/system/cpu/cpu29/online /sys/devices/system/cpu/cpu19/online /sys/devices/system/cpu/cpu6/online /sys/devices/system/cpu/cpu27/online /sys/devices/system/cpu/cpu17/online /sys/devices/system/cpu/cpu4/online /sys/devices/system/cpu/cpu25/online /sys/devices/system/cpu/cpu15/online /sys/devices/system/cpu/cpu2/online /sys/devices/system/cpu/cpu23/online /sys/devices/system/cpu/cpu13/online /sys/devices/system/cpu/cpu31/online

cpu0 online doesn't exist. is it why this case is needed?

In general you should not put cpu0 offline on x86 because it is used to boot the operating system. If it's unavailable then you might not be able to start your system. I am extremely surprised by what you shared about your VM: can you share your kernel boot params? Perhaps it's a case of disabled cpu0 and I bet you haven't altered your configuration.

This is just a standard ubuntu GCE VM, created as follows

$ gcloud beta compute instances create ubuntu-big --machine-type=e2-standard-32 --image-family=ubuntu-2004-lts --image-project=ubuntu-os-cloud --boot-disk-size=256GB --boot-disk-type=pd-ssd --zone=us-central1-c

$ uname -a Linux ubuntu-big 5.4.0-1029-gcp #31-Ubuntu SMP Wed Oct 21 19:38:01 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux

Here's KConfig from

sudo cat /boot/config-$(uname -r)

https://gist.github.com/bobbypage/3e65f6a6ad649f082760b9674bd02c5b

iwankgb · 2021-01-04T22:30:15Z

@bobbypage I have to tested all configurations yet, but I'm confident that code works on RPi4 and Graviton2. Can you take a look and let me know if this approach is fine with you? I will proceed with further testing then.

bobbypage · 2021-01-06T06:13:35Z

utils/sysfs/sysfs.go

+	return isCPUOnline(cpuPath)
+}
+
+// isCPUOnline verifies if a CPU in online. Takes path to /sys/devices/system/cpu/cpu*/ directory as a parameter.


aren't we actually checking /sys/bus/cpu/devices/ which is defined as cpuBusPath above?

by the way what is difference between /sys/bus/cpu/devices/ and /sys/devices/system/cpu/cpu*/ ? :)

We are, you are right. /sys/bus/cpu/devices/ consists of symlinks to /sys/devices/system/cpu/cpu*/, I think.

bobbypage · 2021-01-06T06:20:34Z

utils/sysfs/sysfs.go

-		if ok {
-			if errors.Is(pathErr.Unwrap(), os.ErrNotExist) && isZeroCPU(dir) {
+		// If error occurred and is os.PathError then it might be os.ErrNotExists.
+		pathErr, isPathError := err.(*os.PathError)


is it needed to do type assertion here? maybe can call os.IsNotExist directly, e.g. https://github.com/google/cadvisor/blob/master/machine/operatingsystem_unix.go#L41-L42

utils/sysfs/sysfs.go

machine/machine.go

iwankgb · 2021-02-05T20:28:31Z

@bobbypage it took a lot of time but now this very PR is ready to re-review. I consider it working.

iwankgb · 2021-02-05T22:51:21Z

@bobbypage I have also squashed all the commits into something more manageable :)

bobbypage

thanks for all your work on this, a few minor comments but overall looks great!

utils/sysfs/sysfs.go

machine/topology_test.go

utils/sysfs/sysfs.go

machine/topology_test.go

bobbypage · 2021-02-17T08:58:59Z

utils/sysfs/sysfs.go

+func newCPUList(path string) (*cpuList, error) {
+	online := cpuList{online: map[uint16]interface{}{}}
+	fileContent, err := ioutil.ReadFile(path)
+	if err != nil && os.IsNotExist(err) {


nit: maybe consider refactoring the check of if hotPlugDisabled to a separate function and handling it in top level IsCPUOnline as a short circuit rather than having IsCPUOnline call -> NewCpuList and then cpus.isOnline in case of hotplug being disabled.

utils/sysfs/sysfs_x86.go

utils/sysfs/sysfs.go

iwankgb · 2021-02-17T10:45:04Z

@bobbypage, let me know when you review it - I will squash the commits.

bobbypage · 2021-02-18T02:11:10Z

utils/sysfs/sysfs.go

-	online          map[uint16]interface{}
+	// online maps CPU ID to a struct for each online CPU.
+	online map[uint16]struct{}
+	// hotplugDisabled is true when kernel CPU hotplug is disable and all CPUs are assumed online.
 	hotplugDisabled bool


thanks for adding the hot plug check upfront in IsCPUOnline. Now that we have that check can we remove this and the duplicate check in newCPUList?

iwankgb · 2021-02-28T14:33:43Z

@bobbypage it would be great if we finished this PR. Kubernetes users area still complaining about problems it fixes.

bobbypage · 2021-03-02T09:22:41Z

LGTM, looks great, please squash and we'll be good to go!

Signed-off-by: Maciej "Iwan" Iwanowski <maciej.iwanowski@critical.today>

…ing count of unique properties Signed-off-by: Maciej "Iwan" Iwanowski <maciej.iwanowski@critical.today>

bobbypage · 2021-03-02T23:16:48Z

thanks for all your hard work on this!! LGTM

google-cla bot added the cla: yes label Nov 29, 2020

iwankgb force-pushed the assume_disabled_hotplug_in_no_online branch 2 times, most recently from e45f35d to ca5682e Compare November 29, 2020 22:11

iwankgb marked this pull request as draft November 29, 2020 22:19

iwankgb changed the title ~~Assuming that missing online file indicates CONFIG_HOTPLUG_CPU=n~~ Handling arm64: topology and online information Dec 3, 2020

iwankgb marked this pull request as ready for review December 3, 2020 23:18

iwankgb mentioned this pull request Dec 7, 2020

Feat: use memory limit from cgroup when set and lower #2699

Closed

gjkim42 mentioned this pull request Dec 14, 2020

Kubelet not starting with CRI-O on BTRFS (could not find device) kubernetes/kubernetes#94335

Closed

bobbypage self-assigned this Dec 23, 2020

bobbypage reviewed Dec 30, 2020

View reviewed changes

utils/sysfs/sysfs_arm64.go Outdated Show resolved Hide resolved

iwankgb force-pushed the assume_disabled_hotplug_in_no_online branch from 4152655 to 2fa1a16 Compare December 31, 2020 20:46

iwankgb marked this pull request as draft December 31, 2020 21:40

iwankgb mentioned this pull request Jan 4, 2021

Cadvisor for ARM #1236

Closed

iwankgb force-pushed the assume_disabled_hotplug_in_no_online branch from 5a282b6 to 0e4529d Compare January 4, 2021 19:12

iwankgb commented Jan 4, 2021

View reviewed changes

bobbypage reviewed Jan 6, 2021

View reviewed changes

utils/sysfs/sysfs.go Outdated Show resolved Hide resolved

bobbypage reviewed Jan 6, 2021

View reviewed changes

utils/sysfs/sysfs.go Outdated Show resolved Hide resolved

bobbypage reviewed Jan 6, 2021

View reviewed changes

utils/sysfs/sysfs.go Outdated Show resolved Hide resolved

bobbypage reviewed Jan 6, 2021

View reviewed changes

machine/machine.go Outdated Show resolved Hide resolved

iwankgb force-pushed the assume_disabled_hotplug_in_no_online branch from 44709b1 to 66b9b9d Compare January 10, 2021 21:48

iwankgb force-pushed the assume_disabled_hotplug_in_no_online branch from 1133b45 to 1745c0a Compare February 5, 2021 19:17

iwankgb force-pushed the assume_disabled_hotplug_in_no_online branch 11 times, most recently from 8847489 to 3232139 Compare February 5, 2021 22:50

iwankgb force-pushed the assume_disabled_hotplug_in_no_online branch from 3232139 to 9dedba6 Compare February 6, 2021 06:26

iwankgb mentioned this pull request Feb 12, 2021

cAdvisor running in docker ignore isolcpus #2792

Closed

bobbypage reviewed Feb 17, 2021

View reviewed changes

iwankgb force-pushed the assume_disabled_hotplug_in_no_online branch from 998feb6 to 648523d Compare February 17, 2021 10:20

bobbypage reviewed Feb 18, 2021

View reviewed changes

iwankgb added 3 commits March 2, 2021 10:28

Adding testdata for various platforms.

e444841

Signed-off-by: Maciej "Iwan" Iwanowski <maciej.iwanowski@critical.today>

Determining CPU online state correctly using online CPUs list

60940f4

Signed-off-by: Maciej "Iwan" Iwanowski <maciej.iwanowski@critical.today>

Fixes google#2783 - takes package ID into consideration when calculat…

fa294cb

…ing count of unique properties Signed-off-by: Maciej "Iwan" Iwanowski <maciej.iwanowski@critical.today>

iwankgb force-pushed the assume_disabled_hotplug_in_no_online branch from e69770d to fa294cb Compare March 2, 2021 09:29

bobbypage approved these changes Mar 2, 2021

View reviewed changes

bobbypage merged commit e1657e2 into google:master Mar 2, 2021

iwankgb deleted the assume_disabled_hotplug_in_no_online branch March 3, 2021 05:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handling arm64: topology and online information #2744

Handling arm64: topology and online information #2744

iwankgb commented Nov 29, 2020 •

edited

Loading

iwankgb commented Dec 28, 2020

bobbypage commented Dec 30, 2020

bobbypage commented Dec 30, 2020 •

edited

Loading

iwankgb commented Dec 30, 2020

bobbypage commented Dec 31, 2020

iwankgb commented Dec 31, 2020

iwankgb Jan 4, 2021

bobbypage Jan 6, 2021 •

edited

Loading

iwankgb Jan 4, 2021

bobbypage Jan 6, 2021

bobbypage Jan 6, 2021

iwankgb Jan 7, 2021

bobbypage Jan 7, 2021

iwankgb commented Jan 4, 2021

bobbypage Jan 6, 2021 •

edited

Loading

iwankgb Jan 7, 2021

bobbypage Jan 6, 2021

iwankgb Jan 10, 2021

iwankgb commented Feb 5, 2021

iwankgb commented Feb 5, 2021

bobbypage left a comment

bobbypage Feb 17, 2021

iwankgb commented Feb 17, 2021

bobbypage Feb 18, 2021

iwankgb commented Feb 28, 2021

bobbypage commented Mar 2, 2021

bobbypage commented Mar 2, 2021

Handling arm64: topology and online information #2744

Handling arm64: topology and online information #2744

Conversation

iwankgb commented Nov 29, 2020 • edited Loading

iwankgb commented Dec 28, 2020

bobbypage commented Dec 30, 2020

bobbypage commented Dec 30, 2020 • edited Loading

iwankgb commented Dec 30, 2020

bobbypage commented Dec 31, 2020

iwankgb commented Dec 31, 2020

Choose a reason for hiding this comment

bobbypage Jan 6, 2021 • edited Loading

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

iwankgb commented Jan 4, 2021

bobbypage Jan 6, 2021 • edited Loading

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

iwankgb commented Feb 5, 2021

iwankgb commented Feb 5, 2021

bobbypage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iwankgb commented Feb 17, 2021

Choose a reason for hiding this comment

iwankgb commented Feb 28, 2021

bobbypage commented Mar 2, 2021

bobbypage commented Mar 2, 2021

iwankgb commented Nov 29, 2020 •

edited

Loading

bobbypage commented Dec 30, 2020 •

edited

Loading

bobbypage Jan 6, 2021 •

edited

Loading

bobbypage Jan 6, 2021 •

edited

Loading