Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1303937 > unrolled thread

Re: [RFC] perf record: missing buildid for callstack modules

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC] perf record: missing buildid for callstack modules Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-01-07 23:00 +0100
    Re: [RFC] perf record: missing buildid for callstack modules Stephane Eranian <eranian@google.com> - 2016-01-07 23:10 +0100
      Re: [RFC] perf record: missing buildid for callstack modules Namhyung Kim <namhyung@gmail.com> - 2016-01-08 00:30 +0100
        Re: [RFC] perf record: missing buildid for callstack modules Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-01-08 00:50 +0100
          Re: [RFC] perf record: missing buildid for callstack modules Stephane Eranian <eranian@google.com> - 2016-01-08 19:10 +0100
            Re: [RFC] perf record: missing buildid for callstack modules Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-01-08 19:20 +0100
              Re: [RFC] perf record: missing buildid for callstack modules Peter Zijlstra <peterz@infradead.org> - 2016-01-11 18:40 +0100
                Re: [RFC] perf record: missing buildid for callstack modules Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-01-11 19:30 +0100
                  Re: [RFC] perf record: missing buildid for callstack modules Stephane Eranian <eranian@google.com> - 2016-01-11 21:10 +0100
                Re: [RFC] perf record: missing buildid for callstack modules Ingo Molnar <mingo@kernel.org> - 2016-01-12 11:50 +0100
                  Re: [RFC] perf record: missing buildid for callstack modules Peter Zijlstra <peterz@infradead.org> - 2016-01-12 12:40 +0100
                    Re: [RFC] perf record: missing buildid for callstack modules Ingo Molnar <mingo@kernel.org> - 2016-01-12 13:20 +0100
                      Re: [RFC] perf record: missing buildid for callstack modules Peter Zijlstra <peterz@infradead.org> - 2016-01-12 14:50 +0100
                        Re: [RFC] perf record: missing buildid for callstack modules Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-01-12 15:40 +0100
                          Re: [RFC] perf record: missing buildid for callstack modules Peter Zijlstra <peterz@infradead.org> - 2016-01-12 16:40 +0100
                            Re: [RFC] perf record: missing buildid for callstack modules Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-01-12 16:50 +0100
                              Re: [RFC] perf record: missing buildid for callstack modules Peter Zijlstra <peterz@infradead.org> - 2016-01-12 17:20 +0100
                                Re: [RFC] perf record: missing buildid for callstack modules Peter Zijlstra <peterz@infradead.org> - 2016-01-12 17:30 +0100
                                  Re: [RFC] perf record: missing buildid for callstack modules Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-01-12 18:20 +0100
                                    Re: [RFC] perf record: missing buildid for callstack modules Ingo Molnar <mingo@kernel.org> - 2016-01-13 11:30 +0100
                                      Re: [RFC] perf record: missing buildid for callstack modules Peter Zijlstra <peterz@infradead.org> - 2016-01-13 13:50 +0100
                                        Re: [RFC] perf record: missing buildid for callstack modules Ingo Molnar <mingo@kernel.org> - 2016-01-14 12:30 +0100
                                          Re: [RFC] perf record: missing buildid for callstack modules Peter Zijlstra <peterz@infradead.org> - 2016-01-14 12:40 +0100
                                            Re: [RFC] perf record: missing buildid for callstack modules Stephane Eranian <eranian@google.com> - 2016-01-15 03:00 +0100
                                              Re: [RFC] perf record: missing buildid for callstack modules Peter Zijlstra <peterz@infradead.org> - 2016-01-15 10:40 +0100
                              Re: [RFC] perf record: missing buildid for callstack modules Stephane Eranian <eranian@google.com> - 2016-01-12 22:10 +0100
                    Re: [RFC] perf record: missing buildid for callstack modules One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> - 2016-01-12 14:10 +0100
                    Re: [RFC] perf record: missing buildid for callstack modules Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-01-12 15:40 +0100
                      Re: [RFC] perf record: missing buildid for callstack modules Peter Zijlstra <peterz@infradead.org> - 2016-01-12 16:40 +0100
                        Re: [RFC] perf record: missing buildid for callstack modules Ingo Molnar <mingo@kernel.org> - 2016-01-13 11:30 +0100
                  Re: [RFC] perf record: missing buildid for callstack modules Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-01-12 15:30 +0100
                    Re: [RFC] perf record: missing buildid for callstack modules Ingo Molnar <mingo@kernel.org> - 2016-01-13 11:00 +0100
                      Re: [RFC] perf record: missing buildid for callstack modules Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-01-13 16:30 +0100
            Re: [RFC] perf record: missing buildid for callstack modules Namhyung Kim <namhyung@kernel.org> - 2016-01-09 11:40 +0100
              Re: [RFC] perf record: missing buildid for callstack modules Adrian Hunter <adrian.hunter@intel.com> - 2016-01-11 10:40 +0100
                Re: [RFC] perf record: missing buildid for callstack modules Namhyung Kim <namhyung@kernel.org> - 2016-01-11 12:10 +0100
                  Re: [RFC] perf record: missing buildid for callstack modules Adrian Hunter <adrian.hunter@intel.com> - 2016-01-11 13:00 +0100

Page 2 of 2 — ← Prev page 1 [2]

#1308372

On Wed, Jan 13, 2016 at 11:21:07AM +0100, Ingo Molnar wrote:
> 
> * Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> 
> > > And this would (obviously) be a good time to see what else we'd like to
> > > stuff in there.
> > 
> > Perhaps a version number? Else we'll be using those reserved bits again
> > when we decide to introduce MMAP4 ;-\
> 
> No version numbers please. Cannot we have a size field and be done with it? The 
> size is the 'version', if we only ever expand the record. (which is the typical 
> case)

The current problem with that is that we use the 'remaining' size as
the length field for a string (with the PERF_RECORD_MMAP* records).

We could of course fix that no problem.


---
 include/uapi/linux/perf_event.h | 27 ++++++++++++++++++++++++++-
 kernel/events/core.c            | 35 ++++++++++++++++++++++++-----------
 2 files changed, 50 insertions(+), 12 deletions(-)

diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
index 1afe9623c1a7..b7b673387581 100644
--- a/include/uapi/linux/perf_event.h
+++ b/include/uapi/linux/perf_event.h
@@ -340,7 +340,8 @@ struct perf_event_attr {
 				comm_exec      :  1, /* flag comm events that are due to an exec */
 				use_clockid    :  1, /* use @clockid for time fields */
 				context_switch :  1, /* context switch data */
-				__reserved_1   : 37;
+				mmap3          :  1, /* include mmap with mtime */
+				__reserved_1   : 36;
 
 	union {
 		__u32		wakeup_events;	  /* wakeup every n events */
@@ -856,6 +857,30 @@ enum perf_event_type {
 	 */
 	PERF_RECORD_SWITCH_CPU_WIDE		= 15,
 
+	/*
+	 * The MMAP3 records are an augmented version of MMAP2, they add
+	 * mtime and filename_len, allowing for size based extensions.
+	 *
+	 * struct {
+	 *	struct perf_event_header	header;
+	 *
+	 *	u32				pid, tid;
+	 *	u64				addr;
+	 *	u64				len;
+	 *	u64				pgoff;
+	 *	u32				maj;
+	 *	u32				min;
+	 *	u64				ino;
+	 *	u64				ino_generation;
+	 *	u32				prot, flags;
+	 *	u64				mtime;
+	 *	u32				filename_len;
+	 *	char				filename[2+];
+	 *	struct sample_id		sample_id;
+	 * }
+	 */
+	PERF_RECORD_MMAP3			= 16,
+
 	PERF_RECORD_MAX,			/* non-ABI */
 };
 
diff --git a/kernel/events/core.c b/kernel/events/core.c
index bf8244190d0f..1cf15793f96c 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -5661,7 +5661,7 @@ perf_event_aux(perf_event_aux_output_cb output, void *data,
 /*
  * task tracking -- fork/exit
  *
- * enabled by: attr.comm | attr.mmap | attr.mmap2 | attr.mmap_data | attr.task
+ * enabled by: attr.comm | attr.mmap | attr.mmap2 | attr.mmap3 | attr.mmap_data | attr.task
  */
 
 struct perf_task_event {
@@ -5682,8 +5682,8 @@ struct perf_task_event {
 static int perf_event_task_match(struct perf_event *event)
 {
 	return event->attr.comm  || event->attr.mmap ||
-	       event->attr.mmap2 || event->attr.mmap_data ||
-	       event->attr.task;
+	       event->attr.mmap2 || event->attr.mmap3 ||
+	       event->attr.mmap_data || event->attr.task;
 }
 
 static void perf_event_task_output(struct perf_event *event,
@@ -5867,11 +5867,12 @@ struct perf_mmap_event {
 	struct vm_area_struct	*vma;
 
 	const char		*file_name;
-	int			file_size;
+	u32			file_size;
 	int			maj, min;
 	u64			ino;
 	u64			ino_generation;
 	u32			prot, flags;
+	u64			mtime;
 
 	struct {
 		struct perf_event_header	header;
@@ -5892,11 +5893,10 @@ static int perf_event_mmap_match(struct perf_event *event,
 	int executable = vma->vm_flags & VM_EXEC;
 
 	return (!executable && event->attr.mmap_data) ||
-	       (executable && (event->attr.mmap || event->attr.mmap2));
+	       (executable && (event->attr.mmap || event->attr.mmap2 || event->attr.mmap3));
 }
 
-static void perf_event_mmap_output(struct perf_event *event,
-				   void *data)
+static void perf_event_mmap_output(struct perf_event *event, void *data)
 {
 	struct perf_mmap_event *mmap_event = data;
 	struct perf_output_handle handle;
@@ -5907,7 +5907,7 @@ static void perf_event_mmap_output(struct perf_event *event,
 	if (!perf_event_mmap_match(event, data))
 		return;
 
-	if (event->attr.mmap2) {
+	if (event->attr.mmap2 || event->attr.mmap3) {
 		mmap_event->event_id.header.type = PERF_RECORD_MMAP2;
 		mmap_event->event_id.header.size += sizeof(mmap_event->maj);
 		mmap_event->event_id.header.size += sizeof(mmap_event->min);
@@ -5916,6 +5916,11 @@ static void perf_event_mmap_output(struct perf_event *event,
 		mmap_event->event_id.header.size += sizeof(mmap_event->prot);
 		mmap_event->event_id.header.size += sizeof(mmap_event->flags);
 	}
+	if (event->attr.mmap3) {
+		mmap_event->event_id.header.type = PERF_RECORD_MMAP3;
+		mmap_event->event_id.header.size += sizeof(mmap_event->mtime);
+		mmap_event->event_id.header.size += sizeof(mmap_event->file_size);
+	}
 
 	perf_event_header__init_id(&mmap_event->event_id.header, &sample, event);
 	ret = perf_output_begin(&handle, event,
@@ -5928,7 +5933,7 @@ static void perf_event_mmap_output(struct perf_event *event,
 
 	perf_output_put(&handle, mmap_event->event_id);
 
-	if (event->attr.mmap2) {
+	if (event->attr.mmap2 || event->attr.mmap3) {
 		perf_output_put(&handle, mmap_event->maj);
 		perf_output_put(&handle, mmap_event->min);
 		perf_output_put(&handle, mmap_event->ino);
@@ -5936,6 +5941,10 @@ static void perf_event_mmap_output(struct perf_event *event,
 		perf_output_put(&handle, mmap_event->prot);
 		perf_output_put(&handle, mmap_event->flags);
 	}
+	if (event->attr.mmap3) {
+		perf_output_put(&handle, mmap_event->mtime);
+		perf_output_put(&handle, mmap_event->file_size);
+	}
 
 	__output_copy(&handle, mmap_event->file_name,
 				   mmap_event->file_size);
@@ -5954,8 +5963,9 @@ static void perf_event_mmap_event(struct perf_mmap_event *mmap_event)
 	int maj = 0, min = 0;
 	u64 ino = 0, gen = 0;
 	u32 prot = 0, flags = 0;
+	u64 mtime = 0;
 	unsigned int size;
-	char tmp[16];
+	char tmp[18];
 	char *buf = NULL;
 	char *name;
 
@@ -5984,6 +5994,7 @@ static void perf_event_mmap_event(struct perf_mmap_event *mmap_event)
 		gen = inode->i_generation;
 		maj = MAJOR(dev);
 		min = MINOR(dev);
+		mtime = timespec_to_ns(&inode->i_mtime);
 
 		if (vma->vm_flags & VM_READ)
 			prot |= PROT_READ;
@@ -6043,7 +6054,7 @@ static void perf_event_mmap_event(struct perf_mmap_event *mmap_event)
 	 * zero'd out to avoid leaking random bits to userspace.
 	 */
 	size = strlen(name)+1;
-	while (!IS_ALIGNED(size, sizeof(u64)))
+	while (!IS_ALIGNED(2+size, sizeof(u64)))
 		name[size++] = '\0';
 
 	mmap_event->file_name = name;
@@ -6054,6 +6065,7 @@ static void perf_event_mmap_event(struct perf_mmap_event *mmap_event)
 	mmap_event->ino_generation = gen;
 	mmap_event->prot = prot;
 	mmap_event->flags = flags;
+	mmap_event->mtime = mtime;
 
 	if (!(vma->vm_flags & VM_EXEC))
 		mmap_event->event_id.header.misc |= PERF_RECORD_MISC_MMAP_DATA;
@@ -6096,6 +6108,7 @@ void perf_event_mmap(struct vm_area_struct *vma)
 		/* .ino_generation (attr_mmap2 only) */
 		/* .prot (attr_mmap2 only) */
 		/* .flags (attr_mmap2 only) */
+		/* .mtime (attr_mmap3 only) */
 	};
 
 	perf_event_mmap_event(&mmap_event);

[toc] | [prev] | [next] | [standalone]

#1309192

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

> The current problem with that is that we use the 'remaining' size as
> the length field for a string (with the PERF_RECORD_MMAP* records).
> 
> We could of course fix that no problem.
> 
> 
> ---
>  include/uapi/linux/perf_event.h | 27 ++++++++++++++++++++++++++-
>  kernel/events/core.c            | 35 ++++++++++++++++++++++++-----------
>  2 files changed, 50 insertions(+), 12 deletions(-)
> 
> diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
> index 1afe9623c1a7..b7b673387581 100644
> --- a/include/uapi/linux/perf_event.h
> +++ b/include/uapi/linux/perf_event.h
> @@ -340,7 +340,8 @@ struct perf_event_attr {
>  				comm_exec      :  1, /* flag comm events that are due to an exec */
>  				use_clockid    :  1, /* use @clockid for time fields */
>  				context_switch :  1, /* context switch data */
> -				__reserved_1   : 37;
> +				mmap3          :  1, /* include mmap with mtime */
> +				__reserved_1   : 36;
>  
>  	union {
>  		__u32		wakeup_events;	  /* wakeup every n events */
> @@ -856,6 +857,30 @@ enum perf_event_type {
>  	 */
>  	PERF_RECORD_SWITCH_CPU_WIDE		= 15,
>  
> +	/*
> +	 * The MMAP3 records are an augmented version of MMAP2, they add
> +	 * mtime and filename_len, allowing for size based extensions.
> +	 *
> +	 * struct {
> +	 *	struct perf_event_header	header;
> +	 *
> +	 *	u32				pid, tid;
> +	 *	u64				addr;
> +	 *	u64				len;
> +	 *	u64				pgoff;
> +	 *	u32				maj;
> +	 *	u32				min;
> +	 *	u64				ino;
> +	 *	u64				ino_generation;
> +	 *	u32				prot, flags;
> +	 *	u64				mtime;
> +	 *	u32				filename_len;
> +	 *	char				filename[2+];
> +	 *	struct sample_id		sample_id;
> +	 * }
> +	 */
> +	PERF_RECORD_MMAP3			= 16,
> +
>  	PERF_RECORD_MAX,			/* non-ABI */
>  };

Yeah, very nice!

And this means v3 should be the last ever version - all future extensions can 
happen via the length field.

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

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1309195

On Thu, Jan 14, 2016 at 12:27:34PM +0100, Ingo Molnar wrote:
> > +	 *	u32				filename_len;
> > +	 *	char				filename[2+];

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

except of course that sizeof(u32) == 4 :/

[toc] | [prev] | [next] | [standalone]

#1309807

Peter,

On Thu, Jan 14, 2016 at 3:36 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Thu, Jan 14, 2016 at 12:27:34PM +0100, Ingo Molnar wrote:
>> > +    *      u32                             filename_len;
>> > +    *      char                            filename[2+];
>
>> Acked-by: Ingo Molnar <mingo@kernel.org>
>
> except of course that sizeof(u32) == 4 :/
There is no padding here. Are you concerned about running out of bits
in filename_len?
Any extension possible because header.size - sizeof(mmap3) -
filename_len sizing what's after filename, right?

[toc] | [prev] | [next] | [standalone]

#1309996

On Thu, Jan 14, 2016 at 05:59:48PM -0800, Stephane Eranian wrote:
> Peter,
> 
> On Thu, Jan 14, 2016 at 3:36 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Thu, Jan 14, 2016 at 12:27:34PM +0100, Ingo Molnar wrote:
> >> > +    *      u32                             filename_len;
> >> > +    *      char                            filename[2+];
> >
> >> Acked-by: Ingo Molnar <mingo@kernel.org>
> >
> > except of course that sizeof(u32) == 4 :/

> There is no padding here. Are you concerned about running out of bits
> in filename_len?

No, I just made a mess of it :-)

filename_len should have been u16 and filename should then be 6+8n in
size.

> Any extension possible because header.size - sizeof(mmap3) -
> filename_len sizing what's after filename, right?

Right, current MMAP records use the remaining size as the filename
length, but by explicitly specifying that we can add optional fields.

These fields must be after filename_len, otherwise you'd not be able to
find filename_len and you could not compute the extra size. And given
alignment constraints it makes sense to do it after filename[].

So suppose we wanted to also add atime and ctime, we could do.

  PERF_RECORD_MMAP3 {
    ...
    u16 filename_len;
    char filename[6+8n];

    if (extra_size >= 16) {
      u64 stime;
      u64 ctime;
    };
  }

or something like that.

[toc] | [prev] | [next] | [standalone]

#1307831

On Tue, Jan 12, 2016 at 7:48 AM, Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
> Em Tue, Jan 12, 2016 at 04:34:40PM +0100, Peter Zijlstra escreveu:
>> On Tue, Jan 12, 2016 at 11:38:05AM -0300, Arnaldo Carvalho de Melo wrote:
>> > > Also, just parsing the gigabytes of data that comes out of perf-record
>> > > takes significant time, let alone poking around the filesystem and
>> >
>> > Right, that is what we would elliminate with stashing the content-based
>> > cookie into a PERF_RECORD_MMAP3 record.
>>
>> Again, how would you go about getting that cookie for a DSO? The whole
>> kernel isn't involved with dlopen(), all it sees is a mmap(PROT_EXEC).
>>
>> > BTW, mtime would incur in postprocessing it all.
>>
>> mtime can still warn you if things are non-matching at report time
>> without this post-processing, and thereby solves the problem of staring
>> at broken/wrong data.
>
> How will we collect the mtime for the DSOs in PERF_RECORD_MMAP records
> if we don't look at those records? What mtime are you talking about?
>
I think the post-processing of MMAP could be sped up if they were
saved out-of-band
inside of with the samples. Isn't that what the auxbuffer allows you to do?

>> It doesn't get you right data, but knowing your data is broken allows
>> you to manually do things 'right'.

[toc] | [prev] | [next] | [standalone]

#1307388

> And we cannot, at mmap() time, 'assume' the file is ELF and try prodding
> into it to find a buildid or whatnot.

And you cannot at any time assume that something like NFS won't change
the page contents under you. In fact it's valid (but completely insane)
to use MAP_DISCARD to throw away part of your program and page it back in
again so that a daemon on the other side of the NFS can change the code
you are executing 8)

Alan

[toc] | [prev] | [next] | [standalone]

#1307483

Em Tue, Jan 12, 2016 at 12:35:21PM +0100, Peter Zijlstra escreveu:
> On Tue, Jan 12, 2016 at 11:39:43AM +0100, Ingo Molnar wrote:
> > > Does the kernel even know about the buildid crap? AFAIK the binfmt stuff doesn't 
> > > know or care about things like that. Heck, we support binfmts that do not even 
> > > have a buildid.
> > 
> > The kernel's exec() code does not care about the past, it will execute whatever is 
> > fit to execute right now.
> > 
> > But perf tooling cares very much: it can lead to subtle bugs and bad data if we 
> > display a profile with the wrong DSO or binary. 'Bad' profiles resulting out of 
> > binary mismatch can be very convincing and can send developers down the wrong path 
> > for hours. I'd expect my tooling to not do that.
> 
> Well, it really is rather a rare case, replacing binaries you're
> profiling. Sure, if it happens (by accident or otherwise) it can be a
> pain, but the cost of fixing this 'problem' is huge.

Humm, for most things today, i.e. ELF, most distros (all? maybe this is
a gcc switch that is default on, haven't checked) come with such
pre-computed cookie its just a way for efficiently passing it to the
tooling via a new record. With that, no post processing, etc. But then
someone would need to prototype this...

[acme@zoo linux]$ perf record -h build

 Usage: perf record [<options>] [<command>]
    or: perf record [<options>] -- <command> [<options>]

    -B, --no-buildid      do not collect buildids in perf.data
    -N, --no-buildid-cache
                          do not update the buildid cache

[acme@zoo linux]$

Have you ever played, when you noticed those overheads, with -N? Or just
used the -B big hammer and moved on?

- Arnaldo
 
> > Path names alone (the thing that exec() cares about) are not unique enough to 
> > identify the binary that was profiled. So we need a content hash - hence the 
> > build-ID.
> > 
> > Can you suggest a better solution than a build-time calculated content hash?
> 
> Not really, but the current 'solution' is a massive pain. The result is
> that perf-record needs to do a full scan of the recorded data after
> completion and look for buildids across the system.
> 
> On my system that pass takes longer than the actual workload (of
> building a kernel). Furthermore, the resulting data is useless for me.
> 
> > As for binary formats that suck and don't allow for a content hash: we do our 
> > best, but of course the risk of data mismatch is there. We could perhaps cache the 
> > binary inode's mtime field to at least produce a 'profile data is older than 
> > binary/DSO modification date!' warning. (Which check won't catch all cases, like 
> > cross-system profiling data matches.)
> 
> So my problem with the kernel side thing is that I fear it will, again,
> be a partial solution, and we'll still end up scanning the perf-record
> output, ie. nothing better than we are now.
> 
> Sure, maybe we can have binfmt_elf read the buildid and cache it
> someplace, maybe we can even have the other binfmt thingies do something
> similar (at small cost, we obviously cannot compute hashes over files at
> exec() time, that would upset people).
> 
> But what do we do for DSOs, does dlopen() ever end up in the binfmt
> code? I would think not, I would fully expect the dynamic linker to just
> mmap() the relevant bits and be done with it.
> 
> And we cannot, at mmap() time, 'assume' the file is ELF and try prodding
> into it to find a buildid or whatnot.
> 
> And all for some weird corner case.
> 
> ~ Peter, who thinks buildid stuff stinks.

[toc] | [prev] | [next] | [standalone]

#1307554

On Tue, Jan 12, 2016 at 11:34:54AM -0300, Arnaldo Carvalho de Melo wrote:
> Have you ever played, when you noticed those overheads, with -N? Or just
> used the -B big hammer and moved on?

I yelled on irc, jolsa told me to use -B, I moved on ;-)

Not sure -N really buys me anything, its still slower and I really don't
need any of this.

[toc] | [prev] | [next] | [standalone]

#1308265

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

> On Tue, Jan 12, 2016 at 11:34:54AM -0300, Arnaldo Carvalho de Melo wrote:
>
> > Have you ever played, when you noticed those overheads, with -N? Or just used 
> > the -B big hammer and moved on?
> 
> I yelled on irc, jolsa told me to use -B, I moved on ;-)

So I think we should grow mtime protection, to not display incorrect data - and 
then that should become the default. (in addition to per CPU recording threads.)

Way too slow recording is something I experience as well - and to have a slow 
_performance_ profiling tool is pretty ironic! ;-)

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1307473

Em Tue, Jan 12, 2016 at 11:39:43AM +0100, Ingo Molnar escreveu:
> But perf tooling cares very much: it can lead to subtle bugs and bad data if we 
> display a profile with the wrong DSO or binary. 'Bad' profiles resulting out of 
> binary mismatch can be very convincing and can send developers down the wrong path 
> for hours. I'd expect my tooling to not do that.
 
> Path names alone (the thing that exec() cares about) are not unique enough to 
> identify the binary that was profiled. So we need a content hash - hence the 
> build-ID.
 
> Can you suggest a better solution than a build-time calculated content hash?
 
> As for binary formats that suck and don't allow for a content hash: we do our 
> best, but of course the risk of data mismatch is there. We could perhaps cache the 
> binary inode's mtime field to at least produce a 'profile data is older than 
> binary/DSO modification date!' warning. (Which check won't catch all cases, like 
> cross-system profiling data matches.)

So, we could think of this as: binary formats that want to aid
observability tools to:

1) Detect mismatches in contents for DSOs present at recording time to
   those to be used at analysis time.

2) Find symtabs, DSO binary contents, CFI tables, present in the DSO
   where samples were taken.

Using mtime, as suggested in other messages will help with #1, but not
with #2.

Checking for inefficiencies in the current approach of
right-after-recording post-processing looking for PERF_RECORD_MMAPs,
Adrian suggested something here, also disabling the saving into
~/.debug/ will help, collecting numbers would be great.

But the mtime thing also requires traversing the whole perf.data
contents looking for those paths in PERF_RECORD_MMAP records.

- Arnaldo

[toc] | [prev] | [next] | [standalone]

#1308236

* Arnaldo Carvalho de Melo <acme@kernel.org> wrote:

> Em Tue, Jan 12, 2016 at 11:39:43AM +0100, Ingo Molnar escreveu:
> > But perf tooling cares very much: it can lead to subtle bugs and bad data if we 
> > display a profile with the wrong DSO or binary. 'Bad' profiles resulting out of 
> > binary mismatch can be very convincing and can send developers down the wrong path 
> > for hours. I'd expect my tooling to not do that.
>  
> > Path names alone (the thing that exec() cares about) are not unique enough to 
> > identify the binary that was profiled. So we need a content hash - hence the 
> > build-ID.
>  
> > Can you suggest a better solution than a build-time calculated content hash?
>  
> > As for binary formats that suck and don't allow for a content hash: we do our 
> > best, but of course the risk of data mismatch is there. We could perhaps cache the 
> > binary inode's mtime field to at least produce a 'profile data is older than 
> > binary/DSO modification date!' warning. (Which check won't catch all cases, like 
> > cross-system profiling data matches.)
> 
> So, we could think of this as: binary formats that want to aid
> observability tools to:
> 
> 1) Detect mismatches in contents for DSOs present at recording time to
>    those to be used at analysis time.
> 
> 2) Find symtabs, DSO binary contents, CFI tables, present in the DSO
>    where samples were taken.
> 
> Using mtime, as suggested in other messages will help with #1, but not
> with #2.

But but ... why is #2 a problem with mtime? If we have an out of date record in 
the perf.data, then the perf.data is uninteresting in 99% of the usecases! It's 
out of date, most likely because the binary the developer is working on got 
rebuilt, or the system got upgraded - in both cases the developer does not care 
about the old records anymore...

What matters is #1, to detect mismatches, to be a reliable tool. Once we've 
detected that, we can inform the user and our job is mostly done.

But reliable != perfect time machine. Really, #2 is a second, third order concern 
that should never cause slowdowns on the magnitude that Peter is complaining 
about!

I realize that there might be special workflows (such as system-wide monitoring) 
where collecting at recording time might be useful, but those are not the common 
case - and they should not slow down the common case.

> Checking for inefficiencies in the current approach of
> right-after-recording post-processing looking for PERF_RECORD_MMAPs,
> Adrian suggested something here, also disabling the saving into
> ~/.debug/ will help, collecting numbers would be great.

I think Peter mentioned a number: the kernel build time almost _doubles_ with 
this. That's clearly unacceptable.

> But the mtime thing also requires traversing the whole perf.data
> contents looking for those paths in PERF_RECORD_MMAP records.

But why? Why cannot we do it at perf report time, when we will parse them anyway?

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1308507

Em Wed, Jan 13, 2016 at 10:57:38AM +0100, Ingo Molnar escreveu:
> * Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> > Em Tue, Jan 12, 2016 at 11:39:43AM +0100, Ingo Molnar escreveu:
> > > But perf tooling cares very much: it can lead to subtle bugs and bad data if we 
> > > display a profile with the wrong DSO or binary. 'Bad' profiles resulting out of 
> > > binary mismatch can be very convincing and can send developers down the wrong path 
> > > for hours. I'd expect my tooling to not do that.
 
> > > Path names alone (the thing that exec() cares about) are not unique enough to 
> > > identify the binary that was profiled. So we need a content hash - hence the 
> > > build-ID.
 
> > > Can you suggest a better solution than a build-time calculated content hash?
  
> > > As for binary formats that suck and don't allow for a content hash: we do our 
> > > best, but of course the risk of data mismatch is there. We could perhaps cache the 
> > > binary inode's mtime field to at least produce a 'profile data is older than 
> > > binary/DSO modification date!' warning. (Which check won't catch all cases, like 
> > > cross-system profiling data matches.)

> > So, we could think of this as: binary formats that want to aid
> > observability tools to:

> > 1) Detect mismatches in contents for DSOs present at recording time to
> >    those to be used at analysis time.

> > 2) Find symtabs, DSO binary contents, CFI tables, present in the DSO
> >    where samples were taken.

> > Using mtime, as suggested in other messages will help with #1, but not
> > with #2.
> 
> But but ... why is #2 a problem with mtime? If we have an out of date record in 
> the perf.data, then the perf.data is uninteresting in 99% of the usecases! It's 
> out of date, most likely because the binary the developer is working on got 
> rebuilt, or the system got upgraded - in both cases the developer does not care 
> about the old records anymore...

Oh, with mtime we'll be able to, with some effort, most of the time,
find the right symtab/DSO contents, it is not as good as the
content-based build-id, but it improves the current situation, for, to
reuse an euphemism, 99% of the cases ;-)

It is just a pity that what would arguably be the last step to make
content-based DSO identifiers a first class citizen, already available
mostly everywhere, i.e. in ELF binaries will have to wait a bit more.

With PeterZ's change to make the filename length part of the
PERF_RECORD_MMAP3 we at least leave the door open to including that
cookie without having to introduce PERF_RECORD_MMAP4 :-)

> What matters is #1, to detect mismatches, to be a reliable tool. Once we've
> detected that, we can inform the user and our job is mostly done.

Well, we could tell the user to install the package with that symtab, in fedora
it is called 'foo-debuginfo' and can be installed via, for instance (example
taken from a gdb post somewhere):

  [root@zoo ~]# dnf --enablerepo='*debug*' install /usr/lib/debug/.build-id/3d/f5385c6be529423a8ae3dd39a3deb9425201cc
  <SNIP>
  Using metadata from Wed Jan 13 12:12:16 2016 (0:02:56 hours old)
  Dependencies resolved.
  ==================================================================
   Package            Arch    Version      Repository          Size
  ==================================================================
  Installing:
   glibc-debuginfo    x86_64  2.20-8.fc21  updates-debuginfo  9.2 M

  Transaction Summary
  ==================================================================
  Install  1 Package

  Total download size: 9.2 M
  Installed size: 58 M
  Is this ok [y/N]:

-----------------------------------

I.e. infrastructure is in place to get this "time machine" you mention below, is somewhat
in place to get a symtab for a DSO, be it the latest version of some versions ago.

Which could be useful to understand the behaviour of code in production while an
update can't be applied (not vetted by powers that be, whatever reason).
 
> But reliable != perfect time machine. Really, #2 is a second, third order concern 
> that should never cause slowdowns on the magnitude that Peter is complaining 
> about!

Sure, mtime will improve peterz's usecase, no question about it.
 
> I realize that there might be special workflows (such as system-wide monitoring) 
> where collecting at recording time might be useful, but those are not the common 
> case - and they should not slow down the common case.

Sure, no question about this.
 
> > Checking for inefficiencies in the current approach of
> > right-after-recording post-processing looking for PERF_RECORD_MMAPs,
> > Adrian suggested something here, also disabling the saving into
> > ~/.debug/ will help, collecting numbers would be great.
> 
> I think Peter mentioned a number: the kernel build time almost _doubles_ with 
> this. That's clearly unacceptable.

And for him, most of the time, not a problem, in fact we could argue that he
has not a problem, Stephane seems to have ;-)
 
