From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754052AbcLIPOm (ORCPT ); Fri, 9 Dec 2016 10:14:42 -0500 Received: from mail.kernel.org ([198.145.29.136]:45978 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753790AbcLIPOl (ORCPT ); Fri, 9 Dec 2016 10:14:41 -0500 Date: Fri, 9 Dec 2016 12:14:30 -0300 From: Arnaldo Carvalho de Melo To: Alexis Berlemont Cc: Masami Hiramatsu , linux-kernel@vger.kernel.org, peterz@infradead.org, mingo@redhat.com, alexander.shishkin@linux.intel.com, hemant@linux.vnet.ibm.com Subject: Re: [PATCH v4 2/2] perf probe: add sdt probes arguments into the uprobe cmd string Message-ID: <20161209151430.GN8257@kernel.org> References: <20161126005803.25693-1-alexis.berlemont@gmail.com> <20161126005803.25693-3-alexis.berlemont@gmail.com> <20161207122610.008da636793d38a39accf29b@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161207122610.008da636793d38a39accf29b@kernel.org> X-Url: http://acmel.wordpress.com User-Agent: Mutt/1.7.1 (2016-10-04) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Em Wed, Dec 07, 2016 at 12:26:10PM +0900, Masami Hiramatsu escreveu: > Hello Alexis, > > On Sat, 26 Nov 2016 01:58:03 +0100 > Alexis Berlemont wrote: > > > An sdt probe can be associated with arguments but they were not passed > > to the user probe tracing interface (uprobe_events); this patch adapts > > the sdt argument descriptors according to the uprobe input format. > > Great! Yeah, good to see work in this area! I applied the first patch, with Masami's ack, waiting for his concerns on this one to be addressed, ok? - Arnaldo > > > > As the uprobe parser does not support scaled address mode, perf will > > skip arguments which cannot be adapted to the uprobe format. > > OK, it seems that skipping argument is a good idea :) > I just tried to support fixed-number arguments in probe events, > but skipping it is better with older kernel. > > I have some comments. > > > Here are the results: > > > > $ perf buildid-cache -v --add test_sdt > > $ perf probe -x test_sdt sdt_libfoo:table_frob > > $ perf probe -x test_sdt sdt_libfoo:table_diddle > > $ perf record -e sdt_libfoo:table_frob -e sdt_libfoo:table_diddle test_sdt > > $ perf script > > test_sdt ... 666.255678: sdt_libfoo:table_frob: (4004d7) arg0=0 arg1=0 > > test_sdt ... 666.255683: sdt_libfoo:table_diddle: (40051a) arg0=0 arg1=0 > > test_sdt ... 666.255686: sdt_libfoo:table_frob: (4004d7) arg0=1 arg1=2 > > test_sdt ... 666.255689: sdt_libfoo:table_diddle: (40051a) arg0=3 arg1=4 > > test_sdt ... 666.255692: sdt_libfoo:table_frob: (4004d7) arg0=2 arg1=4 > > test_sdt ... 666.255694: sdt_libfoo:table_diddle: (40051a) arg0=6 arg1=8 > > We'd better start with arg1, since sdt.h and original Dtrace SDT starts > arguments from arg1 (I'm not sure why) and dtrace/systemtap scripts > call it "arg1". > > > > > Signed-off-by: Alexis Berlemont > > --- > > tools/perf/arch/x86/util/perf_regs.c | 18 ++++ > > tools/perf/util/perf_regs.c | 4 + > > tools/perf/util/perf_regs.h | 13 +++ > > tools/perf/util/probe-file.c | 169 ++++++++++++++++++++++++++++++++++- > > 4 files changed, 200 insertions(+), 4 deletions(-) > > > > diff --git a/tools/perf/arch/x86/util/perf_regs.c b/tools/perf/arch/x86/util/perf_regs.c > > index c5db14f..52a1e65 100644 > > --- a/tools/perf/arch/x86/util/perf_regs.c > > +++ b/tools/perf/arch/x86/util/perf_regs.c > > @@ -26,3 +26,21 @@ const struct sample_reg sample_reg_masks[] = { > > #endif > > SMPL_REG_END > > }; > > + > > +const struct sdt_name_reg sdt_reg_renamings[] = { > > + SDT_NAME_REG(eax, ax), > > + SDT_NAME_REG(rax, ax), > > + SDT_NAME_REG(ebx, bx), > > + SDT_NAME_REG(rbx, bx), > > + SDT_NAME_REG(ecx, cx), > > + SDT_NAME_REG(rcx, cx), > > + SDT_NAME_REG(edx, dx), > > + SDT_NAME_REG(rdx, dx), > > + SDT_NAME_REG(esi, si), > > + SDT_NAME_REG(rsi, si), > > + SDT_NAME_REG(edi, di), > > + SDT_NAME_REG(rdi, di), > > + SDT_NAME_REG(ebp, bp), > > + SDT_NAME_REG(rbp, bp), > > + SDT_NAME_REG_END, > > +}; > > It is not enough, rNN registers also have to take care, since > gcc adds 'd', 'w' or 'b'suffixes for those registers to indicate > its size. e.g. r15d means r15 register with 32 lower bits. > What we need is just cut them off, since probe event uses > length modifiers (like :u32) > > > diff --git a/tools/perf/util/perf_regs.c b/tools/perf/util/perf_regs.c > > index c4023f2..1c21150 100644 > > --- a/tools/perf/util/perf_regs.c > > +++ b/tools/perf/util/perf_regs.c > > @@ -6,6 +6,10 @@ const struct sample_reg __weak sample_reg_masks[] = { > > SMPL_REG_END > > }; > > > > +const struct sdt_name_reg __weak sdt_reg_renamings[] = { > > + SDT_NAME_REG_END, > > +}; > > + > > #ifdef HAVE_PERF_REGS_SUPPORT > > int perf_reg_value(u64 *valp, struct regs_dump *regs, int id) > > { > > diff --git a/tools/perf/util/perf_regs.h b/tools/perf/util/perf_regs.h > > index 679d6e4..41815ca 100644 > > --- a/tools/perf/util/perf_regs.h > > +++ b/tools/perf/util/perf_regs.h > > @@ -15,6 +15,19 @@ struct sample_reg { > > > > extern const struct sample_reg sample_reg_masks[]; > > > > +struct sdt_name_reg { > > + const char *sdt_name; > > + const char *uprobe_name; > > +}; > > +#define SDT_NAME_REG(n, m) {.sdt_name = "%" #n, .uprobe_name = "%" #m} > > +#define SDT_NAME_REG_END {.sdt_name = NULL, .uprobe_name = NULL} > > + > > +/* > > + * The table sdt_reg_renamings is used for adjusting gcc/gas-generated > > + * registers before filling the uprobe tracer interface. > > + */ > > +extern const struct sdt_name_reg sdt_reg_renamings[]; > > + > > #ifdef HAVE_PERF_REGS_SUPPORT > > #include > > > > diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c > > index 436b647..75033c7 100644 > > --- a/tools/perf/util/probe-file.c > > +++ b/tools/perf/util/probe-file.c > > @@ -27,6 +27,7 @@ > > #include "probe-event.h" > > #include "probe-file.h" > > #include "session.h" > > +#include "perf_regs.h" > > > > #define MAX_CMDLEN 256 > > > > @@ -687,6 +688,165 @@ static unsigned long long sdt_note__get_addr(struct sdt_note *note) > > : (unsigned long long)note->addr.a64[0]; > > } > > > > +static const char * const type_to_suffix[] = { > > + ":s64", "", "", "", ":s32", "", ":s16", ":s8", > > + "", ":u8", ":u16", "", ":u32", "", "", "", ":u64" > > +}; > > + > > +static int synthesize_sdt_probe_arg(struct strbuf *buf, int i, const char *arg) > > +{ > > + const struct sdt_name_reg *rnames; > > + char *tmp, *desc = strdup(arg); > > + const char *prefix = "", *suffix = ""; > > + int ret = -1; > > + > > + if (desc == NULL) { > > + pr_debug4("Allocation error\n"); > > + return ret; > > + } > > + > > + tmp = strchr(desc, '@'); > > + if (tmp) { > > + long type_idx; > > + /* > > + * Isolate the string number and convert it into a > > + * binary value; this will be an index to get suffix > > + * of the uprobe name (defining the type) > > + */ > > + tmp[0] = '\0'; > > + type_idx = strtol(desc, NULL, 10); > > + if (type_idx == LONG_MIN || > > + type_idx == LONG_MAX) { > > + pr_debug4("Failed to get sdt type\n"); > > + goto error; > > + } > > You must ensure 0 <= type_idx + 8 <= 16 here. > > > + suffix = type_to_suffix[type_idx + 8]; > > + /* Get rid of the sdt prefix which is now useless */ > > + tmp++; > > + memmove(desc, tmp, strlen(tmp) + 1); > > + } > > + > > + /* > > + * The uprobe tracer format does not support all the > > + * addressing modes (notably: in x86 the scaled mode); so, we > > + * detect ',' characters, if there is just one, there is no > > + * use converting the sdt arg into a uprobe one. > > + */ > > + if (strchr(desc, ',')) { > > + pr_debug4("SDT argument format not supported\n"); > > Please print 'desc' by %s too. > > > + goto out; > > + } > > + > > + /* > > + * If the argument addressing mode is indirect, we must check > > + * a few things... > > + */ > > + tmp = strchr(desc, '('); > > + if (tmp) { > > + int j; > > + > > + /* > > + * ...if the addressing mode is indirect with a > > + * positive offset (ex.: "1608(%ax)"), we need to add > > + * a '+' prefix so as to be compliant with uprobe > > + * format. > > + */ > > + if (desc[0] != '+' && desc[0] != '-') > > + prefix = "+"; > > + > > + /* > > + * ...or if the addressing mode is indirect with a symbol > > + * as offset, the argument will not be supported by > > + * the uprobe tracer format; so, let's skip this one. > > + */ > > + for (j = 0; j < tmp - desc; j++) { > > + if (desc[j] != '+' && desc[j] != '-' && > > + !isdigit(desc[j])) > > + goto out; > > + } > > + } > > + > > + /* > > + * The uprobe parser does not support all gas register names; > > + * so, we have to replace them (ex. for x86_64: %rax -> %ax); > > + * the loop below performs all the needed renamings if needed. > > + */ > > + for (rnames = sdt_reg_renamings; rnames->sdt_name != NULL; rnames++) { > > + char *new_desc, *sdt_name; > > + size_t prefix_len, uprobe_len, mid_ofs, desc_len; > > + > > + sdt_name = strstr(desc, rnames->sdt_name); > > + if (sdt_name == NULL) > > + continue; > > It is better to search '%' from the desc and parse it. > And here, we also find fixed numbers which starts with '$', > since that is not supported yet. > > For example, with your patch, I still see some entries which have fixed num. > > $ perf buildid-cache --add /usr/lib64/libglib-2.0.so > $ grep \$[0-9] ~/.debug/usr/lib64/libglib-2.0.so.0.5000.2/fda1ca4181ba7135d41bf3cfadc813a432f31066/probes | tail -n 2 > p:sdt_glib/mem__realloc /usr/lib64/libglib-2.0.so.0.5000.2:0x4f670 arg0=%ax:u64 arg1=%bx:u64 arg2=%bp:u32 arg3=$0:s32 > p:sdt_glib/mem__realloc /usr/lib64/libglib-2.0.so.0.5000.2:0x4f75d arg0=%ax:u64 arg1=%bp:u64 arg2=%bx:u32 arg3=$1:s32 > > These arguments should be skipped. > > Thank you, > > > + > > + new_desc = zalloc(strlen(desc) + 1 + > > + strlen(rnames->uprobe_name) - > > + strlen(rnames->sdt_name)); > > + if (new_desc == NULL) > > + goto error; > > + > > + prefix_len = sdt_name - desc; > > + if (prefix_len != 0) > > + memcpy(new_desc, desc, prefix_len); > > + > > + uprobe_len = strlen(rnames->uprobe_name); > > + memcpy(new_desc + prefix_len, rnames->uprobe_name, uprobe_len); > > + > > + mid_ofs = prefix_len + strlen(rnames->sdt_name); > > + desc_len = strlen(desc); > > + if (mid_ofs < desc_len) > > + memcpy(new_desc + prefix_len + uprobe_len, > > + desc + mid_ofs, desc_len - mid_ofs); > > + > > + free(desc); > > + desc = new_desc; > > + } > > + > > + if (strbuf_addf(buf, " arg%d=%s%s%s", i, prefix, desc, suffix) < 0) > > + goto error; > > + > > +out: > > + ret = 0; > > +error: > > + free(desc); > > + return ret; > > +} > > + > > +static char *synthesize_sdt_probe_command(struct sdt_note *note, > > + const char *pathname, > > + const char *sdtgrp) > > +{ > > + struct strbuf buf; > > + char *ret = NULL, **args; > > + int i, args_count; > > + > > + if (strbuf_init(&buf, 32) < 0) > > + return NULL; > > + > > + if (strbuf_addf(&buf, "p:%s/%s %s:0x%llx", > > + sdtgrp, note->name, pathname, > > + sdt_note__get_addr(note)) < 0) > > + goto error; > > + > > + if (!note->args) > > + goto out; > > + > > + if (note->args) { > > + args = argv_split(note->args, &args_count); > > + > > + for (i = 0; i < args_count; ++i) { > > + if (synthesize_sdt_probe_arg(&buf, i, args[i]) < 0) > > + goto error; > > + } > > + } > > + > > +out: > > + ret = strbuf_detach(&buf, NULL); > > +error: > > + strbuf_release(&buf); > > + return ret; > > +} > > + > > int probe_cache__scan_sdt(struct probe_cache *pcache, const char *pathname) > > { > > struct probe_cache_entry *entry = NULL; > > @@ -723,11 +883,12 @@ int probe_cache__scan_sdt(struct probe_cache *pcache, const char *pathname) > > entry->pev.group = strdup(sdtgrp); > > list_add_tail(&entry->node, &pcache->entries); > > } > > - ret = asprintf(&buf, "p:%s/%s %s:0x%llx", > > - sdtgrp, note->name, pathname, > > - sdt_note__get_addr(note)); > > - if (ret < 0) > > + buf = synthesize_sdt_probe_command(note, pathname, sdtgrp); > > + if (!buf) { > > + ret = -ENOMEM; > > break; > > + } > > + > > strlist__add(entry->tevlist, buf); > > free(buf); > > entry = NULL; > > -- > > 2.10.2 > > > > > -- > Masami Hiramatsu