Message-ID: <20160427152816.GP11033@kernel.org>
Date:	Wed, 27 Apr 2016 12:28:16 -0300
From:	Arnaldo Carvalho de Melo <acme@...nel.org>
To:	Hemant Kumar <hemant@...ux.vnet.ibm.com>
Cc:	Masami Hiramatsu <mhiramat@...nel.org>,
	linux-kernel@...r.kernel.org, Namhyung Kim <namhyung@...nel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...hat.com>,
	Ananth N Mavinakayanahalli <ananth@...ux.vnet.ibm.com>
Subject: Re: [PATCH perf/core v4 14/19] perf buildid-cache: Scan and import
 user SDT events to probe cache

Em Wed, Apr 27, 2016 at 08:49:08PM +0530, Hemant Kumar escreveu:
> 
> 
> On 04/26/2016 02:34 PM, Masami Hiramatsu wrote:
> >From: Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
> >
> >perf buildid-cache --add <binary> scans given binary and add
> >the SDT events to probe cache. "sdt_" prefix is appended for
> >all SDT providers to avoid event-name clash with other pre-defined
> >events. It is possible to use the cached SDT events as other cached
> >events, via perf probe --add "sdt_<provider>:<event>=<event>".
> >
> >e.g.
> >   ----
> >   # perf buildid-cache --add /lib/libc-2.17.so
> >   # perf probe --cache --list | head -n 5
> >   /usr/lib/libc-2.17.so (a6fb821bdf53660eb2c29f778757aef294d3d392):
> >   sdt_libc:setjmp=setjmp
> >   sdt_libc:longjmp=longjmp
> >   sdt_libc:longjmp_target=longjmp_target
> >   sdt_libc:memory_heap_new=memory_heap_new
> >   # perf probe -x /usr/lib/libc-2.17.so \
> >     -a sdt_libc:memory_heap_new=memory_heap_new
> >   Added new event:
> >     sdt_libc:memory_heap_new (on memory_heap_new
> >    in /usr/lib/libc-2.17.so)
> >
> >   You can now use it in all perf tools, such as:
> >
> >           perf record -e sdt_libc:memory_heap_new -aR sleep 1
> >
> >   # perf probe -l
> >     sdt_libc:memory_heap_new (on new_heap+183 in /usr/lib/libc-2.17.so)
> >   ----
> 
> Patch looks good to me. Have a few questions below :
> 
> What about the same binary path having different build-ids. For e.g
> a binary say, "test" has some markers, we add it to the cache. And,
> then the file gets rebuilt again with different build-id now. And we try
> to add to the cache again. It shows multiple entries in the cache :
> # perf probe --cache --list
> /home/hemant/test (157380727e2b3854395aa915dfc91dbccc02058b):
> sdt_user_test:marker1=marker1
> /home/hemant/test (64c0a018636e6d5145b09fc65839c1a4a7899f18):
> sdt_user_test:marker1=marker1
> /home/hemant/test (c9e34759ae95b68fa385831041c5d9e0dd1697fb):
> sdt_user_test:marker1=marker1
> ...
> 
> But, perf list sdt shows only one entry (which it should) :
> # perf list sdt
> 
> List of pre-defined events (to be used in -e):
> 
>   sdt_user_test:marker1                                [SDT event]
> 
> perf probe also works as expected :
> # perf probe -x /home/hemant/test %sdt_user_test:marker1
> Added new event:
>   sdt_user_test:marker1  (on %marker1 in /home/hemant/test)
> 
> You can now use it in all perf tools, such as:
> 
>     perf record -e sdt_user_test:marker1 -aR sleep 1
> 
> So, the question is, do we delete the previous entries for "test" from
> the cache once we get a newer version of "test"?

No, we shouldn't, since those entries may be used for other tasks that
involves using the exact DSO used for a particular perf.data session.

Humm, but you are talking about what cache? The "probe cache" or the
"build-id cache"? My previous statement was about the build-id cache.

For the probe cache, humm, probably we want to keep it as well, we may
have moved that 'test' file to some other place, renamed it, etc, but it
continues being accessible by its content-based identifier (the
build-id) and could be used in ways we don't envision right now.

I.e. the same principle used for the build-id cache should be used for
this probe cache, where we store things by build-id.

We need to prune this from time to time and for this we have:

  perf buildid-cache purge

