Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753366AbcD0PUT (ORCPT ); Wed, 27 Apr 2016 11:20:19 -0400 Received: from e23smtp07.au.ibm.com ([202.81.31.140]:46902 "EHLO e23smtp07.au.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752661AbcD0PUR (ORCPT ); Wed, 27 Apr 2016 11:20:17 -0400 X-IBM-Helo: d23dlp02.au.ibm.com X-IBM-MailFrom: hemant@linux.vnet.ibm.com X-IBM-RcptTo: linux-kernel@vger.kernel.org Message-ID: <5720D86C.3010209@linux.vnet.ibm.com> Date: Wed, 27 Apr 2016 20:49:08 +0530 From: Hemant Kumar User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Masami Hiramatsu CC: Arnaldo Carvalho de Melo , 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 References: <20160426090200.11891.43944.stgit@devbox> <20160426090422.11891.92396.stgit@devbox> In-Reply-To: <20160426090422.11891.92396.stgit@devbox> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 16042715-0025-0000-0000-0000047135F3 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5670 Lines: 159 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"? > 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 > Signed-off-by: Masami Hiramatsu > --- [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