Fix cpu_freq for Apple silicon

[snOm3ad]

Summary

• OS: macOS Ventura 13.3
• Bug fix: yes
• Type: core
• Fixes: [MacOS] psutil.cpu_freq() broken on Apple M1 #1892

Description

Apple SoC returns empty string when querying the cpu frequency using sysctl, however, the min and max frequency values can still be extracted from the IOKit registry. This PR adds a new implementation of cpu_freq for arm64 architecture that fetches these values. It does not try to retrieve the current frequency as this metric is not available in IOKit registry and can only be found using a more involved process.

[dbwiddis]

Some leaking references you need to handle, and an iteration suggestion.

[giampaolo]

Instead of defining 2 separate C functions and catching FileNotFoundError in python I suggest to split the logic in 2 separate functions, and use the C pre-processor to check whether you're on ARM. Something like (pseudo code):

#ifdef SOMETHING_DEFINING_ARM
PyObject *
psutil_cpu_freq(PyObject *self, PyObject *args) {
    ...
}
#else
PyObject *
psutil_cpu_freq(PyObject *self, PyObject *args) {
    ...
}
#endif

[snOm3ad]

Instead of defining 2 separate C functions and catching FileNotFoundError in python I suggest to split the logic in 2 separate functions, and use the C pre-processor to check whether you're on ARM.

Is there a flag that is currently being used? Or would I need to create one to achieve this. I am assuming this would need to be incorporated in the setup.py script?

[dbwiddis]

Is there a flag that is currently being used? Or would I need to create one to achieve this.

I'm pretty sure you can do an ifdef on __aarch64__. See https://opensource.apple.com/source/WTF/WTF-7601.1.46.42/wtf/Platform.h.auto.html

/* CPU(X86_64) - AMD64 / Intel64 / x86_64 64-bit */
#if   defined(__x86_64__) \
    || defined(_M_X64)
#define WTF_CPU_X86_64 1
#endif

/* CPU(ARM64) - Apple */
#if (defined(__arm64__) && defined(__APPLE__)) || defined(__aarch64__)
#define WTF_CPU_ARM64 1
#endif

[snOm3ad]

Thanks Daniel! The only pending question would be what to do about the current frequency? At the moment we don't know if (let alone where) this information is accessible from IOReg. As you mentioned above powermetrics is closed source this makes it especially hard to know where the numbers are coming from.

[dbwiddis]

Thanks Daniel! The only pending question would be what to do about the current frequency? At the moment we don't know if (let alone where) this information is accessible from IOReg.

Actually, we do, or at least @BitesPotatoBacks does, see https://github.com/BitesPotatoBacks/SocPowerBuddy

It doesn't yet work on M2 and thus is probably brittle. What about M3, etc.?

Anyway, the powermetrics "current" value is just a "current average over the last 5 seconds" which isn't really current, is it?

I'm of the opinion that we really shouldn't care about current frequency, based on the logic in this answer: https://apple.stackexchange.com/a/329537/388596

[snOm3ad]

see https://github.com/BitesPotatoBacks/SocPowerBuddy

The sampler code is relying on IOReport functions. This technique is explained on these slides by @freedomtan. The ioreport repository seems particularly interesting.

[freedomtan]

see https://github.com/BitesPotatoBacks/SocPowerBuddy

The sampler code is relying on IOReport functions. This technique is explained on these slides by @freedomtan. The ioreport repository seems particularly interesting.

As far as I can tell, those frequency information are not "real" ones. They are data Apple used for power/energy modeling (which in turn is used for scheduling and others)

[dbwiddis]

Almost there!

[snOm3ad]

@giampaolo

Here's the output for the command you mentioned:

scpufreq(current=3504, min=702, max=3504)
scpufreq(current=3504, min=702, max=3504)
scpufreq(current=3504, min=702, max=3504)
scpufreq(current=3504, min=702, max=3504)
scpufreq(current=3504, min=702, max=3504)

As you can see the current frequency is always the max frequency, and both min and max are fixed values. I had originally intended to return None for the current frequency. I think this is a more transparent approach as it lets users know that we're not making any effort to calculate this metric for them.

Let me know your thoughts. I also updated the description in this PR as well as the HISTORY file.

[dbwiddis]

I had originally intended to return None for the current frequency. I think this is a more transparent approach as it lets users know that we're not making any effort to calculate this metric for them.

If we do that I'd say "unknown" rather than "none". Because there is a frequency, we just don't know it.

However, I stand by my opinion (but its' up to @giampaolo to decide here) that we should either return the max here, or change the x86 code to match whatever we do here. I think it's more important to return a consistent result to current users who just changed their hardware and may be relying on the fact that "current" has always been max on Apple hardware.

Here's what we return on x86:

psutil/psutil/arch/osx/cpu.c

Lines 122 to 124 in 995fa82

	// also available as "hw.cpufrequency" but it's deprecated
	mib[0] = CTL_HW;
	mib[1] = HW_CPU_FREQ;

This is documented as returning the max:

The current frequency is the max frequency in the current mode.

[giampaolo]

I think it's more important to return a consistent result to current users who just changed their hardware and may be relying on the fact that "current" has always been max on Apple hardware.

Yes. If this PR is consistent with the values that we return on x86 then let's merge it.

[giampaolo]

I have fixed the formatting (limited lines to 79 chars).

[CarterLi]

Why not use IOServiceNameMatching("pmgr")? It should be easier and faster.

[dbwiddis]

It should be easier

Perhaps easier to write at the start, but since the work has been done, the "easier" thing to do is leave it as is.

and faster.

I'm not so sure. We're limiting the iteration to a particular service, and then matching a name and stopping on success. Your proposal matches a name across the entire IO Registry may have to check for more matches (and won't stop on success as it needs to collect all matches).

We're probably talking a few nanoseconds here or there but I'm skeptical on it being "faster" without seeing a benchmark.

Then again, I don't know the inner workings of the registry and maybe there's an O(1) lookup somewhere on the name....

[giampaolo]

Sorry, I lost track on this. Is it good to go? Should we merge it?

[snOm3ad]

From my side it's good to go! I did notice that someone put up an issue saying the problem remains when using Docker (#2318). I believe we'd need to add some runtime checks to address this, so not sure if it should be included in the same PR or treated as a different issue?

[giampaolo]

I would say let's treat it as a different issue.

[giampaolo]

Can you move the HISTORY.rst entry on top of the file and also add yourself to CREDITS?

[snOm3ad]

@giampaolo the DCO is failing cause some of your commits weren't signed. Other than that it should be good now! Also thank you for looking into this once again :)

EDIT: might be worth, making the DCO check pass. If just so we can see if there's any new failing tests? Let me know.

[giampaolo]

This causes a failure on arm64, see #2382.

[dbwiddis]

This causes a failure on arm64, see #2382.

The Github-provided VMs do not provide the same drivers as real hardware. Specifically, they don't have P-Cores or E-cores, just virtual CPUs. So grokking the IORegistry for the pmgr just won't work.

See https://eclecticlight.co/2023/10/23/how-does-macos-manage-virtual-cores-on-apple-silicon/ for some interesting discussion, and oshi/oshi#2470 (comment) for my Java-based encounter with this oddness.