Re: [PATCH perf/core v4 14/19] perf buildid-cache: Scan and import user SDT events to probe cache

From: Hemant Kumar
Date: Wed Apr 27 2016 - 16:23:57 EST




On 04/28/2016 01:06 AM, Masami Hiramatsu wrote:
On Wed, 27 Apr 2016 12:28:16 -0300
Arnaldo Carvalho de Melo <acme@xxxxxxxxxx> wrote:

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

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.
Agreed. And you can do it by using perf buildid-cache as below;

# perf buildid-cache --purge /home/hemant/test
# perf buildid-cache --add /home/hemant/test

This actually removes all old binary caches, and it maybe not
what you want. Or, you can try removing cached events too.

# perf probe --cache -d %sdt_user_test:\*

Yeah, it does work for me, but without the '%'.

# perf buildid-cache --add /home/hemant/test

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 :-\
Agreed, current buildid-cache --update does just rescan current
binary, but maybe it should also remove old caches.

Ok.

[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.
This is not such purpose, but just for folding same name SDTs(but different
addresses) on same binary.

I get it. But, I was wondering whether that can be used for comparison as well.

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.
That have been done in build-id.c :)

Ok, probably missed it.

Thank you,


Thanks for the explanation.

--
Thanks,
Hemant Kumar