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.
Signed-off-by: Changbin Du <[email protected]>
---
include/trace/events/power.h | 32 +++++++++++++++++++-------------
1 file changed, 19 insertions(+), 13 deletions(-)
diff --git a/include/trace/events/power.h b/include/trace/events/power.h
index 908977d..39bd6de 100644
--- a/include/trace/events/power.h
+++ b/include/trace/events/power.h
@@ -12,14 +12,14 @@
#define TPS(x) tracepoint_string(x)
-DECLARE_EVENT_CLASS(cpu,
+TRACE_EVENT(cpu_idle,
- 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( int, state )
__field( u32, cpu_id )
),
@@ -28,17 +28,10 @@ DECLARE_EVENT_CLASS(cpu,
__entry->cpu_id = cpu_id;
),
- TP_printk("state=%lu cpu_id=%lu", (unsigned long)__entry->state,
+ TP_printk("state=%d cpu_id=%lu", __entry->state,
(unsigned long)__entry->cpu_id)
);
-DEFINE_EVENT(cpu, cpu_idle,
-
- TP_PROTO(unsigned int state, unsigned int cpu_id),
-
- TP_ARGS(state, cpu_id)
-);
-
TRACE_EVENT(powernv_throttle,
TP_PROTO(int chip_id, const char *reason, int pmax),
@@ -141,11 +134,24 @@ TRACE_EVENT(pstate_sample,
{ PM_EVENT_RESTORE, "restore" }, \
{ PM_EVENT_RECOVER, "recover" })
-DEFINE_EVENT(cpu, cpu_frequency,
+TRACE_EVENT(cpu_frequency,
TP_PROTO(unsigned int frequency, unsigned int cpu_id),
- TP_ARGS(frequency, cpu_id)
+ TP_ARGS(frequency, cpu_id),
+
+ TP_STRUCT__entry(
+ __field( u32, frequency )
+ __field( u32, cpu_id )
+ ),
+
+ TP_fast_assign(
+ __entry->frequency = frequency;
+ __entry->cpu_id = cpu_id;
+ ),
+
+ TP_printk("frequency=%lu cpu_id=%lu", __entry->frequency,
+ (unsigned long)__entry->cpu_id)
);
TRACE_EVENT(device_pm_callback_start,
--
2.7.4
On Sat, 10 Feb 2018 09:37:04 +0800
[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's not true. You split a class into two TRACE_EVENTS. Each
TRACE_EVENT adds approximately 5k of code and data. A DEFINE_EVENT()
adds around 300 bytes. There's better ways to do this,
Please don't add this patch.
-- Steve
>
> Signed-off-by: Changbin Du <[email protected]>
> ---
> include/trace/events/power.h | 32 +++++++++++++++++++-------------
> 1 file changed, 19 insertions(+), 13 deletions(-)
>
> diff --git a/include/trace/events/power.h b/include/trace/events/power.h
> index 908977d..39bd6de 100644
> --- a/include/trace/events/power.h
> +++ b/include/trace/events/power.h
> @@ -12,14 +12,14 @@
>
> #define TPS(x) tracepoint_string(x)
>
> -DECLARE_EVENT_CLASS(cpu,
> +TRACE_EVENT(cpu_idle,
>
> - 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( int, state )
> __field( u32, cpu_id )
> ),
>
> @@ -28,17 +28,10 @@ DECLARE_EVENT_CLASS(cpu,
> __entry->cpu_id = cpu_id;
> ),
>
> - TP_printk("state=%lu cpu_id=%lu", (unsigned long)__entry->state,
> + TP_printk("state=%d cpu_id=%lu", __entry->state,
> (unsigned long)__entry->cpu_id)
> );
>
> -DEFINE_EVENT(cpu, cpu_idle,
> -
> - TP_PROTO(unsigned int state, unsigned int cpu_id),
> -
> - TP_ARGS(state, cpu_id)
> -);
> -
> TRACE_EVENT(powernv_throttle,
>
> TP_PROTO(int chip_id, const char *reason, int pmax),
> @@ -141,11 +134,24 @@ TRACE_EVENT(pstate_sample,
> { PM_EVENT_RESTORE, "restore" }, \
> { PM_EVENT_RECOVER, "recover" })
>
> -DEFINE_EVENT(cpu, cpu_frequency,
> +TRACE_EVENT(cpu_frequency,
>
> TP_PROTO(unsigned int frequency, unsigned int cpu_id),
>
> - TP_ARGS(frequency, cpu_id)
> + TP_ARGS(frequency, cpu_id),
> +
> + TP_STRUCT__entry(
> + __field( u32, frequency )
> + __field( u32, cpu_id )
> + ),
> +
> + TP_fast_assign(
> + __entry->frequency = frequency;
> + __entry->cpu_id = cpu_id;
> + ),
> +
> + TP_printk("frequency=%lu cpu_id=%lu", __entry->frequency,
> + (unsigned long)__entry->cpu_id)
> );
>
> TRACE_EVENT(device_pm_callback_start,
On Fri, Feb 09, 2018 at 09:44:58PM -0500, Steven Rostedt wrote:
> On Sat, 10 Feb 2018 09:37:04 +0800
> [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's not true. You split a class into two TRACE_EVENTS. Each
> TRACE_EVENT adds approximately 5k of code and data. A DEFINE_EVENT()
> adds around 300 bytes. There's better ways to do this,
>
> Please don't add this patch.
>
> -- Steve
Steve, How abount DEFINE_EVENT_PRINT as below?
diff --git a/include/trace/events/power.h b/include/trace/events/power.h
index 908977d..e71ce98 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,12 @@ DECLARE_EVENT_CLASS(cpu,
__entry->cpu_id = cpu_id;
),
- TP_printk("state=%lu cpu_id=%lu", (unsigned long)__entry->state,
- (unsigned long)__entry->cpu_id)
+ TP_printk("state=%d cpu_id=%u", __entry->state, __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 +140,13 @@ 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=%u cpu_id=%lu", __entry->state, __entry->cpu_id)
);
Hi Changbin,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on tip/perf/core]
[also build test WARNING on v4.16-rc1 next-20180212]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/changbin-du-intel-com/tracing-power-Don-t-share-template-for-cpu_idle-and-cpu_frequency/20180212-224719
config: i386-defconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All warnings (new ones prefixed by >>):
In file included from include/trace/define_trace.h:96:0,
from include/trace/events/power.h:516,
from kernel/trace/power-traces.c:15:
include/trace/events/power.h: In function 'trace_raw_output_cpu_frequency':
include/trace/events/power.h:153:12: warning: format '%lu' expects argument of type 'long unsigned int', but argument 3 has type 'u32 {aka unsigned int}' [-Wformat=]
TP_printk("frequency=%lu cpu_id=%lu", __entry->frequency,
^
include/trace/trace_events.h:360:22: note: in definition of macro 'DECLARE_EVENT_CLASS'
trace_seq_printf(s, print); \
^~~~~
include/trace/trace_events.h:79:9: note: in expansion of macro 'PARAMS'
PARAMS(print)); \
^~~~~~
>> include/trace/events/power.h:137:1: note: in expansion of macro 'TRACE_EVENT'
TRACE_EVENT(cpu_frequency,
^~~~~~~~~~~
>> include/trace/events/power.h:153:2: note: in expansion of macro 'TP_printk'
TP_printk("frequency=%lu cpu_id=%lu", __entry->frequency,
^~~~~~~~~
In file included from include/trace/trace_events.h:394:0,
from include/trace/define_trace.h:96,
from include/trace/events/power.h:516,
from kernel/trace/power-traces.c:15:
include/trace/events/power.h:153:25: note: format string is defined here
TP_printk("frequency=%lu cpu_id=%lu", __entry->frequency,
~~^
%u
vim +/TRACE_EVENT +137 include/trace/events/power.h
125
126 #define pm_verb_symbolic(event) \
127 __print_symbolic(event, \
128 { PM_EVENT_SUSPEND, "suspend" }, \
129 { PM_EVENT_RESUME, "resume" }, \
130 { PM_EVENT_FREEZE, "freeze" }, \
131 { PM_EVENT_QUIESCE, "quiesce" }, \
132 { PM_EVENT_HIBERNATE, "hibernate" }, \
133 { PM_EVENT_THAW, "thaw" }, \
134 { PM_EVENT_RESTORE, "restore" }, \
135 { PM_EVENT_RECOVER, "recover" })
136
> 137 TRACE_EVENT(cpu_frequency,
138
139 TP_PROTO(unsigned int frequency, unsigned int cpu_id),
140
141 TP_ARGS(frequency, cpu_id),
142
143 TP_STRUCT__entry(
144 __field( u32, frequency )
145 __field( u32, cpu_id )
146 ),
147
148 TP_fast_assign(
149 __entry->frequency = frequency;
150 __entry->cpu_id = cpu_id;
151 ),
152
> 153 TP_printk("frequency=%lu cpu_id=%lu", __entry->frequency,
154 (unsigned long)__entry->cpu_id)
155 );
156
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Changbin,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on tip/perf/core]
[also build test WARNING on v4.16-rc1 next-20180212]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/changbin-du-intel-com/tracing-power-Don-t-share-template-for-cpu_idle-and-cpu_frequency/20180212-224719
config: i386-randconfig-i1-201806 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All warnings (new ones prefixed by >>):
In file included from include/trace/define_trace.h:96:0,
from include/trace/events/power.h:516,
from kernel/trace/power-traces.c:15:
include/trace/events/power.h: In function 'trace_raw_output_cpu_frequency':
>> include/trace/events/power.h:153:12: warning: format '%lu' expects argument of type 'long unsigned int', but argument 3 has type 'u32 {aka unsigned int}' [-Wformat=]
TP_printk("frequency=%lu cpu_id=%lu", __entry->frequency,
^
include/trace/trace_events.h:360:22: note: in definition of macro 'DECLARE_EVENT_CLASS'
trace_seq_printf(s, print); \
^~~~~
include/trace/trace_events.h:79:9: note: in expansion of macro 'PARAMS'
PARAMS(print)); \
^~~~~~
include/trace/events/power.h:137:1: note: in expansion of macro 'TRACE_EVENT'
TRACE_EVENT(cpu_frequency,
^~~~~~~~~~~
include/trace/events/power.h:153:2: note: in expansion of macro 'TP_printk'
TP_printk("frequency=%lu cpu_id=%lu", __entry->frequency,
^~~~~~~~~
In file included from include/trace/trace_events.h:394:0,
from include/trace/define_trace.h:96,
from include/trace/events/power.h:516,
from kernel/trace/power-traces.c:15:
include/trace/events/power.h:153:25: note: format string is defined here
TP_printk("frequency=%lu cpu_id=%lu", __entry->frequency,
~~^
%u
vim +153 include/trace/events/power.h
138
139 TP_PROTO(unsigned int frequency, unsigned int cpu_id),
140
141 TP_ARGS(frequency, cpu_id),
142
143 TP_STRUCT__entry(
144 __field( u32, frequency )
145 __field( u32, cpu_id )
146 ),
147
148 TP_fast_assign(
149 __entry->frequency = frequency;
150 __entry->cpu_id = cpu_id;
151 ),
152
> 153 TP_printk("frequency=%lu cpu_id=%lu", __entry->frequency,
154 (unsigned long)__entry->cpu_id)
155 );
156
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Sun, 11 Feb 2018 18:50:04 +0800
"Du, Changbin" <[email protected]> wrote:
> Steve, How abount DEFINE_EVENT_PRINT as below?
Yes, DEFINE_EVENT_PRINT is better.
>
> diff --git a/include/trace/events/power.h b/include/trace/events/power.h
> index 908977d..e71ce98 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,12 @@ DECLARE_EVENT_CLASS(cpu,
> __entry->cpu_id = cpu_id;
> ),
>
> - TP_printk("state=%lu cpu_id=%lu", (unsigned long)__entry->state,
> - (unsigned long)__entry->cpu_id)
Yous still need the type casting, because s32/u32 on 32 bit machines
can be defined as "long".
-- Steve
> + TP_printk("state=%d cpu_id=%u", __entry->state, __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 +140,13 @@ 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=%u cpu_id=%lu", __entry->state, __entry->cpu_id)
> );
Thanks, I will improve this change in v2. And also update related docs.
On Mon, Feb 12, 2018 at 12:04:52PM -0500, Steven Rostedt wrote:
> On Sun, 11 Feb 2018 18:50:04 +0800
> "Du, Changbin" <[email protected]> wrote:
>
> > Steve, How abount DEFINE_EVENT_PRINT as below?
>
> Yes, DEFINE_EVENT_PRINT is better.
>
> >
> > diff --git a/include/trace/events/power.h b/include/trace/events/power.h
> > index 908977d..e71ce98 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,12 @@ DECLARE_EVENT_CLASS(cpu,
> > __entry->cpu_id = cpu_id;
> > ),
> >
> > - TP_printk("state=%lu cpu_id=%lu", (unsigned long)__entry->state,
> > - (unsigned long)__entry->cpu_id)
>
> Yous still need the type casting, because s32/u32 on 32 bit machines
> can be defined as "long".
>
> -- Steve
>
> > + TP_printk("state=%d cpu_id=%u", __entry->state, __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 +140,13 @@ 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=%u cpu_id=%lu", __entry->state, __entry->cpu_id)
> > );
>
--
Thanks,
Changbin Du