perf, record: Add clockid parameter

PATCH In review Peter Zijlstra 7 participants 40 messages Mar 27, 2015 – Apr 3, 2015 1 message not in the archive perf events

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Peter Zijlstra

author maintainer

2015-03-27 14:32 UTC #1 lore raw

PATCH 1/1 perf, record: Add clockid parameter

Quoted 1 line · tip-bot for Peter Zijlstra

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

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 tools/perf/builtin-record.c | 3 +++
 tools/perf/perf.h           | 1 +
 tools/perf/util/evsel.c     | 5 +++++
 3 files changed, 9 insertions(+)

tools/perf/builtin-record.c +3 -0

index 18aad239b401..9d4ed884b1c8 100644
@@ -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()
 };

tools/perf/perf.h +1 -0

index c38a085a5571..275c0c58fbbe 100644
@@ -62,6 +62,7 @@ struct record_opts {
 	u64	     user_interval;
 	bool	     sample_transaction;
 	unsigned     initial_delay;
+	clockid_t    clockid;
 };
 
 struct option;

tools/perf/util/evsel.c +5 -0

index 358e5954baa8..309208b16632 100644
@@ -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)

David Ahern

2015-03-27 17:11 UTC #2 lore raw

Quoted 16 lines · Peter Zijlstra

Quoted 1 line · tip-bot for Peter Zijlstra
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.

Quoted 7 lines

Cc Arnaldo Carvalho de Melo <[email protected]>
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
---
  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

Quoted 41 lines

tools/perf/builtin-record.c +3 -0

index 18aad239b401..9d4ed884b1c8 100644
@@ -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()
  };

tools/perf/perf.h +1 -0

index c38a085a5571..275c0c58fbbe 100644
@@ -62,6 +62,7 @@ struct record_opts {
  	u64	     user_interval;
  	bool	     sample_transaction;
  	unsigned     initial_delay;
+	clockid_t    clockid;
  };
 
  struct option;

tools/perf/util/evsel.c +0 -0

index 358e5954baa8..309208b16632 100644
@@ -761,6 +761,11 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts)
  		attr->disabled = 0;

tools/perf/util/evsel.c void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts) · line 771

Quoted 10 lines

tools/perf/util/evsel.c +5 -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())?

Peter Zijlstra

author maintainer

2015-03-27 17:21 UTC #3 lore raw

Quoted 6 lines · David Ahern

Quoted 4 lines

 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

tools/perf/Documentation/perf-record.txt +5 -0

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

Quoted 12 lines

+++ b/tools/perf/util/evsel.c

(quoted diff) +5 -0

@@ -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;
+	}
 }

Quoted 5 lines

Review void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts) · line 771
Quoted 2 lines
(quoted diff) +0 -0
 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.

Quoted 2 lines

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.

David Ahern

2015-03-27 17:35 UTC #4 lore raw

Quoted 7 lines · Peter Zijlstra

Quoted 2 lines
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.

Arnaldo Carvalho de Melo

2015-03-27 22:44 UTC #5 lore raw

Em Fri, Mar 27, 2015 at 11:35:25AM -0600, David Ahern escreveu:

Quoted 12 lines · Peter Zijlstra

Quoted 6 lines
Quoted 2 lines
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

Peter Zijlstra

author maintainer

2015-03-27 21:59 UTC #6 lore raw

Quoted 3 lines · Arnaldo Carvalho de Melo

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

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---

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

tools/perf/Documentation/perf-record.txt +5 -0

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

tools/perf/builtin-record.c +3 -0

@@ -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()
 };

tools/perf/perf.h +1 -0

@@ -62,6 +62,7 @@ struct record_opts {
 	u64	     user_interval;
 	bool	     sample_transaction;
 	unsigned     initial_delay;
+	clockid_t    clockid;
 };
 
 struct option;

tools/perf/util/evsel.c +40 -4

115 lines not on this page · raw message

Stephane Eranian

2015-03-27 22:37 UTC #7 lore raw

Quoted 59 lines · Peter Zijlstra

Quoted 3 lines · Arnaldo Carvalho de Melo
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 <[email protected]>
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 [email protected]
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
---
 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(-)
tools/perf/Documentation/perf-record.txt +5 -0
@@ -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]
tools/perf/builtin-record.c +1 -0
@@ -739,6 +739,7 @@ static struct record record = {
                        .uses_mmap   = true,
                        .default_per_cpu = true,
                },
