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.
Kuriakose is reviewing this fix, he pointed out that the
cpu-change-speed probe is actually using uint64_t's, 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
