diff mbox series

[v2] cpupower: fix TSC MHz calculation for Mperf monitor

Message ID 60562222-6186-4eec-9c20-08b1cebb1311@linux.alibaba.com (mailing list archive)
State New
Headers show
Series [v2] cpupower: fix TSC MHz calculation for Mperf monitor | expand

Commit Message

He Rongguang Dec. 11, 2024, 3:20 a.m. UTC 
From e17f252433c923578e9c386a998200e488d9567d Mon Sep 17 00:00:00 2001
From: He Rongguang <herongguang@linux.alibaba.com>
Date: Thu, 28 Nov 2024 16:50:05 +0800
Subject: [PATCH] cpupower: fix TSC MHz calculation

Commit 'cpupower: Make TSC read per CPU for Mperf monitor' (c2adb1877b7)
changes TSC counter reads per cpu, but left time diff global (from start
of all cpus to end of all cpus), thus diff(time) is too large for a
cpu's tsc counting, resulting in far less than acutal TSC_Mhz and thus
`cpupower monitor` showing far less than actual cpu realtime frequency.

/proc/cpuinfo shows frequency:
cat /proc/cpuinfo | egrep -e 'processor' -e 'MHz'
...
processor : 171
cpu MHz   : 4108.498
...

before fix (System 100% busy):
    | Mperf              || Idle_Stats
 CPU| C0   | Cx   | Freq  || POLL | C1   | C2
 171|  0.77| 99.23|  2279||  0.00|  0.00|  0.00

after fix (System 100% busy):
    | Mperf              || Idle_Stats
 CPU| C0   | Cx   | Freq  || POLL | C1   | C2
 171|  0.46| 99.54|  4095||  0.00|  0.00|  0.00