+               .clockid             = -1,
        },
        .tool = {
                .sample         = process_sample_event,

tools/perf/builtin-record.c struct option __record_options[] = { · line 847

Quoted 5 lines

tools/perf/builtin-record.c +2 -0

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

Quoted 141 lines

tools/perf/builtin-record.c +0 -0

        OPT_END()
 };

tools/perf/perf.h +1 -0

@@ -62,6 +62,7 @@ struct record_opts {
        u64          user_interval;
        bool         sample_transaction;
        unsigned     initial_delay;
+       clockid_t    clockid;
 };
 
 struct option;

tools/perf/util/evsel.c +40 -4

@@ -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;
        }

Peter Zijlstra

author maintainer

2015-03-28 07:56 UTC #8 lore raw

Quoted 5 lines · Stephane Eranian

Review
Quoted 2 lines
(quoted diff) +2 -0
+       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 <[email protected]>
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 [email protected]

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

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

tools/perf/Documentation/perf-record.txt +7 -0

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

tools/perf/builtin-record.c +73 -0

91 lines not on this page · raw message

tools/perf/perf.h +1 -0

@@ -62,6 +62,7 @@ struct record_opts {
 	u64	     user_interval;
 	bool	     sample_transaction;
 	unsigned     initial_delay;
+	clockid_t    clockid;
 };
 
 struct option;

tools/perf/util/evsel.c +40 -4

115 lines not on this page · raw message

David Ahern

2015-03-30 01:00 UTC #9 lore raw

Quoted 11 lines · Peter Zijlstra

Subject: perf, record: Add clockid parameter
From: Peter Zijlstra <[email protected]>
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

Peter Zijlstra

author maintainer

2015-03-30 08:24 UTC #10 lore raw

Quoted 13 lines · David Ahern

Quoted 11 lines · Peter Zijlstra
Subject: perf, record: Add clockid parameter
From: Peter Zijlstra <[email protected]>
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.

Quoted 5 lines

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.

Quoted 1 line

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

tip/master should have it all.

David Ahern

2015-03-30 17:11 UTC #11 lore raw

Quoted 6 lines · Peter Zijlstra

Quoted 5 lines
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

Quoted 8 lines

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.
Quoted 1 line
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

Peter Zijlstra

author maintainer

2015-03-30 09:17 UTC #12 lore raw

In reply to Peter Zijlstra

Review

Quoted 13 lines · Peter Zijlstra

(quoted diff) +13 -0

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

tools/perf/builtin-record.c +4 -1

@@ -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,
 };

David Ahern

2015-03-30 17:17 UTC #13 lore raw

Quoted 9 lines · Peter Zijlstra

I did this on top..

tools/perf/builtin-record.c +0 -1

@@ -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),

tools/perf/builtin-record.c static const struct clockid_map clockids · line 744

Quoted 10 lines

