Li, Aubrey wrote:
>>> Li, Aubrey wrote:
>>>> 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])); }
>>>>>>
>
> Here is a suggested patch:
>
> changelog:
>
> 1) speed cast back to be (uint64_t)
>
> 2) make sure duration is not negative.
I agree with these first two, I need a little more time to properly
check 3 and 4. I'll get back to you as soon as possible.
Thanks,
Rafael
> 3) I failed to understand why the current walk function need to go through
> all CPU to get the dtrace time everytime to fill a per-cpu
> array(g_cpu_power_states).
> That might be the reason why duration < 0. So change to count the specific
> cpu only.
>
> 4) if we have the condition check (dt_state_time > cpu_pow->time_accounted),
> we also
> need to check the opposite case. time_accounted should be reset if it's not
> used by this
> time.
>
> If the following patch makes sense, we'll have to fix the similar cstate
> issue.
>
> Thanks,
> -Aubrey
