[PATCH] perf, record: Add clockid parameter

[PATCH] perf, record: Add clockid parameter

[Peter Zijlstra]

On Fri, Mar 27, 2015 at 04:48:08AM -0700, tip-bot for Peter Zijlstra wrote:
> perf: Add per event clockid support

And here the accompanying userspace; which I'd totally forgotten about.

XXX: do we want to store the clockid in the data file as well, such that
we can verify at perf-inject time the clocks match with our
expectations?

---
Subject: perf, record: Add clockid parameter

Teach perf-record about the new perf_event_attr::{use_clockid, clockid}
fields. Add a simple parameter to set the clock (if any) to be used for
the events to be recorded into the data file.

Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 tools/perf/builtin-record.c | 3 +++
 tools/perf/perf.h           | 1 +
 tools/perf/util/evsel.c     | 5 +++++
 3 files changed, 9 insertions(+)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 18aad239b401..9d4ed884b1c8 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -739,6 +739,7 @@ static struct record record = {
 			.uses_mmap   = true,
 			.default_per_cpu = true,
 		},
+		.clockid             = -1,
 	},
 	.tool = {
 		.sample		= process_sample_event,
@@ -842,6 +843,8 @@ struct option __record_options[] = {
 		    "Sample machine registers on interrupt"),
 	OPT_BOOLEAN(0, "running-time", &record.opts.running_time,
 		    "Record running/enabled time of read (:S) events"),
+	OPT_INTEGER('k', "clockid", &record.opts.clockid,
+		    "clockid to use for events"),
 	OPT_END()
 };
 
diff --git a/tools/perf/perf.h b/tools/perf/perf.h
index c38a085a5571..275c0c58fbbe 100644
--- a/tools/perf/perf.h
+++ b/tools/perf/perf.h
@@ -62,6 +62,7 @@ struct record_opts {
 	u64	     user_interval;
 	bool	     sample_transaction;
 	unsigned     initial_delay;
+	clockid_t    clockid;
 };
 
 struct option;
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 358e5954baa8..309208b16632 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -761,6 +761,11 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts)
 		attr->disabled = 0;
 		attr->enable_on_exec = 0;
 	}
+
+	if (opts->clockid >= 0) {
+		attr->use_clockid = 1;
+		attr->clockid = opts->clockid;
+	}
 }
 
 static int perf_evsel__alloc_fd(struct perf_evsel *evsel, int ncpus, int nthreads)

Re: [tip:perf/timer] perf: Add per event clockid support

[Stephane Eranian]

On Fri, Mar 27, 2015 at 4:48 AM, tip-bot for Peter Zijlstra
<tipbot@zytor.com> wrote:
> Commit-ID:  34f439278cef7b1177f8ce24f9fc81dfc6221d3b
> Gitweb:     http://git.kernel.org/tip/34f439278cef7b1177f8ce24f9fc81dfc6221d3b
> Author:     Peter Zijlstra <peterz@infradead.org>
> AuthorDate: Fri, 20 Feb 2015 14:05:38 +0100
> Committer:  Ingo Molnar <mingo@kernel.org>
> CommitDate: Fri, 27 Mar 2015 10:13:22 +0100
>
> perf: Add per event clockid support
>
> While thinking on the whole clock discussion it occurred to me we have
> two distinct uses of time:
>
>  1) the tracking of event/ctx/cgroup enabled/running/stopped times
>     which includes the self-monitoring support in struct
>     perf_event_mmap_page.
>
>  2) the actual timestamps visible in the data records.
>
> And we've been conflating them.
>
> The first is all about tracking time deltas, nobody should really care
> in what time base that happens, its all relative information, as long
> as its internally consistent it works.
>
> The second however is what people are worried about when having to
> merge their data with external sources. And here we have the
> discussion on MONOTONIC vs MONOTONIC_RAW etc..
>
> Where MONOTONIC is good for correlating between machines (static
> offset), MONOTNIC_RAW is required for correlating against a fixed rate
> hardware clock.
>
> This means configurability; now 1) makes that hard because it needs to
> be internally consistent across groups of unrelated events; which is
> why we had to have a global perf_clock().
>
> However, for 2) it doesn't really matter, perf itself doesn't care
> what it writes into the buffer.
>
> The below patch makes the distinction between these two cases by
> adding perf_event_clock() which is used for the second case. It
> further makes this configurable on a per-event basis, but adds a few
> sanity checks such that we cannot combine events with different clocks
> in confusing ways.
>
> And since we then have per-event configurability we might as well
> retain the 'legacy' behaviour as a default.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Cc: David Ahern <dsahern@gmail.com>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: John Stultz <john.stultz@linaro.org>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Stephane Eranian <eranian@google.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> ---
>  arch/x86/kernel/cpu/perf_event.c | 14 ++++++--
>  include/linux/perf_event.h       |  2 ++
>  include/uapi/linux/perf_event.h  |  6 ++--
>  kernel/events/core.c             | 77 ++++++++++++++++++++++++++++++++++++++--
>  4 files changed, 91 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
> index ac41b3a..0420ebc 100644
> --- a/arch/x86/kernel/cpu/perf_event.c
> +++ b/arch/x86/kernel/cpu/perf_event.c
> @@ -1978,13 +1978,23 @@ void arch_perf_update_userpage(struct perf_event *event,
>
>         data = cyc2ns_read_begin();
>
> +       /*
> +        * Internal timekeeping for enabled/running/stopped times
> +        * is always in the local_clock domain.
> +        */
>         userpg->cap_user_time = 1;
>         userpg->time_mult = data->cyc2ns_mul;
>         userpg->time_shift = data->cyc2ns_shift;
>         userpg->time_offset = data->cyc2ns_offset - now;
>
> -       userpg->cap_user_time_zero = 1;
> -       userpg->time_zero = data->cyc2ns_offset;
> +       /*
> +        * cap_user_time_zero doesn't make sense when we're using a different
> +        * time base for the records.
> +        */
> +       if (event->clock == &local_clock) {
> +               userpg->cap_user_time_zero = 1;
> +               userpg->time_zero = data->cyc2ns_offset;
> +       }
>
>         cyc2ns_read_end(data);
>  }
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index b16eac5..4015540 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -173,6 +173,7 @@ struct perf_event;
>   * pmu::capabilities flags
>   */
>  #define PERF_PMU_CAP_NO_INTERRUPT              0x01
> +#define PERF_PMU_CAP_NO_NMI                    0x02
>
>  /**
>   * struct pmu - generic performance monitoring unit
> @@ -457,6 +458,7 @@ struct perf_event {
>         struct pid_namespace            *ns;
>         u64                             id;
>
> +       u64                             (*clock)(void);
>         perf_overflow_handler_t         overflow_handler;
>         void                            *overflow_handler_context;
>
> diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
> index 1e3cd07..3bb40dda 100644
> --- a/include/uapi/linux/perf_event.h
> +++ b/include/uapi/linux/perf_event.h
> @@ -326,7 +326,8 @@ struct perf_event_attr {
>                                 exclude_callchain_user   : 1, /* exclude user callchains */
>                                 mmap2          :  1, /* include mmap with inode data     */
>                                 comm_exec      :  1, /* flag comm events that are due to an exec */
> -                               __reserved_1   : 39;
> +                               use_clockid    :  1, /* use @clockid for time fields */
> +                               __reserved_1   : 38;
>
>         union {
>                 __u32           wakeup_events;    /* wakeup every n events */
> @@ -355,8 +356,7 @@ struct perf_event_attr {
>          */
>         __u32   sample_stack_user;
>
> -       /* Align to u64. */
> -       __u32   __reserved_2;
> +       __s32   clockid;
>         /*
>          * Defines set of regs to dump for each sample
>          * state captured on:
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index bb1a7c3..c40c2ca 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -327,6 +327,11 @@ static inline u64 perf_clock(void)
>         return local_clock();
>  }
>
> +static inline u64 perf_event_clock(struct perf_event *event)
> +{
> +       return event->clock();
> +}
> +
>  static inline struct perf_cpu_context *
>  __get_cpu_context(struct perf_event_context *ctx)
>  {
> @@ -4762,7 +4767,7 @@ static void __perf_event_header__init_id(struct perf_event_header *header,
>         }
>
>         if (sample_type & PERF_SAMPLE_TIME)
> -               data->time = perf_clock();
> +               data->time = perf_event_clock(event);
>
>         if (sample_type & (PERF_SAMPLE_ID | PERF_SAMPLE_IDENTIFIER))
>                 data->id = primary_event_id(event);
> @@ -5340,6 +5345,8 @@ static void perf_event_task_output(struct perf_event *event,
>         task_event->event_id.tid = perf_event_tid(event, task);
>         task_event->event_id.ptid = perf_event_tid(event, current);
>
> +       task_event->event_id.time = perf_event_clock(event);
> +
>         perf_output_put(&handle, task_event->event_id);
>
>         perf_event__output_id_sample(event, &handle, &sample);
> @@ -5373,7 +5380,7 @@ static void perf_event_task(struct task_struct *task,
>                         /* .ppid */
>                         /* .tid  */
>                         /* .ptid */
> -                       .time = perf_clock(),
> +                       /* .time */
>                 },
>         };
>
> @@ -5749,7 +5756,7 @@ static void perf_log_throttle(struct perf_event *event, int enable)
>                         .misc = 0,
>                         .size = sizeof(throttle_event),
>                 },
> -               .time           = perf_clock(),
> +               .time           = perf_event_clock(event),
>                 .id             = primary_event_id(event),
>                 .stream_id      = event->id,
>         };
> @@ -6293,6 +6300,8 @@ static int perf_swevent_init(struct perf_event *event)
>  static struct pmu perf_swevent = {
>         .task_ctx_nr    = perf_sw_context,
>
> +       .capabilities   = PERF_PMU_CAP_NO_NMI,
> +
>         .event_init     = perf_swevent_init,
>         .add            = perf_swevent_add,
>         .del            = perf_swevent_del,
> @@ -6636,6 +6645,8 @@ static int cpu_clock_event_init(struct perf_event *event)
>  static struct pmu perf_cpu_clock = {
>         .task_ctx_nr    = perf_sw_context,
>
> +       .capabilities   = PERF_PMU_CAP_NO_NMI,
> +
>         .event_init     = cpu_clock_event_init,
>         .add            = cpu_clock_event_add,
>         .del            = cpu_clock_event_del,
> @@ -6715,6 +6726,8 @@ static int task_clock_event_init(struct perf_event *event)
>  static struct pmu perf_task_clock = {
>         .task_ctx_nr    = perf_sw_context,
>
> +       .capabilities   = PERF_PMU_CAP_NO_NMI,
> +
>         .event_init     = task_clock_event_init,
>         .add            = task_clock_event_add,
>         .del            = task_clock_event_del,
> @@ -7200,6 +7213,10 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu,
>                 event->hw.target = task;
>         }
>
> +       event->clock = &local_clock;
> +       if (parent_event)
> +               event->clock = parent_event->clock;
> +
>         if (!overflow_handler && parent_event) {
>                 overflow_handler = parent_event->overflow_handler;
>                 context = parent_event->overflow_handler_context;
> @@ -7422,6 +7439,12 @@ perf_event_set_output(struct perf_event *event, struct perf_event *output_event)
>         if (output_event->cpu == -1 && output_event->ctx != event->ctx)
>                 goto out;
>
> +       /*
> +        * Mixing clocks in the same buffer is trouble you don't need.
> +        */
> +       if (output_event->clock != event->clock)
> +               goto out;
> +
>  set:
>         mutex_lock(&event->mmap_mutex);
>         /* Can't redirect output if we've got an active mmap() */
> @@ -7454,6 +7477,43 @@ static void mutex_lock_double(struct mutex *a, struct mutex *b)
>         mutex_lock_nested(b, SINGLE_DEPTH_NESTING);
>  }
>
> +static int perf_event_set_clock(struct perf_event *event, clockid_t clk_id)
> +{
> +       bool nmi_safe = false;
> +
> +       switch (clk_id) {
> +       case CLOCK_MONOTONIC:
> +               event->clock = &ktime_get_mono_fast_ns;
> +               nmi_safe = true;
> +               break;
> +
> +       case CLOCK_MONOTONIC_RAW:
> +               event->clock = &ktime_get_raw_fast_ns;
> +               nmi_safe = true;
> +               break;
> +
> +       case CLOCK_REALTIME:
> +               event->clock = &ktime_get_real_ns;
> +               break;
> +
> +       case CLOCK_BOOTTIME:
> +               event->clock = &ktime_get_boot_ns;
> +               break;
> +
> +       case CLOCK_TAI:
> +               event->clock = &ktime_get_tai_ns;
> +               break;
> +
Can all those clocks be safely called from an NMI context?

Re: [tip:perf/timer] perf: Add per event clockid support

[Peter Zijlstra]

On Fri, Mar 27, 2015 at 09:31:45AM -0700, Stephane Eranian wrote:
> On Fri, Mar 27, 2015 at 4:48 AM, tip-bot for Peter Zijlstra
> > +static int perf_event_set_clock(struct perf_event *event, clockid_t clk_id)
> > +{
> > +       bool nmi_safe = false;
> > +
> > +       switch (clk_id) {
> > +       case CLOCK_MONOTONIC:
> > +               event->clock = &ktime_get_mono_fast_ns;
> > +               nmi_safe = true;
> > +               break;
> > +
> > +       case CLOCK_MONOTONIC_RAW:
> > +               event->clock = &ktime_get_raw_fast_ns;
> > +               nmi_safe = true;
> > +               break;
> > +
> > +       case CLOCK_REALTIME:
> > +               event->clock = &ktime_get_real_ns;
> > +               break;
> > +
> > +       case CLOCK_BOOTTIME:
> > +               event->clock = &ktime_get_boot_ns;
> > +               break;
> > +
> > +       case CLOCK_TAI:
> > +               event->clock = &ktime_get_tai_ns;
> > +               break;
> > +
> Can all those clocks be safely called from an NMI context?

+       if (!nmi_safe && !(event->pmu->capabilities & PERF_PMU_CAP_NO_NMI))
+               return -EINVAL;

no :-)

Re: [tip:perf/timer] perf: Add per event clockid support

[Stephane Eranian]

On Fri, Mar 27, 2015 at 9:35 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Fri, Mar 27, 2015 at 09:31:45AM -0700, Stephane Eranian wrote:
>> On Fri, Mar 27, 2015 at 4:48 AM, tip-bot for Peter Zijlstra
>> > +static int perf_event_set_clock(struct perf_event *event, clockid_t clk_id)
>> > +{
>> > +       bool nmi_safe = false;
>> > +
>> > +       switch (clk_id) {
>> > +       case CLOCK_MONOTONIC:
>> > +               event->clock = &ktime_get_mono_fast_ns;
>> > +               nmi_safe = true;
>> > +               break;
>> > +
>> > +       case CLOCK_MONOTONIC_RAW:
>> > +               event->clock = &ktime_get_raw_fast_ns;
>> > +               nmi_safe = true;
>> > +               break;
>> > +
>> > +       case CLOCK_REALTIME:
>> > +               event->clock = &ktime_get_real_ns;
>> > +               break;
>> > +
>> > +       case CLOCK_BOOTTIME:
>> > +               event->clock = &ktime_get_boot_ns;
>> > +               break;
>> > +
>> > +       case CLOCK_TAI:
>> > +               event->clock = &ktime_get_tai_ns;
>> > +               break;
>> > +
>> Can all those clocks be safely called from an NMI context?
>
> +       if (!nmi_safe && !(event->pmu->capabilities & PERF_PMU_CAP_NO_NMI))
> +               return -EINVAL;
>
> no :-)

Ok, I see. But on architectures which do not have NMI, they would all
be safe. And
that would work if they set the PERF_PMU_CAP_NO_NMI flag on their
pmu->capabilities.

Next, I am trying to understand how perf is going to expose this. I am
thinking about this
in the context of my jitted code patches. With this approach, the jit
runtime and the perf
tool need to agree on the clock they are using. That mean they need to
advertise or
document the clock they use and there needs to be an option in perf
record to pass
that same clockid.

In my current patch series, using Pawel's clock patch, the kernel advertises in
procfs the clockid used (all events use the same). The jit runtime simply reads
the id and uses it to timestamp jit events.

Re: [tip:perf/timer] perf: Add per event clockid support

[Peter Zijlstra]

On Fri, Mar 27, 2015 at 09:52:25AM -0700, Stephane Eranian wrote:
> Ok, I see. But on architectures which do not have NMI, they would all
> be safe. And that would work if they set the PERF_PMU_CAP_NO_NMI flag
> on their pmu->capabilities.

Indeed so. I have not audited them all as the current 'default' is safe.

> Next, I am trying to understand how perf is going to expose this. I am
> thinking about this in the context of my jitted code patches.

I send you some preliminary patches for this.

> With this approach, the jit runtime and the perf tool need to agree on
> the clock they are using. That mean they need to advertise or document
> the clock they use

This is indeed still missing, I put an XXX question in one of the
emails to this effect. I suppose we should store the used clockid in the
data file. I just haven't figured out how all that code works.

> and there needs to be an option in perf record to pass that same
> clockid.

I did indeed post a patch to do that.

Re: [tip:perf/timer] perf: Add per event clockid support

[Stephane Eranian]

On Fri, Mar 27, 2015 at 9:57 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Fri, Mar 27, 2015 at 09:52:25AM -0700, Stephane Eranian wrote:
>> Ok, I see. But on architectures which do not have NMI, they would all
>> be safe. And that would work if they set the PERF_PMU_CAP_NO_NMI flag
>> on their pmu->capabilities.
>
> Indeed so. I have not audited them all as the current 'default' is safe.
>
>> Next, I am trying to understand how perf is going to expose this. I am
>> thinking about this in the context of my jitted code patches.
>
> I send you some preliminary patches for this.
>
>> With this approach, the jit runtime and the perf tool need to agree on
>> the clock they are using. That mean they need to advertise or document
>> the clock they use
>
> This is indeed still missing, I put an XXX question in one of the
> emails to this effect. I suppose we should store the used clockid in the
> data file. I just haven't figured out how all that code works.
>
>> and there needs to be an option in perf record to pass that same
>> clockid.
>
> I did indeed post a patch to do that.

I will switch my jitted patches to your approach to verify that this
actually works.
Thanks.

Re: [PATCH] perf, record: Add clockid parameter

[David Ahern]

On 3/27/15 8:32 AM, Peter Zijlstra wrote:
> On Fri, Mar 27, 2015 at 04:48:08AM -0700, tip-bot for Peter Zijlstra wrote:
>> perf: Add per event clockid support
>
> And here the accompanying userspace; which I'd totally forgotten about.
>
> XXX: do we want to store the clockid in the data file as well, such that
> we can verify at perf-inject time the clocks match with our
> expectations?
>
> ---
> Subject: perf, record: Add clockid parameter
>
> Teach perf-record about the new perf_event_attr::{use_clockid, clockid}
> fields. Add a simple parameter to set the clock (if any) to be used for
> the events to be recorded into the data file.
>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>   tools/perf/builtin-record.c | 3 +++
>   tools/perf/perf.h           | 1 +
>   tools/perf/util/evsel.c     | 5 +++++
>   3 files changed, 9 insertions(+)

missing Documentation/perf-record.txt update

>
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 18aad239b401..9d4ed884b1c8 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -739,6 +739,7 @@ static struct record record = {
>   			.uses_mmap   = true,
>   			.default_per_cpu = true,
>   		},
> +		.clockid             = -1,
>   	},
>   	.tool = {
>   		.sample		= process_sample_event,
> @@ -842,6 +843,8 @@ struct option __record_options[] = {
>   		    "Sample machine registers on interrupt"),
>   	OPT_BOOLEAN(0, "running-time", &record.opts.running_time,
>   		    "Record running/enabled time of read (:S) events"),
> +	OPT_INTEGER('k', "clockid", &record.opts.clockid,
> +		    "clockid to use for events"),
>   	OPT_END()
>   };
>
> diff --git a/tools/perf/perf.h b/tools/perf/perf.h
> index c38a085a5571..275c0c58fbbe 100644
> --- a/tools/perf/perf.h
> +++ b/tools/perf/perf.h
> @@ -62,6 +62,7 @@ struct record_opts {
>   	u64	     user_interval;
>   	bool	     sample_transaction;
>   	unsigned     initial_delay;
> +	clockid_t    clockid;
>   };
>
>   struct option;
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 358e5954baa8..309208b16632 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -761,6 +761,11 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts)
>   		attr->disabled = 0;
>   		attr->enable_on_exec = 0;
>   	}
> +
> +	if (opts->clockid >= 0) {
> +		attr->use_clockid = 1;
> +		attr->clockid = opts->clockid;
> +	}
>   }
>
>   static int perf_evsel__alloc_fd(struct perf_evsel *evsel, int ncpus, int nthreads)
>

This is a new feature which means use_clockid on older kernels will 
fail. So need to catch that and throw an error -- perhaps yet another 
probe function.

Also, if the intent is to allow clock selection per event should there 
be an event modifier as well (see get_event_modifier())?

Re: [PATCH] perf, record: Add clockid parameter

[Peter Zijlstra]

On Fri, Mar 27, 2015 at 11:11:01AM -0600, David Ahern wrote:
> >  tools/perf/builtin-record.c | 3 +++
> >  tools/perf/perf.h           | 1 +
> >  tools/perf/util/evsel.c     | 5 +++++
> >  3 files changed, 9 insertions(+)
> 
> missing Documentation/perf-record.txt update

--- a/tools/perf/Documentation/perf-record.txt
+++ b/tools/perf/Documentation/perf-record.txt
@@ -250,6 +250,11 @@ is off by default.
 --running-time::
 Record running and enabled time for read events (:S)
 
+-k::
+--clockid::
+Sets the clock id to use for the various time fields in the perf_event_type
+records. See clock_gettime().
+
 SEE ALSO
 --------
 linkperf:perf-stat[1], linkperf:perf-list[1]


> >+++ b/tools/perf/util/evsel.c
> >@@ -761,6 +761,11 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts)
> >  		attr->disabled = 0;
> >  		attr->enable_on_exec = 0;
> >  	}
> >+
> >+	if (opts->clockid >= 0) {
> >+		attr->use_clockid = 1;
> >+		attr->clockid = opts->clockid;
> >+	}
> >  }
> >
> >  static int perf_evsel__alloc_fd(struct perf_evsel *evsel, int ncpus, int nthreads)
> >
> 
> This is a new feature which means use_clockid on older kernels will fail. So
> need to catch that and throw an error -- perhaps yet another probe function.

How does that work? What do I grep to find an example? I figured if the
kernel didn't support the syscall will fail and we'll terminate
someplace.

> Also, if the intent is to allow clock selection per event should there be an
> event modifier as well (see get_event_modifier())?

Nah, its not generally useful to mix clocks in a single recording. That
gets real confused real quick on when which event happened.

The per event configurability is handy to allow different (concurrent)
records different settings.

Re: [PATCH] perf, record: Add clockid parameter

[David Ahern]

On 3/27/15 11:20 AM, Peter Zijlstra wrote:
>> This is a new feature which means use_clockid on older kernels will fail. So
>> need to catch that and throw an error -- perhaps yet another probe function.
>
> How does that work? What do I grep to find an example? I figured if the
> kernel didn't support the syscall will fail and we'll terminate
> someplace.
>

Look at __perf_evsel__open(). In this case you probably do not want to 
fallback but tell the user the clock id option is not supported. The 
problem is deciphering the failure is due to the clock id versus all the 
other failure reasons.

Re: [PATCH] perf, record: Add clockid parameter

[Arnaldo Carvalho de Melo]

Em Fri, Mar 27, 2015 at 11:35:25AM -0600, David Ahern escreveu:
> On 3/27/15 11:20 AM, Peter Zijlstra wrote:
> >>This is a new feature which means use_clockid on older kernels will fail. So
> >>need to catch that and throw an error -- perhaps yet another probe function.

> >How does that work? What do I grep to find an example? I figured if the
> >kernel didn't support the syscall will fail and we'll terminate
> >someplace.

> Look at __perf_evsel__open(). In this case you probably do not want
> to fallback but tell the user the clock id option is not supported.
> The problem is deciphering the failure is due to the clock id versus
> all the other failure reasons.

I.e. we're back to the sys_perf_event_open() error reporting suckz rockz
thing, this time with PeterZ trying to find a way to avoid getting back
to that discussion... /me runz... ;-P

- Arnaldo

Re: [PATCH] perf, record: Add clockid parameter

[Peter Zijlstra]

On Fri, Mar 27, 2015 at 05:15:34PM -0300, Arnaldo Carvalho de Melo wrote:
> I.e. we're back to the sys_perf_event_open() error reporting suckz rockz
> thing, this time with PeterZ trying to find a way to avoid getting back
> to that discussion... /me runz... ;-P

:-)

Not entirely, its just that I've not seen this userspace code in years
and I'm pretty clueless.

How about so then?

---
Subject: perf, record: Add clockid parameter
From: Peter Zijlstra <peterz@infradead.org>
Date: Fri, 27 Mar 2015 15:32:01 +0100

Teach perf-record about the new perf_event_attr::{use_clockid, clockid}
fields. Add a simple parameter to set the clock (if any) to be used for
the events to be recorded into the data file.

Since we store the entire perf_event_attr in the EVENT_DESC section we
also already store the used clockid in the data file.

Cc: acme@redhat.com
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---

 tools/perf/Documentation/perf-record.txt |    5 +++
 tools/perf/builtin-record.c              |    3 ++
 tools/perf/perf.h                        |    1 
 tools/perf/util/evsel.c                  |   44 ++++++++++++++++++++++++++++---
 4 files changed, 49 insertions(+), 4 deletions(-)

--- a/tools/perf/Documentation/perf-record.txt
+++ b/tools/perf/Documentation/perf-record.txt
@@ -250,6 +250,11 @@ is off by default.
 --running-time::
 Record running and enabled time for read events (:S)
 
+-k::
+--clockid::
+Sets the clock id to use for the various time fields in the perf_event_type
+records. See clock_gettime().
+
 SEE ALSO
 --------
 linkperf:perf-stat[1], linkperf:perf-list[1]
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -739,6 +739,7 @@ static struct record record = {
 			.uses_mmap   = true,
 			.default_per_cpu = true,
 		},
+		.clockid             = -1,
 	},
 	.tool = {
 		.sample		= process_sample_event,
@@ -842,6 +843,8 @@ struct option __record_options[] = {
 		    "Sample machine registers on interrupt"),
 	OPT_BOOLEAN(0, "running-time", &record.opts.running_time,
 		    "Record running/enabled time of read (:S) events"),
+	OPT_INTEGER('k', "clockid", &record.opts.clockid,
+		    "clockid to use for events"),
 	OPT_END()
 };
 
--- a/tools/perf/perf.h
+++ b/tools/perf/perf.h
@@ -62,6 +62,7 @@ struct record_opts {
 	u64	     user_interval;
 	bool	     sample_transaction;
 	unsigned     initial_delay;
+	clockid_t    clockid;
 };
 
 struct option;
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -32,6 +32,7 @@ static struct {
 	bool exclude_guest;
 	bool mmap2;
 	bool cloexec;
+	bool clockid;
 } perf_missing_features;
 
 static int perf_evsel__no_extra_init(struct perf_evsel *evsel __maybe_unused)
@@ -761,6 +762,11 @@ void perf_evsel__config(struct perf_evse
 		attr->disabled = 0;
 		attr->enable_on_exec = 0;
 	}
+
+	if (opts->clockid >= 0) {
+		attr->use_clockid = 1;
+		attr->clockid = opts->clockid;
+	}
 }
 
 static int perf_evsel__alloc_fd(struct perf_evsel *evsel, int ncpus, int nthreads)