> > But the mtime thing also requires traversing the whole perf.data
> > contents looking for those paths in PERF_RECORD_MMAP records.
 
> But why? Why cannot we do it at perf report time, when we will parse them anyway?

Hey, I was talking only about 'record' time.

And at record, we don'have to traverse the whole perf.data contents as soon as
we add a new PERF_RECORD_MMAP3, just not with the initial motivation for it (a
content-based cookie), but instead the DSOs's mtime :-)

- Arnaldo

[toc] | [prev] | [next] | [standalone]

#1305209

Hi Stephane,

On Fri, Jan 08, 2016 at 10:01:24AM -0800, Stephane Eranian wrote:
> On Thu, Jan 7, 2016 at 3:47 PM, Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> > Em Fri, Jan 08, 2016 at 07:47:03AM +0900, Namhyung Kim escreveu:
> >> On January 8, 2016 7:00:35 AM GMT+09:00, Stephane Eranian <eranian@google.com> wrote:
> >> >On Thu, Jan 7, 2016 at 1:59 PM, Arnaldo Carvalho de Melo
> >> ><acme@kernel.org> wrote:
> >> >> Em Thu, Jan 07, 2016 at 01:56:14PM -0800, Stephane Eranian escreveu:
> >> >>> Hi,
> >> >>>
> >> >>> Whenever you do:
> >> >>>
> >> >>>     $ perf record -g -a sleep 10
> >> >>>
> >> >>> Perf will collect the callstack for each sample. At the end of the
> >> >>> run, perf record
> >> >>> adds the buildid for all dso with at least one sample. But when it
> >> >does this, it
> >> >>> only looks at the sampled IP and ignore the modules traversed by the
> >> >callstack.
> >> >>> That means that, it is not possible to uniquely identify the modules
> >> >executed,
> >> >>> unless they had at least one IP sample captured. But this is not
> >> >>> always the case.
> >> >>>
> >> >>> How about providing an option to perf record to force collecting
> >> >>> buildid for all IPs
> >> >>> captured in the callstack? I understand that would cost more at the
> >> >end of the
> >> >>> collection, but this would be beneficial to several monitoring
> >> >scenarios.
> >> >>
> >> >> I agree, would consider applying a patch that provides the option but
> >> >> does not do this by default.
> >> >>
> >> >I agree, not the default.
> >>
> >> Hi Stephane,
> >>
> >> Please see
> >>
> >> https://lkml.org/lkml/2015/3/22/249
> >
> >
> > Oops, Stephane, please try this, so that we can finally merge it :-\
> >
> I will try it today. However, I am a bit worried about the performance
> impact. Unless I am missing something in this approach we may end up
> looking up N times the same module if it appears in N callstacks. In
> Andi's suggested approach, there would be only one pass at the beginning
> (or the end of the run). But you could miss some modules if they are gone
> by the time you run the pass.

