Ingo,
On Tue, Oct 26, 2010 at 9:10 AM, Ingo Molnar <mingo@xxxxxxx> wrote:
>
> * Thomas Renninger <trenn@xxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
|