Rafael.Vanoni wrote:
> Li, Aubrey wrote:
>> Rafael Vanoni wrote:
>>
>>> 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!
>>>>>
>>> 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.
>>>
>>
>> When walk the aggregation of the cpufreq,
>> This implementation looks wrong for the specific CPU.
>>
>> for (i = 0; i < g_ncpus; i++) {
>> /* LINTED - alignment */
>> dt_state_time += *((hrtime_t
>> *)(data->dtada_percpu[i])); }
>>
>> I never found the doc for the dtrace data structure definition.
>> What does data->dtada_percpu[i] mean here?
>
> That was the first place I looked ;) It's actually right, changing to
> g_ncpus_observed caused an error.
I don't think so. Of course you should not just change to g_cpus_obsered.
It's to be 1 when add the specific cpu option.
That's why you are still seeing the original issue when observing CPUs other
than
CPU 0. You should tell the dtada_percpu[] which cpu is specified.
>
> Kuriakose is reviewing this fix, he pointed out that the
> cpu-change-speed probe is actually using uint64_t's,
I also pointed that out. Acutally both fixes are fine.
so I changed the
> fix to simply cast the argument in the DTrace script. There's a webrev
> here -> http://cr.opensolaris.org/~rafaelv/p-states/
>
> Thanks.
> Rafael
Before accept the fix for the negetive percent, do you know what does
data->dtada_percpu[i] mean?
Thanks,
-Aubrey
