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

[Shuah Khan <skhan@linuxfoundation.org>]

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