Re: [PATCH] PERF(kernel): Cleanup power events V2

Jean Pihet
2010-10-26T08:08:08Z

Ingo,

On Tue, Oct 26, 2010 at 9:10 AM, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Thomas Renninger <trenn@suse.de> wrote:
>
>> Changes in V2:
>>   - Introduce PWR_EVENT_EXIT instead of 0 to mark non-power state
>>   - Use u32 instead of u64 for cpuid, state which is by far enough

...

>>
>> +#define PWR_EVENT_EXIT 0xFFFFFFFF
>
> Shouldnt this be part of the POWER_ enum? (and you can write -1 there)
>
>> +#ifndef _TRACE_POWER_ENUM_
>> +#define _TRACE_POWER_ENUM_
>> +enum {
>> +     POWER_NONE = 0,
>> +     POWER_CSTATE = 1,
>> +     POWER_PSTATE = 2,
>> +};
>> +#endif
>
> Since we are cleaning up all these events, those enum definitions dont really look
> logical. For example, what is 'POWER_NONE'? Can a CPU have 'no power'?
The enum belongs to the deprecated API so I would rather not touch it.
Keeping the deprecated code isolated will make it easier to remove
later.

>
> Plus:
>
>> +DECLARE_EVENT_CLASS(processor,
>> +
>> +     TP_PROTO(unsigned int state, unsigned int cpu_id),
>> +
>> +     TP_ARGS(state, cpu_id),
>> +
>> +     TP_STRUCT__entry(
>> +             __field(        u32,            state           )
>> +             __field(        u32,            cpu_id          )
>
> Trace entries can carry a cpu_id of the current processor already. Can this cpu_id
> ever be different from that CPU id?
I have no evidence of that (powr mgt on SMP ARM is coming real
soon...) but one can imagine one of the CPUs being the master for PM
decisions.

>
>> +     ),
>> +
>> +     TP_fast_assign(
>> +             __entry->state = state;
>> +             __entry->cpu_id = cpu_id;
>> +     ),
>> +
>> +     TP_printk("state=%lu cpu_id=%lu", (unsigned long)__entry->state,
>> +               (unsigned long)__entry->cpu_id)
>> +);
>> +
>> +DEFINE_EVENT(processor, processor_idle,
>> +
>> +     TP_PROTO(unsigned int state, unsigned int cpu_id),
>> +
>> +          TP_ARGS(state, cpu_id)
>> +);
>> +
>> +#define PWR_EVENT_EXIT 0xFFFFFFFF
>> +
>> +DEFINE_EVENT(processor, processor_frequency,
>> +
>> +     TP_PROTO(unsigned int frequency, unsigned int cpu_id),
>> +
>> +     TP_ARGS(frequency, cpu_id)
>> +);
>
> So, we have a 'state' field in the class, which is used as 'state' by the
> power::cpu_idle event, and as 'frequency' by the power::cpu_freq event?
>
> Are there any architectures that track frequency in Hz, not in kHz? If yes, might
> there ever be a need for the frequency value to be larger than 4.29 GHz? If yes,
> then it wont fit into u32.
>
> Also, might there be a future need to express different types of frequencies? For
> example, should we decide to track turbo frequencies in Intel CPUs, how would that
> be expressed via these events? Are there any architectures and CPUs that somehow
> have some extra attribute to the frequency value?
>
>> +TRACE_EVENT(machine_suspend,
>> +
>> +     TP_PROTO(unsigned int state),
>> +
>> +     TP_ARGS(state),
>> +
>> +     TP_STRUCT__entry(
>> +             __field(        u32,            state           )
>> +     ),
>
> Hm, this event is not used anywhere in the submitted patches. Where is the patch
> that adds usage, and what are the possible values for 'state'?
This will come as a separate patch, which fits all platforms. Cf.
http://marc.info/?l=linux-omap&m=128620575300682&w=2.
The state field is of type suspend_state_t, cf. include/linux/suspend.h
>
>        Ingo
>

Jean
--
To unsubscribe from this list: send the line "unsubscribe linux-trace-users" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html