Aubrey Li wrote:
> Here is a bug and fix from J?rgen.
>
> J?rgen Keil <> wrote:
>> It's a bug in powertop. The dtrace code that powertop is using
>> aggregates on a processorid_t (cpu) and an uint32_t (cpuspeed).
>>
>> But in function pt_cpufreq_dtrace_walk() the cpu speed is
>> read as uint64_t - this is working with bogus values for cpu
>> speed!
>>
>> Suggested fix:
>>
>> diff --git a/usr/src/cmd/powertop/common/cpufreq.c
>> b/usr/src/cmd/powertop/common/cpufreq.c
>> --- a/usr/src/cmd/powertop/common/cpufreq.c
>> +++ b/usr/src/cmd/powertop/common/cpufreq.c
>> @@ -431,7 +431,7 @@ pt_cpufreq_dtrace_walk(const dtrace_aggd
>> dtrace_recdesc_t *cpu_rec, *speed_rec;
>> cpu_power_info_t *cpu_pow;
>> int32_t cpu;
>> - uint64_t speed;
>> + uint32_t speed;
>> hrtime_t dt_state_time = 0;
>> int i;
>>
>> @@ -447,7 +447,7 @@ pt_cpufreq_dtrace_walk(const dtrace_aggd
>> /* LINTED - alignment */
>> cpu = *(int32_t *)(data->dtada_data + cpu_rec->dtrd_offset);
>> /* LINTED - alignment */
>> - speed = *(uint64_t *)(data->dtada_data +
>> + speed = *(uint32_t *)(data->dtada_data +
>> speed_rec->dtrd_offset);
>>
>> if (speed == 0) {
>> --
>
> Or we can remove the (uint32_t) casting in the dtrace script.
> the argument in the dtrace probe :::cpu-change-speed is also uint64_t.
>
> Thanks,
> -Aubrey
Thanks for picking this up, J?rgen.
After applying your patch, the p-state numbers would display negative
values when the sampling interval was shorter than the interval between
firings of the cpu-change-speed probe. I added a check in
pt_cpufreq_stat_account() to fix that. I'm including the diff below.
I'm still seeing the original issue when observing CPUs other than CPU
0. The --cpu option has been on the project gate for a while, but was
only putback last Friday.
Rafael
diff -r 672b03ea4e18 usr/src/cmd/powertop/common/cpufreq.c
--- a/usr/src/cmd/powertop/common/cpufreq.c Fri Apr 10 11:52:32 2009 -0700
+++ b/usr/src/cmd/powertop/common/cpufreq.c Mon Apr 13 10:26:28 2009 -0700
@@ -348,6 +350,9 @@
duration = (hrtime_t)((interval * NANOSEC)) - cpu_pow->dtrace_time;
+ if (duration < 0)
+ duration = 0;
+
for (i = 0; i < g_npstates; i++) {
if (g_pstate_info[i].speed == speed) {
g_pstate_info[i].total_time += duration;
@@ -431,7 +436,7 @@
dtrace_recdesc_t *cpu_rec, *speed_rec;
cpu_power_info_t *cpu_pow;
int32_t cpu;
- uint64_t speed;
+ uint32_t speed;
hrtime_t dt_state_time = 0;
int i;
@@ -447,7 +452,7 @@
/* LINTED - alignment */
cpu = *(int32_t *)(data->dtada_data + cpu_rec->dtrd_offset);
/* LINTED - alignment */
- speed = *(uint64_t *)(data->dtada_data +
+ speed = *(uint32_t *)(data->dtada_data +
speed_rec->dtrd_offset);
if (speed == 0) {
