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

[Hemant Kumar <hemant@linux.vnet.ibm.com>]

On 04/28/2016 01:06 AM, Masami Hiramatsu wrote:
> On Wed, 27 Apr 2016 12:28:16 -0300
> Arnaldo Carvalho de Melo <acme@kernel.org> 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@hitachi.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.
> 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