| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5720D86C.3010209@linux.vnet.ibm.com>
Date: Wed, 27 Apr 2016 20:49:08 +0530
From: Hemant Kumar <hemant@...ux.vnet.ibm.com>
To: Masami Hiramatsu <mhiramat@...nel.org>
CC: Arnaldo Carvalho de Melo <acme@...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
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"?
> 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