Rafael.Vanoni 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])); }
>>>>
>>>> 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.
I doubt this is the right fix.
+++if (duration < 0)
+++ duration = 0;
I could be wrong , but IIRC, we already took this into account. Please take
a look at pt_cpufreq_dtrace_walk() and the comment embedded. the time
accounted is already subtracted.
Can you give more details why sampling iterval < dtrace_time here?
>
>
> 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/l
ib/libdtrace/common/dt_options.c#46
>
> http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/l
ib/libdtrace/common/dt_options.c#874
>
> http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/l
ib/libdtrace/common/dt_options.c#996
>
> Setting the option to anything other than NULL leads to an error.
>
Thanks for the links.
-Aubrey
