Re: [PATCH v2] tracing/power: Polish the tracepoints cpu_idle and cpu_frequency

Re: [PATCH v2] tracing/power: Polish the tracepoints cpu_idle and cpu_frequency

[Rafael J. Wysocki]

[Fix up LKML address.]

On Thu, Mar 1, 2018 at 3:27 AM, Du, Changbin <changbin.du@intel.com> wrote:
> Hi,
> On Wed, Feb 28, 2018 at 11:27:09AM +0100, Rafael J. Wysocki wrote:
>> On Wed, Feb 28, 2018 at 10:20 AM,  <changbin.du@intel.com> wrote:
>> > From: Changbin Du <changbin.du@intel.com>
>> >
>> > The type of state is signed int, convert it to unsigned int looks weird.
>> > (-1 become 4294967295)
>> >    932.123 power:cpu_idle:state=1 cpu_id=0)
>> >    932.125 power:cpu_idle:state=4294967295 cpu_id=0)
>> >    932.132 power:cpu_idle:state=1 cpu_id=0)
>> >    932.133 power:cpu_idle:state=4294967295 cpu_id=0)
>> >
>> > Similarly for cpu_frequency as "state=%lu cpu_id=%lu". User need to read
>> > the code to understand what 'state' means.
>> >
>> > No functional change in this patch.
>>
>> That rather isn't the case if negative values are ever passed to the
>> tracepoint, right?
>>
> yes.
>> Which seems to be the reason why you want to make this change, isn't it?
>>
> yes, to improve readability.
>
>> So maybe fix the code using the tracepoint(s) to avoid passing
>> negative values to it(them)?
> For cpu_idle event, [0, CPUIDLE_STATE_MAX) are used to index the idle state arrary,
> so I think a appropriate value for PWR_EVENT_EXIT is -1 (defined in include/trace/events/power.h).
> Or do you have a better idea? Thanks!

Sorry, I'm not sure what you mean.

I'm saying that the code using the CPU PM tracepoints is not expected
to pass -1 as the CPU number to them.  IOW, neither -1 nor its UL
representation should ever appear in the output of these tracepoints.
If that happens, it is a problem with the code using the tracepoints
which needs to be fixed.  Users should not see any of these values.

Re: [PATCH v2] tracing/power: Polish the tracepoints cpu_idle and cpu_frequency

[Du, Changbin]

> >> That rather isn't the case if negative values are ever passed to the
> >> tracepoint, right?
> >>
> > yes.
> >> Which seems to be the reason why you want to make this change, isn't it?
> >>
> > yes, to improve readability.
> >
> >> So maybe fix the code using the tracepoint(s) to avoid passing
> >> negative values to it(them)?
> > For cpu_idle event, [0, CPUIDLE_STATE_MAX) are used to index the idle state arrary,
> > so I think a appropriate value for PWR_EVENT_EXIT is -1 (defined in include/trace/events/power.h).
> > Or do you have a better idea? Thanks!
> 
> Sorry, I'm not sure what you mean.
> 
> I'm saying that the code using the CPU PM tracepoints is not expected
> to pass -1 as the CPU number to them.  IOW, neither -1 nor its UL
> representation should ever appear in the output of these tracepoints.
> If that happens, it is a problem with the code using the tracepoints
> which needs to be fixed.  Users should not see any of these values.

This patch only changed 'state' field but cpuid. For cpu_idle event, 'state' is
singned value, but for cpu_frequency it is unsinged.
The cpuid is always unsinged value. So no one passes -1 as CPU number.

-- 
Thanks,
Changbin Du

Re: [PATCH v2] tracing/power: Polish the tracepoints cpu_idle and cpu_frequency

[Rafael J. Wysocki]