But that right now is unflexible, we should have a way to ask to control
how much is purged :-\

- Arnaldo

- Arnaldo
 
> >Note that SDT event entries in probe-cache file is somewhat different
> >from normal cached events. Normal one starts with "#", but SDTs are
> >starting with "%".
> >
> >Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
> >Signed-off-by: Masami Hiramatsu <mhiramat@...nel.org>
> >---
> [SNIP]
> >+	list_for_each_entry(note, &sdtlist, note_list) {
> >+		ret = snprintf(sdtgrp, 64, "sdt_%s", note->provider);
> >+		if (ret < 0)
> >+			break;
> >+		/* Try to find same-name entry */
> >+		entry = probe_cache__find_by_name(pcache, sdtgrp, note->name);
> 
> Wouldn't it be better to compare the build-id rather than the event
> name? So, if there is a new sdt event added to a binary, its build-id will
> change. And, if there is no change, the build-id remains the same.
> 
> Only if there is a change in the build-id, we can go for searching the
> event name. This two level check can help optimizing the search.
> 
> >+		if (!entry) {
> >+			entry = probe_cache_entry__new(NULL);
> >+			if (!entry) {
> >+				ret = -ENOMEM;
> >+				break;
> >+			}
> >+			entry->sdt = true;
> >+			ret = asprintf(&entry->spev, "%s:%s=%s", sdtgrp,
> >+					note->name, note->name);
> >+			if (ret < 0)
> >+				break;
> >+			entry->pev.event = strdup(note->name);
> >+			entry->pev.group = strdup(sdtgrp);
> >+			list_add_tail(&entry->list, &pcache->list);
> >+		}
> >+		ret = asprintf(&buf, "p:%s/%s %s:0x%llx",
> >+				sdtgrp, note->name, pathname,
> >+				sdt_note__get_addr(note));
> >+		if (ret < 0)
> >+			break;
> >+		strlist__add(entry->tevlist, buf);
> >+		free(buf);
> >+		entry = NULL;
> >+	}
> >+	if (entry)
> >+		probe_cache_entry__delete(entry);
> >+	cleanup_sdt_note_list(&sdtlist);
> >+	return ret;
> >+}
> >+
> >  static int probe_cache_entry__write(struct probe_cache_entry *entry, int fd)
> >  {
> >  	struct str_node *snode;
> >  	struct iovec iov[3];
> >+	const char *prefix = entry->sdt ? "%" : "#";
> >  	int ret;
> >
> >-	pr_debug("Writing cache: #%s\n", entry->spev);
> >-	iov[0].iov_base = (void *)"#"; iov[0].iov_len = 1;
> >+	pr_debug("Writing cache: %s%s\n", prefix, entry->spev);
> >+	iov[0].iov_base = (void *)prefix; iov[0].iov_len = 1;
> >  	iov[1].iov_base = entry->spev; iov[1].iov_len = strlen(entry->spev);
> >  	iov[2].iov_base = (void *)"\n"; iov[2].iov_len = 1;
> >  	ret = writev(fd, iov, 3);
> >diff --git a/tools/perf/util/probe-file.h b/tools/perf/util/probe-file.h
> >index ba7b6d3..55322f1 100644
> >--- a/tools/perf/util/probe-file.h
> >+++ b/tools/perf/util/probe-file.h
> >@@ -21,6 +21,7 @@ int probe_file__del_strlist(int fd, struct strlist *namelist);
> >  /* Cache of probe definitions */
> >  struct probe_cache_entry {
> >  	struct list_head	list;
> >+	bool			sdt;
> >  	struct perf_probe_event pev;
> >  	char			*spev;
> >  	struct strlist		*tevlist;
> >@@ -35,6 +36,7 @@ struct probe_cache *probe_cache__new(const char *target);
> >  int probe_cache__add_entry(struct probe_cache *pcache,
> >  			   struct perf_probe_event *pev,
> >  			   struct probe_trace_event *tevs, int ntevs);
> >+int probe_cache__scan_sdt(struct probe_cache *pcache, const char *pathname);
> >  int probe_cache__commit(struct probe_cache *pcache);
> >  void probe_cache__delete(struct probe_cache *pcache);
> >  int probe_cache__remove_entries(struct probe_cache *pcache,
> >
> 
> -- 
> Thanks,
> Hemant Kumar

Powered by blists - more mailing lists