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