tools/perf/builtin-record.c +4 -0

 
  	/* 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),

Peter Zijlstra

author maintainer

2015-03-30 19:33 UTC #14 lore raw

Quoted 16 lines · David Ahern

Quoted 15 lines · Peter Zijlstra

I did this on top..

tools/perf/builtin-record.c +0 -1

@@ -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),

Quoted 7 lines

tools/perf/builtin-record.c static const struct clockid_map clockids · line 744
Quoted 4 lines
tools/perf/builtin-record.c +4 -0
+	/* 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.

David Ahern

2015-03-30 19:40 UTC #15 lore raw

Quoted 21 lines · Peter Zijlstra

Quoted 13 lines

tools/perf/builtin-record.c +0 -1

@@ -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),

Quoted 7 lines

tools/perf/builtin-record.c static const struct clockid_map clockids · line 744
Quoted 4 lines
tools/perf/builtin-record.c +4 -0
+	/* 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),

Quoted 2 lines

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.

David Ahern

2015-03-30 17:24 UTC #16 lore raw

In reply to Peter Zijlstra

Quoted 38 lines · Peter Zijlstra

tools/perf/util/evsel.c +16 -1

@@ -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) {

...

Quoted 4 lines

tools/perf/util/evsel.c +0 -0

@@ -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.");

tools/perf/util/evsel.c int perf_evsel__open_strerror(struct per · line 2199

Quoted 10 lines

tools/perf/util/evsel.c +6 -0

  		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.

Peter Zijlstra

author maintainer

2015-03-30 19:33 UTC #17 lore raw

Quoted 9 lines · David Ahern

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:

Quoted 7 lines

    $ 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 :-)

David Ahern

2015-03-30 19:41 UTC #18 lore raw

Quoted 14 lines · Peter Zijlstra

Quoted 13 lines · David Ahern
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.

Quoted 6 lines

Quoted 3 lines
     --> 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?

Stephane Eranian

2015-03-30 19:43 UTC #19 lore raw

Quoted 19 lines · David Ahern

Quoted 18 lines · Peter Zijlstra
Quoted 16 lines · David Ahern
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

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
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.

Peter Zijlstra

author maintainer

2015-03-31 08:20 UTC #20 lore raw

In reply to David Ahern

Quoted 17 lines · David Ahern

Quoted 16 lines · Peter Zijlstra
Quoted 15 lines · David Ahern
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

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
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 <[email protected]>
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 [email protected]

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

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

tools/perf/Documentation/perf-record.txt +7 -0

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

tools/perf/builtin-record.c +78 -0

96 lines not on this page · raw message

tools/perf/perf.h +1 -0

@@ -62,6 +62,7 @@ struct record_opts {
 	u64	     user_interval;
 	bool	     sample_transaction;
 	unsigned     initial_delay;
+	clockid_t    clockid;
 };
 
 struct option;

tools/perf/util/evsel.c +55 -4

132 lines not on this page · raw message

tools/perf/util/header.c +3 -0

@@ -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);
 	}

Peter Zijlstra

author maintainer

2015-03-31 10:46 UTC #21 lore raw

RFC PATCH 1/1 perf tools: unify perf_event_attr printing

Quoted 2 lines · Peter Zijlstra

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

tools/perf/util/Build +1 -0

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

tools/perf/util/evsel.c +26 ~~-152~~

210 lines not on this page · raw message

tools/perf/util/header.c +12 ~~-22~~

@@ -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);
 	}

tools/perf/util/print_attr.h new file +69 -0

69 lines not on this page · raw message

tools/perf/util/print_helper.c new file +52 -0

52 lines not on this page · raw message

tools/perf/util/print_helper.h new file +7 -0

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

Peter Zijlstra

author maintainer

2015-04-01 16:26 UTC #22 lore raw

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

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

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

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

tools/perf/util/Build +1 -0

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

tools/perf/util/evsel.c +29 ~~-152~~

213 lines not on this page · raw message

tools/perf/util/header.c +12 ~~-22~~

@@ -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);
 	}

tools/perf/util/print_attr.h new file +69 -0

69 lines not on this page · raw message

tools/perf/util/print_helper.c new file +52 -0

52 lines not on this page · raw message

tools/perf/util/print_helper.h new file +7 -0

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

Jiri Olsa

2015-04-01 16:52 UTC #23 lore raw

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

Quoted 3 lines · Peter Zijlstra

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

Quoted 524 lines

---

Subject: perf, tools: Merge all perf_event_attr print functions
From: Peter Zijlstra <[email protected]>
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 [email protected]

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

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

tools/perf/util/Build +1 -0

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

tools/perf/util/evsel.c +29 ~~-152~~

@@ -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);

tools/perf/util/header.c +12 ~~-22~~

@@ -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);
 	}

tools/perf/util/print_attr.h new file +69 -0

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

tools/perf/util/print_helper.c new file +52 -0

@@ -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);
+}
+

tools/perf/util/print_helper.h new file +7 -0

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

Adrian Hunter

reviewer

2015-04-02 09:04 UTC #24 lore raw

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

Quoted 6 lines · Jiri Olsa

Quoted 3 lines · Peter Zijlstra
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;
}

Quoted 529 lines

jirka

Quoted 524 lines

---

Subject: perf, tools: Merge all perf_event_attr print functions
From: Peter Zijlstra <[email protected]>
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 [email protected]

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

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

tools/perf/util/Build +1 -0

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

tools/perf/util/evsel.c +29 ~~-152~~

@@ -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);

tools/perf/util/header.c +12 ~~-22~~

@@ -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);
 	}

tools/perf/util/print_attr.h new file +69 -0

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

tools/perf/util/print_helper.c new file +52 -0

@@ -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);
+}
+

tools/perf/util/print_helper.h new file +7 -0

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

Peter Zijlstra

author maintainer

2015-04-02 11:59 UTC #25 lore raw

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

Quoted 2 lines · Adrian Hunter

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

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

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

tools/perf/util/evsel.c +126 ~~-158~~

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

tools/perf/util/evsel.h +6 -0

@@ -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 */