How about this then?

Adrian, is it ok to skip process_buildids() for the auxtrace?

Thanks,
Namhyung


diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt
index 3a1a32f5479f..fbceb631387c 100644
--- a/tools/perf/Documentation/perf-record.txt
+++ b/tools/perf/Documentation/perf-record.txt
@@ -338,6 +338,9 @@ Options passed to clang when compiling BPF scriptlets.
 Specify vmlinux path which has debuginfo.
 (enabled when BPF prologue is on)
 
+--buildid-all::
+Record build-id of all DSOs regardless whether it's actually hit or not.
+
 SEE ALSO
 --------
 linkperf:perf-stat[1], linkperf:perf-list[1]
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index dc4e0adf5c5b..ab18db3153a6 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -50,6 +50,7 @@ struct record {
 	int			realtime_prio;
 	bool			no_buildid;
 	bool			no_buildid_cache;
+	bool			buildid_all;
 	unsigned long long	samples;
 };
 
@@ -755,14 +756,10 @@ out_child:
 		file->size = lseek(perf_data_file__fd(file), 0, SEEK_CUR);
 
 		if (!rec->no_buildid) {
-			process_buildids(rec);
-			/*
-			 * We take all buildids when the file contains
-			 * AUX area tracing data because we do not decode the
-			 * trace because it would take too long.
-			 */
-			if (rec->opts.full_auxtrace)
+			if (rec->buildid_all)
 				dsos__hit_all(rec->session);
+			else
+				process_buildids(rec);
 		}
 		perf_session__write_header(rec->session, rec->evlist, fd, true);
 	}