On Fri, Mar 2, 2018 at 10:41 AM, Du, Changbin <changbin.du@intel.com> wrote:
>> >> That rather isn't the case if negative values are ever passed to the
>> >> tracepoint, right?
>> >>
>> > yes.
>> >> Which seems to be the reason why you want to make this change, isn't it?
>> >>
>> > yes, to improve readability.
>> >
>> >> So maybe fix the code using the tracepoint(s) to avoid passing
>> >> negative values to it(them)?
>> > For cpu_idle event, [0, CPUIDLE_STATE_MAX) are used to index the idle state arrary,
>> > so I think a appropriate value for PWR_EVENT_EXIT is -1 (defined in include/trace/events/power.h).
>> > Or do you have a better idea? Thanks!
>>
>> Sorry, I'm not sure what you mean.
>>
>> I'm saying that the code using the CPU PM tracepoints is not expected
>> to pass -1 as the CPU number to them.  IOW, neither -1 nor its UL
>> representation should ever appear in the output of these tracepoints.
>> If that happens, it is a problem with the code using the tracepoints
>> which needs to be fixed.  Users should not see any of these values.
>
> This patch only changed 'state' field but cpuid. For cpu_idle event, 'state' is
> singned value, but for cpu_frequency it is unsinged.
> The cpuid is always unsinged value. So no one passes -1 as CPU number.

You are right, 'state' not 'cpuid', sorry.

Negative 'state' should not be passed to these tracepoints too, though.

Re: [PATCH v2] tracing/power: Polish the tracepoints cpu_idle and cpu_frequency

[Du, Changbin]

On Fri, Mar 02, 2018 at 11:18:10AM +0100, Rafael J. Wysocki wrote:
> On Fri, Mar 2, 2018 at 10:41 AM, Du, Changbin <changbin.du@intel.com> wrote:
> >> >> That rather isn't the case if negative values are ever passed to the
> >> >> tracepoint, right?
> >> >>
> >> > yes.
> >> >> Which seems to be the reason why you want to make this change, isn't it?
> >> >>
> >> > yes, to improve readability.
> >> >
> >> >> So maybe fix the code using the tracepoint(s) to avoid passing
> >> >> negative values to it(them)?
> >> > For cpu_idle event, [0, CPUIDLE_STATE_MAX) are used to index the idle state arrary,
> >> > so I think a appropriate value for PWR_EVENT_EXIT is -1 (defined in include/trace/events/power.h).
> >> > Or do you have a better idea? Thanks!
> >>
> >> Sorry, I'm not sure what you mean.
> >>
> >> I'm saying that the code using the CPU PM tracepoints is not expected
> >> to pass -1 as the CPU number to them.  IOW, neither -1 nor its UL
> >> representation should ever appear in the output of these tracepoints.
> >> If that happens, it is a problem with the code using the tracepoints
> >> which needs to be fixed.  Users should not see any of these values.
> >
> > This patch only changed 'state' field but cpuid. For cpu_idle event, 'state' is
> > singned value, but for cpu_frequency it is unsinged.
> > The cpuid is always unsinged value. So no one passes -1 as CPU number.
> 
> You are right, 'state' not 'cpuid', sorry.
> 
> Negative 'state' should not be passed to these tracepoints too, though.