tools/perf/util/header.c +7 ~~-22~~

@@ -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);
 	}

Adrian Hunter

reviewer Acked-by

2015-04-02 12:56 UTC #26 lore raw

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

Quoted 5 lines · Peter Zijlstra

Quoted 2 lines · Adrian Hunter
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 :-)

Quoted 2 lines

Something like so then?

Looks fine. Didn't test it, but

Acked-by Adrian Hunter <[email protected]>

Quoted 487 lines

---
Subject: perf, tools: Merge all perf_event_attr print functions
From: Peter Zijlstra <[email protected]>
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 [email protected]

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

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

tools/perf/util/evsel.c +126 ~~-158~~

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

tools/perf/util/evsel.h +6 -0

@@ -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 */

tools/perf/util/header.c +7 ~~-22~~

@@ -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);
 	}

Arnaldo Carvalho de Melo

2015-04-03 16:12 UTC #27 lore raw

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

In reply to Peter Zijlstra

Em Thu, Apr 02, 2015 at 01:59:34PM +0200, Peter Zijlstra escreveu:

Quoted 7 lines · Adrian Hunter

Quoted 2 lines
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

Quoted 484 lines

---
Subject: perf, tools: Merge all perf_event_attr print functions
From: Peter Zijlstra <[email protected]>
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 [email protected]

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

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

tools/perf/util/evsel.c +126 ~~-158~~

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

tools/perf/util/evsel.h +6 -0

@@ -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 */

tools/perf/util/header.c +7 ~~-22~~

@@ -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);
 	}

Arnaldo Carvalho de Melo

2015-04-03 16:14 UTC #28 lore raw

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

Em Fri, Apr 03, 2015 at 01:11:47PM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 5 lines

Em Thu, Apr 02, 2015 at 01:59:34PM +0200, Peter Zijlstra escreveu:
Quoted 4 lines · Adrian Hunter
Quoted 2 lines
But personally I think the "include" approach is too ugly. I would just add a function instead. Something like:

Quoted 7 lines

Quoted 3 lines
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

Ingo Molnar

Acked-by

2015-04-02 08:14 UTC #29 lore raw

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

In reply to Peter Zijlstra

Quoted 92 lines · 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 <[email protected]>
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 [email protected]

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Quoted 8 lines

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

Thanks,

Ingo

Arnaldo Carvalho de Melo

2015-04-02 22:29 UTC #30 lore raw

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

Em Thu, Apr 02, 2015 at 10:12:50AM +0200, Ingo Molnar escreveu:

Quoted 5 lines · Peter Zijlstra

Quoted 2 lines
 tools/perf/util/print_helper.h |    7 +
 6 files changed, 170 insertions(+), 174 deletions(-)
Acked-by Ingo Molnar <[email protected]>

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

- Arnaldo

Jiri Olsa

Acked-by

2015-04-02 09:20 UTC #31 lore raw

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

In reply to Peter Zijlstra

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

SNIP

Quoted 8 lines

tools/perf/util/evsel.c +0 -8

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

tools/perf/util/evsel.c static size_t perf_event_attr__fprintf(s · line 1027

Quoted 10 lines

tools/perf/util/evsel.c +6 -4

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

jirka

David Ahern

2015-03-30 17:33 UTC #32 lore raw

In reply to Peter Zijlstra

Review void perf_evsel__config(struct perf_evse · line 770

Quoted 10 lines · Peter Zijlstra

(quoted diff) +5 -0

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

tools/perf/util/evsel.c +2 -1

index 1abf6919b8a2..27679ab38511 100644
@@ -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:

tools/perf/util/header.c +1 -0

index fb432153e2aa..40bc8d010fcb 100644
@@ -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);
    }

Peter Zijlstra

author maintainer

2015-03-30 19:34 UTC #33 lore raw

Quoted 22 lines · David Ahern

