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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0985bb0c-091e-4e5a-b066-5b9cfd072aa8@linuxfoundation.org>
Date: Wed, 11 Dec 2024 13:54:28 -0700
From: Shuah Khan <skhan@...uxfoundation.org>
To: He Rongguang <herongguang@...ux.alibaba.com>, trenn@...e.com,
 shuah@...nel.org, jwyatt@...hat.com, jkacur@...hat.com
Cc: linux-kernel@...r.kernel.org, shannon.zhao@...ux.alibaba.com,
 linux-pm@...r.kernel.org, Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [PATCH v2] cpupower: fix TSC MHz calculation for Mperf monitor

On 12/10/24 20:20, He Rongguang wrote:
>  From e17f252433c923578e9c386a998200e488d9567d Mon Sep 17 00:00:00 2001
> From: He Rongguang <herongguang@...ux.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@...ux.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