@@ -1036,7 +1042,6 @@ static size_t perf_event_attr__fprintf(s
 	ret += PRINT_ATTR2(exclude_user, exclude_kernel);
 	ret += PRINT_ATTR2(exclude_hv, exclude_idle);
 	ret += PRINT_ATTR2(mmap, comm);
-	ret += PRINT_ATTR2(mmap2, comm_exec);
 	ret += PRINT_ATTR2(freq, inherit_stat);
 	ret += PRINT_ATTR2(enable_on_exec, task);
 	ret += PRINT_ATTR2(watermark, precise_ip);
@@ -1044,6 +1049,8 @@ static size_t perf_event_attr__fprintf(s
 	ret += PRINT_ATTR2(exclude_host, exclude_guest);
 	ret += PRINT_ATTR2N("excl.callchain_kern", exclude_callchain_kernel,
 			    "excl.callchain_user", exclude_callchain_user);
+	ret += PRINT_ATTR2(mmap2, comm_exec);
+	ret += __PRINT_ATTR("%u",,use_clockid);
 
 	ret += PRINT_ATTR_U32(wakeup_events);
 	ret += PRINT_ATTR_U32(wakeup_watermark);
@@ -1055,6 +1062,7 @@ static size_t perf_event_attr__fprintf(s
 	ret += PRINT_ATTR_X64(branch_sample_type);
 	ret += PRINT_ATTR_X64(sample_regs_user);
 	ret += PRINT_ATTR_U32(sample_stack_user);
+	ret += PRINT_ATTR_U32(clockid);
 	ret += PRINT_ATTR_X64(sample_regs_intr);
 
 	ret += fprintf(fp, "%.60s\n", graph_dotted_line);
@@ -1085,6 +1093,8 @@ static int __perf_evsel__open(struct per
 	}
 
 fallback_missing_features:
+	if (perf_missing_features.clockid)
+		evsel->attr.use_clockid = 0;
 	if (perf_missing_features.cloexec)
 		flags &= ~(unsigned long)PERF_FLAG_FD_CLOEXEC;
 	if (perf_missing_features.mmap2)
@@ -1122,6 +1132,16 @@ static int __perf_evsel__open(struct per
 				goto try_fallback;
 			}
 			set_rlimit = NO_CHANGE;
+
+			/*
+			 * If we succeeded but had to kill clockid, fail and
+			 * have perf_evsel__open_strerror() print us a nice
+			 * error.
+			 */
+			if (perf_missing_features.clockid) {
+				err = -EINVAL;
+				goto out_close;
+			}
 		}
 	}
 
@@ -1155,7 +1175,10 @@ static int __perf_evsel__open(struct per
 	if (err != -EINVAL || cpu > 0 || thread > 0)
 		goto out_close;
 
-	if (!perf_missing_features.cloexec && (flags & PERF_FLAG_FD_CLOEXEC)) {
+	if (!perf_missing_features.clockid && evsel->attr.use_clockid) {
+		perf_missing_features.clockid = true;
+		goto fallback_missing_features;
+	} else if (!perf_missing_features.cloexec && (flags & PERF_FLAG_FD_CLOEXEC)) {
 		perf_missing_features.cloexec = true;
 		goto fallback_missing_features;
 	} else if (!perf_missing_features.mmap2 && evsel->attr.mmap2) {
@@ -2063,9 +2086,7 @@ int perf_evsel__fprintf(struct perf_evse
 		if_print(exclude_hv);
 		if_print(exclude_idle);
 		if_print(mmap);
-		if_print(mmap2);
 		if_print(comm);
-		if_print(comm_exec);
 		if_print(freq);
 		if_print(inherit_stat);
 		if_print(enable_on_exec);
@@ -2076,10 +2097,19 @@ int perf_evsel__fprintf(struct perf_evse
 		if_print(sample_id_all);
 		if_print(exclude_host);
 		if_print(exclude_guest);
+		if_print(exclude_callchain_kernel);
+		if_print(exclude_callchain_user);
+		if_print(mmap2);
+		if_print(comm_exec);
+		if_print(use_clockid);
 		if_print(__reserved_1);
 		if_print(wakeup_events);
 		if_print(bp_type);
 		if_print(branch_sample_type);
+		if_print(sample_regs_user);
+		if_print(sample_stack_user);
+		if_print(clockid);
+		if_print(sample_regs_intr);
 	}
 out:
 	fputc('\n', fp);
@@ -2158,6 +2188,12 @@ int perf_evsel__open_strerror(struct per
 	"The PMU counters are busy/taken by another profiler.\n"
 	"We found oprofile daemon running, please stop it and try again.");
 		break;
+
+	case EINVAL:
+		if (perf_missing_features.clockid)
+			return scnprintf(msg, size, "%s", "clockid not supported.");
+		break;
+
 	default:
 		break;
 	}

Re: [PATCH] perf, record: Add clockid parameter

[Stephane Eranian]

On Fri, Mar 27, 2015 at 2:59 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Fri, Mar 27, 2015 at 05:15:34PM -0300, Arnaldo Carvalho de Melo wrote:
>> I.e. we're back to the sys_perf_event_open() error reporting suckz rockz
>> thing, this time with PeterZ trying to find a way to avoid getting back
>> to that discussion... /me runz... ;-P
>
> :-)
>
> Not entirely, its just that I've not seen this userspace code in years
> and I'm pretty clueless.
>
> How about so then?
>
> ---
> Subject: perf, record: Add clockid parameter
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Fri, 27 Mar 2015 15:32:01 +0100
>
> Teach perf-record about the new perf_event_attr::{use_clockid, clockid}
> fields. Add a simple parameter to set the clock (if any) to be used for
> the events to be recorded into the data file.
>
> Since we store the entire perf_event_attr in the EVENT_DESC section we
> also already store the used clockid in the data file.
>
> Cc: acme@redhat.com
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>
>  tools/perf/Documentation/perf-record.txt |    5 +++
>  tools/perf/builtin-record.c              |    3 ++
>  tools/perf/perf.h                        |    1
>  tools/perf/util/evsel.c                  |   44 ++++++++++++++++++++++++++++---
>  4 files changed, 49 insertions(+), 4 deletions(-)
>
> --- a/tools/perf/Documentation/perf-record.txt
> +++ b/tools/perf/Documentation/perf-record.txt
> @@ -250,6 +250,11 @@ is off by default.
>  --running-time::
>  Record running and enabled time for read events (:S)
>
> +-k::
> +--clockid::
> +Sets the clock id to use for the various time fields in the perf_event_type
> +records. See clock_gettime().
> +
>  SEE ALSO
>  --------
>  linkperf:perf-stat[1], linkperf:perf-list[1]
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -739,6 +739,7 @@ static struct record record = {
>                         .uses_mmap   = true,
>                         .default_per_cpu = true,
>                 },
> +               .clockid             = -1,
>         },
>         .tool = {
>                 .sample         = process_sample_event,
> @@ -842,6 +843,8 @@ struct option __record_options[] = {
>                     "Sample machine registers on interrupt"),
>         OPT_BOOLEAN(0, "running-time", &record.opts.running_time,
>                     "Record running/enabled time of read (:S) events"),
> +       OPT_INTEGER('k', "clockid", &record.opts.clockid,
> +                   "clockid to use for events"),

I think you'd want a symbolic name for the clock to make this easier
on the user.

>         OPT_END()
>  };
>
> --- a/tools/perf/perf.h
> +++ b/tools/perf/perf.h
> @@ -62,6 +62,7 @@ struct record_opts {
>         u64          user_interval;
>         bool         sample_transaction;
>         unsigned     initial_delay;
> +       clockid_t    clockid;
>  };
>
>  struct option;
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -32,6 +32,7 @@ static struct {
>         bool exclude_guest;
>         bool mmap2;
>         bool cloexec;
> +       bool clockid;
>  } perf_missing_features;
>
>  static int perf_evsel__no_extra_init(struct perf_evsel *evsel __maybe_unused)
> @@ -761,6 +762,11 @@ void perf_evsel__config(struct perf_evse
>                 attr->disabled = 0;
>                 attr->enable_on_exec = 0;
>         }
> +
> +       if (opts->clockid >= 0) {
> +               attr->use_clockid = 1;
> +               attr->clockid = opts->clockid;
> +       }
>  }
>
>  static int perf_evsel__alloc_fd(struct perf_evsel *evsel, int ncpus, int nthreads)
> @@ -1036,7 +1042,6 @@ static size_t perf_event_attr__fprintf(s
>         ret += PRINT_ATTR2(exclude_user, exclude_kernel);
>         ret += PRINT_ATTR2(exclude_hv, exclude_idle);
>         ret += PRINT_ATTR2(mmap, comm);
> -       ret += PRINT_ATTR2(mmap2, comm_exec);
>         ret += PRINT_ATTR2(freq, inherit_stat);
>         ret += PRINT_ATTR2(enable_on_exec, task);
>         ret += PRINT_ATTR2(watermark, precise_ip);
> @@ -1044,6 +1049,8 @@ static size_t perf_event_attr__fprintf(s
>         ret += PRINT_ATTR2(exclude_host, exclude_guest);
>         ret += PRINT_ATTR2N("excl.callchain_kern", exclude_callchain_kernel,
>                             "excl.callchain_user", exclude_callchain_user);
> +       ret += PRINT_ATTR2(mmap2, comm_exec);
> +       ret += __PRINT_ATTR("%u",,use_clockid);
>
>         ret += PRINT_ATTR_U32(wakeup_events);
>         ret += PRINT_ATTR_U32(wakeup_watermark);
> @@ -1055,6 +1062,7 @@ static size_t perf_event_attr__fprintf(s
>         ret += PRINT_ATTR_X64(branch_sample_type);
>         ret += PRINT_ATTR_X64(sample_regs_user);
>         ret += PRINT_ATTR_U32(sample_stack_user);
> +       ret += PRINT_ATTR_U32(clockid);
>         ret += PRINT_ATTR_X64(sample_regs_intr);
>
>         ret += fprintf(fp, "%.60s\n", graph_dotted_line);
> @@ -1085,6 +1093,8 @@ static int __perf_evsel__open(struct per
>         }
>
>  fallback_missing_features:
> +       if (perf_missing_features.clockid)
> +               evsel->attr.use_clockid = 0;
>         if (perf_missing_features.cloexec)
>                 flags &= ~(unsigned long)PERF_FLAG_FD_CLOEXEC;
>         if (perf_missing_features.mmap2)
> @@ -1122,6 +1132,16 @@ static int __perf_evsel__open(struct per
>                                 goto try_fallback;
>                         }
>                         set_rlimit = NO_CHANGE;
> +
> +                       /*
> +                        * If we succeeded but had to kill clockid, fail and
> +                        * have perf_evsel__open_strerror() print us a nice
> +                        * error.
> +                        */
> +                       if (perf_missing_features.clockid) {
> +                               err = -EINVAL;
> +                               goto out_close;
> +                       }
>                 }
>         }
>
> @@ -1155,7 +1175,10 @@ static int __perf_evsel__open(struct per
>         if (err != -EINVAL || cpu > 0 || thread > 0)
>                 goto out_close;
>
> -       if (!perf_missing_features.cloexec && (flags & PERF_FLAG_FD_CLOEXEC)) {
> +       if (!perf_missing_features.clockid && evsel->attr.use_clockid) {
> +               perf_missing_features.clockid = true;
> +               goto fallback_missing_features;
> +       } else if (!perf_missing_features.cloexec && (flags & PERF_FLAG_FD_CLOEXEC)) {
>                 perf_missing_features.cloexec = true;
>                 goto fallback_missing_features;
>         } else if (!perf_missing_features.mmap2 && evsel->attr.mmap2) {
> @@ -2063,9 +2086,7 @@ int perf_evsel__fprintf(struct perf_evse
>                 if_print(exclude_hv);
>                 if_print(exclude_idle);
>                 if_print(mmap);
> -               if_print(mmap2);
>                 if_print(comm);
> -               if_print(comm_exec);
>                 if_print(freq);
>                 if_print(inherit_stat);
>                 if_print(enable_on_exec);
> @@ -2076,10 +2097,19 @@ int perf_evsel__fprintf(struct perf_evse
>                 if_print(sample_id_all);
>                 if_print(exclude_host);
>                 if_print(exclude_guest);
> +               if_print(exclude_callchain_kernel);
> +               if_print(exclude_callchain_user);
> +               if_print(mmap2);
> +               if_print(comm_exec);
> +               if_print(use_clockid);
>                 if_print(__reserved_1);
>                 if_print(wakeup_events);
>                 if_print(bp_type);
>                 if_print(branch_sample_type);
> +               if_print(sample_regs_user);
> +               if_print(sample_stack_user);
> +               if_print(clockid);
> +               if_print(sample_regs_intr);
>         }
>  out:
>         fputc('\n', fp);
> @@ -2158,6 +2188,12 @@ int perf_evsel__open_strerror(struct per
>         "The PMU counters are busy/taken by another profiler.\n"
>         "We found oprofile daemon running, please stop it and try again.");
>                 break;
> +
> +       case EINVAL:
> +               if (perf_missing_features.clockid)
> +                       return scnprintf(msg, size, "%s", "clockid not supported.");
> +               break;
> +
>         default:
>                 break;
>         }

Re: [PATCH] perf, record: Add clockid parameter

[Stephane Eranian]

On Fri, Mar 27, 2015 at 1:15 PM, Arnaldo Carvalho de Melo
<acme@redhat.com> wrote:
> Em Fri, Mar 27, 2015 at 11:35:25AM -0600, David Ahern escreveu:
>> On 3/27/15 11:20 AM, Peter Zijlstra wrote:
>> >>This is a new feature which means use_clockid on older kernels will fail. So
>> >>need to catch that and throw an error -- perhaps yet another probe function.
>
>> >How does that work? What do I grep to find an example? I figured if the
>> >kernel didn't support the syscall will fail and we'll terminate
>> >someplace.
>
>> Look at __perf_evsel__open(). In this case you probably do not want
>> to fallback but tell the user the clock id option is not supported.
>> The problem is deciphering the failure is due to the clock id versus
>> all the other failure reasons.
>
> I.e. we're back to the sys_perf_event_open() error reporting suckz rockz
> thing, this time with PeterZ trying to find a way to avoid getting back
> to that discussion... /me runz... ;-P
>
Why not have the kernel advertise the perf capabilities (in procfs or
sysfs) instead this
guessing game with the return values?

Re: [PATCH] perf, record: Add clockid parameter

[Peter Zijlstra]

On Fri, Mar 27, 2015 at 03:37:47PM -0700, Stephane Eranian wrote:
> > +       OPT_INTEGER('k', "clockid", &record.opts.clockid,
> > +                   "clockid to use for events"),
> 
> I think you'd want a symbolic name for the clock to make this easier
> on the user.

Sure, here goes.

--
Subject: perf, record: Add clockid parameter
From: Peter Zijlstra <peterz@infradead.org>
Date: Fri, 27 Mar 2015 15:32:01 +0100

Teach perf-record about the new perf_event_attr::{use_clockid, clockid}
fields. Add a simple parameter to set the clock (if any) to be used for
the events to be recorded into the data file.

Since we store the entire perf_event_attr in the EVENT_DESC section we
also already store the used clockid in the data file.

Cc: acme@redhat.com
Cc: jolsa@redhat.com
Cc: eranian@google.com
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 tools/perf/Documentation/perf-record.txt |    7 ++
 tools/perf/builtin-record.c              |   73 +++++++++++++++++++++++++++++++
 tools/perf/perf.h                        |    1 
 tools/perf/util/evsel.c                  |   44 ++++++++++++++++--
 4 files changed, 121 insertions(+), 4 deletions(-)

--- a/tools/perf/Documentation/perf-record.txt
+++ b/tools/perf/Documentation/perf-record.txt
@@ -250,6 +250,13 @@ is off by default.
 --running-time::
 Record running and enabled time for read events (:S)
 
+-k::
+--clockid::
+Sets the clock id to use for the various time fields in the perf_event_type
+records. See clock_gettime(). In particular CLOCK_MONOTONIC and
+CLOCK_MONOTONIC_RAW are supported, some events might also allow
+CLOCK_BOOTTIME, CLOCK_REALTIME and CLOCK_TAI.
+
 SEE ALSO
 --------
 linkperf:perf-stat[1], linkperf:perf-list[1]
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -711,6 +711,75 @@ static int perf_record_config(const char
 	return perf_default_config(var, value, cb);
 }
 
+struct clockid_map {
+	const char *name;
+	int clockid;
+};
+
+#define CLOCKID_MAP(n, c)	\
+	{ .name = n, .clockid = (c), }
+
+#define CLOCKID_END	{ .name = NULL, }
+
+/*
+ * Doesn't appear to have made it into userspace so define here if missing.
+ */
+#ifndef CLOCK_TAI
+#define CLOCK_TAI 11
+#endif
+
+static const struct clockid_map clockids[] = {
+	/* available for all events, NMI safe */
+	CLOCKID_MAP("monotonic", CLOCK_MONOTONIC),
+	CLOCKID_MAP("monotonic_raw", CLOCK_MONOTONIC_RAW),
+	CLOCKID_MAP("raw", CLOCK_MONOTONIC_RAW),
+
+	/* available for some events */
+	CLOCKID_MAP("realtime", CLOCK_REALTIME),
+	CLOCKID_MAP("boottime", CLOCK_BOOTTIME),
+	CLOCKID_MAP("tai", CLOCK_TAI),
+
+	CLOCKID_END,
+};
+
+static int parse_clockid(const struct option *opt, const char *str, int unset)
+{
+	clockid_t *clk = (clockid_t *)opt->value;
+	const struct clockid_map *cm;
+	const char *ostr = str;
+
+	if (unset) {
+		*clk = -1;
+		return 0;
+	}
+
+	/* no arg passed */
+	if (!str)
+		return 0;
+
+	/* no setting it twice */
+	if (*clk != -1)
+		return -1;
+
+	/* if its a number, we're done */
+	if (sscanf(str, "%d", clk) == 1)
+		return 0;
+
+	/* allow a "CLOCK_" prefix to the name */
+	if (!strncasecmp(str, "CLOCK_", 6))
+		str += 6;
+
+	for (cm = clockids; cm->name; cm++) {
+		if (!strcasecmp(str, cm->name)) {
+			*clk = cm->clockid;
+			return 0;
+		}
+	}
+
+	ui__warning("unknown clockid %s, check man page\n", ostr);
+	return -1;
+}
+
 static const char * const __record_usage[] = {
 	"perf record [<options>] [<command>]",
 	"perf record [<options>] -- <command> [<options>]",
@@ -739,6 +808,7 @@ static struct record record = {
 			.uses_mmap   = true,
 			.default_per_cpu = true,
 		},
+		.clockid             = -1,
 	},
 	.tool = {
 		.sample		= process_sample_event,
@@ -842,6 +912,9 @@ struct option __record_options[] = {
 		    "Sample machine registers on interrupt"),
 	OPT_BOOLEAN(0, "running-time", &record.opts.running_time,
 		    "Record running/enabled time of read (:S) events"),
+	OPT_CALLBACK('k', "clockid", &record.opts.clockid,
+		    "clockid", "clockid to use for events, see clock_gettime()",
+		    parse_clockid),
 	OPT_END()
 };
 
--- a/tools/perf/perf.h
+++ b/tools/perf/perf.h
@@ -62,6 +62,7 @@ struct record_opts {
 	u64	     user_interval;
 	bool	     sample_transaction;
 	unsigned     initial_delay;
+	clockid_t    clockid;
 };
 
 struct option;
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -32,6 +32,7 @@ static struct {
 	bool exclude_guest;
 	bool mmap2;
 	bool cloexec;
+	bool clockid;
 } perf_missing_features;
 
 static int perf_evsel__no_extra_init(struct perf_evsel *evsel __maybe_unused)
@@ -761,6 +762,11 @@ void perf_evsel__config(struct perf_evse
 		attr->disabled = 0;
 		attr->enable_on_exec = 0;
 	}
+
+	if (opts->clockid >= 0) {
+		attr->use_clockid = 1;
+		attr->clockid = opts->clockid;
+	}
 }
 
 static int perf_evsel__alloc_fd(struct perf_evsel *evsel, int ncpus, int nthreads)
@@ -1036,7 +1042,6 @@ static size_t perf_event_attr__fprintf(s
 	ret += PRINT_ATTR2(exclude_user, exclude_kernel);
 	ret += PRINT_ATTR2(exclude_hv, exclude_idle);
 	ret += PRINT_ATTR2(mmap, comm);
-	ret += PRINT_ATTR2(mmap2, comm_exec);
 	ret += PRINT_ATTR2(freq, inherit_stat);
 	ret += PRINT_ATTR2(enable_on_exec, task);
 	ret += PRINT_ATTR2(watermark, precise_ip);
@@ -1044,6 +1049,8 @@ static size_t perf_event_attr__fprintf(s
 	ret += PRINT_ATTR2(exclude_host, exclude_guest);
 	ret += PRINT_ATTR2N("excl.callchain_kern", exclude_callchain_kernel,
 			    "excl.callchain_user", exclude_callchain_user);
+	ret += PRINT_ATTR2(mmap2, comm_exec);
+	ret += __PRINT_ATTR("%u",,use_clockid);
 
 	ret += PRINT_ATTR_U32(wakeup_events);
 	ret += PRINT_ATTR_U32(wakeup_watermark);
@@ -1055,6 +1062,7 @@ static size_t perf_event_attr__fprintf(s
 	ret += PRINT_ATTR_X64(branch_sample_type);
 	ret += PRINT_ATTR_X64(sample_regs_user);
 	ret += PRINT_ATTR_U32(sample_stack_user);
+	ret += PRINT_ATTR_U32(clockid);
 	ret += PRINT_ATTR_X64(sample_regs_intr);
 
 	ret += fprintf(fp, "%.60s\n", graph_dotted_line);
@@ -1085,6 +1093,8 @@ static int __perf_evsel__open(struct per
 	}
 
 fallback_missing_features:
+	if (perf_missing_features.clockid)
+		evsel->attr.use_clockid = 0;
 	if (perf_missing_features.cloexec)
 		flags &= ~(unsigned long)PERF_FLAG_FD_CLOEXEC;
 	if (perf_missing_features.mmap2)
@@ -1122,6 +1132,16 @@ static int __perf_evsel__open(struct per
 				goto try_fallback;
 			}
 			set_rlimit = NO_CHANGE;
+
+			/*
+			 * If we succeeded but had to kill clockid, fail and
+			 * have perf_evsel__open_strerror() print us a nice
+			 * error.
+			 */
+			if (perf_missing_features.clockid) {
+				err = -EINVAL;
+				goto out_close;
+			}
 		}
 	}
 
@@ -1155,7 +1175,10 @@ static int __perf_evsel__open(struct per
 	if (err != -EINVAL || cpu > 0 || thread > 0)
 		goto out_close;
 
-	if (!perf_missing_features.cloexec && (flags & PERF_FLAG_FD_CLOEXEC)) {
+	if (!perf_missing_features.clockid && evsel->attr.use_clockid) {
+		perf_missing_features.clockid = true;
+		goto fallback_missing_features;
+	} else if (!perf_missing_features.cloexec && (flags & PERF_FLAG_FD_CLOEXEC)) {
 		perf_missing_features.cloexec = true;
 		goto fallback_missing_features;
 	} else if (!perf_missing_features.mmap2 && evsel->attr.mmap2) {
@@ -2063,9 +2086,7 @@ int perf_evsel__fprintf(struct perf_evse
 		if_print(exclude_hv);
 		if_print(exclude_idle);
 		if_print(mmap);
-		if_print(mmap2);
 		if_print(comm);
-		if_print(comm_exec);
 		if_print(freq);
 		if_print(inherit_stat);
 		if_print(enable_on_exec);
@@ -2076,10 +2097,19 @@ int perf_evsel__fprintf(struct perf_evse
 		if_print(sample_id_all);
 		if_print(exclude_host);
 		if_print(exclude_guest);
+		if_print(exclude_callchain_kernel);
+		if_print(exclude_callchain_user);
+		if_print(mmap2);
+		if_print(comm_exec);
+		if_print(use_clockid);
 		if_print(__reserved_1);
 		if_print(wakeup_events);
 		if_print(bp_type);
 		if_print(branch_sample_type);
+		if_print(sample_regs_user);
+		if_print(sample_stack_user);
+		if_print(clockid);
+		if_print(sample_regs_intr);
 	}
 out:
 	fputc('\n', fp);
@@ -2158,6 +2188,12 @@ int perf_evsel__open_strerror(struct per
 	"The PMU counters are busy/taken by another profiler.\n"
 	"We found oprofile daemon running, please stop it and try again.");
 		break;
+
+	case EINVAL:
+		if (perf_missing_features.clockid)
+			return scnprintf(msg, size, "%s", "clockid not supported.");
+		break;
+
 	default:
 		break;
 	}

Re: [PATCH] perf, record: Add clockid parameter

[David Ahern]

On 3/28/15 1:55 AM, Peter Zijlstra wrote:
> Subject: perf, record: Add clockid parameter
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Fri, 27 Mar 2015 15:32:01 +0100
>
> Teach perf-record about the new perf_event_attr::{use_clockid, clockid}
> fields. Add a simple parameter to set the clock (if any) to be used for
> the events to be recorded into the data file.
>
> Since we store the entire perf_event_attr in the EVENT_DESC section we
> also already store the used clockid in the data file.
>

I am clearly missing some kernel patch to try out this perf patch. I 
have the 4 timekeeper ones; none of those modify perf_event code. What 
other patches are needed? Was this one (or some variant) accepted:

     https://lkml.org/lkml/2015/2/20/236

Perhaps a better question is what tree has all of the kernel side patches?

David

Re: [PATCH] perf, record: Add clockid parameter

[Peter Zijlstra]

On Sun, Mar 29, 2015 at 07:00:19PM -0600, David Ahern wrote:
> On 3/28/15 1:55 AM, Peter Zijlstra wrote:
> >Subject: perf, record: Add clockid parameter
> >From: Peter Zijlstra <peterz@infradead.org>
> >Date: Fri, 27 Mar 2015 15:32:01 +0100
> >
> >Teach perf-record about the new perf_event_attr::{use_clockid, clockid}
> >fields. Add a simple parameter to set the clock (if any) to be used for
> >the events to be recorded into the data file.
> >
> >Since we store the entire perf_event_attr in the EVENT_DESC section we
> >also already store the used clockid in the data file.
> >
> 
> I am clearly missing some kernel patch to try out this perf patch. I have
> the 4 timekeeper ones; none of those modify perf_event code. What other
> patches are needed? Was this one (or some variant) accepted:
> 
>     https://lkml.org/lkml/2015/2/20/236

http://git.kernel.org/tip/34f439278cef7b1177f8ce24f9fc81dfc6221d3b

Jah, I now realize I forgot to post that one, I did post the
monotonic_raw nmi patches again, but forgot about this one.

> Perhaps a better question is what tree has all of the kernel side patches?

tip/master should have it all.

Re: [PATCH] perf, record: Add clockid parameter

[Peter Zijlstra]

On Sat, Mar 28, 2015 at 08:55:49AM +0100, Peter Zijlstra wrote:
> +static const struct clockid_map clockids[] = {
> +	/* available for all events, NMI safe */
> +	CLOCKID_MAP("monotonic", CLOCK_MONOTONIC),
> +	CLOCKID_MAP("monotonic_raw", CLOCK_MONOTONIC_RAW),
> +	CLOCKID_MAP("raw", CLOCK_MONOTONIC_RAW),
> +
> +	/* available for some events */
> +	CLOCKID_MAP("realtime", CLOCK_REALTIME),
> +	CLOCKID_MAP("boottime", CLOCK_BOOTTIME),
> +	CLOCKID_MAP("tai", CLOCK_TAI),
> +
> +	CLOCKID_END,
> +};


I did this on top..

--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -732,13 +732,16 @@ static const struct clockid_map clockids
 	/* available for all events, NMI safe */
 	CLOCKID_MAP("monotonic", CLOCK_MONOTONIC),
 	CLOCKID_MAP("monotonic_raw", CLOCK_MONOTONIC_RAW),
-	CLOCKID_MAP("raw", CLOCK_MONOTONIC_RAW),
 
 	/* available for some events */
 	CLOCKID_MAP("realtime", CLOCK_REALTIME),
 	CLOCKID_MAP("boottime", CLOCK_BOOTTIME),
 	CLOCKID_MAP("tai", CLOCK_TAI),
 
+	/* available for the lazy */
+	CLOCKID_MAP("mono", CLOCK_MONOTONIC),
+	CLOCKID_MAP("raw", CLOCK_MONOTONIC_RAW),
+
 	CLOCKID_END,
 };
 

Re: [PATCH] perf, record: Add clockid parameter

[David Ahern]

On 3/30/15 2:24 AM, Peter Zijlstra wrote:
>> I am clearly missing some kernel patch to try out this perf patch. I have
>> the 4 timekeeper ones; none of those modify perf_event code. What other
>> patches are needed? Was this one (or some variant) accepted:
>>
>>      https://lkml.org/lkml/2015/2/20/236
>
> http://git.kernel.org/tip/34f439278cef7b1177f8ce24f9fc81dfc6221d3b
>
> Jah, I now realize I forgot to post that one, I did post the
> monotonic_raw nmi patches again, but forgot about this one.
>
>> Perhaps a better question is what tree has all of the kernel side patches?
>
> tip/master should have it all.

To summarize for others wanting to use on older kernels you need:
   876e78818def
   4a4ad80d32ce
   3b3f5db075c3
   c0321900d8ca
   669cdcbe49e2
   34f439278cef

Re: [PATCH] perf, record: Add clockid parameter

[David Ahern]

On 3/30/15 3:17 AM, Peter Zijlstra wrote:
> I did this on top..
>
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -732,13 +732,16 @@ static const struct clockid_map clockids
>   	/* available for all events, NMI safe */
>   	CLOCKID_MAP("monotonic", CLOCK_MONOTONIC),
>   	CLOCKID_MAP("monotonic_raw", CLOCK_MONOTONIC_RAW),
> -	CLOCKID_MAP("raw", CLOCK_MONOTONIC_RAW),
>
>   	/* available for some events */
>   	CLOCKID_MAP("realtime", CLOCK_REALTIME),
>   	CLOCKID_MAP("boottime", CLOCK_BOOTTIME),
>   	CLOCKID_MAP("tai", CLOCK_TAI),
>
> +	/* available for the lazy */
> +	CLOCKID_MAP("mono", CLOCK_MONOTONIC),
> +	CLOCKID_MAP("raw", CLOCK_MONOTONIC_RAW),
> +

how about this one as well:
CLOCKID_MAP("real", CLOCK_REALTIME),

Re: [PATCH] perf, record: Add clockid parameter

[David Ahern]

On 3/28/15 1:55 AM, Peter Zijlstra wrote:
> @@ -1085,6 +1093,8 @@ static int __perf_evsel__open(struct per
>   	}
>
>   fallback_missing_features:
> +	if (perf_missing_features.clockid)
> +		evsel->attr.use_clockid = 0;
>   	if (perf_missing_features.cloexec)
>   		flags &= ~(unsigned long)PERF_FLAG_FD_CLOEXEC;
>   	if (perf_missing_features.mmap2)
> @@ -1122,6 +1132,16 @@ static int __perf_evsel__open(struct per
>   				goto try_fallback;
>   			}
>   			set_rlimit = NO_CHANGE;
> +
> +			/*
> +			 * If we succeeded but had to kill clockid, fail and
> +			 * have perf_evsel__open_strerror() print us a nice
> +			 * error.
> +			 */
> +			if (perf_missing_features.clockid) {
> +				err = -EINVAL;
> +				goto out_close;
> +			}
>   		}
>   	}
>
> @@ -1155,7 +1175,10 @@ static int __perf_evsel__open(struct per
>   	if (err != -EINVAL || cpu > 0 || thread > 0)
>   		goto out_close;
>
> -	if (!perf_missing_features.cloexec && (flags & PERF_FLAG_FD_CLOEXEC)) {
> +	if (!perf_missing_features.clockid && evsel->attr.use_clockid) {
> +		perf_missing_features.clockid = true;
> +		goto fallback_missing_features;
> +	} else if (!perf_missing_features.cloexec && (flags & PERF_FLAG_FD_CLOEXEC)) {
>   		perf_missing_features.cloexec = true;
>   		goto fallback_missing_features;
>   	} else if (!perf_missing_features.mmap2 && evsel->attr.mmap2) {

...

> @@ -2158,6 +2188,12 @@ int perf_evsel__open_strerror(struct per
>   	"The PMU counters are busy/taken by another profiler.\n"
>   	"We found oprofile daemon running, please stop it and try again.");
>   		break;
> +
> +	case EINVAL:
> +		if (perf_missing_features.clockid)
> +			return scnprintf(msg, size, "%s", "clockid not supported.");
> +		break;
> +
>   	default:
>   		break;
>   	}
>

This works but the result is not always intuitive as to why it failed.

On a kernel that does not support the clock id you get:
     $ perf sched record -k mono -- sleep 1
     Error:
     clockid not supported.

And on a kernel that supports clockid but not for NMI:

     $ perf record -k realtime -a -- sleep 1
     Error:
     clockid not supported.

     --> H/W counters so realtime is not allowed

Same message though different root causes.

Re: [PATCH] perf, record: Add clockid parameter

[David Ahern]

On 3/28/15 1:55 AM, Peter Zijlstra wrote:
> @@ -761,6 +762,11 @@ void perf_evsel__config(struct perf_evse
>   		attr->disabled = 0;
>   		attr->enable_on_exec = 0;
>   	}
> +
> +	if (opts->clockid >= 0) {
> +		attr->use_clockid = 1;
> +		attr->clockid = opts->clockid;
> +	}
>   }

One more: you need to set attr->clockid to -1 if use_clockid is not set 
so that the analysis side knows whether attr->clockid was used. 
Otherwise it defaults to 0 == CLOCK_REALTIME which is misleading.


diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 1abf6919b8a2..27679ab38511 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -766,7 +766,8 @@ void perf_evsel__config(struct perf_evsel *evsel, 
struct record_opts *opts)
     if (opts->clockid >= 0) {
         attr->use_clockid = 1;
         attr->clockid = opts->clockid;
-   }
+   } else
+       attr->clockid = -1;
  }


Dumping the setting in the header is useful as well:

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index fb432153e2aa..40bc8d010fcb 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -1098,6 +1098,7 @@ static void print_event_desc(struct perf_header 
*ph, int fd, FILE *fp)
             }
             fprintf(fp, " }");
         }
+       fprintf(fp, ", clockid = %d", evsel->attr.clockid);

         fputc('\n', fp);
     }

Re: [PATCH] perf, record: Add clockid parameter

[Peter Zijlstra]

On Mon, Mar 30, 2015 at 11:17:22AM -0600, David Ahern wrote:
> On 3/30/15 3:17 AM, Peter Zijlstra wrote:
> >I did this on top..
> >
> >--- a/tools/perf/builtin-record.c
> >+++ b/tools/perf/builtin-record.c
> >@@ -732,13 +732,16 @@ static const struct clockid_map clockids
> >  	/* available for all events, NMI safe */
> >  	CLOCKID_MAP("monotonic", CLOCK_MONOTONIC),
> >  	CLOCKID_MAP("monotonic_raw", CLOCK_MONOTONIC_RAW),
> >-	CLOCKID_MAP("raw", CLOCK_MONOTONIC_RAW),
> >
> >  	/* available for some events */
> >  	CLOCKID_MAP("realtime", CLOCK_REALTIME),
> >  	CLOCKID_MAP("boottime", CLOCK_BOOTTIME),
> >  	CLOCKID_MAP("tai", CLOCK_TAI),
> >
> >+	/* available for the lazy */
> >+	CLOCKID_MAP("mono", CLOCK_MONOTONIC),
> >+	CLOCKID_MAP("raw", CLOCK_MONOTONIC_RAW),
> >+
> 
> how about this one as well:
> CLOCKID_MAP("real", CLOCK_REALTIME),

CLOCK_REALTIME is of limited usability because its not NMI safe.

Re: [PATCH] perf, record: Add clockid parameter

[Peter Zijlstra]

On Mon, Mar 30, 2015 at 11:24:12AM -0600, David Ahern wrote:
> This works but the result is not always intuitive as to why it failed.
> 
> On a kernel that does not support the clock id you get:
>     $ perf sched record -k mono -- sleep 1
>     Error:
>     clockid not supported.
> 
> And on a kernel that supports clockid but not for NMI:
> 
>     $ perf record -k realtime -a -- sleep 1
>     Error:
>     clockid not supported.
> 
>     --> H/W counters so realtime is not allowed
> 
> Same message though different root causes.

Heh, ok I can fudge that :-)