@@ -1138,6 +1135,8 @@ struct option __record_options[] = {
 		   "options passed to clang when compiling BPF scriptlets"),
 	OPT_STRING(0, "vmlinux", &symbol_conf.vmlinux_name,
 		   "file", "vmlinux pathname"),
+	OPT_BOOLEAN(0, "buildid-all", &record.buildid_all,
+		    "Record build-id of all DSOs regardless of hits"),
 	OPT_END()
 };
 
@@ -1255,6 +1254,14 @@ int cmd_record(int argc, const char **argv, const char *prefix __maybe_unused)
 	if (err)
 		goto out_symbol_exit;
 
+	/*
+	 * We take all buildids when the file contains
+	 * AUX area tracing data because we do not decode the
+	 * trace because it would take too long.
+	 */
+	if (rec->opts.full_auxtrace)
+		rec->buildid_all = true;
+
 	if (record_opts__config(&rec->opts)) {
 		err = -EINVAL;
 		goto out_symbol_exit;

[toc] | [prev] | [next] | [standalone]

#1305977

On 09/01/16 12:31, Namhyung Kim wrote:
> Hi Stephane,
> 
> On Fri, Jan 08, 2016 at 10:01:24AM -0800, Stephane Eranian wrote:
>> On Thu, Jan 7, 2016 at 3:47 PM, Arnaldo Carvalho de Melo
>> <acme@kernel.org> wrote:
>>> Em Fri, Jan 08, 2016 at 07:47:03AM +0900, Namhyung Kim escreveu:
>>>> On January 8, 2016 7:00:35 AM GMT+09:00, Stephane Eranian <eranian@google.com> wrote:
>>>>> On Thu, Jan 7, 2016 at 1:59 PM, Arnaldo Carvalho de Melo
>>>>> <acme@kernel.org> wrote:
>>>>>> Em Thu, Jan 07, 2016 at 01:56:14PM -0800, Stephane Eranian escreveu:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Whenever you do:
>>>>>>>
>>>>>>>     $ perf record -g -a sleep 10
>>>>>>>
>>>>>>> Perf will collect the callstack for each sample. At the end of the
>>>>>>> run, perf record
>>>>>>> adds the buildid for all dso with at least one sample. But when it
>>>>> does this, it
>>>>>>> only looks at the sampled IP and ignore the modules traversed by the
>>>>> callstack.
>>>>>>> That means that, it is not possible to uniquely identify the modules
>>>>> executed,
>>>>>>> unless they had at least one IP sample captured. But this is not
>>>>>>> always the case.
>>>>>>>
>>>>>>> How about providing an option to perf record to force collecting
>>>>>>> buildid for all IPs
>>>>>>> captured in the callstack? I understand that would cost more at the
>>>>> end of the
>>>>>>> collection, but this would be beneficial to several monitoring
>>>>> scenarios.
>>>>>>
>>>>>> I agree, would consider applying a patch that provides the option but
>>>>>> does not do this by default.
>>>>>>
>>>>> I agree, not the default.
>>>>
>>>> Hi Stephane,
>>>>
>>>> Please see
>>>>
>>>> https://lkml.org/lkml/2015/3/22/249
>>>
>>>
>>> Oops, Stephane, please try this, so that we can finally merge it :-\
>>>
>> I will try it today. However, I am a bit worried about the performance
>> impact. Unless I am missing something in this approach we may end up
>> looking up N times the same module if it appears in N callstacks. In
>> Andi's suggested approach, there would be only one pass at the beginning
>> (or the end of the run). But you could miss some modules if they are gone
>> by the time you run the pass.
> 
> How about this then?
> 
> Adrian, is it ok to skip process_buildids() for the auxtrace?

If you don't post-process (i.e. call process_buildids), then where do the
DSOs come from? i.e. dsos__hit_all() just hits the DSOs that exist.

> 
> Thanks,
> Namhyung
> 
> 
> diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt
> index 3a1a32f5479f..fbceb631387c 100644
> --- a/tools/perf/Documentation/perf-record.txt
> +++ b/tools/perf/Documentation/perf-record.txt
> @@ -338,6 +338,9 @@ Options passed to clang when compiling BPF scriptlets.
>  Specify vmlinux path which has debuginfo.
>  (enabled when BPF prologue is on)
>  
> +--buildid-all::
> +Record build-id of all DSOs regardless whether it's actually hit or not.
> +
>  SEE ALSO
>  --------
>  linkperf:perf-stat[1], linkperf:perf-list[1]
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index dc4e0adf5c5b..ab18db3153a6 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -50,6 +50,7 @@ struct record {
>  	int			realtime_prio;
>  	bool			no_buildid;
>  	bool			no_buildid_cache;
> +	bool			buildid_all;
>  	unsigned long long	samples;
>  };
>  
> @@ -755,14 +756,10 @@ out_child:
>  		file->size = lseek(perf_data_file__fd(file), 0, SEEK_CUR);
>  
>  		if (!rec->no_buildid) {
> -			process_buildids(rec);
> -			/*
> -			 * We take all buildids when the file contains
> -			 * AUX area tracing data because we do not decode the
> -			 * trace because it would take too long.
> -			 */
> -			if (rec->opts.full_auxtrace)
> +			if (rec->buildid_all)
>  				dsos__hit_all(rec->session);
> +			else
> +				process_buildids(rec);
>  		}
>  		perf_session__write_header(rec->session, rec->evlist, fd, true);
>  	}
> @@ -1138,6 +1135,8 @@ struct option __record_options[] = {
>  		   "options passed to clang when compiling BPF scriptlets"),
>  	OPT_STRING(0, "vmlinux", &symbol_conf.vmlinux_name,
>  		   "file", "vmlinux pathname"),
> +	OPT_BOOLEAN(0, "buildid-all", &record.buildid_all,
> +		    "Record build-id of all DSOs regardless of hits"),
>  	OPT_END()
>  };
>  
> @@ -1255,6 +1254,14 @@ int cmd_record(int argc, const char **argv, const char *prefix __maybe_unused)
>  	if (err)
>  		goto out_symbol_exit;
>  
> +	/*
> +	 * We take all buildids when the file contains
> +	 * AUX area tracing data because we do not decode the
> +	 * trace because it would take too long.
> +	 */
> +	if (rec->opts.full_auxtrace)
> +		rec->buildid_all = true;
> +
>  	if (record_opts__config(&rec->opts)) {
>  		err = -EINVAL;
>  		goto out_symbol_exit;
> 

[toc] | [prev] | [next] | [standalone]

#1306071

Hi Adrian,

On Mon, Jan 11, 2016 at 11:27:56AM +0200, Adrian Hunter wrote:
> On 09/01/16 12:31, Namhyung Kim wrote:
> > Hi Stephane,
> > 
> > On Fri, Jan 08, 2016 at 10:01:24AM -0800, Stephane Eranian wrote:
> >> On Thu, Jan 7, 2016 at 3:47 PM, Arnaldo Carvalho de Melo
> >> <acme@kernel.org> wrote:
> >>> Em Fri, Jan 08, 2016 at 07:47:03AM +0900, Namhyung Kim escreveu:
> >>>> On January 8, 2016 7:00:35 AM GMT+09:00, Stephane Eranian <eranian@google.com> wrote:
> >>>>> On Thu, Jan 7, 2016 at 1:59 PM, Arnaldo Carvalho de Melo
> >>>>> <acme@kernel.org> wrote:
> >>>>>> Em Thu, Jan 07, 2016 at 01:56:14PM -0800, Stephane Eranian escreveu:
> >>>>>>> Hi,
> >>>>>>>
> >>>>>>> Whenever you do:
> >>>>>>>
> >>>>>>>     $ perf record -g -a sleep 10
> >>>>>>>
> >>>>>>> Perf will collect the callstack for each sample. At the end of the
> >>>>>>> run, perf record
> >>>>>>> adds the buildid for all dso with at least one sample. But when it
> >>>>> does this, it
> >>>>>>> only looks at the sampled IP and ignore the modules traversed by the
> >>>>> callstack.
> >>>>>>> That means that, it is not possible to uniquely identify the modules
> >>>>> executed,
> >>>>>>> unless they had at least one IP sample captured. But this is not
> >>>>>>> always the case.
> >>>>>>>
> >>>>>>> How about providing an option to perf record to force collecting
> >>>>>>> buildid for all IPs
> >>>>>>> captured in the callstack? I understand that would cost more at the
> >>>>> end of the
> >>>>>>> collection, but this would be beneficial to several monitoring
> >>>>> scenarios.
> >>>>>>
> >>>>>> I agree, would consider applying a patch that provides the option but
> >>>>>> does not do this by default.
> >>>>>>
> >>>>> I agree, not the default.
> >>>>
> >>>> Hi Stephane,
> >>>>
> >>>> Please see
> >>>>
> >>>> https://lkml.org/lkml/2015/3/22/249
> >>>
> >>>
> >>> Oops, Stephane, please try this, so that we can finally merge it :-\
> >>>
> >> I will try it today. However, I am a bit worried about the performance
> >> impact. Unless I am missing something in this approach we may end up
> >> looking up N times the same module if it appears in N callstacks. In
> >> Andi's suggested approach, there would be only one pass at the beginning
> >> (or the end of the run). But you could miss some modules if they are gone
> >> by the time you run the pass.
> > 
> > How about this then?
> > 
> > Adrian, is it ok to skip process_buildids() for the auxtrace?
> 
> If you don't post-process (i.e. call process_buildids), then where do the
> DSOs come from? i.e. dsos__hit_all() just hits the DSOs that exist.

Ah, right.  I somehow thought that it was processed already elsewhere.

Then, how about this?



From 38b2bc273329afe4334a11d87d20ef71639132fb Mon Sep 17 00:00:00 2001
From: Namhyung Kim <namhyung@kernel.org>
Date: Sat, 9 Jan 2016 22:37:55 +0900
Subject: [PATCH] perf record: Add --buildid-all option

The --buildid-all option is to record build-id of all DSOs in the file.
It might be very costly to postprocess samples to find which DSO hits.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/Documentation/perf-record.txt |  3 +++
 tools/perf/builtin-record.c              | 26 ++++++++++++++++++++------
 2 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt
index 3a1a32f5479f..fbceb631387c 100644
--- a/tools/perf/Documentation/perf-record.txt
+++ b/tools/perf/Documentation/perf-record.txt
@@ -338,6 +338,9 @@ Options passed to clang when compiling BPF scriptlets.
 Specify vmlinux path which has debuginfo.
 (enabled when BPF prologue is on)
 
+--buildid-all::
+Record build-id of all DSOs regardless whether it's actually hit or not.
+
 SEE ALSO
 --------
 linkperf:perf-stat[1], linkperf:perf-list[1]
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index dc4e0adf5c5b..319712a4e02b 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -50,6 +50,7 @@ struct record {
 	int			realtime_prio;
 	bool			no_buildid;
 	bool			no_buildid_cache;
+	bool			buildid_all;
 	unsigned long long	samples;
 };
 
@@ -362,6 +363,13 @@ static int process_buildids(struct record *rec)
 	 */
 	symbol_conf.ignore_vmlinux_buildid = true;
 
+	/*
+	 * If --buildid-all is given, it marks all DSO regardless of hits,
+	 * so no need to process samples.
+	 */
+	if (rec->buildid_all)
+		rec->tool.sample = NULL;
+
 	return perf_session__process_events(session);
 }
 