The current situtation is that 'state' can be negative for event cpu_idle :(. This
is why I made this change.

-- 
Thanks,
Changbin Du

Re: [PATCH v2] tracing/power: Polish the tracepoints cpu_idle and cpu_frequency

[Rafael J. Wysocki]

On 3/2/2018 11:15 AM, Du, Changbin wrote:
> On Fri, Mar 02, 2018 at 11:18:10AM +0100, Rafael J. Wysocki wrote:
>> On Fri, Mar 2, 2018 at 10:41 AM, Du, Changbin <changbin.du@intel.com> wrote:
>>>>>> That rather isn't the case if negative values are ever passed to the
>>>>>> tracepoint, right?
>>>>>>
>>>>> yes.
>>>>>> Which seems to be the reason why you want to make this change, isn't it?
>>>>>>
>>>>> yes, to improve readability.
>>>>>
>>>>>> So maybe fix the code using the tracepoint(s) to avoid passing
>>>>>> negative values to it(them)?
>>>>> For cpu_idle event, [0, CPUIDLE_STATE_MAX) are used to index the idle state arrary,
>>>>> so I think a appropriate value for PWR_EVENT_EXIT is -1 (defined in include/trace/events/power.h).
>>>>> Or do you have a better idea? Thanks!
>>>> Sorry, I'm not sure what you mean.
>>>>
>>>> I'm saying that the code using the CPU PM tracepoints is not expected
>>>> to pass -1 as the CPU number to them.  IOW, neither -1 nor its UL
>>>> representation should ever appear in the output of these tracepoints.
>>>> If that happens, it is a problem with the code using the tracepoints
>>>> which needs to be fixed.  Users should not see any of these values.
>>> This patch only changed 'state' field but cpuid. For cpu_idle event, 'state' is
>>> singned value, but for cpu_frequency it is unsinged.
>>> The cpuid is always unsinged value. So no one passes -1 as CPU number.
>> You are right, 'state' not 'cpuid', sorry.
>>
>> Negative 'state' should not be passed to these tracepoints too, though.
> The current situtation is that 'state' can be negative for event cpu_idle :(. This
> is why I made this change.
>
And which is why I said that IMO it would be better to change the 
current situation.

Your patch makes the results of it slightly less confusing to a human 
reader of the tracepoint output, but the situation is still unchanged 
after it.

And what if someone has a script built around these tracepoints that 
knows how to handle the UL representation of -1, but doesn't know how to 
parse "-1"?  They would need to update the script after your change, 
wouldn't they?  And why would it be OK to inflict that work on them just 
to improve the readability of the output for humans?

Re: [PATCH v2] tracing/power: Polish the tracepoints cpu_idle and cpu_frequency

[Du, Changbin]

On Fri, Mar 02, 2018 at 11:39:16AM +0100, Rafael J. Wysocki wrote:
> On 3/2/2018 11:15 AM, Du, Changbin wrote:
> > On Fri, Mar 02, 2018 at 11:18:10AM +0100, Rafael J. Wysocki wrote:
> > > On Fri, Mar 2, 2018 at 10:41 AM, Du, Changbin <changbin.du@intel.com> wrote:
> > > > > > > That rather isn't the case if negative values are ever passed to the
> > > > > > > tracepoint, right?
> > > > > > > 
> > > > > > yes.
> > > > > > > Which seems to be the reason why you want to make this change, isn't it?
> > > > > > > 
> > > > > > yes, to improve readability.
> > > > > > 
> > > > > > > So maybe fix the code using the tracepoint(s) to avoid passing
> > > > > > > negative values to it(them)?
> > > > > > For cpu_idle event, [0, CPUIDLE_STATE_MAX) are used to index the idle state arrary,
> > > > > > so I think a appropriate value for PWR_EVENT_EXIT is -1 (defined in include/trace/events/power.h).
> > > > > > Or do you have a better idea? Thanks!
> > > > > Sorry, I'm not sure what you mean.
> > > > > 
> > > > > I'm saying that the code using the CPU PM tracepoints is not expected
> > > > > to pass -1 as the CPU number to them.  IOW, neither -1 nor its UL
> > > > > representation should ever appear in the output of these tracepoints.
> > > > > If that happens, it is a problem with the code using the tracepoints
> > > > > which needs to be fixed.  Users should not see any of these values.
> > > > This patch only changed 'state' field but cpuid. For cpu_idle event, 'state' is
> > > > singned value, but for cpu_frequency it is unsinged.
> > > > The cpuid is always unsinged value. So no one passes -1 as CPU number.
> > > You are right, 'state' not 'cpuid', sorry.
> > > 
> > > Negative 'state' should not be passed to these tracepoints too, though.
> > The current situtation is that 'state' can be negative for event cpu_idle :(. This
> > is why I made this change.
> > 
> And which is why I said that IMO it would be better to change the current
> situation.
> 
> Your patch makes the results of it slightly less confusing to a human reader
> of the tracepoint output, but the situation is still unchanged after it.
> 
> And what if someone has a script built around these tracepoints that knows
> how to handle the UL representation of -1, but doesn't know how to parse
> "-1"?  They would need to update the script after your change, wouldn't
> they?  And why would it be OK to inflict that work on them just to improve
> the readability of the output for humans?
>
yeah, I can guarantee all in kernel tools updated but people's private script.
For me, I just read the raw event for debug purpose. It is fair enough that leave
code as it was considering users' private tool based on this event.

> 
> 

-- 
Thanks,
Changbin Du

Re: [PATCH v2] tracing/power: Polish the tracepoints cpu_idle and cpu_frequency

[Steven Rostedt]

[ Resending due to typo in LKML address ]

On Wed, 14 Feb 2018 10:40:38 +0800
changbin.du@intel.com wrote:

> From: Changbin Du <changbin.du@intel.com>
> 
> The type of state is signed int, convert it to unsigned int looks weird.
> (-1 become 4294967295)
>    932.123 power:cpu_idle:state=1 cpu_id=0)
>    932.125 power:cpu_idle:state=4294967295 cpu_id=0)
>    932.132 power:cpu_idle:state=1 cpu_id=0)
>    932.133 power:cpu_idle:state=4294967295 cpu_id=0)
> 
> Similarly for cpu_frequency as "state=%lu cpu_id=%lu". User need to read
> the code to understand what 'state' means.
> 
> No functional change in this patch.
> 
> Cc: Steven Rostedt (VMware) <rostedt@goodmis.org>