Re: [PATCH] perf, record: Add clockid parameter

[Peter Zijlstra]

On Mon, Mar 30, 2015 at 11:33:42AM -0600, David Ahern wrote:
> On 3/28/15 1:55 AM, Peter Zijlstra wrote:
> >@@ -761,6 +762,11 @@ void perf_evsel__config(struct perf_evse
> >  		attr->disabled = 0;
> >  		attr->enable_on_exec = 0;
> >  	}
> >+
> >+	if (opts->clockid >= 0) {
> >+		attr->use_clockid = 1;
> >+		attr->clockid = opts->clockid;
> >+	}
> >  }
> 
> One more: you need to set attr->clockid to -1 if use_clockid is not set so
> that the analysis side knows whether attr->clockid was used. Otherwise it
> defaults to 0 == CLOCK_REALTIME which is misleading.
> 
> 
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 1abf6919b8a2..27679ab38511 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -766,7 +766,8 @@ void perf_evsel__config(struct perf_evsel *evsel, struct
> record_opts *opts)
>     if (opts->clockid >= 0) {
>         attr->use_clockid = 1;
>         attr->clockid = opts->clockid;
> -   }
> +   } else
> +       attr->clockid = -1;
>  }

No, we must not have a !0 value in ->clockid when we do not set
use_clockid. The kernel checks for nonzero tail values.

Re: [PATCH] perf, record: Add clockid parameter

[David Ahern]

On 3/30/15 1:32 PM, Peter Zijlstra wrote:

>>> --- a/tools/perf/builtin-record.c
>>> +++ b/tools/perf/builtin-record.c
>>> @@ -732,13 +732,16 @@ static const struct clockid_map clockids
>>>   	/* available for all events, NMI safe */
>>>   	CLOCKID_MAP("monotonic", CLOCK_MONOTONIC),
>>>   	CLOCKID_MAP("monotonic_raw", CLOCK_MONOTONIC_RAW),
>>> -	CLOCKID_MAP("raw", CLOCK_MONOTONIC_RAW),
>>>
>>>   	/* available for some events */
>>>   	CLOCKID_MAP("realtime", CLOCK_REALTIME),
>>>   	CLOCKID_MAP("boottime", CLOCK_BOOTTIME),
>>>   	CLOCKID_MAP("tai", CLOCK_TAI),
>>>
>>> +	/* available for the lazy */
>>> +	CLOCKID_MAP("mono", CLOCK_MONOTONIC),
>>> +	CLOCKID_MAP("raw", CLOCK_MONOTONIC_RAW),
>>> +
>>
>> how about this one as well:
>> CLOCKID_MAP("real", CLOCK_REALTIME),
>
> CLOCK_REALTIME is of limited usability because its not NMI safe.
>

sure, but you have the long version above. I was also suggesting the 
shortcut version -- 5 fewer letters.

Re: [PATCH] perf, record: Add clockid parameter

[David Ahern]

On 3/30/15 1:33 PM, Peter Zijlstra wrote:
> On Mon, Mar 30, 2015 at 11:24:12AM -0600, David Ahern wrote:
>> This works but the result is not always intuitive as to why it failed.
>>
>> On a kernel that does not support the clock id you get:
>>      $ perf sched record -k mono -- sleep 1
>>      Error:
>>      clockid not supported.
>>
>> And on a kernel that supports clockid but not for NMI:
>>
>>      $ perf record -k realtime -a -- sleep 1
>>      Error:
>>      clockid not supported.
>>
>>      --> H/W counters so realtime is not allowed
>>
>> Same message though different root causes.
>
> Heh, ok I can fudge that :-)
>

What about having the kernel return 'not supported' error for the latter 
-- H/W counters with unsafe clock?

Re: [PATCH] perf, record: Add clockid parameter

[Stephane Eranian]

On Mon, Mar 30, 2015 at 12:41 PM, David Ahern <dsahern@gmail.com> wrote:
> On 3/30/15 1:33 PM, Peter Zijlstra wrote:
>>
>> On Mon, Mar 30, 2015 at 11:24:12AM -0600, David Ahern wrote:
>>>
>>> This works but the result is not always intuitive as to why it failed.
>>>
>>> On a kernel that does not support the clock id you get:
>>>      $ perf sched record -k mono -- sleep 1
>>>      Error:
>>>      clockid not supported.
>>>
>>> And on a kernel that supports clockid but not for NMI:
>>>
>>>      $ perf record -k realtime -a -- sleep 1
>>>      Error:
>>>      clockid not supported.
>>>
>>>      --> H/W counters so realtime is not allowed
>>>
>>> Same message though different root causes.
>>
>>
>> Heh, ok I can fudge that :-)
>>
>
> What about having the kernel return 'not supported' error for the latter --
> H/W counters with unsafe clock?

I have reassembled all the pieces from Peter's patch. I will post it
with V6 of the JIT patches today.

Re: [PATCH] perf, record: Add clockid parameter

[David Ahern]

On 3/30/15 1:34 PM, Peter Zijlstra wrote:
>> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
>> index 1abf6919b8a2..27679ab38511 100644
>> --- a/tools/perf/util/evsel.c
>> +++ b/tools/perf/util/evsel.c
>> @@ -766,7 +766,8 @@ void perf_evsel__config(struct perf_evsel *evsel, struct
>> record_opts *opts)
>>      if (opts->clockid >= 0) {
>>          attr->use_clockid = 1;
>>          attr->clockid = opts->clockid;
>> -   }
>> +   } else
>> +       attr->clockid = -1;
>>   }
>
> No, we must not have a !0 value in ->clockid when we do not set
> use_clockid. The kernel checks for nonzero tail values.
>

oops, missed that -- use_clockid is part of the attr so it will be in 
the file header.

David

Re: [PATCH] perf, record: Add clockid parameter

[Peter Zijlstra]

On Mon, Mar 30, 2015 at 01:41:12PM -0600, David Ahern wrote:
> On 3/30/15 1:33 PM, Peter Zijlstra wrote:
> >On Mon, Mar 30, 2015 at 11:24:12AM -0600, David Ahern wrote:
> >>This works but the result is not always intuitive as to why it failed.
> >>
> >>On a kernel that does not support the clock id you get:
> >>     $ perf sched record -k mono -- sleep 1
> >>     Error:
> >>     clockid not supported.
> >>
> >>And on a kernel that supports clockid but not for NMI:
> >>
> >>     $ perf record -k realtime -a -- sleep 1
> >>     Error:
> >>     clockid not supported.
> >>
> >>     --> H/W counters so realtime is not allowed
> >>
> >>Same message though different root causes.
> >
> >Heh, ok I can fudge that :-)
> >
> 
> What about having the kernel return 'not supported' error for the latter --
> H/W counters with unsafe clock?

That's bound to only work for a little while until something else starts
returning -EOPNOTSUPP too.


Fwiw, having 3 incomplete ways to print perf_event_attr() is
disgusting.

---
Subject: perf,record: Add clockid parameter
From: Peter Zijlstra <peterz@infradead.org>
Date: Tue, 31 Mar 2015 00:19:31 +0200

Teach perf-record about the new perf_event_attr:{use_+clockid, clockid}
fields. Add a simple parameter to set the clock (if any) to be used for
the events to be recorded into the dat file.

Cc: jolsa@redhat.com
Cc: dsahern@gmail.com
Cc: adrian.hunter@intel.com
Cc: pawel.moll@arm.com
Cc: acme@redhat.com
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 tools/perf/Documentation/perf-record.txt |    7 ++
 tools/perf/builtin-record.c              |   78 +++++++++++++++++++++++++++++++
 tools/perf/perf.h                        |    1 
 tools/perf/util/evsel.c                  |   59 +++++++++++++++++++++--
 tools/perf/util/header.c                 |    3 +
 5 files changed, 144 insertions(+), 4 deletions(-)

--- a/tools/perf/Documentation/perf-record.txt
+++ b/tools/perf/Documentation/perf-record.txt
@@ -250,6 +250,13 @@ is off by default.
 --running-time::
 Record running and enabled time for read events (:S)
 
+-k::
+--clockid::
+Sets the clock id to use for the various time fields in the perf_event_type
+records. See clock_gettime(). In particular CLOCK_MONOTONIC and
+CLOCK_MONOTONIC_RAW are supported, some events might also allow
+CLOCK_BOOTTIME, CLOCK_REALTIME and CLOCK_TAI.
+
 SEE ALSO
 --------
 linkperf:perf-stat[1], linkperf:perf-list[1]
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -711,6 +711,80 @@ static int perf_record_config(const char
 	return perf_default_config(var, value, cb);
 }
 
+struct clockid_map {
+	const char *name;
+	int clockid;
+};
+
+#define CLOCKID_MAP(n, c)	\
+	{ .name = n, .clockid = (c), }
+
+#define CLOCKID_END	{ .name = NULL, }
+
+/*
+ * Doesn't appear to have made it into userspace so define here if missing.
+ */
+#ifndef CLOCK_TAI
+#define CLOCK_TAI 11
+#endif
+
+static const struct clockid_map clockids[] = {
+	/* available for all events, NMI safe */
+	CLOCKID_MAP("monotonic", CLOCK_MONOTONIC),
+	CLOCKID_MAP("monotonic_raw", CLOCK_MONOTONIC_RAW),
+
+	/* available for some events */
+	CLOCKID_MAP("realtime", CLOCK_REALTIME),
+	CLOCKID_MAP("boottime", CLOCK_BOOTTIME),
+	CLOCKID_MAP("tai", CLOCK_TAI),
+
+	/* available for the lazy */
+	CLOCKID_MAP("mono", CLOCK_MONOTONIC),
+	CLOCKID_MAP("raw", CLOCK_MONOTONIC_RAW),
+	CLOCKID_MAP("real", CLOCK_REALTIME),
+	CLOCKID_MAP("boot", CLOCK_BOOTTIME),
+
+	CLOCKID_END,
+};
+
+static int parse_clockid(const struct option *opt, const char *str, int unset)
+{
+	clockid_t *clk = (clockid_t *)opt->value;
+	const struct clockid_map *cm;
+	const char *ostr = str;
+
+	if (unset) {
+		*clk = -1;
+		return 0;
+	}
+
+	/* no arg passed */
+	if (!str)
+		return 0;
+
+	/* no setting it twice */
+	if (*clk != -1)
+		return -1;
+
+	/* if its a number, we're done */
+	if (sscanf(str, "%d", clk) == 1)
+		return 0;
+
+	/* allow a "CLOCK_" prefix to the name */
+	if (!strncasecmp(str, "CLOCK_", 6))
+		str += 6;
+
+	for (cm = clockids; cm->name; cm++) {
+		if (!strcasecmp(str, cm->name)) {
+			*clk = cm->clockid;
+			return 0;
+		}
+	}
+
+	ui__warning("unknown clockid %s, check man page\n", ostr);
+	return -1;
+}
+
 static const char * const __record_usage[] = {
 	"perf record [<options>] [<command>]",
 	"perf record [<options>] -- <command> [<options>]",
@@ -739,6 +813,7 @@ static struct record record = {
 			.uses_mmap   = true,
 			.default_per_cpu = true,
 		},
+		.clockid             = -1,
 	},
 	.tool = {
 		.sample		= process_sample_event,
@@ -842,6 +917,9 @@ struct option __record_options[] = {
 		    "Sample machine registers on interrupt"),
 	OPT_BOOLEAN(0, "running-time", &record.opts.running_time,
 		    "Record running/enabled time of read (:S) events"),
+	OPT_CALLBACK('k', "clockid", &record.opts.clockid,
+	"clockid", "clockid to use for events, see clock_gettime()",
+	parse_clockid),
 	OPT_END()
 };
 
--- a/tools/perf/perf.h
+++ b/tools/perf/perf.h
@@ -62,6 +62,7 @@ struct record_opts {
 	u64	     user_interval;
 	bool	     sample_transaction;
 	unsigned     initial_delay;
+	clockid_t    clockid;
 };
 
 struct option;
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -32,8 +32,12 @@ static struct {
 	bool exclude_guest;
 	bool mmap2;
 	bool cloexec;
+	bool clockid;
+	bool clockid_wrong;
 } perf_missing_features;
 
+static clockid_t clockid;
+
 static int perf_evsel__no_extra_init(struct perf_evsel *evsel __maybe_unused)
 {
 	return 0;
@@ -761,6 +765,12 @@ void perf_evsel__config(struct perf_evse
 		attr->disabled = 0;
 		attr->enable_on_exec = 0;
 	}
+
+	clockid = opts->clockid;
+	if (opts->clockid >= 0) {
+		attr->use_clockid = 1;
+		attr->clockid = opts->clockid;
+	}
 }
 
 static int perf_evsel__alloc_fd(struct perf_evsel *evsel, int ncpus, int nthreads)