@@ -756,12 +764,8 @@ out_child:
 
 		if (!rec->no_buildid) {
 			process_buildids(rec);
-			/*
-			 * We take all buildids when the file contains
-			 * AUX area tracing data because we do not decode the
-			 * trace because it would take too long.
-			 */
-			if (rec->opts.full_auxtrace)
+
+			if (rec->buildid_all)
 				dsos__hit_all(rec->session);
 		}
 		perf_session__write_header(rec->session, rec->evlist, fd, true);
@@ -1138,6 +1142,8 @@ struct option __record_options[] = {
 		   "options passed to clang when compiling BPF scriptlets"),
 	OPT_STRING(0, "vmlinux", &symbol_conf.vmlinux_name,
 		   "file", "vmlinux pathname"),
+	OPT_BOOLEAN(0, "buildid-all", &record.buildid_all,
+		    "Record build-id of all DSOs regardless of hits"),
 	OPT_END()
 };
 
@@ -1255,6 +1261,14 @@ int cmd_record(int argc, const char **argv, const char *prefix __maybe_unused)
 	if (err)
 		goto out_symbol_exit;
 
+	/*
+	 * We take all buildids when the file contains
+	 * AUX area tracing data because we do not decode the
+	 * trace because it would take too long.
+	 */
+	if (rec->opts.full_auxtrace)
+		rec->buildid_all = true;
+
 	if (record_opts__config(&rec->opts)) {
 		err = -EINVAL;
 		goto out_symbol_exit;
-- 
2.6.4

[toc] | [prev] | [next] | [standalone]

#1306111

On 11/01/16 13:02, Namhyung Kim wrote:
> Hi Adrian,
> 
> On Mon, Jan 11, 2016 at 11:27:56AM +0200, Adrian Hunter wrote:
>> On 09/01/16 12:31, Namhyung Kim wrote:
>>> Hi Stephane,
>>>
>>> On Fri, Jan 08, 2016 at 10:01:24AM -0800, Stephane Eranian wrote:
>>>> On Thu, Jan 7, 2016 at 3:47 PM, Arnaldo Carvalho de Melo
>>>> <acme@kernel.org> wrote:
>>>>> Em Fri, Jan 08, 2016 at 07:47:03AM +0900, Namhyung Kim escreveu:
>>>>>> On January 8, 2016 7:00:35 AM GMT+09:00, Stephane Eranian <eranian@google.com> wrote:
>>>>>>> On Thu, Jan 7, 2016 at 1:59 PM, Arnaldo Carvalho de Melo
>>>>>>> <acme@kernel.org> wrote:
>>>>>>>> Em Thu, Jan 07, 2016 at 01:56:14PM -0800, Stephane Eranian escreveu:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> Whenever you do:
>>>>>>>>>
>>>>>>>>>     $ perf record -g -a sleep 10
>>>>>>>>>
>>>>>>>>> Perf will collect the callstack for each sample. At the end of the
>>>>>>>>> run, perf record
>>>>>>>>> adds the buildid for all dso with at least one sample. But when it
>>>>>>> does this, it
>>>>>>>>> only looks at the sampled IP and ignore the modules traversed by the
>>>>>>> callstack.
>>>>>>>>> That means that, it is not possible to uniquely identify the modules
>>>>>>> executed,
>>>>>>>>> unless they had at least one IP sample captured. But this is not
>>>>>>>>> always the case.
>>>>>>>>>
>>>>>>>>> How about providing an option to perf record to force collecting
>>>>>>>>> buildid for all IPs
>>>>>>>>> captured in the callstack? I understand that would cost more at the
>>>>>>> end of the
>>>>>>>>> collection, but this would be beneficial to several monitoring
>>>>>>> scenarios.
>>>>>>>>
>>>>>>>> I agree, would consider applying a patch that provides the option but
>>>>>>>> does not do this by default.
>>>>>>>>
>>>>>>> I agree, not the default.
>>>>>>
>>>>>> Hi Stephane,
>>>>>>
>>>>>> Please see
>>>>>>
>>>>>> https://lkml.org/lkml/2015/3/22/249
>>>>>
>>>>>
>>>>> Oops, Stephane, please try this, so that we can finally merge it :-\
>>>>>
>>>> I will try it today. However, I am a bit worried about the performance
>>>> impact. Unless I am missing something in this approach we may end up
>>>> looking up N times the same module if it appears in N callstacks. In
>>>> Andi's suggested approach, there would be only one pass at the beginning
>>>> (or the end of the run). But you could miss some modules if they are gone
>>>> by the time you run the pass.
>>>
>>> How about this then?
>>>
>>> Adrian, is it ok to skip process_buildids() for the auxtrace?
>>
>> If you don't post-process (i.e. call process_buildids), then where do the
>> DSOs come from? i.e. dsos__hit_all() just hits the DSOs that exist.
> 
> Ah, right.  I somehow thought that it was processed already elsewhere.
> 
> Then, how about this?

Looks OK for auxtrace.

In other respects you should probably clarify what happens when --no-buildid
and --buildid-all are used together.  Maybe it should be an error.

> 
> 
> 
>>From 38b2bc273329afe4334a11d87d20ef71639132fb Mon Sep 17 00:00:00 2001
> From: Namhyung Kim <namhyung@kernel.org>
> Date: Sat, 9 Jan 2016 22:37:55 +0900
> Subject: [PATCH] perf record: Add --buildid-all option
> 
> The --buildid-all option is to record build-id of all DSOs in the file.
> It might be very costly to postprocess samples to find which DSO hits.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/Documentation/perf-record.txt |  3 +++
>  tools/perf/builtin-record.c              | 26 ++++++++++++++++++++------
>  2 files changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt
> index 3a1a32f5479f..fbceb631387c 100644
> --- a/tools/perf/Documentation/perf-record.txt
> +++ b/tools/perf/Documentation/perf-record.txt
> @@ -338,6 +338,9 @@ Options passed to clang when compiling BPF scriptlets.
>  Specify vmlinux path which has debuginfo.
>  (enabled when BPF prologue is on)
>  
> +--buildid-all::
> +Record build-id of all DSOs regardless whether it's actually hit or not.
> +
>  SEE ALSO
>  --------
>  linkperf:perf-stat[1], linkperf:perf-list[1]
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index dc4e0adf5c5b..319712a4e02b 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -50,6 +50,7 @@ struct record {
>  	int			realtime_prio;
>  	bool			no_buildid;
>  	bool			no_buildid_cache;
> +	bool			buildid_all;
>  	unsigned long long	samples;
>  };
>  
> @@ -362,6 +363,13 @@ static int process_buildids(struct record *rec)
>  	 */
>  	symbol_conf.ignore_vmlinux_buildid = true;
>  
> +	/*
> +	 * If --buildid-all is given, it marks all DSO regardless of hits,
> +	 * so no need to process samples.
> +	 */
> +	if (rec->buildid_all)
> +		rec->tool.sample = NULL;
> +
>  	return perf_session__process_events(session);
>  }
>  
> @@ -756,12 +764,8 @@ out_child:
>  
>  		if (!rec->no_buildid) {
>  			process_buildids(rec);
> -			/*
> -			 * We take all buildids when the file contains
> -			 * AUX area tracing data because we do not decode the
> -			 * trace because it would take too long.
> -			 */
> -			if (rec->opts.full_auxtrace)
> +
> +			if (rec->buildid_all)
>  				dsos__hit_all(rec->session);
>  		}
>  		perf_session__write_header(rec->session, rec->evlist, fd, true);
> @@ -1138,6 +1142,8 @@ struct option __record_options[] = {
>  		   "options passed to clang when compiling BPF scriptlets"),
>  	OPT_STRING(0, "vmlinux", &symbol_conf.vmlinux_name,
>  		   "file", "vmlinux pathname"),
> +	OPT_BOOLEAN(0, "buildid-all", &record.buildid_all,
> +		    "Record build-id of all DSOs regardless of hits"),
>  	OPT_END()
>  };
>  
> @@ -1255,6 +1261,14 @@ int cmd_record(int argc, const char **argv, const char *prefix __maybe_unused)
>  	if (err)
>  		goto out_symbol_exit;
>  
> +	/*
> +	 * We take all buildids when the file contains
> +	 * AUX area tracing data because we do not decode the
> +	 * trace because it would take too long.
> +	 */
> +	if (rec->opts.full_auxtrace)
> +		rec->buildid_all = true;
> +
>  	if (record_opts__config(&rec->opts)) {
>  		err = -EINVAL;
>  		goto out_symbol_exit;
> 

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web