Review void perf_evsel__config(struct perf_evse · line 770
Quoted 10 lines · Peter Zijlstra
(quoted diff) +5 -0
@@ -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.
tools/perf/util/evsel.c +0 -0
index 1abf6919b8a2..27679ab38511 100644

tools/perf/util/evsel.c void perf_evsel__config(struct perf_evsel *evsel, struct · line 772

Quoted 8 lines

tools/perf/util/evsel.c +2 -1

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

David Ahern

2015-03-30 19:46 UTC #34 lore raw

Quoted 8 lines · Peter Zijlstra

Quoted 8 lines

tools/perf/util/evsel.c +0 -0

index 1abf6919b8a2..27679ab38511 100644
@@ -766,7 +766,8 @@ void perf_evsel__config(struct perf_evsel *evsel, struct
 record_opts *opts)
     if (opts->clockid >= 0) {
         attr->use_clockid = 1;

Quoted 9 lines

tools/perf/util/evsel.c void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts) · line 771
Quoted 5 lines
tools/perf/util/evsel.c +2 -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

Stephane Eranian

2015-03-27 23:07 UTC #35 lore raw

In reply to Arnaldo Carvalho de Melo

Quoted 9 lines · Arnaldo Carvalho de Melo

Em Fri, Mar 27, 2015 at 11:35:25AM -0600, David Ahern escreveu:
Quoted 7 lines · Peter Zijlstra
Quoted 6 lines
Quoted 2 lines
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.

Quoted 9 lines

Quoted 4 lines
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?

Stephane Eranian

2015-03-27 16:31 UTC #36 lore raw

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

In reply to a message not in the archive

Quoted 264 lines · tip-bot for Peter Zijlstra

Commit-ID:  34f439278cef7b1177f8ce24f9fc81dfc6221d3b
Gitweb:     http://git.kernel.org/tip/34f439278cef7b1177f8ce24f9fc81dfc6221d3b
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Fri, 20 Feb 2015 14:05:38 +0100
Committer:  Ingo Molnar <[email protected]>
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) <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Arnaldo Carvalho de Melo <[email protected]>

Cc David Ahern <[email protected]>

Cc Jiri Olsa <[email protected]>

Cc John Stultz <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Stephane Eranian <[email protected]>

Cc Thomas Gleixner <[email protected]>

Signed-off-by Ingo Molnar <[email protected]>

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

arch/x86/kernel/cpu/perf_event.c +12 -2

index ac41b3a..0420ebc 100644
@@ -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);
 }

include/linux/perf_event.h +2 -0

index b16eac5..4015540 100644
@@ -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;

include/uapi/linux/perf_event.h +3 -3

index 1e3cd07..3bb40dda 100644
@@ -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:

kernel/events/core.c +43 -3

index bb1a7c3..c40c2ca 100644
@@ -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;

kernel/events/core.c static void mutex_lock_double(struct mutex *a, struct mutex *b) · line 7506

Quoted 10 lines

kernel/events/core.c +10 -0

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

Peter Zijlstra

author maintainer

2015-03-27 16:36 UTC #37 lore raw

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

Quoted 21 lines · Stephane Eranian

On Fri, Mar 27, 2015 at 4:48 AM, tip-bot for Peter Zijlstra

Quoted 20 lines

(quoted diff) +20 -0

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

Quoted 8 lines

Review
Quoted 7 lines
(quoted diff) +7 -0
+               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 :-)

Stephane Eranian

2015-03-27 16:55 UTC #38 lore raw

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

Quoted 34 lines · Peter Zijlstra

Quoted 29 lines · Stephane Eranian

On Fri, Mar 27, 2015 at 4:48 AM, tip-bot for Peter Zijlstra

Quoted 17 lines

(quoted diff) +17 -0

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

Review

Quoted 10 lines

(quoted diff) +10 -0

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

(quoted diff) +2 -0

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

Quoted 1 line

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.

Peter Zijlstra

author maintainer

2015-03-27 16:57 UTC #39 lore raw

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

Quoted 3 lines · Stephane Eranian

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.

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 2 lines

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

I did indeed post a patch to do that.

Stephane Eranian

2015-03-27 17:00 UTC #40 lore raw

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

Quoted 16 lines · Peter Zijlstra

Quoted 3 lines · Stephane Eranian
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.
Quoted 2 lines
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.
Quoted 3 lines
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

Quoted 8 lines

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.
Quoted 2 lines
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.