@@ -1036,7 +1046,6 @@ static size_t perf_event_attr__fprintf(s
 	ret += PRINT_ATTR2(exclude_user, exclude_kernel);
 	ret += PRINT_ATTR2(exclude_hv, exclude_idle);
 	ret += PRINT_ATTR2(mmap, comm);
-	ret += PRINT_ATTR2(mmap2, comm_exec);
 	ret += PRINT_ATTR2(freq, inherit_stat);
 	ret += PRINT_ATTR2(enable_on_exec, task);
 	ret += PRINT_ATTR2(watermark, precise_ip);
@@ -1044,6 +1053,9 @@ static size_t perf_event_attr__fprintf(s
 	ret += PRINT_ATTR2(exclude_host, exclude_guest);
 	ret += PRINT_ATTR2N("excl.callchain_kern", exclude_callchain_kernel,
 			    "excl.callchain_user", exclude_callchain_user);
+	ret += PRINT_ATTR2(mmap2, comm_exec);
+	ret += __PRINT_ATTR("%u",,use_clockid);
+
 
 	ret += PRINT_ATTR_U32(wakeup_events);
 	ret += PRINT_ATTR_U32(wakeup_watermark);
@@ -1055,6 +1067,7 @@ static size_t perf_event_attr__fprintf(s
 	ret += PRINT_ATTR_X64(branch_sample_type);
 	ret += PRINT_ATTR_X64(sample_regs_user);
 	ret += PRINT_ATTR_U32(sample_stack_user);
+	ret += PRINT_ATTR_U32(clockid);
 	ret += PRINT_ATTR_X64(sample_regs_intr);
 
 	ret += fprintf(fp, "%.60s\n", graph_dotted_line);
@@ -1085,6 +1098,12 @@ static int __perf_evsel__open(struct per
 	}
 
 fallback_missing_features:
+	if (perf_missing_features.clockid_wrong)
+		evsel->attr.clockid = CLOCK_MONOTONIC; /* should always work */
+	if (perf_missing_features.clockid) {
+		evsel->attr.use_clockid = 0;
+		evsel->attr.clockid = 0;
+	}
 	if (perf_missing_features.cloexec)
 		flags &= ~(unsigned long)PERF_FLAG_FD_CLOEXEC;
 	if (perf_missing_features.mmap2)
@@ -1122,6 +1141,17 @@ static int __perf_evsel__open(struct per
 				goto try_fallback;
 			}
 			set_rlimit = NO_CHANGE;
+
+			/*
+			 * If we succeeded but had to kill clockid, fail and
+			 * have perf_evsel__open_strerror() print us a nice
+			 * error.
+			 */
+			if (perf_missing_features.clockid ||
+			    perf_missing_features.clockid_wrong) {
+				err = -EINVAL;
+				goto out_close;
+			}
 		}
 	}
 
@@ -1155,7 +1185,17 @@ static int __perf_evsel__open(struct per
 	if (err != -EINVAL || cpu > 0 || thread > 0)
 		goto out_close;
 
-	if (!perf_missing_features.cloexec && (flags & PERF_FLAG_FD_CLOEXEC)) {
+	/*
+	 * Must probe features in the order they were added to the
+	 * perf_event_attr interface.
+	 */
+	if (!perf_missing_features.clockid_wrong && evsel->attr.use_clockid) {
+		perf_missing_features.clockid_wrong = true;
+		goto fallback_missing_features;
+	} else if (!perf_missing_features.clockid && evsel->attr.use_clockid) {
+		perf_missing_features.clockid = true;
+		goto fallback_missing_features;
+	} else if (!perf_missing_features.cloexec && (flags & PERF_FLAG_FD_CLOEXEC)) {
 		perf_missing_features.cloexec = true;
 		goto fallback_missing_features;
 	} else if (!perf_missing_features.mmap2 && evsel->attr.mmap2) {
@@ -2063,9 +2103,7 @@ int perf_evsel__fprintf(struct perf_evse
 		if_print(exclude_hv);
 		if_print(exclude_idle);
 		if_print(mmap);
-		if_print(mmap2);
 		if_print(comm);
-		if_print(comm_exec);
 		if_print(freq);
 		if_print(inherit_stat);
 		if_print(enable_on_exec);
@@ -2076,10 +2114,17 @@ int perf_evsel__fprintf(struct perf_evse
 		if_print(sample_id_all);
 		if_print(exclude_host);
 		if_print(exclude_guest);
+		if_print(mmap2);
+		if_print(comm_exec);
+		if_print(use_clockid);
 		if_print(__reserved_1);
 		if_print(wakeup_events);
 		if_print(bp_type);
 		if_print(branch_sample_type);
+		if_print(sample_regs_user);
+		if_print(sample_stack_user);
+		if_print(clockid);
+		if_print(sample_regs_intr);
 	}
 out:
 	fputc('\n', fp);
@@ -2158,6 +2203,12 @@ int perf_evsel__open_strerror(struct per
 	"The PMU counters are busy/taken by another profiler.\n"
 	"We found oprofile daemon running, please stop it and try again.");
 		break;
+	case EINVAL:
+		if (perf_missing_features.clockid)
+			return scnprintf(msg, size, "clockid feature not supported.");
+		if (perf_missing_features.clockid_wrong)
+			return scnprintf(msg, size, "wrong clockid (%d).", clockid);
+		break;
 	default:
 		break;
 	}
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -1098,6 +1098,9 @@ static void print_event_desc(struct perf
 			}
 			fprintf(fp, " }");
 		}
+		if (evsel->attr.use_clockid)
+			fprintf(fp, ", clockid = %d", evsel->attr.clockid);
+
 
 		fputc('\n', fp);
 	}

[RFC][PATCH] perf tools: unify perf_event_attr printing

[Peter Zijlstra]

On Tue, Mar 31, 2015 at 10:19:55AM +0200, Peter Zijlstra wrote:
> Fwiw, having 3 incomplete ways to print perf_event_attr() is
> disgusting.

How about something like so? Its not identical, but at least its
complete and consistent.

---
 tools/perf/util/print_attr.h     |   69 +++++++++++++++
 tools/perf/util/print_helper.c   |   52 +++++++++++
 tools/perf/util/print_helper.h   |    7 +
 tools/perf/util/Build            |    1 
 tools/perf/util/evsel.c          |  178 +++++----------------------------------
 tools/perf/util/header.c         |   34 ++-----
 6 files changed, 167 insertions(+), 174 deletions(-)

--- a/tools/perf/util/Build
+++ b/tools/perf/util/Build
@@ -74,6 +74,7 @@ libperf-y += data.o
 libperf-$(CONFIG_X86) += tsc.o
 libperf-y += cloexec.o
 libperf-y += thread-stack.o
+libperf-y += print_helper.o
 
 libperf-$(CONFIG_LIBELF) += symbol-elf.o
 libperf-$(CONFIG_LIBELF) += probe-event.o
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -26,6 +26,7 @@
 #include "perf_regs.h"
 #include "debug.h"
 #include "trace-event.h"
+#include "print_helper.h"
 
 static struct {
 	bool sample_id_all;
@@ -1011,21 +1012,6 @@ static int get_group_fd(struct perf_evse
 	return fd;
 }
 
-#define __PRINT_ATTR(fmt, cast, field)  \
-	fprintf(fp, "  %-19s "fmt"\n", #field, cast attr->field)
-
-#define PRINT_ATTR_U32(field)  __PRINT_ATTR("%u" , , field)
-#define PRINT_ATTR_X32(field)  __PRINT_ATTR("%#x", , field)
-#define PRINT_ATTR_U64(field)  __PRINT_ATTR("%" PRIu64, (uint64_t), field)
-#define PRINT_ATTR_X64(field)  __PRINT_ATTR("%#"PRIx64, (uint64_t), field)
-
-#define PRINT_ATTR2N(name1, field1, name2, field2)	\
-	fprintf(fp, "  %-19s %u    %-19s %u\n",		\
-	name1, attr->field1, name2, attr->field2)
-
-#define PRINT_ATTR2(field1, field2) \
-	PRINT_ATTR2N(#field1, field1, #field2, field2)
-
 static size_t perf_event_attr__fprintf(struct perf_event_attr *attr, FILE *fp)
 {
 	size_t ret = 0;
@@ -1033,42 +1019,15 @@ static size_t perf_event_attr__fprintf(s
 	ret += fprintf(fp, "%.60s\n", graph_dotted_line);
 	ret += fprintf(fp, "perf_event_attr:\n");
 
-	ret += PRINT_ATTR_U32(type);
-	ret += PRINT_ATTR_U32(size);
-	ret += PRINT_ATTR_X64(config);
-	ret += PRINT_ATTR_U64(sample_period);
-	ret += PRINT_ATTR_U64(sample_freq);
-	ret += PRINT_ATTR_X64(sample_type);
-	ret += PRINT_ATTR_X64(read_format);
-
-	ret += PRINT_ATTR2(disabled, inherit);
-	ret += PRINT_ATTR2(pinned, exclusive);
-	ret += PRINT_ATTR2(exclude_user, exclude_kernel);
-	ret += PRINT_ATTR2(exclude_hv, exclude_idle);
-	ret += PRINT_ATTR2(mmap, comm);
-	ret += PRINT_ATTR2(freq, inherit_stat);
-	ret += PRINT_ATTR2(enable_on_exec, task);
-	ret += PRINT_ATTR2(watermark, precise_ip);
-	ret += PRINT_ATTR2(mmap_data, sample_id_all);
-	ret += PRINT_ATTR2(exclude_host, exclude_guest);
-	ret += PRINT_ATTR2N("excl.callchain_kern", exclude_callchain_kernel,
-			    "excl.callchain_user", exclude_callchain_user);
-	ret += PRINT_ATTR2(mmap2, comm_exec);
-	ret += __PRINT_ATTR("%u",,use_clockid);
-
-
-	ret += PRINT_ATTR_U32(wakeup_events);
-	ret += PRINT_ATTR_U32(wakeup_watermark);
-	ret += PRINT_ATTR_X32(bp_type);
-	ret += PRINT_ATTR_X64(bp_addr);
-	ret += PRINT_ATTR_X64(config1);
-	ret += PRINT_ATTR_U64(bp_len);
-	ret += PRINT_ATTR_X64(config2);
-	ret += PRINT_ATTR_X64(branch_sample_type);
-	ret += PRINT_ATTR_X64(sample_regs_user);
-	ret += PRINT_ATTR_U32(sample_stack_user);
-	ret += PRINT_ATTR_U32(clockid);
-	ret += PRINT_ATTR_X64(sample_regs_intr);
+#define PRINT_ATTR(_n, _f, _p)			\
+do {						\
+	ret += fprintf(fp, "  %-19s ", _n);	\
+	ret += _p(fp, attr->_f);		\
+} while (0)
+
+#include "util/print_attr.h"
+
+#undef PRINT_ATTR
 
 	ret += fprintf(fp, "%.60s\n", graph_dotted_line);
 
@@ -1996,64 +1955,6 @@ static int comma_fprintf(FILE *fp, bool
 	return ret;
 }
 
-static int __if_fprintf(FILE *fp, bool *first, const char *field, u64 value)
-{
-	if (value == 0)
-		return 0;
-
-	return comma_fprintf(fp, first, " %s: %" PRIu64, field, value);
-}
-
-#define if_print(field) printed += __if_fprintf(fp, &first, #field, evsel->attr.field)
-
-struct bit_names {
-	int bit;
-	const char *name;
-};
-
-static int bits__fprintf(FILE *fp, const char *field, u64 value,
-			 struct bit_names *bits, bool *first)
-{
-	int i = 0, printed = comma_fprintf(fp, first, " %s: ", field);
-	bool first_bit = true;
-
-	do {
-		if (value & bits[i].bit) {
-			printed += fprintf(fp, "%s%s", first_bit ? "" : "|", bits[i].name);
-			first_bit = false;
-		}
-	} while (bits[++i].name != NULL);
-
-	return printed;
-}
-
-static int sample_type__fprintf(FILE *fp, bool *first, u64 value)
-{
-#define bit_name(n) { PERF_SAMPLE_##n, #n }
-	struct bit_names bits[] = {
-		bit_name(IP), bit_name(TID), bit_name(TIME), bit_name(ADDR),
-		bit_name(READ), bit_name(CALLCHAIN), bit_name(ID), bit_name(CPU),
-		bit_name(PERIOD), bit_name(STREAM_ID), bit_name(RAW),
-		bit_name(BRANCH_STACK), bit_name(REGS_USER), bit_name(STACK_USER),
-		bit_name(IDENTIFIER), bit_name(REGS_INTR),
-		{ .name = NULL, }
-	};
-#undef bit_name
-	return bits__fprintf(fp, "sample_type", value, bits, first);
-}
-
-static int read_format__fprintf(FILE *fp, bool *first, u64 value)
-{
-#define bit_name(n) { PERF_FORMAT_##n, #n }
-	struct bit_names bits[] = {
-		bit_name(TOTAL_TIME_ENABLED), bit_name(TOTAL_TIME_RUNNING),
-		bit_name(ID), bit_name(GROUP),
-		{ .name = NULL, }
-	};
-#undef bit_name
-	return bits__fprintf(fp, "read_format", value, bits, first);
-}
-
 int perf_evsel__fprintf(struct perf_evsel *evsel,
 			struct perf_attr_details *details, FILE *fp)
 {
@@ -2080,51 +1981,24 @@ int perf_evsel__fprintf(struct perf_evse
 
 	printed += fprintf(fp, "%s", perf_evsel__name(evsel));
 
-	if (details->verbose || details->freq) {
-		printed += comma_fprintf(fp, &first, " sample_freq=%" PRIu64,
-					 (u64)evsel->attr.sample_freq);
-	}
 
 	if (details->verbose) {
-		if_print(type);
-		if_print(config);
-		if_print(config1);
-		if_print(config2);
-		if_print(size);
-		printed += sample_type__fprintf(fp, &first, evsel->attr.sample_type);
-		if (evsel->attr.read_format)
-			printed += read_format__fprintf(fp, &first, evsel->attr.read_format);
-		if_print(disabled);
-		if_print(inherit);
-		if_print(pinned);
-		if_print(exclusive);
-		if_print(exclude_user);
-		if_print(exclude_kernel);
-		if_print(exclude_hv);
-		if_print(exclude_idle);
-		if_print(mmap);
-		if_print(comm);
-		if_print(freq);
-		if_print(inherit_stat);
-		if_print(enable_on_exec);
-		if_print(task);
-		if_print(watermark);
-		if_print(precise_ip);
-		if_print(mmap_data);
-		if_print(sample_id_all);
-		if_print(exclude_host);
-		if_print(exclude_guest);
-		if_print(mmap2);
-		if_print(comm_exec);
-		if_print(use_clockid);
-		if_print(__reserved_1);
-		if_print(wakeup_events);
-		if_print(bp_type);
-		if_print(branch_sample_type);
-		if_print(sample_regs_user);
-		if_print(sample_stack_user);
-		if_print(clockid);
-		if_print(sample_regs_intr);
+
+#define PRINT_ATTR(_n, _f, _p)						\
+do {									\
+	if (evsel->attr._f) {						\
+		printed += comma_fprintf(fp, &first, " %s: ", _n);	\
+		printed += _p(fp, evsel->attr._f);			\
+	}								\
+} while (0)
+
+#include "util/print_attr.h"
+
+#undef PRINT_ATTR
+
+	} else if (details->freq) {
+		printed += comma_fprintf(fp, &first, " sample_freq=%" PRIu64,
+					 (u64)evsel->attr.sample_freq);
 	}
 out:
 	fputc('\n', fp);
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -23,6 +23,7 @@
 #include "strbuf.h"
 #include "build-id.h"
 #include "data.h"
+#include "print_helper.h"
 
 static u32 header_argc;
 static const char **header_argv;
@@ -1069,26 +1070,6 @@ static void print_event_desc(struct perf
 	for (evsel = events; evsel->attr.size; evsel++) {
 		fprintf(fp, "# event : name = %s, ", evsel->name);
 
-		fprintf(fp, "type = %d, config = 0x%"PRIx64
-			    ", config1 = 0x%"PRIx64", config2 = 0x%"PRIx64,
-				evsel->attr.type,
-				(u64)evsel->attr.config,
-				(u64)evsel->attr.config1,
-				(u64)evsel->attr.config2);
-
-		fprintf(fp, ", excl_usr = %d, excl_kern = %d",
-				evsel->attr.exclude_user,
-				evsel->attr.exclude_kernel);
-
-		fprintf(fp, ", excl_host = %d, excl_guest = %d",
-				evsel->attr.exclude_host,
-				evsel->attr.exclude_guest);
-
-		fprintf(fp, ", precise_ip = %d", evsel->attr.precise_ip);
-
-		fprintf(fp, ", attr_mmap2 = %d", evsel->attr.mmap2);
-		fprintf(fp, ", attr_mmap  = %d", evsel->attr.mmap);
-		fprintf(fp, ", attr_mmap_data = %d", evsel->attr.mmap_data);
 		if (evsel->ids) {
 			fprintf(fp, ", id = {");
 			for (j = 0, id = evsel->id; j < evsel->ids; j++, id++) {
@@ -1098,9 +1079,18 @@ static void print_event_desc(struct perf
 			}
 			fprintf(fp, " }");
 		}
-		if (evsel->attr.use_clockid)
-			fprintf(fp, ", clockid = %d", evsel->attr.clockid);
 
+#define PRINT_ATTR(_n, _f, _p)			\
+do {						\
+	if (evsel->attr._f) {			\
+		fprintf(fp, ", %s =", _n);	\
+		_p(fp, evsel->attr._f);		\
+	}					\
+} while (0)
+
+#include "util/print_attr.h"
+
+#undef PRINT_ATTR
 
 		fputc('\n', fp);
 	}
--- /dev/null
+++ b/tools/perf/util/print_attr.h
@@ -0,0 +1,69 @@
+
+#ifndef PRINT_ATTR
+/*
+ * #define PRINT_ATTR(name, field, print) 		\
+ * do {							\
+ * 	fprintf(fp, " %s = ", name);			\
+ * 	print(fp, attr->field);				\
+ * } while (0)
+ */
+#error "General Error and Major Fault yell at you!"
+#endif
+
+#define p_hex(fp, val)		fprintf(fp, "%"PRIx64, (uint64_t)(val))
+#define p_unsigned(fp, val)	fprintf(fp, "%"PRIu64, (uint64_t)(val))
+#define p_signed(fp, val)	fprintf(fp, "%"PRId64, (int64_t)(val))
+#define p_sample_type(fp, val)	sample_type__fprintf(fp, val)
+#define p_read_format(fp, val)	read_format__fprintf(fp, val)
+
+#define PRINT_ATTRf(field, print) PRINT_ATTR(#field, field, print)
+
+PRINT_ATTRf(type, p_unsigned);
+PRINT_ATTRf(size, p_unsigned);
+PRINT_ATTRf(config, p_hex);
+PRINT_ATTR("{ sample_period, sample_freq }", sample_period, p_unsigned);
+PRINT_ATTRf(sample_type, p_sample_type);
+PRINT_ATTRf(read_format, p_read_format);
+
+PRINT_ATTRf(disabled, p_unsigned);
+PRINT_ATTRf(inherit, p_unsigned);
+PRINT_ATTRf(pinned, p_unsigned);
+PRINT_ATTRf(exclusive, p_unsigned);
+PRINT_ATTRf(exclude_user, p_unsigned);
+PRINT_ATTRf(exclude_kernel, p_unsigned);
+PRINT_ATTRf(exclude_hv, p_unsigned);
+PRINT_ATTRf(exclude_idle, p_unsigned);
+PRINT_ATTRf(mmap, p_unsigned);
+PRINT_ATTRf(comm, p_unsigned);
+PRINT_ATTRf(freq, p_unsigned);
+PRINT_ATTRf(inherit_stat, p_unsigned);
+PRINT_ATTRf(enable_on_exec, p_unsigned);
+PRINT_ATTRf(task, p_unsigned);
+PRINT_ATTRf(watermark, p_unsigned);
+PRINT_ATTRf(precise_ip, p_unsigned);
+PRINT_ATTRf(mmap_data, p_unsigned);
+PRINT_ATTRf(sample_id_all, p_unsigned);
+PRINT_ATTRf(exclude_host, p_unsigned);
+PRINT_ATTRf(exclude_guest, p_unsigned);
+PRINT_ATTRf(exclude_callchain_kernel, p_unsigned);
+PRINT_ATTRf(exclude_callchain_user, p_unsigned);
+PRINT_ATTRf(mmap2, p_unsigned);
+PRINT_ATTRf(comm_exec, p_unsigned);
+PRINT_ATTRf(use_clockid, p_unsigned);
+
+PRINT_ATTR("{ wakeup_events, wakeup_watermark }", wakeup_events, p_unsigned);
+PRINT_ATTRf(bp_type, p_unsigned);
+PRINT_ATTR("{ bp_addr, config1 }", bp_addr, p_hex);
+PRINT_ATTR("{ bp_len, config2 }", bp_len, p_hex);
+PRINT_ATTRf(sample_regs_user, p_hex);
+PRINT_ATTRf(sample_stack_user, p_unsigned);
+PRINT_ATTRf(clockid, p_signed);
+PRINT_ATTRf(sample_regs_intr, p_hex);
+
+#undef PRINT_ATTRf
+
+#undef p_hex
+#undef p_unsigned
+#undef p_signed
+#undef p_sample_type
+#undef p_read_format
--- /dev/null
+++ b/tools/perf/util/print_helper.c
@@ -0,0 +1,52 @@
+
+#include <linux/perf_event.h>
+#include "util.h"
+#include "print_helper.h"
+
+struct bit_names {
+	int bit;
+	const char *name;
+};
+
+static int bits__fprintf(FILE *fp, u64 value, struct bit_names *bits)
+{
+	int i = 0, printed = 0;
+	bool first_bit = true;
+
+	do {
+		if (value & bits[i].bit) {
+			printed += fprintf(fp, "%s%s", first_bit ? "" : "|", bits[i].name);
+			first_bit = false;
+		}
+	} while (bits[++i].name != NULL);
+
+	return printed;
+}
+
+int sample_type__fprintf(FILE *fp, u64 value)
+{
+#define bit_name(n) { PERF_SAMPLE_##n, #n }
+	struct bit_names bits[] = {
+		bit_name(IP), bit_name(TID), bit_name(TIME), bit_name(ADDR),
+		bit_name(READ), bit_name(CALLCHAIN), bit_name(ID), bit_name(CPU),
+		bit_name(PERIOD), bit_name(STREAM_ID), bit_name(RAW),
+		bit_name(BRANCH_STACK), bit_name(REGS_USER), bit_name(STACK_USER),
+		bit_name(IDENTIFIER), bit_name(REGS_INTR),
+		{ .name = NULL, }
+	};
+#undef bit_name
+	return bits__fprintf(fp, value, bits);
+}
+
+int read_format__fprintf(FILE *fp, u64 value)
+{
+#define bit_name(n) { PERF_FORMAT_##n, #n }
+	struct bit_names bits[] = {
+		bit_name(TOTAL_TIME_ENABLED), bit_name(TOTAL_TIME_RUNNING),
+		bit_name(ID), bit_name(GROUP),
+		{ .name = NULL, }
+	};
+#undef bit_name
+	return bits__fprintf(fp, value, bits);
+}
+
--- /dev/null
+++ b/tools/perf/util/print_helper.h
@@ -0,0 +1,7 @@
+#ifndef PRINT_HELPER_H
+#define PRINT_HELPER_H
+
+extern int sample_type__fprintf(FILE *fp, u64 value);
+extern int read_format__fprintf(FILE *fp, u64 value);
+
+#endif

Re: [RFC][PATCH] perf tools: unify perf_event_attr printing

[Peter Zijlstra]

With some feedback from Jolsa, who showed me how to trigger the actual
outputs.

---

Subject: perf, tools: Merge all perf_event_attr print functions
From: Peter Zijlstra <peterz@infradead.org>
Date: Tue Mar 31 13:01:54 CEST 2015

Currently there's 3 (that I found) different and incomplete
implementations of printing perf_event_attr.

This is quite silly. Merge the lot.

While this patch does not retain the exact form all printing that I
found is debug output and thus it should not be critical.

Also, I cannot find a single print_event_desc() caller.

Pre:

$ perf record -vv -e cycles -- sleep 1
------------------------------------------------------------
perf_event_attr:
  type                0
  size                104
  config              0
  sample_period       4000
  sample_freq         4000
  sample_type         0x107
  read_format         0
  disabled            1    inherit             1
  pinned              0    exclusive           0
  exclude_user        0    exclude_kernel      0
  exclude_hv          0    exclude_idle        0
  mmap                1    comm                1
  mmap2               1    comm_exec           1
  freq                1    inherit_stat        0
  enable_on_exec      1    task                1
  watermark           0    precise_ip          0
  mmap_data           0    sample_id_all       1
  exclude_host        0    exclude_guest       1
  excl.callchain_kern 0    excl.callchain_user 0
  wakeup_events       0
  wakeup_watermark    0
  bp_type             0
  bp_addr             0
  config1             0
  bp_len              0
  config2             0
  branch_sample_type  0
  sample_regs_user    0
  sample_stack_user   0
  sample_regs_intr    0
------------------------------------------------------------

$ perf evlist  -vv
cycles: sample_freq=4000, size: 104, sample_type: IP|TID|TIME|PERIOD,
disabled: 1, inherit: 1, mmap: 1, mmap2: 1, comm: 1, comm_exec: 1,
freq: 1, enable_on_exec: 1, task: 1, sample_id_all: 1, exclude_guest:
1

Post:

$ ./perf record -vv -e cycles -- sleep 1 
------------------------------------------------------------
perf_event_attr:
  size                             112
  { sample_period, sample_freq }   4000
  sample_type                      IP|TID|TIME|PERIOD
  disabled                         1
  inherit                          1
  mmap                             1
  comm                             1
  freq                             1
  enable_on_exec                   1
  task                             1
  sample_id_all                    1
  exclude_guest                    1
  mmap2                            1
  comm_exec                        1
------------------------------------------------------------

$ ./perf evlist  -vv
cycles: size: 112, { sample_period, sample_freq }: 4000, sample_type:
IP|TID|TIME|PERIOD, disabled: 1, inherit: 1, mmap: 1, comm: 1, freq:
1, enable_on_exec: 1, task: 1, sample_id_all: 1, exclude_guest: 1,
mmap2: 1, comm_exec: 1

Cc: acme@redhat.com
Cc: jolsa@redhat.com
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 tools/perf/util/Build          |    1 
 tools/perf/util/evsel.c        |  181 ++++++-----------------------------------
 tools/perf/util/header.c       |   34 ++-----
 tools/perf/util/print_attr.h   |   69 +++++++++++++++
 tools/perf/util/print_helper.c |   52 +++++++++++
 tools/perf/util/print_helper.h |    7 +
 6 files changed, 170 insertions(+), 174 deletions(-)

--- a/tools/perf/util/Build
+++ b/tools/perf/util/Build
@@ -74,6 +74,7 @@ libperf-y += data.o
 libperf-$(CONFIG_X86) += tsc.o
 libperf-y += cloexec.o
 libperf-y += thread-stack.o
+libperf-y += print_helper.o
 
 libperf-$(CONFIG_LIBELF) += symbol-elf.o
 libperf-$(CONFIG_LIBELF) += probe-event.o
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -26,6 +26,7 @@
 #include "perf_regs.h"
 #include "debug.h"
 #include "trace-event.h"
+#include "print_helper.h"
 
 static struct {
 	bool sample_id_all;
@@ -1011,21 +1012,6 @@ static int get_group_fd(struct perf_evse
 	return fd;
 }
 
-#define __PRINT_ATTR(fmt, cast, field)  \
-	fprintf(fp, "  %-19s "fmt"\n", #field, cast attr->field)
-
-#define PRINT_ATTR_U32(field)  __PRINT_ATTR("%u" , , field)
-#define PRINT_ATTR_X32(field)  __PRINT_ATTR("%#x", , field)
-#define PRINT_ATTR_U64(field)  __PRINT_ATTR("%" PRIu64, (uint64_t), field)
-#define PRINT_ATTR_X64(field)  __PRINT_ATTR("%#"PRIx64, (uint64_t), field)
-
-#define PRINT_ATTR2N(name1, field1, name2, field2)	\
-	fprintf(fp, "  %-19s %u    %-19s %u\n",		\
-	name1, attr->field1, name2, attr->field2)
-
-#define PRINT_ATTR2(field1, field2) \
-	PRINT_ATTR2N(#field1, field1, #field2, field2)
-
 static size_t perf_event_attr__fprintf(struct perf_event_attr *attr, FILE *fp)
 {
 	size_t ret = 0;
@@ -1033,42 +1019,18 @@ static size_t perf_event_attr__fprintf(s
 	ret += fprintf(fp, "%.60s\n", graph_dotted_line);
 	ret += fprintf(fp, "perf_event_attr:\n");
 
-	ret += PRINT_ATTR_U32(type);
-	ret += PRINT_ATTR_U32(size);
-	ret += PRINT_ATTR_X64(config);
-	ret += PRINT_ATTR_U64(sample_period);
-	ret += PRINT_ATTR_U64(sample_freq);
-	ret += PRINT_ATTR_X64(sample_type);
-	ret += PRINT_ATTR_X64(read_format);
-
-	ret += PRINT_ATTR2(disabled, inherit);
-	ret += PRINT_ATTR2(pinned, exclusive);
-	ret += PRINT_ATTR2(exclude_user, exclude_kernel);
-	ret += PRINT_ATTR2(exclude_hv, exclude_idle);
-	ret += PRINT_ATTR2(mmap, comm);
-	ret += PRINT_ATTR2(freq, inherit_stat);
-	ret += PRINT_ATTR2(enable_on_exec, task);
-	ret += PRINT_ATTR2(watermark, precise_ip);
-	ret += PRINT_ATTR2(mmap_data, sample_id_all);
-	ret += PRINT_ATTR2(exclude_host, exclude_guest);
-	ret += PRINT_ATTR2N("excl.callchain_kern", exclude_callchain_kernel,
-			    "excl.callchain_user", exclude_callchain_user);
-	ret += PRINT_ATTR2(mmap2, comm_exec);
-	ret += __PRINT_ATTR("%u",,use_clockid);
-
-
-	ret += PRINT_ATTR_U32(wakeup_events);
-	ret += PRINT_ATTR_U32(wakeup_watermark);
-	ret += PRINT_ATTR_X32(bp_type);
-	ret += PRINT_ATTR_X64(bp_addr);
-	ret += PRINT_ATTR_X64(config1);
-	ret += PRINT_ATTR_U64(bp_len);
-	ret += PRINT_ATTR_X64(config2);
-	ret += PRINT_ATTR_X64(branch_sample_type);
-	ret += PRINT_ATTR_X64(sample_regs_user);
-	ret += PRINT_ATTR_U32(sample_stack_user);
-	ret += PRINT_ATTR_U32(clockid);
-	ret += PRINT_ATTR_X64(sample_regs_intr);
+#define PRINT_ATTR(_n, _f, _p)			\
+do {						\
+	if (attr->_f) {				\
+	ret += fprintf(fp, "  %-32s ", _n);	\
+	ret += _p(fp, attr->_f);		\
+	ret += fprintf(fp, "\n");		\
+	}					\
+} while (0)
+
+#include "util/print_attr.h"
+
+#undef PRINT_ATTR
 
 	ret += fprintf(fp, "%.60s\n", graph_dotted_line);
 
@@ -1996,64 +1958,6 @@ static int comma_fprintf(FILE *fp, bool
 	return ret;
 }
 
-static int __if_fprintf(FILE *fp, bool *first, const char *field, u64 value)
-{
-	if (value == 0)
-		return 0;
-
-	return comma_fprintf(fp, first, " %s: %" PRIu64, field, value);
-}
-
-#define if_print(field) printed += __if_fprintf(fp, &first, #field, evsel->attr.field)
-
-struct bit_names {
-	int bit;
-	const char *name;
-};
-
-static int bits__fprintf(FILE *fp, const char *field, u64 value,
-			 struct bit_names *bits, bool *first)
-{
-	int i = 0, printed = comma_fprintf(fp, first, " %s: ", field);
-	bool first_bit = true;
-
-	do {
-		if (value & bits[i].bit) {
-			printed += fprintf(fp, "%s%s", first_bit ? "" : "|", bits[i].name);
-			first_bit = false;
-		}
-	} while (bits[++i].name != NULL);
-
-	return printed;
-}
-
-static int sample_type__fprintf(FILE *fp, bool *first, u64 value)
-{
-#define bit_name(n) { PERF_SAMPLE_##n, #n }
-	struct bit_names bits[] = {
-		bit_name(IP), bit_name(TID), bit_name(TIME), bit_name(ADDR),
-		bit_name(READ), bit_name(CALLCHAIN), bit_name(ID), bit_name(CPU),
-		bit_name(PERIOD), bit_name(STREAM_ID), bit_name(RAW),
-		bit_name(BRANCH_STACK), bit_name(REGS_USER), bit_name(STACK_USER),
-		bit_name(IDENTIFIER), bit_name(REGS_INTR),
-		{ .name = NULL, }
-	};
-#undef bit_name
-	return bits__fprintf(fp, "sample_type", value, bits, first);
-}
-
-static int read_format__fprintf(FILE *fp, bool *first, u64 value)
-{
-#define bit_name(n) { PERF_FORMAT_##n, #n }
-	struct bit_names bits[] = {
-		bit_name(TOTAL_TIME_ENABLED), bit_name(TOTAL_TIME_RUNNING),
-		bit_name(ID), bit_name(GROUP),
-		{ .name = NULL, }
-	};
-#undef bit_name
-	return bits__fprintf(fp, "read_format", value, bits, first);
-}
-
 int perf_evsel__fprintf(struct perf_evsel *evsel,
 			struct perf_attr_details *details, FILE *fp)
 {
@@ -2080,51 +1984,24 @@ int perf_evsel__fprintf(struct perf_evse
 
 	printed += fprintf(fp, "%s", perf_evsel__name(evsel));
 
-	if (details->verbose || details->freq) {
-		printed += comma_fprintf(fp, &first, " sample_freq=%" PRIu64,
-					 (u64)evsel->attr.sample_freq);
-	}
 
 	if (details->verbose) {
-		if_print(type);
-		if_print(config);
-		if_print(config1);
-		if_print(config2);
-		if_print(size);
-		printed += sample_type__fprintf(fp, &first, evsel->attr.sample_type);
-		if (evsel->attr.read_format)
-			printed += read_format__fprintf(fp, &first, evsel->attr.read_format);
-		if_print(disabled);
-		if_print(inherit);
-		if_print(pinned);
-		if_print(exclusive);
-		if_print(exclude_user);
-		if_print(exclude_kernel);
-		if_print(exclude_hv);
-		if_print(exclude_idle);
-		if_print(mmap);
-		if_print(comm);
-		if_print(freq);
-		if_print(inherit_stat);
-		if_print(enable_on_exec);
-		if_print(task);
-		if_print(watermark);
-		if_print(precise_ip);
-		if_print(mmap_data);
-		if_print(sample_id_all);
-		if_print(exclude_host);
-		if_print(exclude_guest);
-		if_print(mmap2);
-		if_print(comm_exec);
-		if_print(use_clockid);
-		if_print(__reserved_1);
-		if_print(wakeup_events);
-		if_print(bp_type);
-		if_print(branch_sample_type);
-		if_print(sample_regs_user);
-		if_print(sample_stack_user);
-		if_print(clockid);
-		if_print(sample_regs_intr);
+
+#define PRINT_ATTR(_n, _f, _p)						\
+do {									\
+	if (evsel->attr._f) {						\
+		printed += comma_fprintf(fp, &first, " %s: ", _n);	\
+		printed += _p(fp, evsel->attr._f);			\
+	}								\
+} while (0)
+
+#include "util/print_attr.h"
+
+#undef PRINT_ATTR
+
+	} else if (details->freq) {
+		printed += comma_fprintf(fp, &first, " sample_freq=%" PRIu64,
+					 (u64)evsel->attr.sample_freq);
 	}
 out:
 	fputc('\n', fp);
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -23,6 +23,7 @@
 #include "strbuf.h"
 #include "build-id.h"
 #include "data.h"
+#include "print_helper.h"
 
 static u32 header_argc;
 static const char **header_argv;
@@ -1069,26 +1070,6 @@ static void print_event_desc(struct perf
 	for (evsel = events; evsel->attr.size; evsel++) {
 		fprintf(fp, "# event : name = %s, ", evsel->name);
 
-		fprintf(fp, "type = %d, config = 0x%"PRIx64
-			    ", config1 = 0x%"PRIx64", config2 = 0x%"PRIx64,
-				evsel->attr.type,
-				(u64)evsel->attr.config,
-				(u64)evsel->attr.config1,
-				(u64)evsel->attr.config2);
-
-		fprintf(fp, ", excl_usr = %d, excl_kern = %d",
-				evsel->attr.exclude_user,
-				evsel->attr.exclude_kernel);
-
-		fprintf(fp, ", excl_host = %d, excl_guest = %d",
-				evsel->attr.exclude_host,
-				evsel->attr.exclude_guest);
-
-		fprintf(fp, ", precise_ip = %d", evsel->attr.precise_ip);
-
-		fprintf(fp, ", attr_mmap2 = %d", evsel->attr.mmap2);
-		fprintf(fp, ", attr_mmap  = %d", evsel->attr.mmap);
-		fprintf(fp, ", attr_mmap_data = %d", evsel->attr.mmap_data);
 		if (evsel->ids) {
 			fprintf(fp, ", id = {");
 			for (j = 0, id = evsel->id; j < evsel->ids; j++, id++) {
@@ -1098,9 +1079,18 @@ static void print_event_desc(struct perf
 			}
 			fprintf(fp, " }");
 		}
-		if (evsel->attr.use_clockid)
-			fprintf(fp, ", clockid = %d", evsel->attr.clockid);
 
+#define PRINT_ATTR(_n, _f, _p)			\
+do {						\
+	if (evsel->attr._f) {			\
+		fprintf(fp, ", %s =", _n);	\
+		_p(fp, evsel->attr._f);		\
+	}					\
+} while (0)
+
+#include "util/print_attr.h"
+
+#undef PRINT_ATTR
 
 		fputc('\n', fp);
 	}
--- /dev/null
+++ b/tools/perf/util/print_attr.h
@@ -0,0 +1,69 @@
+
+#ifndef PRINT_ATTR
+/*
+ * #define PRINT_ATTR(name, field, print) 		\
+ * do {							\
+ * 	fprintf(fp, " %s = ", name);			\
+ * 	print(fp, attr->field);				\
+ * } while (0)
+ */
+#error "General Error and Major Fault yell at you!"
+#endif
+
+#define p_hex(fp, val)		fprintf(fp, "%"PRIx64, (uint64_t)(val))
+#define p_unsigned(fp, val)	fprintf(fp, "%"PRIu64, (uint64_t)(val))
+#define p_signed(fp, val)	fprintf(fp, "%"PRId64, (int64_t)(val))
+#define p_sample_type(fp, val)	sample_type__fprintf(fp, val)
+#define p_read_format(fp, val)	read_format__fprintf(fp, val)
+
+#define PRINT_ATTRf(field, print) PRINT_ATTR(#field, field, print)
+
+PRINT_ATTRf(type, p_unsigned);
+PRINT_ATTRf(size, p_unsigned);
+PRINT_ATTRf(config, p_hex);
+PRINT_ATTR("{ sample_period, sample_freq }", sample_period, p_unsigned);
+PRINT_ATTRf(sample_type, p_sample_type);
+PRINT_ATTRf(read_format, p_read_format);
+
+PRINT_ATTRf(disabled, p_unsigned);
+PRINT_ATTRf(inherit, p_unsigned);
+PRINT_ATTRf(pinned, p_unsigned);
+PRINT_ATTRf(exclusive, p_unsigned);
+PRINT_ATTRf(exclude_user, p_unsigned);
+PRINT_ATTRf(exclude_kernel, p_unsigned);
+PRINT_ATTRf(exclude_hv, p_unsigned);
+PRINT_ATTRf(exclude_idle, p_unsigned);
+PRINT_ATTRf(mmap, p_unsigned);
+PRINT_ATTRf(comm, p_unsigned);
+PRINT_ATTRf(freq, p_unsigned);
+PRINT_ATTRf(inherit_stat, p_unsigned);
+PRINT_ATTRf(enable_on_exec, p_unsigned);
+PRINT_ATTRf(task, p_unsigned);
+PRINT_ATTRf(watermark, p_unsigned);
+PRINT_ATTRf(precise_ip, p_unsigned);
+PRINT_ATTRf(mmap_data, p_unsigned);
+PRINT_ATTRf(sample_id_all, p_unsigned);
+PRINT_ATTRf(exclude_host, p_unsigned);
+PRINT_ATTRf(exclude_guest, p_unsigned);
+PRINT_ATTRf(exclude_callchain_kernel, p_unsigned);
+PRINT_ATTRf(exclude_callchain_user, p_unsigned);
+PRINT_ATTRf(mmap2, p_unsigned);
+PRINT_ATTRf(comm_exec, p_unsigned);
+PRINT_ATTRf(use_clockid, p_unsigned);
+
+PRINT_ATTR("{ wakeup_events, wakeup_watermark }", wakeup_events, p_unsigned);
+PRINT_ATTRf(bp_type, p_unsigned);
+PRINT_ATTR("{ bp_addr, config1 }", bp_addr, p_hex);
+PRINT_ATTR("{ bp_len, config2 }", bp_len, p_hex);
+PRINT_ATTRf(sample_regs_user, p_hex);
+PRINT_ATTRf(sample_stack_user, p_unsigned);
+PRINT_ATTRf(clockid, p_signed);
+PRINT_ATTRf(sample_regs_intr, p_hex);
+
+#undef PRINT_ATTRf
+
+#undef p_hex
+#undef p_unsigned
+#undef p_signed
+#undef p_sample_type
+#undef p_read_format
--- /dev/null
+++ b/tools/perf/util/print_helper.c
@@ -0,0 +1,52 @@
+
+#include <linux/perf_event.h>
+#include "util.h"
+#include "print_helper.h"
+
+struct bit_names {
+	int bit;
+	const char *name;
+};
+
+static int bits__fprintf(FILE *fp, u64 value, struct bit_names *bits)
+{
+	int i = 0, printed = 0;
+	bool first_bit = true;
+
+	do {
+		if (value & bits[i].bit) {
+			printed += fprintf(fp, "%s%s", first_bit ? "" : "|", bits[i].name);
+			first_bit = false;
+		}
+	} while (bits[++i].name != NULL);
+
+	return printed;
+}
+
+int sample_type__fprintf(FILE *fp, u64 value)
+{
+#define bit_name(n) { PERF_SAMPLE_##n, #n }
+	struct bit_names bits[] = {
+		bit_name(IP), bit_name(TID), bit_name(TIME), bit_name(ADDR),
+		bit_name(READ), bit_name(CALLCHAIN), bit_name(ID), bit_name(CPU),
+		bit_name(PERIOD), bit_name(STREAM_ID), bit_name(RAW),
+		bit_name(BRANCH_STACK), bit_name(REGS_USER), bit_name(STACK_USER),
+		bit_name(IDENTIFIER), bit_name(REGS_INTR),
+		{ .name = NULL, }
+	};
+#undef bit_name
+	return bits__fprintf(fp, value, bits);
+}
+
+int read_format__fprintf(FILE *fp, u64 value)
+{
+#define bit_name(n) { PERF_FORMAT_##n, #n }
+	struct bit_names bits[] = {
+		bit_name(TOTAL_TIME_ENABLED), bit_name(TOTAL_TIME_RUNNING),
+		bit_name(ID), bit_name(GROUP),
+		{ .name = NULL, }
+	};
+#undef bit_name
+	return bits__fprintf(fp, value, bits);
+}
+
--- /dev/null
+++ b/tools/perf/util/print_helper.h
@@ -0,0 +1,7 @@
+#ifndef PRINT_HELPER_H
+#define PRINT_HELPER_H
+
+extern int sample_type__fprintf(FILE *fp, u64 value);
+extern int read_format__fprintf(FILE *fp, u64 value);
+
+#endif

Re: [RFC][PATCH] perf tools: unify perf_event_attr printing

[Jiri Olsa]

On Wed, Apr 01, 2015 at 06:26:38PM +0200, Peter Zijlstra wrote:
> 
> With some feedback from Jolsa, who showed me how to trigger the actual
> outputs.

adding Adrian to CC as he's the original author AFAIK

jirka

> 
> ---
> 
> Subject: perf, tools: Merge all perf_event_attr print functions
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Tue Mar 31 13:01:54 CEST 2015
> 
> Currently there's 3 (that I found) different and incomplete
> implementations of printing perf_event_attr.
> 
> This is quite silly. Merge the lot.
> 
> While this patch does not retain the exact form all printing that I
> found is debug output and thus it should not be critical.
> 
> Also, I cannot find a single print_event_desc() caller.
> 
> Pre:
> 
> $ perf record -vv -e cycles -- sleep 1
> ------------------------------------------------------------
> perf_event_attr:
>   type                0
>   size                104
>   config              0
>   sample_period       4000
>   sample_freq         4000
>   sample_type         0x107
>   read_format         0
>   disabled            1    inherit             1
>   pinned              0    exclusive           0
>   exclude_user        0    exclude_kernel      0
>   exclude_hv          0    exclude_idle        0
>   mmap                1    comm                1
>   mmap2               1    comm_exec           1
>   freq                1    inherit_stat        0
>   enable_on_exec      1    task                1
>   watermark           0    precise_ip          0
>   mmap_data           0    sample_id_all       1
>   exclude_host        0    exclude_guest       1
>   excl.callchain_kern 0    excl.callchain_user 0
>   wakeup_events       0
>   wakeup_watermark    0
>   bp_type             0
>   bp_addr             0
>   config1             0
>   bp_len              0
>   config2             0
>   branch_sample_type  0
>   sample_regs_user    0
>   sample_stack_user   0
>   sample_regs_intr    0
> ------------------------------------------------------------
> 
> $ perf evlist  -vv
> cycles: sample_freq=4000, size: 104, sample_type: IP|TID|TIME|PERIOD,
> disabled: 1, inherit: 1, mmap: 1, mmap2: 1, comm: 1, comm_exec: 1,
> freq: 1, enable_on_exec: 1, task: 1, sample_id_all: 1, exclude_guest:
> 1
> 
> Post:
> 
> $ ./perf record -vv -e cycles -- sleep 1 
> ------------------------------------------------------------
> perf_event_attr:
>   size                             112
>   { sample_period, sample_freq }   4000
>   sample_type                      IP|TID|TIME|PERIOD
>   disabled                         1
>   inherit                          1
>   mmap                             1
>   comm                             1
>   freq                             1
>   enable_on_exec                   1
>   task                             1
>   sample_id_all                    1
>   exclude_guest                    1
>   mmap2                            1
>   comm_exec                        1
> ------------------------------------------------------------
> 
> $ ./perf evlist  -vv
> cycles: size: 112, { sample_period, sample_freq }: 4000, sample_type:
> IP|TID|TIME|PERIOD, disabled: 1, inherit: 1, mmap: 1, comm: 1, freq:
> 1, enable_on_exec: 1, task: 1, sample_id_all: 1, exclude_guest: 1,
> mmap2: 1, comm_exec: 1
> 
> Cc: acme@redhat.com
> Cc: jolsa@redhat.com
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  tools/perf/util/Build          |    1 
>  tools/perf/util/evsel.c        |  181 ++++++-----------------------------------
>  tools/perf/util/header.c       |   34 ++-----
>  tools/perf/util/print_attr.h   |   69 +++++++++++++++
>  tools/perf/util/print_helper.c |   52 +++++++++++
>  tools/perf/util/print_helper.h |    7 +
>  6 files changed, 170 insertions(+), 174 deletions(-)
> 
> --- a/tools/perf/util/Build
> +++ b/tools/perf/util/Build
> @@ -74,6 +74,7 @@ libperf-y += data.o
>  libperf-$(CONFIG_X86) += tsc.o
>  libperf-y += cloexec.o
>  libperf-y += thread-stack.o
> +libperf-y += print_helper.o
>  
>  libperf-$(CONFIG_LIBELF) += symbol-elf.o
>  libperf-$(CONFIG_LIBELF) += probe-event.o
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -26,6 +26,7 @@
>  #include "perf_regs.h"
>  #include "debug.h"
>  #include "trace-event.h"
> +#include "print_helper.h"
>  
>  static struct {
>  	bool sample_id_all;
> @@ -1011,21 +1012,6 @@ static int get_group_fd(struct perf_evse
>  	return fd;
>  }
>  
> -#define __PRINT_ATTR(fmt, cast, field)  \
> -	fprintf(fp, "  %-19s "fmt"\n", #field, cast attr->field)
> -
> -#define PRINT_ATTR_U32(field)  __PRINT_ATTR("%u" , , field)
> -#define PRINT_ATTR_X32(field)  __PRINT_ATTR("%#x", , field)
> -#define PRINT_ATTR_U64(field)  __PRINT_ATTR("%" PRIu64, (uint64_t), field)
> -#define PRINT_ATTR_X64(field)  __PRINT_ATTR("%#"PRIx64, (uint64_t), field)
> -
> -#define PRINT_ATTR2N(name1, field1, name2, field2)	\
> -	fprintf(fp, "  %-19s %u    %-19s %u\n",		\
> -	name1, attr->field1, name2, attr->field2)
> -
> -#define PRINT_ATTR2(field1, field2) \
> -	PRINT_ATTR2N(#field1, field1, #field2, field2)
> -
>  static size_t perf_event_attr__fprintf(struct perf_event_attr *attr, FILE *fp)
>  {
>  	size_t ret = 0;
> @@ -1033,42 +1019,18 @@ static size_t perf_event_attr__fprintf(s
>  	ret += fprintf(fp, "%.60s\n", graph_dotted_line);
>  	ret += fprintf(fp, "perf_event_attr:\n");
>  
> -	ret += PRINT_ATTR_U32(type);
> -	ret += PRINT_ATTR_U32(size);
> -	ret += PRINT_ATTR_X64(config);
> -	ret += PRINT_ATTR_U64(sample_period);
> -	ret += PRINT_ATTR_U64(sample_freq);
> -	ret += PRINT_ATTR_X64(sample_type);
> -	ret += PRINT_ATTR_X64(read_format);
> -
> -	ret += PRINT_ATTR2(disabled, inherit);
> -	ret += PRINT_ATTR2(pinned, exclusive);
> -	ret += PRINT_ATTR2(exclude_user, exclude_kernel);
> -	ret += PRINT_ATTR2(exclude_hv, exclude_idle);
> -	ret += PRINT_ATTR2(mmap, comm);
> -	ret += PRINT_ATTR2(freq, inherit_stat);
> -	ret += PRINT_ATTR2(enable_on_exec, task);
> -	ret += PRINT_ATTR2(watermark, precise_ip);
> -	ret += PRINT_ATTR2(mmap_data, sample_id_all);
> -	ret += PRINT_ATTR2(exclude_host, exclude_guest);
> -	ret += PRINT_ATTR2N("excl.callchain_kern", exclude_callchain_kernel,
> -			    "excl.callchain_user", exclude_callchain_user);
> -	ret += PRINT_ATTR2(mmap2, comm_exec);
> -	ret += __PRINT_ATTR("%u",,use_clockid);
> -
> -
> -	ret += PRINT_ATTR_U32(wakeup_events);
> -	ret += PRINT_ATTR_U32(wakeup_watermark);
> -	ret += PRINT_ATTR_X32(bp_type);
> -	ret += PRINT_ATTR_X64(bp_addr);
> -	ret += PRINT_ATTR_X64(config1);
> -	ret += PRINT_ATTR_U64(bp_len);
> -	ret += PRINT_ATTR_X64(config2);
> -	ret += PRINT_ATTR_X64(branch_sample_type);
> -	ret += PRINT_ATTR_X64(sample_regs_user);
> -	ret += PRINT_ATTR_U32(sample_stack_user);
> -	ret += PRINT_ATTR_U32(clockid);
> -	ret += PRINT_ATTR_X64(sample_regs_intr);
> +#define PRINT_ATTR(_n, _f, _p)			\
> +do {						\
> +	if (attr->_f) {				\
> +	ret += fprintf(fp, "  %-32s ", _n);	\
> +	ret += _p(fp, attr->_f);		\
> +	ret += fprintf(fp, "\n");		\
> +	}					\
> +} while (0)
> +
> +#include "util/print_attr.h"
> +
> +#undef PRINT_ATTR
>  
>  	ret += fprintf(fp, "%.60s\n", graph_dotted_line);
>  
> @@ -1996,64 +1958,6 @@ static int comma_fprintf(FILE *fp, bool
>  	return ret;
>  }
>  
> -static int __if_fprintf(FILE *fp, bool *first, const char *field, u64 value)
> -{
> -	if (value == 0)
> -		return 0;
> -
> -	return comma_fprintf(fp, first, " %s: %" PRIu64, field, value);
> -}
> -
> -#define if_print(field) printed += __if_fprintf(fp, &first, #field, evsel->attr.field)
> -
> -struct bit_names {
> -	int bit;
> -	const char *name;
> -};
> -
> -static int bits__fprintf(FILE *fp, const char *field, u64 value,
> -			 struct bit_names *bits, bool *first)
> -{
> -	int i = 0, printed = comma_fprintf(fp, first, " %s: ", field);
> -	bool first_bit = true;
> -
> -	do {
> -		if (value & bits[i].bit) {
> -			printed += fprintf(fp, "%s%s", first_bit ? "" : "|", bits[i].name);
> -			first_bit = false;
> -		}
> -	} while (bits[++i].name != NULL);
> -
> -	return printed;
> -}
> -
> -static int sample_type__fprintf(FILE *fp, bool *first, u64 value)
> -{
> -#define bit_name(n) { PERF_SAMPLE_##n, #n }
> -	struct bit_names bits[] = {
> -		bit_name(IP), bit_name(TID), bit_name(TIME), bit_name(ADDR),
> -		bit_name(READ), bit_name(CALLCHAIN), bit_name(ID), bit_name(CPU),
> -		bit_name(PERIOD), bit_name(STREAM_ID), bit_name(RAW),
> -		bit_name(BRANCH_STACK), bit_name(REGS_USER), bit_name(STACK_USER),
> -		bit_name(IDENTIFIER), bit_name(REGS_INTR),
> -		{ .name = NULL, }
> -	};
> -#undef bit_name
> -	return bits__fprintf(fp, "sample_type", value, bits, first);
> -}
> -
> -static int read_format__fprintf(FILE *fp, bool *first, u64 value)
> -{
> -#define bit_name(n) { PERF_FORMAT_##n, #n }
> -	struct bit_names bits[] = {
> -		bit_name(TOTAL_TIME_ENABLED), bit_name(TOTAL_TIME_RUNNING),
> -		bit_name(ID), bit_name(GROUP),
> -		{ .name = NULL, }
> -	};
> -#undef bit_name
> -	return bits__fprintf(fp, "read_format", value, bits, first);
> -}
> -
>  int perf_evsel__fprintf(struct perf_evsel *evsel,
>  			struct perf_attr_details *details, FILE *fp)
>  {
> @@ -2080,51 +1984,24 @@ int perf_evsel__fprintf(struct perf_evse
>  
>  	printed += fprintf(fp, "%s", perf_evsel__name(evsel));
>  
> -	if (details->verbose || details->freq) {
> -		printed += comma_fprintf(fp, &first, " sample_freq=%" PRIu64,
> -					 (u64)evsel->attr.sample_freq);
> -	}
>  
>  	if (details->verbose) {
> -		if_print(type);
> -		if_print(config);
> -		if_print(config1);
> -		if_print(config2);
> -		if_print(size);
> -		printed += sample_type__fprintf(fp, &first, evsel->attr.sample_type);
> -		if (evsel->attr.read_format)
> -			printed += read_format__fprintf(fp, &first, evsel->attr.read_format);
> -		if_print(disabled);
> -		if_print(inherit);
> -		if_print(pinned);
> -		if_print(exclusive);
> -		if_print(exclude_user);
> -		if_print(exclude_kernel);
> -		if_print(exclude_hv);
> -		if_print(exclude_idle);
> -		if_print(mmap);
> -		if_print(comm);
> -		if_print(freq);
> -		if_print(inherit_stat);
> -		if_print(enable_on_exec);
> -		if_print(task);
> -		if_print(watermark);
> -		if_print(precise_ip);
> -		if_print(mmap_data);
> -		if_print(sample_id_all);
> -		if_print(exclude_host);
> -		if_print(exclude_guest);
> -		if_print(mmap2);
> -		if_print(comm_exec);
> -		if_print(use_clockid);
> -		if_print(__reserved_1);
> -		if_print(wakeup_events);
> -		if_print(bp_type);
> -		if_print(branch_sample_type);
> -		if_print(sample_regs_user);
> -		if_print(sample_stack_user);
> -		if_print(clockid);
> -		if_print(sample_regs_intr);
> +
> +#define PRINT_ATTR(_n, _f, _p)						\
> +do {									\
> +	if (evsel->attr._f) {						\
> +		printed += comma_fprintf(fp, &first, " %s: ", _n);	\
> +		printed += _p(fp, evsel->attr._f);			\
> +	}								\
> +} while (0)
> +
> +#include "util/print_attr.h"
> +
> +#undef PRINT_ATTR
> +
> +	} else if (details->freq) {
> +		printed += comma_fprintf(fp, &first, " sample_freq=%" PRIu64,
> +					 (u64)evsel->attr.sample_freq);
>  	}
>  out:
>  	fputc('\n', fp);
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -23,6 +23,7 @@
>  #include "strbuf.h"
>  #include "build-id.h"
>  #include "data.h"
> +#include "print_helper.h"
>  
>  static u32 header_argc;
>  static const char **header_argv;
> @@ -1069,26 +1070,6 @@ static void print_event_desc(struct perf
>  	for (evsel = events; evsel->attr.size; evsel++) {
>  		fprintf(fp, "# event : name = %s, ", evsel->name);
>  
> -		fprintf(fp, "type = %d, config = 0x%"PRIx64
> -			    ", config1 = 0x%"PRIx64", config2 = 0x%"PRIx64,
> -				evsel->attr.type,
> -				(u64)evsel->attr.config,
> -				(u64)evsel->attr.config1,
> -				(u64)evsel->attr.config2);
> -
> -		fprintf(fp, ", excl_usr = %d, excl_kern = %d",
> -				evsel->attr.exclude_user,
> -				evsel->attr.exclude_kernel);
> -
> -		fprintf(fp, ", excl_host = %d, excl_guest = %d",
> -				evsel->attr.exclude_host,
> -				evsel->attr.exclude_guest);
> -
> -		fprintf(fp, ", precise_ip = %d", evsel->attr.precise_ip);
> -
> -		fprintf(fp, ", attr_mmap2 = %d", evsel->attr.mmap2);
> -		fprintf(fp, ", attr_mmap  = %d", evsel->attr.mmap);
> -		fprintf(fp, ", attr_mmap_data = %d", evsel->attr.mmap_data);
>  		if (evsel->ids) {
>  			fprintf(fp, ", id = {");
>  			for (j = 0, id = evsel->id; j < evsel->ids; j++, id++) {
> @@ -1098,9 +1079,18 @@ static void print_event_desc(struct perf
>  			}
>  			fprintf(fp, " }");
>  		}
> -		if (evsel->attr.use_clockid)
> -			fprintf(fp, ", clockid = %d", evsel->attr.clockid);
>  
> +#define PRINT_ATTR(_n, _f, _p)			\
> +do {						\
> +	if (evsel->attr._f) {			\
> +		fprintf(fp, ", %s =", _n);	\
> +		_p(fp, evsel->attr._f);		\
> +	}					\
> +} while (0)
> +
> +#include "util/print_attr.h"
> +
> +#undef PRINT_ATTR
>  
>  		fputc('\n', fp);
>  	}
> --- /dev/null
> +++ b/tools/perf/util/print_attr.h
> @@ -0,0 +1,69 @@
> +
> +#ifndef PRINT_ATTR
> +/*
> + * #define PRINT_ATTR(name, field, print) 		\
> + * do {							\
> + * 	fprintf(fp, " %s = ", name);			\
> + * 	print(fp, attr->field);				\
> + * } while (0)
> + */
> +#error "General Error and Major Fault yell at you!"
> +#endif
> +
> +#define p_hex(fp, val)		fprintf(fp, "%"PRIx64, (uint64_t)(val))
> +#define p_unsigned(fp, val)	fprintf(fp, "%"PRIu64, (uint64_t)(val))
> +#define p_signed(fp, val)	fprintf(fp, "%"PRId64, (int64_t)(val))
> +#define p_sample_type(fp, val)	sample_type__fprintf(fp, val)
> +#define p_read_format(fp, val)	read_format__fprintf(fp, val)
> +
> +#define PRINT_ATTRf(field, print) PRINT_ATTR(#field, field, print)
> +
> +PRINT_ATTRf(type, p_unsigned);
> +PRINT_ATTRf(size, p_unsigned);
> +PRINT_ATTRf(config, p_hex);
> +PRINT_ATTR("{ sample_period, sample_freq }", sample_period, p_unsigned);
> +PRINT_ATTRf(sample_type, p_sample_type);
> +PRINT_ATTRf(read_format, p_read_format);
> +
> +PRINT_ATTRf(disabled, p_unsigned);
> +PRINT_ATTRf(inherit, p_unsigned);
> +PRINT_ATTRf(pinned, p_unsigned);
> +PRINT_ATTRf(exclusive, p_unsigned);
> +PRINT_ATTRf(exclude_user, p_unsigned);
> +PRINT_ATTRf(exclude_kernel, p_unsigned);
> +PRINT_ATTRf(exclude_hv, p_unsigned);
> +PRINT_ATTRf(exclude_idle, p_unsigned);
> +PRINT_ATTRf(mmap, p_unsigned);
> +PRINT_ATTRf(comm, p_unsigned);
> +PRINT_ATTRf(freq, p_unsigned);
> +PRINT_ATTRf(inherit_stat, p_unsigned);
> +PRINT_ATTRf(enable_on_exec, p_unsigned);
> +PRINT_ATTRf(task, p_unsigned);
> +PRINT_ATTRf(watermark, p_unsigned);
> +PRINT_ATTRf(precise_ip, p_unsigned);
> +PRINT_ATTRf(mmap_data, p_unsigned);
> +PRINT_ATTRf(sample_id_all, p_unsigned);
> +PRINT_ATTRf(exclude_host, p_unsigned);
> +PRINT_ATTRf(exclude_guest, p_unsigned);
> +PRINT_ATTRf(exclude_callchain_kernel, p_unsigned);
> +PRINT_ATTRf(exclude_callchain_user, p_unsigned);
> +PRINT_ATTRf(mmap2, p_unsigned);
> +PRINT_ATTRf(comm_exec, p_unsigned);
> +PRINT_ATTRf(use_clockid, p_unsigned);
> +
> +PRINT_ATTR("{ wakeup_events, wakeup_watermark }", wakeup_events, p_unsigned);
> +PRINT_ATTRf(bp_type, p_unsigned);
> +PRINT_ATTR("{ bp_addr, config1 }", bp_addr, p_hex);
> +PRINT_ATTR("{ bp_len, config2 }", bp_len, p_hex);
> +PRINT_ATTRf(sample_regs_user, p_hex);
> +PRINT_ATTRf(sample_stack_user, p_unsigned);
> +PRINT_ATTRf(clockid, p_signed);
> +PRINT_ATTRf(sample_regs_intr, p_hex);
> +
> +#undef PRINT_ATTRf
> +
> +#undef p_hex
> +#undef p_unsigned
> +#undef p_signed
> +#undef p_sample_type
> +#undef p_read_format
> --- /dev/null
> +++ b/tools/perf/util/print_helper.c
> @@ -0,0 +1,52 @@
> +
> +#include <linux/perf_event.h>
> +#include "util.h"
> +#include "print_helper.h"
> +
> +struct bit_names {
> +	int bit;
> +	const char *name;
> +};
> +
> +static int bits__fprintf(FILE *fp, u64 value, struct bit_names *bits)
> +{
> +	int i = 0, printed = 0;
> +	bool first_bit = true;
> +
> +	do {
> +		if (value & bits[i].bit) {
> +			printed += fprintf(fp, "%s%s", first_bit ? "" : "|", bits[i].name);
> +			first_bit = false;
> +		}
> +	} while (bits[++i].name != NULL);
> +
> +	return printed;
> +}
> +
> +int sample_type__fprintf(FILE *fp, u64 value)
> +{
> +#define bit_name(n) { PERF_SAMPLE_##n, #n }
> +	struct bit_names bits[] = {
> +		bit_name(IP), bit_name(TID), bit_name(TIME), bit_name(ADDR),
> +		bit_name(READ), bit_name(CALLCHAIN), bit_name(ID), bit_name(CPU),
> +		bit_name(PERIOD), bit_name(STREAM_ID), bit_name(RAW),
> +		bit_name(BRANCH_STACK), bit_name(REGS_USER), bit_name(STACK_USER),
> +		bit_name(IDENTIFIER), bit_name(REGS_INTR),
> +		{ .name = NULL, }
> +	};
> +#undef bit_name
> +	return bits__fprintf(fp, value, bits);
> +}
> +
> +int read_format__fprintf(FILE *fp, u64 value)
> +{
> +#define bit_name(n) { PERF_FORMAT_##n, #n }
> +	struct bit_names bits[] = {
> +		bit_name(TOTAL_TIME_ENABLED), bit_name(TOTAL_TIME_RUNNING),
> +		bit_name(ID), bit_name(GROUP),
> +		{ .name = NULL, }
> +	};
> +#undef bit_name
> +	return bits__fprintf(fp, value, bits);
> +}
> +
> --- /dev/null
> +++ b/tools/perf/util/print_helper.h
> @@ -0,0 +1,7 @@
> +#ifndef PRINT_HELPER_H
> +#define PRINT_HELPER_H
> +
> +extern int sample_type__fprintf(FILE *fp, u64 value);
> +extern int read_format__fprintf(FILE *fp, u64 value);
> +
> +#endif

Re: [RFC][PATCH] perf tools: unify perf_event_attr printing

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> With some feedback from Jolsa, who showed me how to trigger the actual
> outputs.
> 
> ---
> 
> Subject: perf, tools: Merge all perf_event_attr print functions
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Tue Mar 31 13:01:54 CEST 2015
> 
> Currently there's 3 (that I found) different and incomplete
> implementations of printing perf_event_attr.
> 
> This is quite silly. Merge the lot.
> 
> While this patch does not retain the exact form all printing that I
> found is debug output and thus it should not be critical.
> 
> Also, I cannot find a single print_event_desc() caller.
> 
> Pre:
> 
> $ perf record -vv -e cycles -- sleep 1
> ------------------------------------------------------------
> perf_event_attr:
>   type                0
>   size                104
>   config              0
>   sample_period       4000
>   sample_freq         4000
>   sample_type         0x107
>   read_format         0
>   disabled            1    inherit             1
>   pinned              0    exclusive           0
>   exclude_user        0    exclude_kernel      0
>   exclude_hv          0    exclude_idle        0
>   mmap                1    comm                1
>   mmap2               1    comm_exec           1
>   freq                1    inherit_stat        0
>   enable_on_exec      1    task                1
>   watermark           0    precise_ip          0
>   mmap_data           0    sample_id_all       1
>   exclude_host        0    exclude_guest       1
>   excl.callchain_kern 0    excl.callchain_user 0
>   wakeup_events       0
>   wakeup_watermark    0
>   bp_type             0
>   bp_addr             0
>   config1             0
>   bp_len              0
>   config2             0
>   branch_sample_type  0
>   sample_regs_user    0
>   sample_stack_user   0
>   sample_regs_intr    0
> ------------------------------------------------------------
> 
> $ perf evlist  -vv
> cycles: sample_freq=4000, size: 104, sample_type: IP|TID|TIME|PERIOD,
> disabled: 1, inherit: 1, mmap: 1, mmap2: 1, comm: 1, comm_exec: 1,
> freq: 1, enable_on_exec: 1, task: 1, sample_id_all: 1, exclude_guest:
> 1
> 
> Post:
> 
> $ ./perf record -vv -e cycles -- sleep 1 
> ------------------------------------------------------------
> perf_event_attr:
>   size                             112
>   { sample_period, sample_freq }   4000
>   sample_type                      IP|TID|TIME|PERIOD
>   disabled                         1
>   inherit                          1
>   mmap                             1
>   comm                             1
>   freq                             1
>   enable_on_exec                   1
>   task                             1
>   sample_id_all                    1
>   exclude_guest                    1
>   mmap2                            1
>   comm_exec                        1
> ------------------------------------------------------------
> 
> $ ./perf evlist  -vv
> cycles: size: 112, { sample_period, sample_freq }: 4000, sample_type:
> IP|TID|TIME|PERIOD, disabled: 1, inherit: 1, mmap: 1, comm: 1, freq:
> 1, enable_on_exec: 1, task: 1, sample_id_all: 1, exclude_guest: 1,
> mmap2: 1, comm_exec: 1
> 
> Cc: acme@redhat.com
> Cc: jolsa@redhat.com
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  tools/perf/util/Build          |    1 
>  tools/perf/util/evsel.c        |  181 ++++++-----------------------------------
>  tools/perf/util/header.c       |   34 ++-----
>  tools/perf/util/print_attr.h   |   69 +++++++++++++++
>  tools/perf/util/print_helper.c |   52 +++++++++++
>  tools/perf/util/print_helper.h |    7 +
>  6 files changed, 170 insertions(+), 174 deletions(-)

Acked-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo

Re: [RFC][PATCH] perf tools: unify perf_event_attr printing

[Adrian Hunter]

On 01/04/15 19:52, Jiri Olsa wrote:
> On Wed, Apr 01, 2015 at 06:26:38PM +0200, Peter Zijlstra wrote:
>>
>> With some feedback from Jolsa, who showed me how to trigger the actual
>> outputs.
> 
> adding Adrian to CC as he's the original author AFAIK

I wanted a compact format, and the omission of null/zero values achieves that.

But personally I think the "include" approach is too ugly. I would just
add a function instead. Something like:

#define p_unsigned(val)	snprintf(value_buf, BUF_SIZE, "%"PRIu64, (uint64_t)(val))

#define PRINT_ATTRN(name, field, print)			\
	if (!skip_zero || attr->field) {		\
		snprintf(name_buf, BUF_SIZE, name);	\
		print(attr->field);			\
		prt(name_buf, value_buf, priv);		\
	}

#define PRINT_ATTR(field, print) PRINT_ATTRN(#field, field, print)

typedef int (*attr_print_cb_t)(const char *name, const char *value, void *priv);

int perf_event_attr__print(struct perf_event_attr *attr, attr_print_cb_t prt, bool skip_zero, void *priv)
{
	char name_buf[BUF_SIZE];
	char value_buf[BUF_SIZE];
	int err;

	PRINT_ATTR(type, p_unsigned);

	etc

	return 0;
}


Then it calling it looks like:


static int perf_event_attr__fprintf_one(const char *name, const char *value, void *priv)
{
	size_t *ret = priv;

	*ret += fprintf(fp, "  %-32s %s\n", name, value);
	return 0;
}

static size_t perf_event_attr__fprintf(struct perf_event_attr *attr, FILE *fp)
{
	size_t ret = 0;

	ret += fprintf(fp, "%.60s\n", graph_dotted_line);
	ret += fprintf(fp, "perf_event_attr:\n");

	perf_event_attr__print(attr, perf_event_attr__fprintf_one, true, &ret);

	ret += fprintf(fp, "%.60s\n", graph_dotted_line);

	return ret;
}


> 
> jirka
> 
>>
>> ---
>>
>> Subject: perf, tools: Merge all perf_event_attr print functions
>> From: Peter Zijlstra <peterz@infradead.org>
>> Date: Tue Mar 31 13:01:54 CEST 2015
>>
>> Currently there's 3 (that I found) different and incomplete
>> implementations of printing perf_event_attr.
>>
>> This is quite silly. Merge the lot.
>>
>> While this patch does not retain the exact form all printing that I
>> found is debug output and thus it should not be critical.
>>
>> Also, I cannot find a single print_event_desc() caller.
>>
>> Pre:
>>
>> $ perf record -vv -e cycles -- sleep 1
>> ------------------------------------------------------------
>> perf_event_attr:
>>   type                0
>>   size                104
>>   config              0
>>   sample_period       4000
>>   sample_freq         4000
>>   sample_type         0x107
>>   read_format         0
>>   disabled            1    inherit             1
>>   pinned              0    exclusive           0
>>   exclude_user        0    exclude_kernel      0
>>   exclude_hv          0    exclude_idle        0
>>   mmap                1    comm                1
>>   mmap2               1    comm_exec           1
>>   freq                1    inherit_stat        0
>>   enable_on_exec      1    task                1
>>   watermark           0    precise_ip          0
>>   mmap_data           0    sample_id_all       1
>>   exclude_host        0    exclude_guest       1
>>   excl.callchain_kern 0    excl.callchain_user 0
>>   wakeup_events       0
>>   wakeup_watermark    0
>>   bp_type             0
>>   bp_addr             0
>>   config1             0
>>   bp_len              0
>>   config2             0
>>   branch_sample_type  0
>>   sample_regs_user    0
>>   sample_stack_user   0
>>   sample_regs_intr    0
>> ------------------------------------------------------------
>>
>> $ perf evlist  -vv
>> cycles: sample_freq=4000, size: 104, sample_type: IP|TID|TIME|PERIOD,
>> disabled: 1, inherit: 1, mmap: 1, mmap2: 1, comm: 1, comm_exec: 1,
>> freq: 1, enable_on_exec: 1, task: 1, sample_id_all: 1, exclude_guest:
>> 1
>>
>> Post:
>>
>> $ ./perf record -vv -e cycles -- sleep 1 
>> ------------------------------------------------------------
>> perf_event_attr:
>>   size                             112
>>   { sample_period, sample_freq }   4000
>>   sample_type                      IP|TID|TIME|PERIOD
>>   disabled                         1
>>   inherit                          1
>>   mmap                             1
>>   comm                             1
>>   freq                             1
>>   enable_on_exec                   1
>>   task                             1
>>   sample_id_all                    1
>>   exclude_guest                    1
>>   mmap2                            1
>>   comm_exec                        1
>> ------------------------------------------------------------
>>
>> $ ./perf evlist  -vv
>> cycles: size: 112, { sample_period, sample_freq }: 4000, sample_type:
>> IP|TID|TIME|PERIOD, disabled: 1, inherit: 1, mmap: 1, comm: 1, freq:
>> 1, enable_on_exec: 1, task: 1, sample_id_all: 1, exclude_guest: 1,
>> mmap2: 1, comm_exec: 1
>>
>> Cc: acme@redhat.com
>> Cc: jolsa@redhat.com
>> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
>> ---
>>  tools/perf/util/Build          |    1 
>>  tools/perf/util/evsel.c        |  181 ++++++-----------------------------------
>>  tools/perf/util/header.c       |   34 ++-----
>>  tools/perf/util/print_attr.h   |   69 +++++++++++++++
>>  tools/perf/util/print_helper.c |   52 +++++++++++
>>  tools/perf/util/print_helper.h |    7 +
>>  6 files changed, 170 insertions(+), 174 deletions(-)
>>
>> --- a/tools/perf/util/Build
>> +++ b/tools/perf/util/Build
>> @@ -74,6 +74,7 @@ libperf-y += data.o
>>  libperf-$(CONFIG_X86) += tsc.o
>>  libperf-y += cloexec.o
>>  libperf-y += thread-stack.o
>> +libperf-y += print_helper.o
>>  
>>  libperf-$(CONFIG_LIBELF) += symbol-elf.o
>>  libperf-$(CONFIG_LIBELF) += probe-event.o
>> --- a/tools/perf/util/evsel.c
>> +++ b/tools/perf/util/evsel.c
>> @@ -26,6 +26,7 @@
>>  #include "perf_regs.h"
>>  #include "debug.h"
>>  #include "trace-event.h"
>> +#include "print_helper.h"
>>  
>>  static struct {
>>  	bool sample_id_all;
>> @@ -1011,21 +1012,6 @@ static int get_group_fd(struct perf_evse
>>  	return fd;
>>  }
>>  
>> -#define __PRINT_ATTR(fmt, cast, field)  \
>> -	fprintf(fp, "  %-19s "fmt"\n", #field, cast attr->field)
>> -
>> -#define PRINT_ATTR_U32(field)  __PRINT_ATTR("%u" , , field)
>> -#define PRINT_ATTR_X32(field)  __PRINT_ATTR("%#x", , field)
>> -#define PRINT_ATTR_U64(field)  __PRINT_ATTR("%" PRIu64, (uint64_t), field)
>> -#define PRINT_ATTR_X64(field)  __PRINT_ATTR("%#"PRIx64, (uint64_t), field)
>> -
>> -#define PRINT_ATTR2N(name1, field1, name2, field2)	\
>> -	fprintf(fp, "  %-19s %u    %-19s %u\n",		\
>> -	name1, attr->field1, name2, attr->field2)
>> -
>> -#define PRINT_ATTR2(field1, field2) \
>> -	PRINT_ATTR2N(#field1, field1, #field2, field2)
>> -
>>  static size_t perf_event_attr__fprintf(struct perf_event_attr *attr, FILE *fp)
>>  {
>>  	size_t ret = 0;
>> @@ -1033,42 +1019,18 @@ static size_t perf_event_attr__fprintf(s
>>  	ret += fprintf(fp, "%.60s\n", graph_dotted_line);
>>  	ret += fprintf(fp, "perf_event_attr:\n");
>>  
>> -	ret += PRINT_ATTR_U32(type);
>> -	ret += PRINT_ATTR_U32(size);
>> -	ret += PRINT_ATTR_X64(config);
>> -	ret += PRINT_ATTR_U64(sample_period);
>> -	ret += PRINT_ATTR_U64(sample_freq);
>> -	ret += PRINT_ATTR_X64(sample_type);
>> -	ret += PRINT_ATTR_X64(read_format);
>> -
>> -	ret += PRINT_ATTR2(disabled, inherit);
>> -	ret += PRINT_ATTR2(pinned, exclusive);
>> -	ret += PRINT_ATTR2(exclude_user, exclude_kernel);
>> -	ret += PRINT_ATTR2(exclude_hv, exclude_idle);
>> -	ret += PRINT_ATTR2(mmap, comm);
>> -	ret += PRINT_ATTR2(freq, inherit_stat);
>> -	ret += PRINT_ATTR2(enable_on_exec, task);
>> -	ret += PRINT_ATTR2(watermark, precise_ip);
>> -	ret += PRINT_ATTR2(mmap_data, sample_id_all);
>> -	ret += PRINT_ATTR2(exclude_host, exclude_guest);
>> -	ret += PRINT_ATTR2N("excl.callchain_kern", exclude_callchain_kernel,
>> -			    "excl.callchain_user", exclude_callchain_user);
>> -	ret += PRINT_ATTR2(mmap2, comm_exec);
>> -	ret += __PRINT_ATTR("%u",,use_clockid);
>> -
>> -
>> -	ret += PRINT_ATTR_U32(wakeup_events);
>> -	ret += PRINT_ATTR_U32(wakeup_watermark);
>> -	ret += PRINT_ATTR_X32(bp_type);
>> -	ret += PRINT_ATTR_X64(bp_addr);
>> -	ret += PRINT_ATTR_X64(config1);
>> -	ret += PRINT_ATTR_U64(bp_len);
>> -	ret += PRINT_ATTR_X64(config2);
>> -	ret += PRINT_ATTR_X64(branch_sample_type);
>> -	ret += PRINT_ATTR_X64(sample_regs_user);
>> -	ret += PRINT_ATTR_U32(sample_stack_user);
>> -	ret += PRINT_ATTR_U32(clockid);
>> -	ret += PRINT_ATTR_X64(sample_regs_intr);
>> +#define PRINT_ATTR(_n, _f, _p)			\
>> +do {						\
>> +	if (attr->_f) {				\
>> +	ret += fprintf(fp, "  %-32s ", _n);	\
>> +	ret += _p(fp, attr->_f);		\
>> +	ret += fprintf(fp, "\n");		\
>> +	}					\
>> +} while (0)
>> +
>> +#include "util/print_attr.h"
>> +
>> +#undef PRINT_ATTR
>>  
>>  	ret += fprintf(fp, "%.60s\n", graph_dotted_line);
>>  
>> @@ -1996,64 +1958,6 @@ static int comma_fprintf(FILE *fp, bool
>>  	return ret;
>>  }
>>  
>> -static int __if_fprintf(FILE *fp, bool *first, const char *field, u64 value)
>> -{
>> -	if (value == 0)
>> -		return 0;
>> -
>> -	return comma_fprintf(fp, first, " %s: %" PRIu64, field, value);
>> -}
>> -
>> -#define if_print(field) printed += __if_fprintf(fp, &first, #field, evsel->attr.field)
>> -
>> -struct bit_names {
>> -	int bit;
>> -	const char *name;
>> -};
>> -
>> -static int bits__fprintf(FILE *fp, const char *field, u64 value,
>> -			 struct bit_names *bits, bool *first)
>> -{
>> -	int i = 0, printed = comma_fprintf(fp, first, " %s: ", field);
>> -	bool first_bit = true;
>> -
>> -	do {
>> -		if (value & bits[i].bit) {
>> -			printed += fprintf(fp, "%s%s", first_bit ? "" : "|", bits[i].name);
>> -			first_bit = false;
>> -		}
>> -	} while (bits[++i].name != NULL);
>> -
>> -	return printed;
>> -}
>> -
>> -static int sample_type__fprintf(FILE *fp, bool *first, u64 value)
>> -{
>> -#define bit_name(n) { PERF_SAMPLE_##n, #n }
>> -	struct bit_names bits[] = {
>> -		bit_name(IP), bit_name(TID), bit_name(TIME), bit_name(ADDR),
>> -		bit_name(READ), bit_name(CALLCHAIN), bit_name(ID), bit_name(CPU),
>> -		bit_name(PERIOD), bit_name(STREAM_ID), bit_name(RAW),
>> -		bit_name(BRANCH_STACK), bit_name(REGS_USER), bit_name(STACK_USER),
>> -		bit_name(IDENTIFIER), bit_name(REGS_INTR),
>> -		{ .name = NULL, }
>> -	};
>> -#undef bit_name
>> -	return bits__fprintf(fp, "sample_type", value, bits, first);
>> -}
>> -
>> -static int read_format__fprintf(FILE *fp, bool *first, u64 value)
>> -{
>> -#define bit_name(n) { PERF_FORMAT_##n, #n }
>> -	struct bit_names bits[] = {
>> -		bit_name(TOTAL_TIME_ENABLED), bit_name(TOTAL_TIME_RUNNING),
>> -		bit_name(ID), bit_name(GROUP),
>> -		{ .name = NULL, }
>> -	};
>> -#undef bit_name
>> -	return bits__fprintf(fp, "read_format", value, bits, first);
>> -}
>> -
>>  int perf_evsel__fprintf(struct perf_evsel *evsel,
>>  			struct perf_attr_details *details, FILE *fp)
>>  {
>> @@ -2080,51 +1984,24 @@ int perf_evsel__fprintf(struct perf_evse
>>  
>>  	printed += fprintf(fp, "%s", perf_evsel__name(evsel));
>>  
>> -	if (details->verbose || details->freq) {
>> -		printed += comma_fprintf(fp, &first, " sample_freq=%" PRIu64,
>> -					 (u64)evsel->attr.sample_freq);
>> -	}
>>  
>>  	if (details->verbose) {
>> -		if_print(type);
>> -		if_print(config);
>> -		if_print(config1);
>> -		if_print(config2);
>> -		if_print(size);
>> -		printed += sample_type__fprintf(fp, &first, evsel->attr.sample_type);
>> -		if (evsel->attr.read_format)
>> -			printed += read_format__fprintf(fp, &first, evsel->attr.read_format);
>> -		if_print(disabled);
>> -		if_print(inherit);
>> -		if_print(pinned);
>> -		if_print(exclusive);
>> -		if_print(exclude_user);
>> -		if_print(exclude_kernel);
>> -		if_print(exclude_hv);
>> -		if_print(exclude_idle);
>> -		if_print(mmap);
>> -		if_print(comm);
>> -		if_print(freq);
>> -		if_print(inherit_stat);
>> -		if_print(enable_on_exec);
>> -		if_print(task);
>> -		if_print(watermark);
>> -		if_print(precise_ip);
>> -		if_print(mmap_data);
>> -		if_print(sample_id_all);
>> -		if_print(exclude_host);
>> -		if_print(exclude_guest);
>> -		if_print(mmap2);
>> -		if_print(comm_exec);
>> -		if_print(use_clockid);
>> -		if_print(__reserved_1);
>> -		if_print(wakeup_events);
>> -		if_print(bp_type);
>> -		if_print(branch_sample_type);
>> -		if_print(sample_regs_user);
>> -		if_print(sample_stack_user);
>> -		if_print(clockid);
>> -		if_print(sample_regs_intr);
>> +
>> +#define PRINT_ATTR(_n, _f, _p)						\
>> +do {									\
>> +	if (evsel->attr._f) {						\
>> +		printed += comma_fprintf(fp, &first, " %s: ", _n);	\
>> +		printed += _p(fp, evsel->attr._f);			\
>> +	}								\
>> +} while (0)
>> +
>> +#include "util/print_attr.h"
>> +
>> +#undef PRINT_ATTR
>> +
>> +	} else if (details->freq) {
>> +		printed += comma_fprintf(fp, &first, " sample_freq=%" PRIu64,
>> +					 (u64)evsel->attr.sample_freq);
>>  	}
>>  out:
>>  	fputc('\n', fp);
>> --- a/tools/perf/util/header.c
>> +++ b/tools/perf/util/header.c
>> @@ -23,6 +23,7 @@
>>  #include "strbuf.h"
>>  #include "build-id.h"
>>  #include "data.h"
>> +#include "print_helper.h"
>>  
>>  static u32 header_argc;
>>  static const char **header_argv;
>> @@ -1069,26 +1070,6 @@ static void print_event_desc(struct perf
>>  	for (evsel = events; evsel->attr.size; evsel++) {
>>  		fprintf(fp, "# event : name = %s, ", evsel->name);
>>  
>> -		fprintf(fp, "type = %d, config = 0x%"PRIx64
>> -			    ", config1 = 0x%"PRIx64", config2 = 0x%"PRIx64,
>> -				evsel->attr.type,
>> -				(u64)evsel->attr.config,
>> -				(u64)evsel->attr.config1,
>> -				(u64)evsel->attr.config2);
>> -
>> -		fprintf(fp, ", excl_usr = %d, excl_kern = %d",
>> -				evsel->attr.exclude_user,
>> -				evsel->attr.exclude_kernel);
>> -
>> -		fprintf(fp, ", excl_host = %d, excl_guest = %d",
>> -				evsel->attr.exclude_host,
>> -				evsel->attr.exclude_guest);
>> -
>> -		fprintf(fp, ", precise_ip = %d", evsel->attr.precise_ip);
>> -
>> -		fprintf(fp, ", attr_mmap2 = %d", evsel->attr.mmap2);
>> -		fprintf(fp, ", attr_mmap  = %d", evsel->attr.mmap);
>> -		fprintf(fp, ", attr_mmap_data = %d", evsel->attr.mmap_data);
>>  		if (evsel->ids) {
>>  			fprintf(fp, ", id = {");
>>  			for (j = 0, id = evsel->id; j < evsel->ids; j++, id++) {
>> @@ -1098,9 +1079,18 @@ static void print_event_desc(struct perf
>>  			}
>>  			fprintf(fp, " }");
>>  		}
>> -		if (evsel->attr.use_clockid)
>> -			fprintf(fp, ", clockid = %d", evsel->attr.clockid);
>>  
>> +#define PRINT_ATTR(_n, _f, _p)			\
>> +do {						\
>> +	if (evsel->attr._f) {			\
>> +		fprintf(fp, ", %s =", _n);	\
>> +		_p(fp, evsel->attr._f);		\
>> +	}					\
>> +} while (0)
>> +
>> +#include "util/print_attr.h"
>> +
>> +#undef PRINT_ATTR
>>  
>>  		fputc('\n', fp);
>>  	}
>> --- /dev/null
>> +++ b/tools/perf/util/print_attr.h
>> @@ -0,0 +1,69 @@
>> +
>> +#ifndef PRINT_ATTR
>> +/*
>> + * #define PRINT_ATTR(name, field, print) 		\
>> + * do {							\
>> + * 	fprintf(fp, " %s = ", name);			\
>> + * 	print(fp, attr->field);				\
>> + * } while (0)
>> + */
>> +#error "General Error and Major Fault yell at you!"
>> +#endif
>> +
>> +#define p_hex(fp, val)		fprintf(fp, "%"PRIx64, (uint64_t)(val))
>> +#define p_unsigned(fp, val)	fprintf(fp, "%"PRIu64, (uint64_t)(val))
>> +#define p_signed(fp, val)	fprintf(fp, "%"PRId64, (int64_t)(val))
>> +#define p_sample_type(fp, val)	sample_type__fprintf(fp, val)
>> +#define p_read_format(fp, val)	read_format__fprintf(fp, val)
>> +
>> +#define PRINT_ATTRf(field, print) PRINT_ATTR(#field, field, print)
>> +
>> +PRINT_ATTRf(type, p_unsigned);
>> +PRINT_ATTRf(size, p_unsigned);
>> +PRINT_ATTRf(config, p_hex);
>> +PRINT_ATTR("{ sample_period, sample_freq }", sample_period, p_unsigned);
>> +PRINT_ATTRf(sample_type, p_sample_type);
>> +PRINT_ATTRf(read_format, p_read_format);
>> +
>> +PRINT_ATTRf(disabled, p_unsigned);
>> +PRINT_ATTRf(inherit, p_unsigned);
>> +PRINT_ATTRf(pinned, p_unsigned);
>> +PRINT_ATTRf(exclusive, p_unsigned);
>> +PRINT_ATTRf(exclude_user, p_unsigned);
>> +PRINT_ATTRf(exclude_kernel, p_unsigned);
>> +PRINT_ATTRf(exclude_hv, p_unsigned);
>> +PRINT_ATTRf(exclude_idle, p_unsigned);
>> +PRINT_ATTRf(mmap, p_unsigned);
>> +PRINT_ATTRf(comm, p_unsigned);
>> +PRINT_ATTRf(freq, p_unsigned);
>> +PRINT_ATTRf(inherit_stat, p_unsigned);
>> +PRINT_ATTRf(enable_on_exec, p_unsigned);
>> +PRINT_ATTRf(task, p_unsigned);
>> +PRINT_ATTRf(watermark, p_unsigned);
>> +PRINT_ATTRf(precise_ip, p_unsigned);
>> +PRINT_ATTRf(mmap_data, p_unsigned);
>> +PRINT_ATTRf(sample_id_all, p_unsigned);
>> +PRINT_ATTRf(exclude_host, p_unsigned);
>> +PRINT_ATTRf(exclude_guest, p_unsigned);
>> +PRINT_ATTRf(exclude_callchain_kernel, p_unsigned);
>> +PRINT_ATTRf(exclude_callchain_user, p_unsigned);
>> +PRINT_ATTRf(mmap2, p_unsigned);
>> +PRINT_ATTRf(comm_exec, p_unsigned);
>> +PRINT_ATTRf(use_clockid, p_unsigned);
>> +
>> +PRINT_ATTR("{ wakeup_events, wakeup_watermark }", wakeup_events, p_unsigned);
>> +PRINT_ATTRf(bp_type, p_unsigned);
>> +PRINT_ATTR("{ bp_addr, config1 }", bp_addr, p_hex);
>> +PRINT_ATTR("{ bp_len, config2 }", bp_len, p_hex);
>> +PRINT_ATTRf(sample_regs_user, p_hex);
>> +PRINT_ATTRf(sample_stack_user, p_unsigned);
>> +PRINT_ATTRf(clockid, p_signed);
>> +PRINT_ATTRf(sample_regs_intr, p_hex);
>> +
>> +#undef PRINT_ATTRf
>> +
>> +#undef p_hex
>> +#undef p_unsigned
>> +#undef p_signed
>> +#undef p_sample_type
>> +#undef p_read_format
>> --- /dev/null
>> +++ b/tools/perf/util/print_helper.c
>> @@ -0,0 +1,52 @@
>> +
>> +#include <linux/perf_event.h>
>> +#include "util.h"
>> +#include "print_helper.h"
>> +
>> +struct bit_names {
>> +	int bit;
>> +	const char *name;
>> +};
>> +
>> +static int bits__fprintf(FILE *fp, u64 value, struct bit_names *bits)
>> +{
>> +	int i = 0, printed = 0;
>> +	bool first_bit = true;
>> +
>> +	do {
>> +		if (value & bits[i].bit) {
>> +			printed += fprintf(fp, "%s%s", first_bit ? "" : "|", bits[i].name);
>> +			first_bit = false;
>> +		}
>> +	} while (bits[++i].name != NULL);
>> +
>> +	return printed;
>> +}
>> +
>> +int sample_type__fprintf(FILE *fp, u64 value)
>> +{
>> +#define bit_name(n) { PERF_SAMPLE_##n, #n }
>> +	struct bit_names bits[] = {
>> +		bit_name(IP), bit_name(TID), bit_name(TIME), bit_name(ADDR),
>> +		bit_name(READ), bit_name(CALLCHAIN), bit_name(ID), bit_name(CPU),
>> +		bit_name(PERIOD), bit_name(STREAM_ID), bit_name(RAW),
>> +		bit_name(BRANCH_STACK), bit_name(REGS_USER), bit_name(STACK_USER),
>> +		bit_name(IDENTIFIER), bit_name(REGS_INTR),
>> +		{ .name = NULL, }
>> +	};
>> +#undef bit_name
>> +	return bits__fprintf(fp, value, bits);
>> +}
>> +
>> +int read_format__fprintf(FILE *fp, u64 value)
>> +{
>> +#define bit_name(n) { PERF_FORMAT_##n, #n }
>> +	struct bit_names bits[] = {
>> +		bit_name(TOTAL_TIME_ENABLED), bit_name(TOTAL_TIME_RUNNING),
>> +		bit_name(ID), bit_name(GROUP),
>> +		{ .name = NULL, }
>> +	};
>> +#undef bit_name
>> +	return bits__fprintf(fp, value, bits);
>> +}
>> +
>> --- /dev/null
>> +++ b/tools/perf/util/print_helper.h
>> @@ -0,0 +1,7 @@
>> +#ifndef PRINT_HELPER_H
>> +#define PRINT_HELPER_H
>> +
>> +extern int sample_type__fprintf(FILE *fp, u64 value);
>> +extern int read_format__fprintf(FILE *fp, u64 value);
>> +
>> +#endif
> 
> 

Re: [RFC][PATCH] perf tools: unify perf_event_attr printing

[Jiri Olsa]

On Wed, Apr 01, 2015 at 06:26:38PM +0200, Peter Zijlstra wrote:

SNIP

> -	ret += PRINT_ATTR_U32(wakeup_events);
> -	ret += PRINT_ATTR_U32(wakeup_watermark);
> -	ret += PRINT_ATTR_X32(bp_type);
> -	ret += PRINT_ATTR_X64(bp_addr);
> -	ret += PRINT_ATTR_X64(config1);
> -	ret += PRINT_ATTR_U64(bp_len);
> -	ret += PRINT_ATTR_X64(config2);
> -	ret += PRINT_ATTR_X64(branch_sample_type);
> -	ret += PRINT_ATTR_X64(sample_regs_user);
> -	ret += PRINT_ATTR_U32(sample_stack_user);
> -	ret += PRINT_ATTR_U32(clockid);
> -	ret += PRINT_ATTR_X64(sample_regs_intr);
> +#define PRINT_ATTR(_n, _f, _p)			\
> +do {						\
> +	if (attr->_f) {				\
> +	ret += fprintf(fp, "  %-32s ", _n);	\
> +	ret += _p(fp, attr->_f);		\
> +	ret += fprintf(fp, "\n");		\

missing indent ;-) other it seems ok

Acked-by: Jiri Olsa <jolsa@kernel.org>

jirka

Re: [RFC][PATCH] perf tools: unify perf_event_attr printing

[Peter Zijlstra]

On Thu, Apr 02, 2015 at 12:01:53PM +0300, Adrian Hunter wrote:
> But personally I think the "include" approach is too ugly. I would just
> add a function instead. Something like:

You've not stared at the kernel tracepoint code long enough ;-)

Something like so then?

---
Subject: perf, tools: Merge all perf_event_attr print functions
From: Peter Zijlstra <peterz@infradead.org>
Date: Thu Apr 2 11:49:23 CEST 2015

Currently there's 3 (that I found) different and incomplete
implementations of printing perf_event_attr.

This is quite silly. Merge the lot.

While this patch does not retain the exact form all printing that I
found is debug output and thus it should not be critical.

Also, I cannot find a single print_event_desc() caller.

Pre:

$ perf record -vv -e cycles -- sleep 1
------------------------------------------------------------
perf_event_attr:
  type                0
  size                104
  config              0
  sample_period       4000
  sample_freq         4000
  sample_type         0x107
  read_format         0
  disabled            1    inherit             1
  pinned              0    exclusive           0
  exclude_user        0    exclude_kernel      0
  exclude_hv          0    exclude_idle        0
  mmap                1    comm                1
  mmap2               1    comm_exec           1
  freq                1    inherit_stat        0
  enable_on_exec      1    task                1
  watermark           0    precise_ip          0
  mmap_data           0    sample_id_all       1
  exclude_host        0    exclude_guest       1
  excl.callchain_kern 0    excl.callchain_user 0
  wakeup_events       0
  wakeup_watermark    0
  bp_type             0
  bp_addr             0
  config1             0
  bp_len              0
  config2             0
  branch_sample_type  0
  sample_regs_user    0
  sample_stack_user   0
  sample_regs_intr    0
------------------------------------------------------------

$ perf evlist  -vv
cycles: sample_freq=4000, size: 104, sample_type: IP|TID|TIME|PERIOD,
disabled: 1, inherit: 1, mmap: 1, mmap2: 1, comm: 1, comm_exec: 1,
freq: 1, enable_on_exec: 1, task: 1, sample_id_all: 1, exclude_guest:
1

Post:

$ ./perf record -vv -e cycles -- sleep 1 
------------------------------------------------------------
perf_event_attr:
  size                             112
  { sample_period, sample_freq }   4000
  sample_type                      IP|TID|TIME|PERIOD
  disabled                         1
  inherit                          1
  mmap                             1
  comm                             1
  freq                             1
  enable_on_exec                   1
  task                             1
  sample_id_all                    1
  exclude_guest                    1
  mmap2                            1
  comm_exec                        1
------------------------------------------------------------

$ ./perf evlist  -vv
cycles: size: 112, { sample_period, sample_freq }: 4000, sample_type:
IP|TID|TIME|PERIOD, disabled: 1, inherit: 1, mmap: 1, comm: 1, freq:
1, enable_on_exec: 1, task: 1, sample_id_all: 1, exclude_guest: 1,
mmap2: 1, comm_exec: 1

Cc: acme@redhat.com
Cc: jolsa@redhat.com
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 tools/perf/util/evsel.c  |  284 ++++++++++++++++++++---------------------------
 tools/perf/util/evsel.h  |    6 
 tools/perf/util/header.c |   29 +---
 3 files changed, 139 insertions(+), 180 deletions(-)

--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -1011,70 +1011,126 @@ static int get_group_fd(struct perf_evse
 	return fd;
 }
 
-#define __PRINT_ATTR(fmt, cast, field)  \
-	fprintf(fp, "  %-19s "fmt"\n", #field, cast attr->field)
+struct bit_names {
+	int bit;
+	const char *name;
+};
+
+static void __p_bits(char *buf, size_t size, u64 value, struct bit_names *bits)
+{
+	bool first_bit = true;
+	int i = 0;
+
+	do {
+		if (value & bits[i].bit) {
+			buf += scnprintf(buf, size, "%s%s", first_bit ? "" : "|", bits[i].name);
+			first_bit = false;
+		}
+	} while (bits[++i].name != NULL);
+}
+
+static void __p_sample_type(char *buf, size_t size, u64 value)
+{
+#define bit_name(n) { PERF_SAMPLE_##n, #n }
+	struct bit_names bits[] = {
+		bit_name(IP), bit_name(TID), bit_name(TIME), bit_name(ADDR),
+		bit_name(READ), bit_name(CALLCHAIN), bit_name(ID), bit_name(CPU),
+		bit_name(PERIOD), bit_name(STREAM_ID), bit_name(RAW),
+		bit_name(BRANCH_STACK), bit_name(REGS_USER), bit_name(STACK_USER),
+		bit_name(IDENTIFIER), bit_name(REGS_INTR),
+		{ .name = NULL, }
+	};
+#undef bit_name
+	__p_bits(buf, size, value, bits);
+}
 
-#define PRINT_ATTR_U32(field)  __PRINT_ATTR("%u" , , field)
-#define PRINT_ATTR_X32(field)  __PRINT_ATTR("%#x", , field)
-#define PRINT_ATTR_U64(field)  __PRINT_ATTR("%" PRIu64, (uint64_t), field)
-#define PRINT_ATTR_X64(field)  __PRINT_ATTR("%#"PRIx64, (uint64_t), field)
-
-#define PRINT_ATTR2N(name1, field1, name2, field2)	\
-	fprintf(fp, "  %-19s %u    %-19s %u\n",		\
-	name1, attr->field1, name2, attr->field2)
-
-#define PRINT_ATTR2(field1, field2) \
-	PRINT_ATTR2N(#field1, field1, #field2, field2)
-
-static size_t perf_event_attr__fprintf(struct perf_event_attr *attr, FILE *fp)
-{
-	size_t ret = 0;
-
-	ret += fprintf(fp, "%.60s\n", graph_dotted_line);
-	ret += fprintf(fp, "perf_event_attr:\n");
-
-	ret += PRINT_ATTR_U32(type);
-	ret += PRINT_ATTR_U32(size);
-	ret += PRINT_ATTR_X64(config);
-	ret += PRINT_ATTR_U64(sample_period);
-	ret += PRINT_ATTR_U64(sample_freq);
-	ret += PRINT_ATTR_X64(sample_type);
-	ret += PRINT_ATTR_X64(read_format);
-
-	ret += PRINT_ATTR2(disabled, inherit);
-	ret += PRINT_ATTR2(pinned, exclusive);
-	ret += PRINT_ATTR2(exclude_user, exclude_kernel);
-	ret += PRINT_ATTR2(exclude_hv, exclude_idle);
-	ret += PRINT_ATTR2(mmap, comm);
-	ret += PRINT_ATTR2(freq, inherit_stat);
-	ret += PRINT_ATTR2(enable_on_exec, task);
-	ret += PRINT_ATTR2(watermark, precise_ip);
-	ret += PRINT_ATTR2(mmap_data, sample_id_all);
-	ret += PRINT_ATTR2(exclude_host, exclude_guest);
-	ret += PRINT_ATTR2N("excl.callchain_kern", exclude_callchain_kernel,
-			    "excl.callchain_user", exclude_callchain_user);
-	ret += PRINT_ATTR2(mmap2, comm_exec);
-	ret += __PRINT_ATTR("%u",,use_clockid);
-
-
-	ret += PRINT_ATTR_U32(wakeup_events);
-	ret += PRINT_ATTR_U32(wakeup_watermark);
-	ret += PRINT_ATTR_X32(bp_type);
-	ret += PRINT_ATTR_X64(bp_addr);
-	ret += PRINT_ATTR_X64(config1);
-	ret += PRINT_ATTR_U64(bp_len);
-	ret += PRINT_ATTR_X64(config2);
-	ret += PRINT_ATTR_X64(branch_sample_type);
-	ret += PRINT_ATTR_X64(sample_regs_user);
-	ret += PRINT_ATTR_U32(sample_stack_user);
-	ret += PRINT_ATTR_U32(clockid);
-	ret += PRINT_ATTR_X64(sample_regs_intr);
+static void __p_read_format(char *buf, size_t size, u64 value)
+{
+#define bit_name(n) { PERF_FORMAT_##n, #n }
+	struct bit_names bits[] = {
+		bit_name(TOTAL_TIME_ENABLED), bit_name(TOTAL_TIME_RUNNING),
+		bit_name(ID), bit_name(GROUP),
+		{ .name = NULL, }
+	};
+#undef bit_name
+	__p_bits(buf, size, value, bits);
+}
+
+#define BUF_SIZE		1024
+
+#define p_hex(val)		snprintf(buf, BUF_SIZE, "%"PRIx64, (uint64_t)(val))
+#define p_unsigned(val)		snprintf(buf, BUF_SIZE, "%"PRIu64, (uint64_t)(val))
+#define p_signed(val)		snprintf(buf, BUF_SIZE, "%"PRId64, (int64_t)(val))
+#define p_sample_type(val)	__p_sample_type(buf, BUF_SIZE, val)
+#define p_read_format(val)	__p_read_format(buf, BUF_SIZE, val)
+
+#define PRINT_ATTRn(_n, _f, _p)				\
+do {							\
+	if (attr->_f) {					\
+		_p(attr->_f);				\
+		ret += attr__fprintf(fp, _n, buf, priv);\
+	}						\
+} while (0)
 
-	ret += fprintf(fp, "%.60s\n", graph_dotted_line);
+#define PRINT_ATTRf(_f, _p)	PRINT_ATTRn(#_f, _f, _p)
+
+int perf_event_attr__fprintf(FILE *fp, struct perf_event_attr *attr,
+			     attr__fprintf_f attr__fprintf, void *priv)
+{
+	char buf[BUF_SIZE];
+	int ret = 0;
+
+	PRINT_ATTRf(type, p_unsigned);
+	PRINT_ATTRf(size, p_unsigned);
+	PRINT_ATTRf(config, p_hex);
+	PRINT_ATTRn("{ sample_period, sample_freq }", sample_period, p_unsigned);
+	PRINT_ATTRf(sample_type, p_sample_type);
+	PRINT_ATTRf(read_format, p_read_format);
+
+	PRINT_ATTRf(disabled, p_unsigned);
+	PRINT_ATTRf(inherit, p_unsigned);
+	PRINT_ATTRf(pinned, p_unsigned);
+	PRINT_ATTRf(exclusive, p_unsigned);
+	PRINT_ATTRf(exclude_user, p_unsigned);
+	PRINT_ATTRf(exclude_kernel, p_unsigned);
+	PRINT_ATTRf(exclude_hv, p_unsigned);
+	PRINT_ATTRf(exclude_idle, p_unsigned);
+	PRINT_ATTRf(mmap, p_unsigned);
+	PRINT_ATTRf(comm, p_unsigned);
+	PRINT_ATTRf(freq, p_unsigned);
+	PRINT_ATTRf(inherit_stat, p_unsigned);
+	PRINT_ATTRf(enable_on_exec, p_unsigned);
+	PRINT_ATTRf(task, p_unsigned);
+	PRINT_ATTRf(watermark, p_unsigned);
+	PRINT_ATTRf(precise_ip, p_unsigned);
+	PRINT_ATTRf(mmap_data, p_unsigned);
+	PRINT_ATTRf(sample_id_all, p_unsigned);
+	PRINT_ATTRf(exclude_host, p_unsigned);
+	PRINT_ATTRf(exclude_guest, p_unsigned);
+	PRINT_ATTRf(exclude_callchain_kernel, p_unsigned);
+	PRINT_ATTRf(exclude_callchain_user, p_unsigned);
+	PRINT_ATTRf(mmap2, p_unsigned);
+	PRINT_ATTRf(comm_exec, p_unsigned);
+	PRINT_ATTRf(use_clockid, p_unsigned);
+
+	PRINT_ATTRn("{ wakeup_events, wakeup_watermark }", wakeup_events, p_unsigned);
+	PRINT_ATTRf(bp_type, p_unsigned);
+	PRINT_ATTRn("{ bp_addr, config1 }", bp_addr, p_hex);
+	PRINT_ATTRn("{ bp_len, config2 }", bp_len, p_hex);
+	PRINT_ATTRf(sample_regs_user, p_hex);
+	PRINT_ATTRf(sample_stack_user, p_unsigned);
+	PRINT_ATTRf(clockid, p_signed);
+	PRINT_ATTRf(sample_regs_intr, p_hex);
 
 	return ret;
 }
 
+static int __open_attr__fprintf(FILE *fp, const char *name, const char *val,
+				void *priv __attribute__((unused)))
+{
+	return fprintf(fp, "  %-32s %s\n", name, val);
+}
+
 static int __perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
 			      struct thread_map *threads)
 {
@@ -1114,8 +1170,12 @@ static int __perf_evsel__open(struct per
 	if (perf_missing_features.sample_id_all)
 		evsel->attr.sample_id_all = 0;
 
-	if (verbose >= 2)
-		perf_event_attr__fprintf(&evsel->attr, stderr);
+	if (verbose >= 2) {
+		fprintf(stderr, "%.60s\n", graph_dotted_line);
+		fprintf(stderr, "perf_event_attr:\n");
+		perf_event_attr__fprintf(stderr, &evsel->attr, __open_attr__fprintf, NULL);
+		fprintf(stderr, "%.60s\n", graph_dotted_line);
+	}
 
 	for (cpu = 0; cpu < cpus->nr; cpu++) {
 
@@ -1996,62 +2056,9 @@ static int comma_fprintf(FILE *fp, bool
 	return ret;
 }
 
-static int __if_fprintf(FILE *fp, bool *first, const char *field, u64 value)
+static int __print_attr__fprintf(FILE *fp, const char *name, const char *val, void *priv)
 {
-	if (value == 0)
-		return 0;
-
-	return comma_fprintf(fp, first, " %s: %" PRIu64, field, value);
-}
-
-#define if_print(field) printed += __if_fprintf(fp, &first, #field, evsel->attr.field)
-
-struct bit_names {
-	int bit;
-	const char *name;
-};
-
-static int bits__fprintf(FILE *fp, const char *field, u64 value,
-			 struct bit_names *bits, bool *first)
-{
-	int i = 0, printed = comma_fprintf(fp, first, " %s: ", field);
-	bool first_bit = true;
-
-	do {
-		if (value & bits[i].bit) {
-			printed += fprintf(fp, "%s%s", first_bit ? "" : "|", bits[i].name);
-			first_bit = false;
-		}
-	} while (bits[++i].name != NULL);
-
-	return printed;
-}
-
-static int sample_type__fprintf(FILE *fp, bool *first, u64 value)
-{
-#define bit_name(n) { PERF_SAMPLE_##n, #n }
-	struct bit_names bits[] = {
-		bit_name(IP), bit_name(TID), bit_name(TIME), bit_name(ADDR),
-		bit_name(READ), bit_name(CALLCHAIN), bit_name(ID), bit_name(CPU),
-		bit_name(PERIOD), bit_name(STREAM_ID), bit_name(RAW),
-		bit_name(BRANCH_STACK), bit_name(REGS_USER), bit_name(STACK_USER),
-		bit_name(IDENTIFIER), bit_name(REGS_INTR),
-		{ .name = NULL, }
-	};
-#undef bit_name
-	return bits__fprintf(fp, "sample_type", value, bits, first);
-}
-
-static int read_format__fprintf(FILE *fp, bool *first, u64 value)
-{
-#define bit_name(n) { PERF_FORMAT_##n, #n }
-	struct bit_names bits[] = {
-		bit_name(TOTAL_TIME_ENABLED), bit_name(TOTAL_TIME_RUNNING),
-		bit_name(ID), bit_name(GROUP),
-		{ .name = NULL, }
-	};
-#undef bit_name
-	return bits__fprintf(fp, "read_format", value, bits, first);
+	return comma_fprintf(fp, (bool *)priv, " %s: %s", name, val);
 }
 
 int perf_evsel__fprintf(struct perf_evsel *evsel,
@@ -2080,52 +2087,13 @@ int perf_evsel__fprintf(struct perf_evse
 
 	printed += fprintf(fp, "%s", perf_evsel__name(evsel));
 
-	if (details->verbose || details->freq) {
+	if (details->verbose) {
+		printed += perf_event_attr__fprintf(fp, &evsel->attr,
+						    __print_attr__fprintf, &first);
+	} else if (details->freq) {
 		printed += comma_fprintf(fp, &first, " sample_freq=%" PRIu64,
 					 (u64)evsel->attr.sample_freq);
 	}
-
-	if (details->verbose) {
-		if_print(type);
-		if_print(config);
-		if_print(config1);
-		if_print(config2);
-		if_print(size);
-		printed += sample_type__fprintf(fp, &first, evsel->attr.sample_type);
-		if (evsel->attr.read_format)
-			printed += read_format__fprintf(fp, &first, evsel->attr.read_format);
-		if_print(disabled);
-		if_print(inherit);
-		if_print(pinned);
-		if_print(exclusive);
-		if_print(exclude_user);
-		if_print(exclude_kernel);
-		if_print(exclude_hv);
-		if_print(exclude_idle);
-		if_print(mmap);
-		if_print(comm);
-		if_print(freq);
-		if_print(inherit_stat);
-		if_print(enable_on_exec);
-		if_print(task);
-		if_print(watermark);
-		if_print(precise_ip);
-		if_print(mmap_data);
-		if_print(sample_id_all);
-		if_print(exclude_host);
-		if_print(exclude_guest);
-		if_print(mmap2);
-		if_print(comm_exec);
-		if_print(use_clockid);
-		if_print(__reserved_1);
-		if_print(wakeup_events);
-		if_print(bp_type);
-		if_print(branch_sample_type);
-		if_print(sample_regs_user);
-		if_print(sample_stack_user);
-		if_print(clockid);
-		if_print(sample_regs_intr);
-	}
 out:
 	fputc('\n', fp);
 	return ++printed;
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -359,4 +359,10 @@ static inline bool has_branch_callstack(
 {
 	return evsel->attr.branch_sample_type & PERF_SAMPLE_BRANCH_CALL_STACK;
 }
+
+typedef int (*attr__fprintf_f)(FILE *, const char *, const char *, void *);
+
+int perf_event_attr__fprintf(FILE *fp, struct perf_event_attr *attr,
+			     attr__fprintf_f attr__fprintf, void *priv);
+
 #endif /* __PERF_EVSEL_H */
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -1055,6 +1055,12 @@ read_event_desc(struct perf_header *ph,
 	goto out;
 }
 
+static int __desc_attr__fprintf(FILE *fp, const char *name, const char *val,
+				void *priv __attribute__((unused)))
+{
+	return fprintf(fp, ", %s = %s", name, val);
+}
+
 static void print_event_desc(struct perf_header *ph, int fd, FILE *fp)
 {
 	struct perf_evsel *evsel, *events = read_event_desc(ph, fd);
@@ -1069,26 +1075,6 @@ static void print_event_desc(struct perf
 	for (evsel = events; evsel->attr.size; evsel++) {
 		fprintf(fp, "# event : name = %s, ", evsel->name);
 
-		fprintf(fp, "type = %d, config = 0x%"PRIx64
-			    ", config1 = 0x%"PRIx64", config2 = 0x%"PRIx64,
-				evsel->attr.type,
-				(u64)evsel->attr.config,
-				(u64)evsel->attr.config1,
-				(u64)evsel->attr.config2);
-
-		fprintf(fp, ", excl_usr = %d, excl_kern = %d",
-				evsel->attr.exclude_user,
-				evsel->attr.exclude_kernel);
-
-		fprintf(fp, ", excl_host = %d, excl_guest = %d",
-				evsel->attr.exclude_host,
-				evsel->attr.exclude_guest);
-
-		fprintf(fp, ", precise_ip = %d", evsel->attr.precise_ip);
-
-		fprintf(fp, ", attr_mmap2 = %d", evsel->attr.mmap2);
-		fprintf(fp, ", attr_mmap  = %d", evsel->attr.mmap);
-		fprintf(fp, ", attr_mmap_data = %d", evsel->attr.mmap_data);
 		if (evsel->ids) {
 			fprintf(fp, ", id = {");
 			for (j = 0, id = evsel->id; j < evsel->ids; j++, id++) {
@@ -1098,9 +1084,8 @@ static void print_event_desc(struct perf
 			}
 			fprintf(fp, " }");
 		}
-		if (evsel->attr.use_clockid)
-			fprintf(fp, ", clockid = %d", evsel->attr.clockid);
 
+		perf_event_attr__fprintf(fp, &evsel->attr, __desc_attr__fprintf, NULL);
 
 		fputc('\n', fp);
 	}

Re: [RFC][PATCH] perf tools: unify perf_event_attr printing

[Adrian Hunter]

On 02/04/15 14:59, Peter Zijlstra wrote:
> On Thu, Apr 02, 2015 at 12:01:53PM +0300, Adrian Hunter wrote:
>> But personally I think the "include" approach is too ugly. I would just
>> add a function instead. Something like:
> 
> You've not stared at the kernel tracepoint code long enough ;-)

I will try some day when I feel too sane :-)

> 
> Something like so then?

Looks fine. Didn't test it, but

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> 
> ---
> Subject: perf, tools: Merge all perf_event_attr print functions
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Thu Apr 2 11:49:23 CEST 2015
> 
> Currently there's 3 (that I found) different and incomplete
> implementations of printing perf_event_attr.
> 
> This is quite silly. Merge the lot.
> 
> While this patch does not retain the exact form all printing that I
> found is debug output and thus it should not be critical.
> 
> Also, I cannot find a single print_event_desc() caller.
> 
> Pre:
> 
> $ perf record -vv -e cycles -- sleep 1
> ------------------------------------------------------------
> perf_event_attr:
>   type                0
>   size                104
>   config              0
>   sample_period       4000
>   sample_freq         4000
>   sample_type         0x107
>   read_format         0
>   disabled            1    inherit             1
>   pinned              0    exclusive           0
>   exclude_user        0    exclude_kernel      0
>   exclude_hv          0    exclude_idle        0
>   mmap                1    comm                1
>   mmap2               1    comm_exec           1
>   freq                1    inherit_stat        0
>   enable_on_exec      1    task                1
>   watermark           0    precise_ip          0
>   mmap_data           0    sample_id_all       1
>   exclude_host        0    exclude_guest       1
>   excl.callchain_kern 0    excl.callchain_user 0
>   wakeup_events       0
>   wakeup_watermark    0
>   bp_type             0
>   bp_addr             0
>   config1             0
>   bp_len              0
>   config2             0
>   branch_sample_type  0
>   sample_regs_user    0
>   sample_stack_user   0
>   sample_regs_intr    0
> ------------------------------------------------------------
> 
> $ perf evlist  -vv
> cycles: sample_freq=4000, size: 104, sample_type: IP|TID|TIME|PERIOD,
> disabled: 1, inherit: 1, mmap: 1, mmap2: 1, comm: 1, comm_exec: 1,
> freq: 1, enable_on_exec: 1, task: 1, sample_id_all: 1, exclude_guest:
> 1
> 
> Post:
> 
> $ ./perf record -vv -e cycles -- sleep 1 
> ------------------------------------------------------------
> perf_event_attr:
>   size                             112
>   { sample_period, sample_freq }   4000
>   sample_type                      IP|TID|TIME|PERIOD
>   disabled                         1
>   inherit                          1
>   mmap                             1
>   comm                             1
>   freq                             1
>   enable_on_exec                   1
>   task                             1
>   sample_id_all                    1
>   exclude_guest                    1
>   mmap2                            1
>   comm_exec                        1
> ------------------------------------------------------------
> 
> $ ./perf evlist  -vv
> cycles: size: 112, { sample_period, sample_freq }: 4000, sample_type:
> IP|TID|TIME|PERIOD, disabled: 1, inherit: 1, mmap: 1, comm: 1, freq:
> 1, enable_on_exec: 1, task: 1, sample_id_all: 1, exclude_guest: 1,
> mmap2: 1, comm_exec: 1
> 
> Cc: acme@redhat.com
> Cc: jolsa@redhat.com
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  tools/perf/util/evsel.c  |  284 ++++++++++++++++++++---------------------------
>  tools/perf/util/evsel.h  |    6 
>  tools/perf/util/header.c |   29 +---
>  3 files changed, 139 insertions(+), 180 deletions(-)
> 
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -1011,70 +1011,126 @@ static int get_group_fd(struct perf_evse
>  	return fd;
>  }
>  
> -#define __PRINT_ATTR(fmt, cast, field)  \
> -	fprintf(fp, "  %-19s "fmt"\n", #field, cast attr->field)
> +struct bit_names {
> +	int bit;
> +	const char *name;
> +};
> +
> +static void __p_bits(char *buf, size_t size, u64 value, struct bit_names *bits)
> +{
> +	bool first_bit = true;
> +	int i = 0;
> +
> +	do {
> +		if (value & bits[i].bit) {
> +			buf += scnprintf(buf, size, "%s%s", first_bit ? "" : "|", bits[i].name);
> +			first_bit = false;
> +		}
> +	} while (bits[++i].name != NULL);
> +}
> +
> +static void __p_sample_type(char *buf, size_t size, u64 value)
> +{
> +#define bit_name(n) { PERF_SAMPLE_##n, #n }
> +	struct bit_names bits[] = {
> +		bit_name(IP), bit_name(TID), bit_name(TIME), bit_name(ADDR),
> +		bit_name(READ), bit_name(CALLCHAIN), bit_name(ID), bit_name(CPU),
> +		bit_name(PERIOD), bit_name(STREAM_ID), bit_name(RAW),
> +		bit_name(BRANCH_STACK), bit_name(REGS_USER), bit_name(STACK_USER),
> +		bit_name(IDENTIFIER), bit_name(REGS_INTR),
> +		{ .name = NULL, }
> +	};
> +#undef bit_name
> +	__p_bits(buf, size, value, bits);
> +}
>  
> -#define PRINT_ATTR_U32(field)  __PRINT_ATTR("%u" , , field)
> -#define PRINT_ATTR_X32(field)  __PRINT_ATTR("%#x", , field)
> -#define PRINT_ATTR_U64(field)  __PRINT_ATTR("%" PRIu64, (uint64_t), field)
> -#define PRINT_ATTR_X64(field)  __PRINT_ATTR("%#"PRIx64, (uint64_t), field)
> -
> -#define PRINT_ATTR2N(name1, field1, name2, field2)	\
> -	fprintf(fp, "  %-19s %u    %-19s %u\n",		\
> -	name1, attr->field1, name2, attr->field2)
> -
> -#define PRINT_ATTR2(field1, field2) \
> -	PRINT_ATTR2N(#field1, field1, #field2, field2)
> -
> -static size_t perf_event_attr__fprintf(struct perf_event_attr *attr, FILE *fp)
> -{
> -	size_t ret = 0;
> -
> -	ret += fprintf(fp, "%.60s\n", graph_dotted_line);
> -	ret += fprintf(fp, "perf_event_attr:\n");
> -
> -	ret += PRINT_ATTR_U32(type);
> -	ret += PRINT_ATTR_U32(size);
> -	ret += PRINT_ATTR_X64(config);
> -	ret += PRINT_ATTR_U64(sample_period);
> -	ret += PRINT_ATTR_U64(sample_freq);
> -	ret += PRINT_ATTR_X64(sample_type);
> -	ret += PRINT_ATTR_X64(read_format);
> -
> -	ret += PRINT_ATTR2(disabled, inherit);
> -	ret += PRINT_ATTR2(pinned, exclusive);
> -	ret += PRINT_ATTR2(exclude_user, exclude_kernel);
> -	ret += PRINT_ATTR2(exclude_hv, exclude_idle);
> -	ret += PRINT_ATTR2(mmap, comm);
> -	ret += PRINT_ATTR2(freq, inherit_stat);
> -	ret += PRINT_ATTR2(enable_on_exec, task);
> -	ret += PRINT_ATTR2(watermark, precise_ip);
> -	ret += PRINT_ATTR2(mmap_data, sample_id_all);
> -	ret += PRINT_ATTR2(exclude_host, exclude_guest);
> -	ret += PRINT_ATTR2N("excl.callchain_kern", exclude_callchain_kernel,
> -			    "excl.callchain_user", exclude_callchain_user);
> -	ret += PRINT_ATTR2(mmap2, comm_exec);
> -	ret += __PRINT_ATTR("%u",,use_clockid);
> -
> -
> -	ret += PRINT_ATTR_U32(wakeup_events);
> -	ret += PRINT_ATTR_U32(wakeup_watermark);
> -	ret += PRINT_ATTR_X32(bp_type);
> -	ret += PRINT_ATTR_X64(bp_addr);
> -	ret += PRINT_ATTR_X64(config1);
> -	ret += PRINT_ATTR_U64(bp_len);
> -	ret += PRINT_ATTR_X64(config2);
> -	ret += PRINT_ATTR_X64(branch_sample_type);
> -	ret += PRINT_ATTR_X64(sample_regs_user);
> -	ret += PRINT_ATTR_U32(sample_stack_user);
> -	ret += PRINT_ATTR_U32(clockid);
> -	ret += PRINT_ATTR_X64(sample_regs_intr);
> +static void __p_read_format(char *buf, size_t size, u64 value)
> +{
> +#define bit_name(n) { PERF_FORMAT_##n, #n }
> +	struct bit_names bits[] = {
> +		bit_name(TOTAL_TIME_ENABLED), bit_name(TOTAL_TIME_RUNNING),
> +		bit_name(ID), bit_name(GROUP),
> +		{ .name = NULL, }
> +	};
> +#undef bit_name
> +	__p_bits(buf, size, value, bits);
> +}
> +
> +#define BUF_SIZE		1024
> +
> +#define p_hex(val)		snprintf(buf, BUF_SIZE, "%"PRIx64, (uint64_t)(val))
> +#define p_unsigned(val)		snprintf(buf, BUF_SIZE, "%"PRIu64, (uint64_t)(val))
> +#define p_signed(val)		snprintf(buf, BUF_SIZE, "%"PRId64, (int64_t)(val))
> +#define p_sample_type(val)	__p_sample_type(buf, BUF_SIZE, val)
> +#define p_read_format(val)	__p_read_format(buf, BUF_SIZE, val)
> +
> +#define PRINT_ATTRn(_n, _f, _p)				\
> +do {							\
> +	if (attr->_f) {					\
> +		_p(attr->_f);				\
> +		ret += attr__fprintf(fp, _n, buf, priv);\
> +	}						\
> +} while (0)
>  
> -	ret += fprintf(fp, "%.60s\n", graph_dotted_line);
> +#define PRINT_ATTRf(_f, _p)	PRINT_ATTRn(#_f, _f, _p)
> +
> +int perf_event_attr__fprintf(FILE *fp, struct perf_event_attr *attr,
> +			     attr__fprintf_f attr__fprintf, void *priv)
> +{
> +	char buf[BUF_SIZE];
> +	int ret = 0;
> +
> +	PRINT_ATTRf(type, p_unsigned);
> +	PRINT_ATTRf(size, p_unsigned);
> +	PRINT_ATTRf(config, p_hex);
> +	PRINT_ATTRn("{ sample_period, sample_freq }", sample_period, p_unsigned);
> +	PRINT_ATTRf(sample_type, p_sample_type);
> +	PRINT_ATTRf(read_format, p_read_format);
> +
> +	PRINT_ATTRf(disabled, p_unsigned);
> +	PRINT_ATTRf(inherit, p_unsigned);
> +	PRINT_ATTRf(pinned, p_unsigned);
> +	PRINT_ATTRf(exclusive, p_unsigned);
> +	PRINT_ATTRf(exclude_user, p_unsigned);
> +	PRINT_ATTRf(exclude_kernel, p_unsigned);
> +	PRINT_ATTRf(exclude_hv, p_unsigned);
> +	PRINT_ATTRf(exclude_idle, p_unsigned);
> +	PRINT_ATTRf(mmap, p_unsigned);
> +	PRINT_ATTRf(comm, p_unsigned);
> +	PRINT_ATTRf(freq, p_unsigned);
> +	PRINT_ATTRf(inherit_stat, p_unsigned);
> +	PRINT_ATTRf(enable_on_exec, p_unsigned);
> +	PRINT_ATTRf(task, p_unsigned);
> +	PRINT_ATTRf(watermark, p_unsigned);
> +	PRINT_ATTRf(precise_ip, p_unsigned);
> +	PRINT_ATTRf(mmap_data, p_unsigned);
> +	PRINT_ATTRf(sample_id_all, p_unsigned);
> +	PRINT_ATTRf(exclude_host, p_unsigned);
> +	PRINT_ATTRf(exclude_guest, p_unsigned);
> +	PRINT_ATTRf(exclude_callchain_kernel, p_unsigned);
> +	PRINT_ATTRf(exclude_callchain_user, p_unsigned);
> +	PRINT_ATTRf(mmap2, p_unsigned);
> +	PRINT_ATTRf(comm_exec, p_unsigned);
> +	PRINT_ATTRf(use_clockid, p_unsigned);
> +
> +	PRINT_ATTRn("{ wakeup_events, wakeup_watermark }", wakeup_events, p_unsigned);
> +	PRINT_ATTRf(bp_type, p_unsigned);
> +	PRINT_ATTRn("{ bp_addr, config1 }", bp_addr, p_hex);
> +	PRINT_ATTRn("{ bp_len, config2 }", bp_len, p_hex);
> +	PRINT_ATTRf(sample_regs_user, p_hex);
> +	PRINT_ATTRf(sample_stack_user, p_unsigned);
> +	PRINT_ATTRf(clockid, p_signed);
> +	PRINT_ATTRf(sample_regs_intr, p_hex);
>  
>  	return ret;
>  }
>  
> +static int __open_attr__fprintf(FILE *fp, const char *name, const char *val,
> +				void *priv __attribute__((unused)))
> +{
> +	return fprintf(fp, "  %-32s %s\n", name, val);
> +}
> +
>  static int __perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
>  			      struct thread_map *threads)
>  {
> @@ -1114,8 +1170,12 @@ static int __perf_evsel__open(struct per
>  	if (perf_missing_features.sample_id_all)
>  		evsel->attr.sample_id_all = 0;
>  
> -	if (verbose >= 2)
> -		perf_event_attr__fprintf(&evsel->attr, stderr);
> +	if (verbose >= 2) {
> +		fprintf(stderr, "%.60s\n", graph_dotted_line);
> +		fprintf(stderr, "perf_event_attr:\n");
> +		perf_event_attr__fprintf(stderr, &evsel->attr, __open_attr__fprintf, NULL);
> +		fprintf(stderr, "%.60s\n", graph_dotted_line);
> +	}
>  
>  	for (cpu = 0; cpu < cpus->nr; cpu++) {
>  
> @@ -1996,62 +2056,9 @@ static int comma_fprintf(FILE *fp, bool
>  	return ret;
>  }
>  
> -static int __if_fprintf(FILE *fp, bool *first, const char *field, u64 value)
> +static int __print_attr__fprintf(FILE *fp, const char *name, const char *val, void *priv)
>  {
> -	if (value == 0)
> -		return 0;
> -
> -	return comma_fprintf(fp, first, " %s: %" PRIu64, field, value);
> -}
> -
> -#define if_print(field) printed += __if_fprintf(fp, &first, #field, evsel->attr.field)
> -
> -struct bit_names {
> -	int bit;
> -	const char *name;
> -};
> -
> -static int bits__fprintf(FILE *fp, const char *field, u64 value,
> -			 struct bit_names *bits, bool *first)
> -{
> -	int i = 0, printed = comma_fprintf(fp, first, " %s: ", field);
> -	bool first_bit = true;
> -
> -	do {
> -		if (value & bits[i].bit) {
> -			printed += fprintf(fp, "%s%s", first_bit ? "" : "|", bits[i].name);
> -			first_bit = false;
> -		}
> -	} while (bits[++i].name != NULL);
> -
> -	return printed;
> -}
> -
> -static int sample_type__fprintf(FILE *fp, bool *first, u64 value)
> -{
> -#define bit_name(n) { PERF_SAMPLE_##n, #n }
> -	struct bit_names bits[] = {
> -		bit_name(IP), bit_name(TID), bit_name(TIME), bit_name(ADDR),
> -		bit_name(READ), bit_name(CALLCHAIN), bit_name(ID), bit_name(CPU),
> -		bit_name(PERIOD), bit_name(STREAM_ID), bit_name(RAW),
> -		bit_name(BRANCH_STACK), bit_name(REGS_USER), bit_name(STACK_USER),
> -		bit_name(IDENTIFIER), bit_name(REGS_INTR),
> -		{ .name = NULL, }
> -	};
> -#undef bit_name
> -	return bits__fprintf(fp, "sample_type", value, bits, first);
> -}
> -
> -static int read_format__fprintf(FILE *fp, bool *first, u64 value)
> -{
> -#define bit_name(n) { PERF_FORMAT_##n, #n }
> -	struct bit_names bits[] = {
> -		bit_name(TOTAL_TIME_ENABLED), bit_name(TOTAL_TIME_RUNNING),
> -		bit_name(ID), bit_name(GROUP),
> -		{ .name = NULL, }
> -	};
> -#undef bit_name
> -	return bits__fprintf(fp, "read_format", value, bits, first);
> +	return comma_fprintf(fp, (bool *)priv, " %s: %s", name, val);
>  }
>  
>  int perf_evsel__fprintf(struct perf_evsel *evsel,
> @@ -2080,52 +2087,13 @@ int perf_evsel__fprintf(struct perf_evse
>  
>  	printed += fprintf(fp, "%s", perf_evsel__name(evsel));
>  
> -	if (details->verbose || details->freq) {
> +	if (details->verbose) {
> +		printed += perf_event_attr__fprintf(fp, &evsel->attr,
> +						    __print_attr__fprintf, &first);
> +	} else if (details->freq) {
>  		printed += comma_fprintf(fp, &first, " sample_freq=%" PRIu64,
>  					 (u64)evsel->attr.sample_freq);
>  	}
> -
> -	if (details->verbose) {
> -		if_print(type);
> -		if_print(config);
> -		if_print(config1);
> -		if_print(config2);
> -		if_print(size);
> -		printed += sample_type__fprintf(fp, &first, evsel->attr.sample_type);
> -		if (evsel->attr.read_format)
> -			printed += read_format__fprintf(fp, &first, evsel->attr.read_format);
> -		if_print(disabled);
> -		if_print(inherit);
> -		if_print(pinned);
> -		if_print(exclusive);
> -		if_print(exclude_user);
> -		if_print(exclude_kernel);
> -		if_print(exclude_hv);
> -		if_print(exclude_idle);
> -		if_print(mmap);
> -		if_print(comm);
> -		if_print(freq);
> -		if_print(inherit_stat);
> -		if_print(enable_on_exec);
> -		if_print(task);
> -		if_print(watermark);
> -		if_print(precise_ip);
> -		if_print(mmap_data);
> -		if_print(sample_id_all);
> -		if_print(exclude_host);
> -		if_print(exclude_guest);
> -		if_print(mmap2);
> -		if_print(comm_exec);
> -		if_print(use_clockid);
> -		if_print(__reserved_1);
> -		if_print(wakeup_events);
> -		if_print(bp_type);
> -		if_print(branch_sample_type);
> -		if_print(sample_regs_user);
> -		if_print(sample_stack_user);
> -		if_print(clockid);
> -		if_print(sample_regs_intr);
> -	}
>  out:
>  	fputc('\n', fp);
>  	return ++printed;
> --- a/tools/perf/util/evsel.h
> +++ b/tools/perf/util/evsel.h
> @@ -359,4 +359,10 @@ static inline bool has_branch_callstack(
>  {
>  	return evsel->attr.branch_sample_type & PERF_SAMPLE_BRANCH_CALL_STACK;
>  }
> +
> +typedef int (*attr__fprintf_f)(FILE *, const char *, const char *, void *);
> +
> +int perf_event_attr__fprintf(FILE *fp, struct perf_event_attr *attr,
> +			     attr__fprintf_f attr__fprintf, void *priv);
> +
>  #endif /* __PERF_EVSEL_H */
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -1055,6 +1055,12 @@ read_event_desc(struct perf_header *ph,
>  	goto out;
>  }
>  
> +static int __desc_attr__fprintf(FILE *fp, const char *name, const char *val,
> +				void *priv __attribute__((unused)))
> +{
> +	return fprintf(fp, ", %s = %s", name, val);
> +}
> +
>  static void print_event_desc(struct perf_header *ph, int fd, FILE *fp)
>  {
>  	struct perf_evsel *evsel, *events = read_event_desc(ph, fd);
> @@ -1069,26 +1075,6 @@ static void print_event_desc(struct perf
>  	for (evsel = events; evsel->attr.size; evsel++) {
>  		fprintf(fp, "# event : name = %s, ", evsel->name);
>  
> -		fprintf(fp, "type = %d, config = 0x%"PRIx64
> -			    ", config1 = 0x%"PRIx64", config2 = 0x%"PRIx64,
> -				evsel->attr.type,
> -				(u64)evsel->attr.config,
> -				(u64)evsel->attr.config1,
> -				(u64)evsel->attr.config2);
> -
> -		fprintf(fp, ", excl_usr = %d, excl_kern = %d",
> -				evsel->attr.exclude_user,
> -				evsel->attr.exclude_kernel);
> -
> -		fprintf(fp, ", excl_host = %d, excl_guest = %d",
> -				evsel->attr.exclude_host,
> -				evsel->attr.exclude_guest);
> -
> -		fprintf(fp, ", precise_ip = %d", evsel->attr.precise_ip);
> -
> -		fprintf(fp, ", attr_mmap2 = %d", evsel->attr.mmap2);
> -		fprintf(fp, ", attr_mmap  = %d", evsel->attr.mmap);
> -		fprintf(fp, ", attr_mmap_data = %d", evsel->attr.mmap_data);
>  		if (evsel->ids) {
>  			fprintf(fp, ", id = {");
>  			for (j = 0, id = evsel->id; j < evsel->ids; j++, id++) {
> @@ -1098,9 +1084,8 @@ static void print_event_desc(struct perf
>  			}
>  			fprintf(fp, " }");
>  		}
> -		if (evsel->attr.use_clockid)
> -			fprintf(fp, ", clockid = %d", evsel->attr.clockid);
>  
> +		perf_event_attr__fprintf(fp, &evsel->attr, __desc_attr__fprintf, NULL);
>  
>  		fputc('\n', fp);
>  	}
> 
> 

Re: [RFC][PATCH] perf tools: unify perf_event_attr printing

[Arnaldo Carvalho de Melo]

Em Thu, Apr 02, 2015 at 10:12:50AM +0200, Ingo Molnar escreveu:
> * Peter Zijlstra <peterz@infradead.org> wrote:
> >  tools/perf/util/print_helper.h |    7 +
> >  6 files changed, 170 insertions(+), 174 deletions(-)
> 
> Acked-by: Ingo Molnar <mingo@kernel.org>

I'll get this in tomorrow, just finishing a pull req now,

- Arnaldo

Re: [RFC][PATCH] perf tools: unify perf_event_attr printing

[Arnaldo Carvalho de Melo]

Em Thu, Apr 02, 2015 at 01:59:34PM +0200, Peter Zijlstra escreveu:
> On Thu, Apr 02, 2015 at 12:01:53PM +0300, Adrian Hunter wrote:
> > But personally I think the "include" approach is too ugly. I would just
> > add a function instead. Something like:
> 
> You've not stared at the kernel tracepoint code long enough ;-)
> 
> Something like so then?

Ok, this one seems to have gotten acks from Ingo, Jiri and Adrian, but
doesn't apply because it needs that clock_id one first, searching the
situation of that part...

- Arnaldo
 
> ---
> Subject: perf, tools: Merge all perf_event_attr print functions
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Thu Apr 2 11:49:23 CEST 2015
> 
> Currently there's 3 (that I found) different and incomplete
> implementations of printing perf_event_attr.
> 
> This is quite silly. Merge the lot.
> 
> While this patch does not retain the exact form all printing that I
> found is debug output and thus it should not be critical.
> 
> Also, I cannot find a single print_event_desc() caller.
> 
> Pre:
> 
> $ perf record -vv -e cycles -- sleep 1
> ------------------------------------------------------------
> perf_event_attr:
>   type                0
>   size                104
>   config              0
>   sample_period       4000
>   sample_freq         4000
>   sample_type         0x107
>   read_format         0
>   disabled            1    inherit             1
>   pinned              0    exclusive           0
>   exclude_user        0    exclude_kernel      0
>   exclude_hv          0    exclude_idle        0
>   mmap                1    comm                1
>   mmap2               1    comm_exec           1
>   freq                1    inherit_stat        0
>   enable_on_exec      1    task                1
>   watermark           0    precise_ip          0
>   mmap_data           0    sample_id_all       1
>   exclude_host        0    exclude_guest       1
>   excl.callchain_kern 0    excl.callchain_user 0
>   wakeup_events       0
>   wakeup_watermark    0
>   bp_type             0
>   bp_addr             0
>   config1             0
>   bp_len              0
>   config2             0
>   branch_sample_type  0
>   sample_regs_user    0
>   sample_stack_user   0
>   sample_regs_intr    0
> ------------------------------------------------------------
> 
> $ perf evlist  -vv
> cycles: sample_freq=4000, size: 104, sample_type: IP|TID|TIME|PERIOD,
> disabled: 1, inherit: 1, mmap: 1, mmap2: 1, comm: 1, comm_exec: 1,
> freq: 1, enable_on_exec: 1, task: 1, sample_id_all: 1, exclude_guest:
> 1
> 
> Post:
> 
> $ ./perf record -vv -e cycles -- sleep 1 
> ------------------------------------------------------------
> perf_event_attr:
>   size                             112
>   { sample_period, sample_freq }   4000
>   sample_type                      IP|TID|TIME|PERIOD
>   disabled                         1
>   inherit                          1
>   mmap                             1
>   comm                             1
>   freq                             1
>   enable_on_exec                   1
>   task                             1
>   sample_id_all                    1
>   exclude_guest                    1
>   mmap2                            1
>   comm_exec                        1
> ------------------------------------------------------------
> 
> $ ./perf evlist  -vv
> cycles: size: 112, { sample_period, sample_freq }: 4000, sample_type:
> IP|TID|TIME|PERIOD, disabled: 1, inherit: 1, mmap: 1, comm: 1, freq:
> 1, enable_on_exec: 1, task: 1, sample_id_all: 1, exclude_guest: 1,
> mmap2: 1, comm_exec: 1
> 
> Cc: acme@redhat.com
> Cc: jolsa@redhat.com
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  tools/perf/util/evsel.c  |  284 ++++++++++++++++++++---------------------------
>  tools/perf/util/evsel.h  |    6 
>  tools/perf/util/header.c |   29 +---
>  3 files changed, 139 insertions(+), 180 deletions(-)
> 
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -1011,70 +1011,126 @@ static int get_group_fd(struct perf_evse
>  	return fd;
>  }
>  
> -#define __PRINT_ATTR(fmt, cast, field)  \
> -	fprintf(fp, "  %-19s "fmt"\n", #field, cast attr->field)
> +struct bit_names {
> +	int bit;
> +	const char *name;
> +};
> +
> +static void __p_bits(char *buf, size_t size, u64 value, struct bit_names *bits)
> +{
> +	bool first_bit = true;
> +	int i = 0;
> +
> +	do {
> +		if (value & bits[i].bit) {
> +			buf += scnprintf(buf, size, "%s%s", first_bit ? "" : "|", bits[i].name);
> +			first_bit = false;
> +		}
> +	} while (bits[++i].name != NULL);
> +}
> +
> +static void __p_sample_type(char *buf, size_t size, u64 value)
> +{
> +#define bit_name(n) { PERF_SAMPLE_##n, #n }
> +	struct bit_names bits[] = {
> +		bit_name(IP), bit_name(TID), bit_name(TIME), bit_name(ADDR),
> +		bit_name(READ), bit_name(CALLCHAIN), bit_name(ID), bit_name(CPU),
> +		bit_name(PERIOD), bit_name(STREAM_ID), bit_name(RAW),
> +		bit_name(BRANCH_STACK), bit_name(REGS_USER), bit_name(STACK_USER),
> +		bit_name(IDENTIFIER), bit_name(REGS_INTR),
> +		{ .name = NULL, }
> +	};
> +#undef bit_name
> +	__p_bits(buf, size, value, bits);
> +}
>  
> -#define PRINT_ATTR_U32(field)  __PRINT_ATTR("%u" , , field)
> -#define PRINT_ATTR_X32(field)  __PRINT_ATTR("%#x", , field)
> -#define PRINT_ATTR_U64(field)  __PRINT_ATTR("%" PRIu64, (uint64_t), field)
> -#define PRINT_ATTR_X64(field)  __PRINT_ATTR("%#"PRIx64, (uint64_t), field)
> -
> -#define PRINT_ATTR2N(name1, field1, name2, field2)	\
> -	fprintf(fp, "  %-19s %u    %-19s %u\n",		\
> -	name1, attr->field1, name2, attr->field2)
> -
> -#define PRINT_ATTR2(field1, field2) \
> -	PRINT_ATTR2N(#field1, field1, #field2, field2)
> -
> -static size_t perf_event_attr__fprintf(struct perf_event_attr *attr, FILE *fp)
> -{
> -	size_t ret = 0;
> -
> -	ret += fprintf(fp, "%.60s\n", graph_dotted_line);
> -	ret += fprintf(fp, "perf_event_attr:\n");
> -
> -	ret += PRINT_ATTR_U32(type);
> -	ret += PRINT_ATTR_U32(size);
> -	ret += PRINT_ATTR_X64(config);
> -	ret += PRINT_ATTR_U64(sample_period);
> -	ret += PRINT_ATTR_U64(sample_freq);
> -	ret += PRINT_ATTR_X64(sample_type);
> -	ret += PRINT_ATTR_X64(read_format);
> -
> -	ret += PRINT_ATTR2(disabled, inherit);
> -	ret += PRINT_ATTR2(pinned, exclusive);
> -	ret += PRINT_ATTR2(exclude_user, exclude_kernel);
> -	ret += PRINT_ATTR2(exclude_hv, exclude_idle);
> -	ret += PRINT_ATTR2(mmap, comm);
> -	ret += PRINT_ATTR2(freq, inherit_stat);
> -	ret += PRINT_ATTR2(enable_on_exec, task);
> -	ret += PRINT_ATTR2(watermark, precise_ip);
> -	ret += PRINT_ATTR2(mmap_data, sample_id_all);
> -	ret += PRINT_ATTR2(exclude_host, exclude_guest);
> -	ret += PRINT_ATTR2N("excl.callchain_kern", exclude_callchain_kernel,
> -			    "excl.callchain_user", exclude_callchain_user);
> -	ret += PRINT_ATTR2(mmap2, comm_exec);
> -	ret += __PRINT_ATTR("%u",,use_clockid);
> -
> -
> -	ret += PRINT_ATTR_U32(wakeup_events);
> -	ret += PRINT_ATTR_U32(wakeup_watermark);
> -	ret += PRINT_ATTR_X32(bp_type);
> -	ret += PRINT_ATTR_X64(bp_addr);
> -	ret += PRINT_ATTR_X64(config1);
> -	ret += PRINT_ATTR_U64(bp_len);
> -	ret += PRINT_ATTR_X64(config2);
> -	ret += PRINT_ATTR_X64(branch_sample_type);
> -	ret += PRINT_ATTR_X64(sample_regs_user);
> -	ret += PRINT_ATTR_U32(sample_stack_user);
> -	ret += PRINT_ATTR_U32(clockid);
> -	ret += PRINT_ATTR_X64(sample_regs_intr);
> +static void __p_read_format(char *buf, size_t size, u64 value)
> +{
> +#define bit_name(n) { PERF_FORMAT_##n, #n }
> +	struct bit_names bits[] = {
> +		bit_name(TOTAL_TIME_ENABLED), bit_name(TOTAL_TIME_RUNNING),
> +		bit_name(ID), bit_name(GROUP),
> +		{ .name = NULL, }
> +	};
> +#undef bit_name
> +	__p_bits(buf, size, value, bits);
> +}
> +
> +#define BUF_SIZE		1024
> +
> +#define p_hex(val)		snprintf(buf, BUF_SIZE, "%"PRIx64, (uint64_t)(val))
> +#define p_unsigned(val)		snprintf(buf, BUF_SIZE, "%"PRIu64, (uint64_t)(val))
> +#define p_signed(val)		snprintf(buf, BUF_SIZE, "%"PRId64, (int64_t)(val))
> +#define p_sample_type(val)	__p_sample_type(buf, BUF_SIZE, val)
> +#define p_read_format(val)	__p_read_format(buf, BUF_SIZE, val)
> +
> +#define PRINT_ATTRn(_n, _f, _p)				\
> +do {							\
> +	if (attr->_f) {					\
> +		_p(attr->_f);				\
> +		ret += attr__fprintf(fp, _n, buf, priv);\
> +	}						\
> +} while (0)
>  
> -	ret += fprintf(fp, "%.60s\n", graph_dotted_line);
> +#define PRINT_ATTRf(_f, _p)	PRINT_ATTRn(#_f, _f, _p)
> +
> +int perf_event_attr__fprintf(FILE *fp, struct perf_event_attr *attr,
> +			     attr__fprintf_f attr__fprintf, void *priv)
> +{
> +	char buf[BUF_SIZE];
> +	int ret = 0;
> +
> +	PRINT_ATTRf(type, p_unsigned);
> +	PRINT_ATTRf(size, p_unsigned);
> +	PRINT_ATTRf(config, p_hex);
> +	PRINT_ATTRn("{ sample_period, sample_freq }", sample_period, p_unsigned);
> +	PRINT_ATTRf(sample_type, p_sample_type);
> +	PRINT_ATTRf(read_format, p_read_format);
> +
> +	PRINT_ATTRf(disabled, p_unsigned);
> +	PRINT_ATTRf(inherit, p_unsigned);
> +	PRINT_ATTRf(pinned, p_unsigned);
> +	PRINT_ATTRf(exclusive, p_unsigned);
> +	PRINT_ATTRf(exclude_user, p_unsigned);
> +	PRINT_ATTRf(exclude_kernel, p_unsigned);
> +	PRINT_ATTRf(exclude_hv, p_unsigned);
> +	PRINT_ATTRf(exclude_idle, p_unsigned);
> +	PRINT_ATTRf(mmap, p_unsigned);
> +	PRINT_ATTRf(comm, p_unsigned);
> +	PRINT_ATTRf(freq, p_unsigned);
> +	PRINT_ATTRf(inherit_stat, p_unsigned);
> +	PRINT_ATTRf(enable_on_exec, p_unsigned);
> +	PRINT_ATTRf(task, p_unsigned);
> +	PRINT_ATTRf(watermark, p_unsigned);
> +	PRINT_ATTRf(precise_ip, p_unsigned);
> +	PRINT_ATTRf(mmap_data, p_unsigned);
> +	PRINT_ATTRf(sample_id_all, p_unsigned);
> +	PRINT_ATTRf(exclude_host, p_unsigned);
> +	PRINT_ATTRf(exclude_guest, p_unsigned);
> +	PRINT_ATTRf(exclude_callchain_kernel, p_unsigned);
> +	PRINT_ATTRf(exclude_callchain_user, p_unsigned);
> +	PRINT_ATTRf(mmap2, p_unsigned);
> +	PRINT_ATTRf(comm_exec, p_unsigned);
> +	PRINT_ATTRf(use_clockid, p_unsigned);
> +
> +	PRINT_ATTRn("{ wakeup_events, wakeup_watermark }", wakeup_events, p_unsigned);
> +	PRINT_ATTRf(bp_type, p_unsigned);
> +	PRINT_ATTRn("{ bp_addr, config1 }", bp_addr, p_hex);
> +	PRINT_ATTRn("{ bp_len, config2 }", bp_len, p_hex);
> +	PRINT_ATTRf(sample_regs_user, p_hex);
> +	PRINT_ATTRf(sample_stack_user, p_unsigned);
> +	PRINT_ATTRf(clockid, p_signed);
> +	PRINT_ATTRf(sample_regs_intr, p_hex);
>  
>  	return ret;
>  }
>  
> +static int __open_attr__fprintf(FILE *fp, const char *name, const char *val,
> +				void *priv __attribute__((unused)))
> +{
> +	return fprintf(fp, "  %-32s %s\n", name, val);
> +}
> +
>  static int __perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
>  			      struct thread_map *threads)
>  {
> @@ -1114,8 +1170,12 @@ static int __perf_evsel__open(struct per
>  	if (perf_missing_features.sample_id_all)
>  		evsel->attr.sample_id_all = 0;
>  
> -	if (verbose >= 2)
> -		perf_event_attr__fprintf(&evsel->attr, stderr);
> +	if (verbose >= 2) {
> +		fprintf(stderr, "%.60s\n", graph_dotted_line);
> +		fprintf(stderr, "perf_event_attr:\n");
> +		perf_event_attr__fprintf(stderr, &evsel->attr, __open_attr__fprintf, NULL);
> +		fprintf(stderr, "%.60s\n", graph_dotted_line);
> +	}
>  
>  	for (cpu = 0; cpu < cpus->nr; cpu++) {
>  
> @@ -1996,62 +2056,9 @@ static int comma_fprintf(FILE *fp, bool
>  	return ret;
>  }
>  
> -static int __if_fprintf(FILE *fp, bool *first, const char *field, u64 value)
> +static int __print_attr__fprintf(FILE *fp, const char *name, const char *val, void *priv)
>  {
> -	if (value == 0)
> -		return 0;
> -
> -	return comma_fprintf(fp, first, " %s: %" PRIu64, field, value);
> -}
> -
> -#define if_print(field) printed += __if_fprintf(fp, &first, #field, evsel->attr.field)
> -
> -struct bit_names {
> -	int bit;
> -	const char *name;
> -};
> -
> -static int bits__fprintf(FILE *fp, const char *field, u64 value,
> -			 struct bit_names *bits, bool *first)
> -{
> -	int i = 0, printed = comma_fprintf(fp, first, " %s: ", field);
> -	bool first_bit = true;
> -
> -	do {
> -		if (value & bits[i].bit) {
> -			printed += fprintf(fp, "%s%s", first_bit ? "" : "|", bits[i].name);
> -			first_bit = false;
> -		}
> -	} while (bits[++i].name != NULL);
> -
> -	return printed;
> -}
> -
> -static int sample_type__fprintf(FILE *fp, bool *first, u64 value)
> -{
> -#define bit_name(n) { PERF_SAMPLE_##n, #n }
> -	struct bit_names bits[] = {
> -		bit_name(IP), bit_name(TID), bit_name(TIME), bit_name(ADDR),
> -		bit_name(READ), bit_name(CALLCHAIN), bit_name(ID), bit_name(CPU),
> -		bit_name(PERIOD), bit_name(STREAM_ID), bit_name(RAW),
> -		bit_name(BRANCH_STACK), bit_name(REGS_USER), bit_name(STACK_USER),
> -		bit_name(IDENTIFIER), bit_name(REGS_INTR),
> -		{ .name = NULL, }
> -	};
> -#undef bit_name
> -	return bits__fprintf(fp, "sample_type", value, bits, first);
> -}
> -
> -static int read_format__fprintf(FILE *fp, bool *first, u64 value)
> -{
> -#define bit_name(n) { PERF_FORMAT_##n, #n }
> -	struct bit_names bits[] = {
> -		bit_name(TOTAL_TIME_ENABLED), bit_name(TOTAL_TIME_RUNNING),
> -		bit_name(ID), bit_name(GROUP),
> -		{ .name = NULL, }
> -	};
> -#undef bit_name
> -	return bits__fprintf(fp, "read_format", value, bits, first);
> +	return comma_fprintf(fp, (bool *)priv, " %s: %s", name, val);
>  }
>  
>  int perf_evsel__fprintf(struct perf_evsel *evsel,
> @@ -2080,52 +2087,13 @@ int perf_evsel__fprintf(struct perf_evse
>  
>  	printed += fprintf(fp, "%s", perf_evsel__name(evsel));
>  
> -	if (details->verbose || details->freq) {
> +	if (details->verbose) {
> +		printed += perf_event_attr__fprintf(fp, &evsel->attr,
> +						    __print_attr__fprintf, &first);
> +	} else if (details->freq) {
>  		printed += comma_fprintf(fp, &first, " sample_freq=%" PRIu64,
>  					 (u64)evsel->attr.sample_freq);
>  	}
> -
> -	if (details->verbose) {
> -		if_print(type);
> -		if_print(config);
> -		if_print(config1);
> -		if_print(config2);
> -		if_print(size);
> -		printed += sample_type__fprintf(fp, &first, evsel->attr.sample_type);
> -		if (evsel->attr.read_format)
> -			printed += read_format__fprintf(fp, &first, evsel->attr.read_format);
> -		if_print(disabled);
> -		if_print(inherit);
> -		if_print(pinned);
> -		if_print(exclusive);
> -		if_print(exclude_user);
> -		if_print(exclude_kernel);
> -		if_print(exclude_hv);
> -		if_print(exclude_idle);
> -		if_print(mmap);
> -		if_print(comm);
> -		if_print(freq);
> -		if_print(inherit_stat);
> -		if_print(enable_on_exec);
> -		if_print(task);
> -		if_print(watermark);
> -		if_print(precise_ip);
> -		if_print(mmap_data);
> -		if_print(sample_id_all);
> -		if_print(exclude_host);
> -		if_print(exclude_guest);
> -		if_print(mmap2);
> -		if_print(comm_exec);
> -		if_print(use_clockid);
> -		if_print(__reserved_1);
> -		if_print(wakeup_events);
> -		if_print(bp_type);
> -		if_print(branch_sample_type);
> -		if_print(sample_regs_user);
> -		if_print(sample_stack_user);
> -		if_print(clockid);
> -		if_print(sample_regs_intr);
> -	}
>  out:
>  	fputc('\n', fp);
>  	return ++printed;
> --- a/tools/perf/util/evsel.h
> +++ b/tools/perf/util/evsel.h
> @@ -359,4 +359,10 @@ static inline bool has_branch_callstack(
>  {
>  	return evsel->attr.branch_sample_type & PERF_SAMPLE_BRANCH_CALL_STACK;
>  }
> +
> +typedef int (*attr__fprintf_f)(FILE *, const char *, const char *, void *);
> +
> +int perf_event_attr__fprintf(FILE *fp, struct perf_event_attr *attr,
> +			     attr__fprintf_f attr__fprintf, void *priv);
> +
>  #endif /* __PERF_EVSEL_H */
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -1055,6 +1055,12 @@ read_event_desc(struct perf_header *ph,
>  	goto out;
>  }
>  
> +static int __desc_attr__fprintf(FILE *fp, const char *name, const char *val,
> +				void *priv __attribute__((unused)))
> +{
> +	return fprintf(fp, ", %s = %s", name, val);
> +}
> +
>  static void print_event_desc(struct perf_header *ph, int fd, FILE *fp)
>  {
>  	struct perf_evsel *evsel, *events = read_event_desc(ph, fd);
> @@ -1069,26 +1075,6 @@ static void print_event_desc(struct perf
>  	for (evsel = events; evsel->attr.size; evsel++) {
>  		fprintf(fp, "# event : name = %s, ", evsel->name);
>  
> -		fprintf(fp, "type = %d, config = 0x%"PRIx64
> -			    ", config1 = 0x%"PRIx64", config2 = 0x%"PRIx64,
> -				evsel->attr.type,
> -				(u64)evsel->attr.config,
> -				(u64)evsel->attr.config1,
> -				(u64)evsel->attr.config2);
> -
> -		fprintf(fp, ", excl_usr = %d, excl_kern = %d",
> -				evsel->attr.exclude_user,
> -				evsel->attr.exclude_kernel);
> -
> -		fprintf(fp, ", excl_host = %d, excl_guest = %d",
> -				evsel->attr.exclude_host,
> -				evsel->attr.exclude_guest);
> -
> -		fprintf(fp, ", precise_ip = %d", evsel->attr.precise_ip);
> -
> -		fprintf(fp, ", attr_mmap2 = %d", evsel->attr.mmap2);
> -		fprintf(fp, ", attr_mmap  = %d", evsel->attr.mmap);
> -		fprintf(fp, ", attr_mmap_data = %d", evsel->attr.mmap_data);
>  		if (evsel->ids) {
>  			fprintf(fp, ", id = {");
>  			for (j = 0, id = evsel->id; j < evsel->ids; j++, id++) {
> @@ -1098,9 +1084,8 @@ static void print_event_desc(struct perf
>  			}
>  			fprintf(fp, " }");
>  		}
> -		if (evsel->attr.use_clockid)
> -			fprintf(fp, ", clockid = %d", evsel->attr.clockid);
>  
> +		perf_event_attr__fprintf(fp, &evsel->attr, __desc_attr__fprintf, NULL);
>  
>  		fputc('\n', fp);
>  	}

Re: [RFC][PATCH] perf tools: unify perf_event_attr printing

[Arnaldo Carvalho de Melo]

Em Fri, Apr 03, 2015 at 01:11:47PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Thu, Apr 02, 2015 at 01:59:34PM +0200, Peter Zijlstra escreveu:
> > On Thu, Apr 02, 2015 at 12:01:53PM +0300, Adrian Hunter wrote:
> > > But personally I think the "include" approach is too ugly. I would just
> > > add a function instead. Something like:
> > 
> > You've not stared at the kernel tracepoint code long enough ;-)
> > 
> > Something like so then?
> 
> Ok, this one seems to have gotten acks from Ingo, Jiri and Adrian, but
> doesn't apply because it needs that clock_id one first, searching the
> situation of that part...

Hey Peter, would you be so kind and resubmit this (at least) two part
series? I.e. the first one adding the clockid and the second, with the
consolidation of the perf_event_attr printing?

- Arnaldo