>From the tracing perspective:

Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve

> Signed-off-by: Changbin Du <changbin.du@intel.com>
> 
> ---
> v2:
>   o rename title.
>   o define tracepoint with DEFINE_EVENT_PRINT().
> ---
>  Documentation/trace/events-power.txt |  4 ++--
>  include/trace/events/power.h         | 17 ++++++++++-------
>  2 files changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/trace/events-power.txt b/Documentation/trace/events-power.txt
> index 21d514c..9485039 100644
> --- a/Documentation/trace/events-power.txt
> +++ b/Documentation/trace/events-power.txt
> @@ -23,8 +23,8 @@ Cf. include/trace/events/power.h for the events definitions.
>  A 'cpu' event class gathers the CPU-related events: cpuidle and
>  cpufreq.
>  
> -cpu_idle		"state=%lu cpu_id=%lu"
> -cpu_frequency		"state=%lu cpu_id=%lu"
> +cpu_idle		"state=%ld cpu_id=%lu"
> +cpu_frequency		"frequency=%lu cpu_id=%lu"
>  
>  A suspend event is used to indicate the system going in and out of the
>  suspend mode:
> diff --git a/include/trace/events/power.h b/include/trace/events/power.h
> index 908977d..f1b82de 100644
> --- a/include/trace/events/power.h
> +++ b/include/trace/events/power.h
> @@ -14,12 +14,12 @@
>  
>  DECLARE_EVENT_CLASS(cpu,
>  
> -	TP_PROTO(unsigned int state, unsigned int cpu_id),
> +	TP_PROTO(int state, unsigned int cpu_id),
>  
>  	TP_ARGS(state, cpu_id),
>  
>  	TP_STRUCT__entry(
> -		__field(	u32,		state		)
> +		__field(	s32,		state		)
>  		__field(	u32,		cpu_id		)
>  	),
>  
> @@ -28,13 +28,13 @@ DECLARE_EVENT_CLASS(cpu,
>  		__entry->cpu_id = cpu_id;
>  	),
>  
> -	TP_printk("state=%lu cpu_id=%lu", (unsigned long)__entry->state,
> +	TP_printk("state=%ld cpu_id=%lu", (long)__entry->state,
>  		  (unsigned long)__entry->cpu_id)
>  );
>  
>  DEFINE_EVENT(cpu, cpu_idle,
>  
> -	TP_PROTO(unsigned int state, unsigned int cpu_id),
> +	TP_PROTO(int state, unsigned int cpu_id),
>  
>  	TP_ARGS(state, cpu_id)
>  );
> @@ -141,11 +141,14 @@ TRACE_EVENT(pstate_sample,
>  		{ PM_EVENT_RESTORE, "restore" }, \
>  		{ PM_EVENT_RECOVER, "recover" })
>  
> -DEFINE_EVENT(cpu, cpu_frequency,
> +DEFINE_EVENT_PRINT(cpu, cpu_frequency,
>  
> -	TP_PROTO(unsigned int frequency, unsigned int cpu_id),
> +	TP_PROTO(int state, unsigned int cpu_id),
>  
> -	TP_ARGS(frequency, cpu_id)
> +	TP_ARGS(state, cpu_id),
> +
> +	TP_printk("frequency=%lu cpu_id=%lu",  (unsigned long)__entry->state,
> +		  (unsigned long)__entry->cpu_id)
>  );
>  
>  TRACE_EVENT(device_pm_callback_start,

Re: [PATCH v2] tracing/power: Polish the tracepoints cpu_idle and cpu_frequency

[Du, Changbin]

On Tue, Feb 27, 2018 at 05:39:38PM -0500, Steven Rostedt wrote:
> On Tue, 27 Feb 2018 17:35:27 +0800
> "Du, Changbin" <changbin.du@intel.com> wrote:
> 
> > > From the tracing perspective:
> > > 
> > > Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > > 
> > > -- Steve
> > >  
> > Hi Steve, will you pick this or someoneelse?
> 
> I maintain the tracing infrastructure, but the tracing use cases are
> maintained by the maintainers of the users of the trace events. That
> is, who added these trace events? They are the ones most affected by
> these changes.
> 
> For example, it looks like Rafael J. Wysocki, is the one that added
> trace_cpu_frequency(). He's the one that is affected by this change,
> and is the one that you need to have take it.
>
Got it, thanks!

Hi Wysocki, could you take a look?

> -- Steve
> 

-- 
Thanks,
Changbin Du

Re: [PATCH v2] tracing/power: Polish the tracepoints cpu_idle and cpu_frequency

[Rafael J. Wysocki]

On 2/28/2018 3:45 AM, Du, Changbin wrote:
> On Tue, Feb 27, 2018 at 05:39:38PM -0500, Steven Rostedt wrote:
>> On Tue, 27 Feb 2018 17:35:27 +0800
>> "Du, Changbin" <changbin.du@intel.com> wrote:
>>
>>>>  From the tracing perspective:
>>>>
>>>> Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
>>>>
>>>> -- Steve
>>>>   
>>> Hi Steve, will you pick this or someoneelse?
>> I maintain the tracing infrastructure, but the tracing use cases are
>> maintained by the maintainers of the users of the trace events. That
>> is, who added these trace events? They are the ones most affected by
>> these changes.
>>
>> For example, it looks like Rafael J. Wysocki, is the one that added
>> trace_cpu_frequency(). He's the one that is affected by this change,
>> and is the one that you need to have take it.
>>
> Got it, thanks!
>
> Hi Wysocki, could you take a look?

Please send the patch(es) to linux-pm@vger.kernel.org with a CC to me 
and I will take care of them.

Thanks,
Rafael

Re: [PATCH v2] tracing/power: Polish the tracepoints cpu_idle and cpu_frequency

[Du, Changbin]

On Wed, Feb 28, 2018 at 10:14:41AM +0100, Rafael J. Wysocki wrote:
> On 2/28/2018 3:45 AM, Du, Changbin wrote:
> > On Tue, Feb 27, 2018 at 05:39:38PM -0500, Steven Rostedt wrote:
> > > On Tue, 27 Feb 2018 17:35:27 +0800
> > > "Du, Changbin" <changbin.du@intel.com> wrote:
> > > 
> > > > >  From the tracing perspective:
> > > > > 
> > > > > Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > > > > 
> > > > > -- Steve
> > > > Hi Steve, will you pick this or someoneelse?
> > > I maintain the tracing infrastructure, but the tracing use cases are
> > > maintained by the maintainers of the users of the trace events. That
> > > is, who added these trace events? They are the ones most affected by
> > > these changes.
> > > 
> > > For example, it looks like Rafael J. Wysocki, is the one that added
> > > trace_cpu_frequency(). He's the one that is affected by this change,
> > > and is the one that you need to have take it.
> > > 
> > Got it, thanks!
> > 
> > Hi Wysocki, could you take a look?
> 
> Please send the patch(es) to linux-pm@vger.kernel.org with a CC to me and I
> will take care of them.
> 
sure~

> Thanks,
> Rafael
> 

-- 
Thanks,
Changbin Du