Fixes: c2adb1877b76 ("cpupower: Make TSC read per CPU for Mperf monitor")
Signed-off-by: He Rongguang <herongguang@linux.alibaba.com>
---
Changes in v2:
- Fix scripts/checkpatch.pl style warnings.
- Link to v1:
https://lore.kernel.org/linux-pm/269b8bb2-85b5-4cc9-9354-a1270f2eed35@linux.alibaba.com/T/#u
---
 .../cpupower/utils/idle_monitor/mperf_monitor.c   | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

 	{
@@ -174,7 +174,7 @@ static int mperf_get_count_percent(unsigned int id,
double *percent,
 		dprint("%s: TSC Ref - mperf_diff: %llu, tsc_diff: %llu\n",
 		       mperf_cstates[id].name, mperf_diff, tsc_diff);
 	} else if (max_freq_mode == MAX_FREQ_SYSFS) {
-		timediff = max_frequency * timespec_diff_us(time_start, time_end);
+		timediff = max_frequency * timespec_diff_us(time_start[cpu],
time_end[cpu]);
 		*percent = 100.0 * mperf_diff / timediff;
 		dprint("%s: MAXFREQ - mperf_diff: %llu, time_diff: %llu\n",
 		       mperf_cstates[id].name, mperf_diff, timediff);
@@ -207,7 +207,7 @@ static int mperf_get_count_freq(unsigned int id,
unsigned long long *count,
 	if (max_freq_mode == MAX_FREQ_TSC_REF) {
 		/* Calculate max_freq from TSC count */
 		tsc_diff = tsc_at_measure_end[cpu] - tsc_at_measure_start[cpu];
-		time_diff = timespec_diff_us(time_start, time_end);
+		time_diff = timespec_diff_us(time_start[cpu], time_end[cpu]);
 		max_frequency = tsc_diff / time_diff;
 	}

@@ -226,9 +226,8 @@ static int mperf_start(void)
 {
 	int cpu;

-	clock_gettime(CLOCK_REALTIME, &time_start);
-
 	for (cpu = 0; cpu < cpu_count; cpu++) {
+		clock_gettime(CLOCK_REALTIME, &time_start[cpu]);
 		mperf_get_tsc(&tsc_at_measure_start[cpu]);
 		mperf_init_stats(cpu);
 	}
@@ -243,9 +242,9 @@ static int mperf_stop(void)
 	for (cpu = 0; cpu < cpu_count; cpu++) {
 		mperf_measure_stats(cpu);
 		mperf_get_tsc(&tsc_at_measure_end[cpu]);
+		clock_gettime(CLOCK_REALTIME, &time_end[cpu]);
 	}

-	clock_gettime(CLOCK_REALTIME, &time_end);
 	return 0;
 }

@@ -349,6 +348,8 @@ struct cpuidle_monitor *mperf_register(void)
 	aperf_current_count = calloc(cpu_count, sizeof(unsigned long long));
 	tsc_at_measure_start = calloc(cpu_count, sizeof(unsigned long long));
 	tsc_at_measure_end = calloc(cpu_count, sizeof(unsigned long long));
+	time_start = calloc(cpu_count, sizeof(struct timespec));
+	time_end = calloc(cpu_count, sizeof(struct timespec));
 	mperf_monitor.name_len = strlen(mperf_monitor.name);
 	return &mperf_monitor;
 }
@@ -361,6 +362,8 @@ void mperf_unregister(void)
 	free(aperf_current_count);
 	free(tsc_at_measure_start);
 	free(tsc_at_measure_end);
+	free(time_start);
+	free(time_end);
 	free(is_valid);
 }

Comments

Shuah Khan Dec. 11, 2024, 8:54 p.m. UTC | #1 
On 12/10/24 20:20, He Rongguang wrote:
>  From e17f252433c923578e9c386a998200e488d9567d Mon Sep 17 00:00:00 2001
> From: He Rongguang <herongguang@linux.alibaba.com>
> Date: Thu, 28 Nov 2024 16:50:05 +0800
> Subject: [PATCH] cpupower: fix TSC MHz calculation
> 
> Commit 'cpupower: Make TSC read per CPU for Mperf monitor' (c2adb1877b7)
> changes TSC counter reads per cpu, but left time diff global (from start
> of all cpus to end of all cpus), thus diff(time) is too large for a
> cpu's tsc counting, resulting in far less than acutal TSC_Mhz and thus
> `cpupower monitor` showing far less than actual cpu realtime frequency.
> 
> /proc/cpuinfo shows frequency:
> cat /proc/cpuinfo | egrep -e 'processor' -e 'MHz'
> ...
> processor : 171
> cpu MHz   : 4108.498
> ...
> 
> before fix (System 100% busy):
>      | Mperf              || Idle_Stats
>   CPU| C0   | Cx   | Freq  || POLL | C1   | C2
>   171|  0.77| 99.23|  2279||  0.00|  0.00|  0.00
> 
> after fix (System 100% busy):
>      | Mperf              || Idle_Stats
>   CPU| C0   | Cx   | Freq  || POLL | C1   | C2
>   171|  0.46| 99.54|  4095||  0.00|  0.00|  0.00
> 
> Fixes: c2adb1877b76 ("cpupower: Make TSC read per CPU for Mperf monitor")
> Signed-off-by: He Rongguang <herongguang@linux.alibaba.com>
> ---
> Changes in v2:
> - Fix scripts/checkpatch.pl style warnings.
> - Link to v1:
> https://lore.kernel.org/linux-pm/269b8bb2-85b5-4cc9-9354-a1270f2eed35@linux.alibaba.com/T/#u
> ---

Are you sure you sent me the right patch. I am seeing exact
same errors. How are you sending your patches.

Here are the problems I am seeing:

ERROR: patch seems to be corrupt (line wrapped?)
#103: FILE: :173:
double *percent,

WARNING: It's generally not useful to have the filename in the file
#108: FILE: :177:
+		timediff = max_frequency * timespec_diff_us(time_start[cpu],

WARNING: It's generally not useful to have the filename in the file
#119: FILE: :210:
+		time_diff = timespec_diff_us(time_start[cpu], time_end[cpu]);

WARNING: It's generally not useful to have the filename in the file
#130: FILE: :230:
+		clock_gettime(CLOCK_REALTIME, &time_start[cpu]);

WARNING: It's generally not useful to have the filename in the file
#138: FILE: :245:
+		clock_gettime(CLOCK_REALTIME, &time_end[cpu]);

WARNING: It's generally not useful to have the filename in the file
#149: FILE: :351:
+	time_start = calloc(cpu_count, sizeof(struct timespec));

WARNING: It's generally not useful to have the filename in the file
#150: FILE: :352:
+	time_end = calloc(cpu_count, sizeof(struct timespec));

WARNING: It's generally not useful to have the filename in the file
#158: FILE: :365:
+	free(time_start);

WARNING: It's generally not useful to have the filename in the file
#159: FILE: :366:
+	free(time_end);

thanks,
-- Shuah
He Rongguang Dec. 12, 2024, 2:17 a.m. UTC | #2 
在 2024/12/12 4:54, Shuah Khan 写道:
> 
> Are you sure you sent me the right patch. I am seeing exact
> same errors. How are you sending your patches.
> 
> Here are the problems I am seeing:
> 
> ERROR: patch seems to be corrupt (line wrapped?)
> #103: FILE: :173:
> double *percent,
> 
> WARNING: It's generally not useful to have the filename in the file
> #108: FILE: :177:
> +        timediff = max_frequency * timespec_diff_us(time_start[cpu],
> 
> WARNING: It's generally not useful to have the filename in the file
> #119: FILE: :210:
> +        time_diff = timespec_diff_us(time_start[cpu], time_end[cpu]);
> 
> WARNING: It's generally not useful to have the filename in the file
> #130: FILE: :230:
> +        clock_gettime(CLOCK_REALTIME, &time_start[cpu]);
> 
> WARNING: It's generally not useful to have the filename in the file
> #138: FILE: :245:
> +        clock_gettime(CLOCK_REALTIME, &time_end[cpu]);
> 
> WARNING: It's generally not useful to have the filename in the file
> #149: FILE: :351:
> +    time_start = calloc(cpu_count, sizeof(struct timespec));
> 
> WARNING: It's generally not useful to have the filename in the file
> #150: FILE: :352:
> +    time_end = calloc(cpu_count, sizeof(struct timespec));
> 
> WARNING: It's generally not useful to have the filename in the file
> #158: FILE: :365:
> +    free(time_start);
> 
> WARNING: It's generally not useful to have the filename in the file
> #159: FILE: :366:
> +    free(time_end);
> 
> thanks,
> -- Shuah

I am very sorry, it's thunderbird auto wrap my patch, sent a v3, currently seems ok:

$ ./scripts/checkpatch.pl test.patch
total: 0 errors, 0 warnings, 60 lines checked

test.patch has no obvious style problems and is ready for submission.
diff mbox series

Patch

diff --git a/tools/power/cpupower/utils/idle_monitor/mperf_monitor.c
b/tools/power/cpupower/utils/idle_monitor/mperf_monitor.c
index ae6af354a81d..08a399b0be28 100644
--- a/tools/power/cpupower/utils/idle_monitor/mperf_monitor.c
+++ b/tools/power/cpupower/utils/idle_monitor/mperf_monitor.c
@@ -33,7 +33,7 @@  static int mperf_get_count_percent(unsigned int
self_id, double *percent,
 				   unsigned int cpu);
 static int mperf_get_count_freq(unsigned int id, unsigned long long *count,
 				unsigned int cpu);
-static struct timespec time_start, time_end;
+static struct timespec *time_start, *time_end;

 static cstate_t mperf_cstates[MPERF_CSTATE_COUNT] = {