2018-03-02 09:26:16

by Rafael J. Wysocki

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

[Fix up LKML address.]

On Thu, Mar 1, 2018 at 3:27 AM, Du, Changbin <[email protected]> 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, <[email protected]> wrote:
>> > From: Changbin Du <[email protected]>
>> >
>> > 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.


2018-03-02 10:19:56

by Rafael J. Wysocki

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

On Fri, Mar 2, 2018 at 10:41 AM, Du, Changbin <[email protected]> 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.

2018-03-02 10:40:46

by Wysocki, Rafael J

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

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 <[email protected]> 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?




2018-03-02 14:36:19

by Du, Changbin

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

> >> 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

2018-03-02 17:42:24

by Du, Changbin

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

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 <[email protected]> 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

2018-03-02 17:59:47

by Du, Changbin

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

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 <[email protected]> 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