Skip to content

Commit

Permalink
[sysinfo] Fix CPU Frequency reading on AMD Ryzen CPU's (#1117)
Browse files Browse the repository at this point in the history
Currently, i get:
```
Run on (32 X 7326.56 MHz CPU s)
CPU Caches:
  L1 Data 32 KiB (x16)
  L1 Instruction 32 KiB (x16)
  L2 Unified 512 KiB (x16)
  L3 Unified 32768 KiB (x2)
```
which seems mostly right, except that the frequency is rather bogus.
Yes, i guess the CPU could theoretically achieve that,
but i have 3.6GHz configured, and scaling disabled.
So we clearly read the wrong thing.

With this fix, i now get the expected
```
Run on (32 X 3598.53 MHz CPU s)
CPU Caches:
  L1 Data 32 KiB (x16)
  L1 Instruction 32 KiB (x16)
  L2 Unified 512 KiB (x16)
  L3 Unified 32768 KiB (x2)
```
  • Loading branch information
LebedevRI authored Apr 23, 2021
1 parent 69054ae commit c05843a
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 7 deletions.
2 changes: 1 addition & 1 deletion include/benchmark/benchmark.h
Original file line number Diff line number Diff line change
Expand Up @@ -1324,9 +1324,9 @@ struct CPUInfo {
};

int num_cpus;
Scaling scaling;
double cycles_per_second;
std::vector<CacheInfo> caches;
Scaling scaling;
std::vector<double> load_avg;

static const CPUInfo& Get();
Expand Down
22 changes: 16 additions & 6 deletions src/sysinfo.cc
Original file line number Diff line number Diff line change
Expand Up @@ -530,7 +530,11 @@ int GetNumCPUs() {
BENCHMARK_UNREACHABLE();
}

double GetCPUCyclesPerSecond() {
double GetCPUCyclesPerSecond(CPUInfo::Scaling scaling) {
// Currently, scaling is only used on linux path here,
// suppress diagnostics about it being unused on other paths.
(void)scaling;

#if defined BENCHMARK_OS_LINUX || defined BENCHMARK_OS_CYGWIN
long freq;

Expand All @@ -541,8 +545,15 @@ double GetCPUCyclesPerSecond() {
// cannot always be relied upon. The same reasons apply to /proc/cpuinfo as
// well.
if (ReadFromFile("/sys/devices/system/cpu/cpu0/tsc_freq_khz", &freq)
// If CPU scaling is in effect, we want to use the *maximum* frequency,
// not whatever CPU speed some random processor happens to be using now.
// If CPU scaling is disabled, use the the *current* frequency.
// Note that we specifically don't want to read cpuinfo_cur_freq,
// because it is only readable by root.
|| (scaling == CPUInfo::Scaling::DISABLED &&
ReadFromFile("/sys/devices/system/cpu/cpu0/cpufreq/scaling_cur_freq",
&freq))
// Otherwise, if CPU scaling may be in effect, we want to use
// the *maximum* frequency, not whatever CPU speed some random processor
// happens to be using now.
|| ReadFromFile("/sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_max_freq",
&freq)) {
// The value is in kHz (as the file name suggests). For example, on a
Expand Down Expand Up @@ -701,12 +712,11 @@ const CPUInfo& CPUInfo::Get() {

CPUInfo::CPUInfo()
: num_cpus(GetNumCPUs()),
cycles_per_second(GetCPUCyclesPerSecond()),
caches(GetCacheSizes()),
scaling(CpuScaling(num_cpus)),
cycles_per_second(GetCPUCyclesPerSecond(scaling)),
caches(GetCacheSizes()),
load_avg(GetLoadAvg()) {}


const SystemInfo& SystemInfo::Get() {
static const SystemInfo* info = new SystemInfo();
return *info;
Expand Down

0 comments on commit c05843a

Please sign in to comment.