Alexander Kolbasov wrote:
>> The PowerTop prototype is getting to be in pretty good shape. It's
>> tracking wake-ups from all the relevant events:
>>
>> - timeouts / callouts
>> - cyclic firings (including clock)
>> - Interrupts
>>
>
> That's really cool!
>
> A couple of comments:
>
> usr/src/uts/common/io/cpudrv.c:
>
>
> 540 DTRACE_PROBE3(cpu_change_speed,
> 541 processorid_t, cpudsp->cpu_id,
> 542 uint_t, cpupm->cur_spd ? cpupm->cur_spd->speed : 0,
> 543 uint_t, new_spd->speed);
>
> I suggest rewriting this probe as
>
> DTRACE_PROBE3(cpu_change_speed,
> cpudrv_devstate_t *, cpudsp,
> cpudrv_pm_t *, cpupm,
> cpudrv_pm_spd_t *, new_spd)
>
> This should reduce disabled probe effect because all these dereferences and
> conditionals are actually executed even when the probe is disabled.
>
Agreed. Thanks. This will require additional dereferencing on behalf of
the client, but eventually that would be hidden behind translators anyway.
> usr/src/uts/i86pc/os/mp_pc.c:
>
> 246 void
> 247 mach_cpu_idle(void)
> 248 {
> 249 DTRACE_PROBE1(cstate_transition, uint_t, 1);
> 250
> 251 tlb_going_idle();
> 252 i86_halt();
> 253 tlb_service();
> 254
> 255 DTRACE_PROBE1(cstate_transition, uint_t, 0);
> 256 }
>
> This is a bit odd - why do you need to pass extra 1/0 argument instead of
> using different names for in/out transitions?
>
The 1/0 is the c-state that we are transitioning to, since we only
support C1 currently, it's a bit hard coded...but that will probably
change when we have support for deeper c-states.
Thanks
-Eric
> - akolb
>
>
