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])); }
>>>
>>> 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.
Actually, changing g_ncpus to 1 in
for (i = 0; i < g_ncpus; i++)
would cause the loop to not even be executed. The code should do
something like this:
if (PTOP_ON_CPU) {
dt_state_time += *((hrtime_t *)(
data->dtada_percpu[g_observed_cpu]));
} else {
for (i = 0; i < g_ncpus; i++) {
/* LINTED - alignment */
dt_state_time += *((hrtime_t *)
(data->dtada_percpu[i]));
}
}
> 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.
The problem was unrelated to this issue, one of the other issues I'm
working on was interfering.
>> 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?
It contains the aggregation's data on a per CPU basis. It's an option
set with dtrace_setopt(dtp, "aggpercpu", 0), which apparently can't be
turned off.
http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/lib/libdtrace/common/dt_options.c#46
http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/lib/libdtrace/common/dt_options.c#874
http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/lib/libdtrace/common/dt_options.c#996
Setting the option to anything other than NULL leads to an error.
Rafael
