Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752767AbcD0TgN (ORCPT ); Wed, 27 Apr 2016 15:36:13 -0400 Received: from mail.kernel.org ([198.145.29.136]:46925 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752022AbcD0TgM (ORCPT ); Wed, 27 Apr 2016 15:36:12 -0400 Date: Thu, 28 Apr 2016 04:36:07 +0900 From: Masami Hiramatsu To: Arnaldo Carvalho de Melo Cc: Hemant Kumar , Masami Hiramatsu , linux-kernel@vger.kernel.org, Namhyung Kim , Peter Zijlstra , Ingo Molnar , Ananth N Mavinakayanahalli Subject: Re: [PATCH perf/core v4 14/19] perf buildid-cache: Scan and import user SDT events to probe cache Message-Id: <20160428043607.d8e89514c33f4831b564972a@kernel.org> In-Reply-To: <20160427152816.GP11033@kernel.org> References: <20160426090200.11891.43944.stgit@devbox> <20160426090422.11891.92396.stgit@devbox> <5720D86C.3010209@linux.vnet.ibm.com> <20160427152816.GP11033@kernel.org> X-Mailer: Sylpheed 3.4.3 (GTK+ 2.24.28; x86_64-redhat-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5077 Lines: 132 On Wed, 27 Apr 2016 12:28:16 -0300 Arnaldo Carvalho de Melo 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 > > > > > >perf buildid-cache --add 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_:=". > > > > > >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:\* # 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. > > [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. > > > > 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 :) Thank you, -- Masami Hiramatsu