From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751160AbdAXIue (ORCPT ); Tue, 24 Jan 2017 03:50:34 -0500 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:59129 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750892AbdAXIuc (ORCPT ); Tue, 24 Jan 2017 03:50:32 -0500 Subject: Re: [PATCH v5 2/2] perf probe: add sdt probes arguments into the uprobe cmd string To: Alexis Berlemont References: <20161214000732.1710-1-alexis.berlemont@gmail.com> <20161214000732.1710-3-alexis.berlemont@gmail.com> Cc: linux-kernel@vger.kernel.org, peterz@infradead.org, mingo@redhat.com, acme@kernel.org, alexander.shishkin@linux.intel.com, Ravi Bangoria From: Ravi Bangoria Date: Tue, 24 Jan 2017 14:20:21 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: <20161214000732.1710-3-alexis.berlemont@gmail.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 X-Content-Scanned: Fidelis XPS MAILER x-cbid: 17012408-0008-0000-0000-00000701A668 X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00006488; HX=3.00000240; KW=3.00000007; PH=3.00000004; SC=3.00000200; SDB=6.00811922; UDB=6.00395908; IPR=6.00589338; BA=6.00005084; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00014022; XFM=3.00000011; UTC=2017-01-24 08:50:30 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17012408-0009-0000-0000-00003F4336B7 Message-Id: <5887154D.60705@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-01-24_06:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=2 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1612050000 definitions=main-1701240076 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Alexis, On Wednesday 14 December 2016 05:37 AM, 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. > > As the uprobe parser does not support scaled address mode, perf will > skip arguments which cannot be adapted to the uprobe format. > > 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 > > Signed-off-by: Alexis Berlemont > --- > tools/perf/arch/x86/util/perf_regs.c | 83 +++++++++++++++++ > tools/perf/util/perf_regs.c | 6 ++ > tools/perf/util/perf_regs.h | 6 ++ > tools/perf/util/probe-file.c | 170 ++++++++++++++++++++++++++++++++++- > 4 files changed, 261 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..09a7f55 100644 > --- a/tools/perf/arch/x86/util/perf_regs.c > +++ b/tools/perf/arch/x86/util/perf_regs.c > @@ -1,4 +1,7 @@ > +#include > + > #include "../../perf.h" > +#include "../../util/util.h" > #include "../../util/perf_regs.h" > > const struct sample_reg sample_reg_masks[] = { > @@ -26,3 +29,83 @@ const struct sample_reg sample_reg_masks[] = { > #endif > SMPL_REG_END > }; > + > +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} > + > +static 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, > +}; I see many markers uses %rsp. Such markers are failing at perf probe. Please add renaming entry for %rsp and %esp. For example: $ readelf -n /usr/lib64/libpython3.5m.so.1.0 ... Name: function__entry Arguments: 8@224(%rsp) 8@232(%rsp) -4@240(%rsp) 8@%rbx $ sudo ./perf probe sdt_python:function__entry Failed to write event: Invalid argument Please upgrade your kernel to at least 3.14 to have access to feature +224(%rsp):u64 Error: Failed to add events. This code does not handle rNN registers with postfix('b', 'w', 'd'). Such markers are failing at perf probe. For example: $ readelf -n /usr/lib64/libperl.so.5.24.0 ... Name: sub__return Arguments: 8@%rax 8@%r8 4@%r9d 8@%rsi $ sudo ./perf probe -v sdt_perl:sub__entry ... Opening /sys/kernel/debug/tracing//uprobe_events write=1 Writing event: p:sdt_perl/sub__entry /usr/lib64/libperl.so.5.24.0:0xbb780 arg1=%ax:u64 arg2=%r8:u64 arg3=%r9d:u32 arg4=%si:u64 Failed to write event: Invalid argument Error: Failed to add events. Reason: Invalid argument (Code: -22) Can we add them like: /* rNN registers */ SDT_NAME_REG(r8b, r8), SDT_NAME_REG(r8w, r8), SDT_NAME_REG(r8d, r8), SDT_NAME_REG(r9b, r9), ... SDT_NAME_REG(r14d, r14), SDT_NAME_REG(r15b, r15), SDT_NAME_REG(r15w, r15), SDT_NAME_REG(r15d, r15), and ... > + > +int sdt_rename_register(char **pdesc, char *old_name) > +{ > + const struct sdt_name_reg *rnames = sdt_reg_renamings; > + char *new_desc, *old_desc = *pdesc; > + size_t prefix_len, sdt_len, uprobe_len, old_desc_len, offset; > + int ret = -1; > + > + while (ret != 0 && rnames->sdt_name != NULL) { > + sdt_len = strlen(rnames->sdt_name); > + ret = strncmp(old_name, rnames->sdt_name, sdt_len); > + rnames += !!ret; > + } > + > + if (rnames->sdt_name == NULL) > + return 0; > + > + sdt_len = strlen(rnames->sdt_name); > + uprobe_len = strlen(rnames->uprobe_name); > + old_desc_len = strlen(old_desc) + 1; > + > + new_desc = zalloc(old_desc_len + uprobe_len - sdt_len); > + if (new_desc == NULL) > + return -1; > + > + /* Copy the chars before the register name (at least '%') */ > + prefix_len = old_name - old_desc; > + memcpy(new_desc, old_desc, prefix_len); > + > + /* Copy the new register name */ > + memcpy(new_desc + prefix_len, rnames->uprobe_name, uprobe_len); > + > + /* Copy the chars after the register name (if need be) */ > + offset = prefix_len + sdt_len; ... remove below if(){} block. > + if (offset < old_desc_len) { > + /* > + * The orginal register name can be suffixed by 'b', > + * 'w' or 'd' to indicate its size; so, we need to > + * skip this char if we met one. > + */ > + char sfx = old_desc[offset]; > + > + if (sfx == 'b' || sfx == 'w' || sfx == 'd') > + offset++; > + } > + > + if (offset < old_desc_len) > + memcpy(new_desc + prefix_len + uprobe_len, > + old_desc + offset, old_desc_len - offset); > + > + free(old_desc); > + *pdesc = new_desc; > + > + return 0; > +} > diff --git a/tools/perf/util/perf_regs.c b/tools/perf/util/perf_regs.c > index c4023f2..a37e593 100644 > --- a/tools/perf/util/perf_regs.c > +++ b/tools/perf/util/perf_regs.c > @@ -6,6 +6,12 @@ const struct sample_reg __weak sample_reg_masks[] = { > SMPL_REG_END > }; > > +int __weak sdt_rename_register(char **pdesc __maybe_unused, > + char *old_name __maybe_unused) > +{ > + return 0; > +} > + > #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..7544a15 100644 > --- a/tools/perf/util/perf_regs.h > +++ b/tools/perf/util/perf_regs.h > @@ -15,6 +15,12 @@ struct sample_reg { > > extern const struct sample_reg sample_reg_masks[]; > > +/* > + * The table sdt_reg_renamings is used for adjusting gcc/gas-generated > + * registers before filling the uprobe tracer interface. > + */ > +int sdt_rename_register(char **pdesc, char *old_name); > + > #ifdef HAVE_PERF_REGS_SUPPORT > #include > > diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c > index 436b647..d8a169e 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,166 @@ 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) > +{ > + 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); > + /* Check that the conversion went OK */ > + if (type_idx == LONG_MIN || type_idx == LONG_MAX) { > + pr_debug4("Failed to parse sdt type\n"); > + goto error; > + } > + /* Check that the converted value is OK */ > + if (type_idx < -8 || type_idx > 8) { > + pr_debug4("Failed to get a valid sdt type\n"); > + goto error; > + } > + suffix = type_to_suffix[type_idx + 8]; > + /* Get rid of the sdt prefix which is now useless */ > + tmp++; > + memmove(desc, tmp, strlen(tmp) + 1); > + } > + Below code is x86 specific. For example, Powerpc uses "i" for constant arguments not "$". Powerpc does not use symbol as offset etc. SDT marker argument is in N@OP format. Can we divide this function into two components. One parses N and other parses OP. Parsing OP will be arch specific code and should be moved under /tools/perf/arch/. > + /* > + * 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("Skipping unsupported SDT argument; %s\n", desc); > + 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])) { > + pr_debug4("Skipping unsupported SDT argument; " > + "%s\n", desc); > + goto out; > + } > + } > + } > + > + /* > + * The uprobe tracer format does not support constants; if we > + * find one in the current argument, let's skip the argument. > + */ > + if (strchr(desc, '$')) { > + pr_debug4("Skipping unsupported SDT argument; %s\n", desc); > + 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 looks for the register names (starting with > + * a '%' and tries to perform the needed renamings. > + */ > + tmp = strchr(desc, '%'); > + while (tmp) { > + size_t offset = tmp - desc; > + > + ret = sdt_rename_register(&desc, desc + offset); > + if (ret < 0) > + goto error; > + > + /* > + * The desc pointer might have changed; so, let's not > + * try to reuse tmp for next lookup > + */ > + tmp = strchr(desc + offset + 1, '%'); > + } > + > + if (strbuf_addf(buf, " arg%d=%s%s%s", i + 1, prefix, desc, suffix) < 0) > + goto error; > + > +out: > + ret = 0; > +error: > + free(desc); > + return ret; > +} Thanks, Ravi