From: Hemant Kumar <hemant@linux.vnet.ibm.com>
To: Alexis Berlemont <alexis.berlemont@gmail.com>,
linux-kernel@vger.kernel.org
Cc: peterz@infradead.org, mingo@redhat.com, acme@kernel.org,
alexander.shishkin@linux.intel.com
Subject: Re: [PATCH v2 2/2] perf probe: add sdt probes arguments into the uprobe cmd string
Date: Mon, 21 Nov 2016 15:55:06 +0530 [thread overview]
Message-ID: <81ed2f2c-9569-c5bb-246d-1a4da7ec4607@linux.vnet.ibm.com> (raw)
In-Reply-To: <20161118235637.21651-3-alexis.berlemont@gmail.com>
Hi Alexis,
On 11/19/2016 05:26 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
Working for some binaries and not working for others. On some digging, I
think, this patchset doesn't take care of the offset sign. So, when we
try to probe with these patches applied :
# ./perf probe -x /lib64/libpthread.so.0 sdt_libpthread:pthread_start
Failed to write event: Invalid argument
Error: Failed to add events.
With -v, the string to be written in uprobe_events is shown as :
...
Writing event: p:sdt_libpthread/pthread_start
/usr/lib64/libpthread-2.23.so:0x75b8 arg0=%ax:u64 arg1=1600(%ax):u64
arg2=1608(%ax):u64
...
The offsets mentioned in the string, for e.g., arg1=1600(%ax):u64 (to be
written into the uprobe_events file don't have any sign. That is the
issue here, i.e, we need to have a sign before the offsets.
This works fine :
# echo "p:sdt_libpthread/pthread_start
/usr/lib64/libpthread-2.23.so:0x75b8 arg0=%ax:u64 arg1=+1600(%ax):u64
arg2=+1608(%ax):u64" > /sys/kernel/debug/tracing/uprobe_events
For the negative offsets, i.e., '-' sign before the offsets, this works
fine because, it takes the sign as is.
So, we need to explicitly mention the offset sign.
--
Thanks,
Hemant Kumar
> $ 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 <alexis.berlemont@gmail.com>
> ---
> 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 | 141 ++++++++++++++++++++++++++++++++++-
> 4 files changed, 172 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,
> +};
> 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 <perf_regs.h>
>
> diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
> index 436b647..587763d 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,137 @@ 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 *suffix = "";
> + int ret = -1;
> +
> + if (desc == NULL) {
> + pr_debug4("Allocation error\n");
> + return ret;
> + }
> +
> + /*
> + * 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");
> + goto out;
> + }
> +
> + 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;
> + }
> + 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 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;
> +
> + 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", i, 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 +855,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;
next prev parent reply other threads:[~2016-11-21 10:25 UTC|newest]
Thread overview: 77+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-11-16 23:55 [PATCH 0/2] perf: add support of SDT probes arguments Alexis Berlemont
2016-11-16 23:56 ` [PATCH 1/2] perf sdt: add scanning of sdt probles arguments Alexis Berlemont
2016-11-16 23:56 ` [PATCH 2/2] perf probe: add sdt probes arguments into the uprobe cmd string Alexis Berlemont
2016-11-17 9:04 ` Hemant Kumar
2016-11-18 23:56 ` [PATCH v2 0/2] " Alexis Berlemont
2016-11-18 23:56 ` [PATCH v2 1/2] perf sdt: add scanning of sdt probles arguments Alexis Berlemont
2016-11-25 14:40 ` Arnaldo Carvalho de Melo
2016-11-26 0:58 ` [PATCH v4 0/2] perf probe: add sdt probes arguments into the uprobe cmd string Alexis Berlemont
2016-12-05 23:42 ` Alexis Berlemont
2016-12-06 14:45 ` Arnaldo Carvalho de Melo
2016-11-26 0:58 ` [PATCH v4 1/2] perf sdt: add scanning of sdt probles arguments Alexis Berlemont
2016-12-07 2:44 ` Masami Hiramatsu
2016-11-26 0:58 ` [PATCH v4 2/2] perf probe: add sdt probes arguments into the uprobe cmd string Alexis Berlemont
2016-12-07 3:26 ` Masami Hiramatsu
2016-12-09 15:14 ` Arnaldo Carvalho de Melo
2016-12-10 10:00 ` Masami Hiramatsu
2016-12-14 0:07 ` [PATCH v5 0/2] " Alexis Berlemont
2016-12-14 7:36 ` Ingo Molnar
2017-01-23 11:23 ` Ravi Bangoria
2017-02-22 22:41 ` Alexis Berlemont
2017-01-24 6:58 ` Ravi Bangoria
2017-01-24 8:22 ` Ingo Molnar
2017-01-24 8:36 ` Ravi Bangoria
2017-02-02 11:11 ` [PATCH 0/5] perf/sdt: Argument support for x86 and powepc Ravi Bangoria
2017-02-02 11:11 ` [PATCH 1/5] perf/sdt: Show proper hint Ravi Bangoria
2017-02-02 13:40 ` Ingo Molnar
2017-02-02 16:20 ` Arnaldo Carvalho de Melo
2017-02-03 10:26 ` [PATCH v2] " Ravi Bangoria
2017-02-03 15:18 ` Arnaldo Carvalho de Melo
2017-02-07 7:53 ` Ingo Molnar
2017-02-07 15:50 ` Arnaldo Carvalho de Melo
2017-02-07 8:00 ` Ingo Molnar
2017-02-16 10:16 ` [RFC] perf/sdt: Directly record SDT event with 'perf record' Ravi Bangoria
2017-02-20 7:08 ` Ingo Molnar
2017-02-20 8:21 ` Ravi Bangoria
2017-02-20 8:42 ` Ingo Molnar
2017-02-20 11:01 ` Ravi Bangoria
2017-02-20 14:11 ` Arnaldo Carvalho de Melo
2017-02-23 8:13 ` Ravi Bangoria
2017-02-23 12:48 ` Arnaldo Carvalho de Melo
2017-02-07 1:13 ` [PATCH v2] perf/sdt: Show proper hint Masami Hiramatsu
2017-02-10 7:44 ` [tip:perf/core] perf sdt: Show proper hint when event not yet in place via 'perf probe' tip-bot for Ravi Bangoria
2017-02-02 11:11 ` [PATCH 2/5] perf/sdt/x86: Add renaming logic for rNN and other registers Ravi Bangoria
2017-02-07 3:11 ` Masami Hiramatsu
2017-03-21 14:08 ` Arnaldo Carvalho de Melo
2017-03-24 18:45 ` [tip:perf/core] perf sdt x86: " tip-bot for Ravi Bangoria
2017-02-02 11:11 ` [PATCH 3/5] perf/sdt/x86: Move OP parser to tools/perf/arch/x86/ Ravi Bangoria
2017-02-07 3:11 ` Masami Hiramatsu
2017-02-07 5:22 ` Ravi Bangoria
2017-03-21 14:10 ` Arnaldo Carvalho de Melo
2017-03-21 23:00 ` Masami Hiramatsu
2017-03-22 11:22 ` Arnaldo Carvalho de Melo
2017-03-21 14:55 ` Masami Hiramatsu
2017-02-02 11:11 ` [PATCH 4/5] perf/sdt/powerpc: Add argument support Ravi Bangoria
2017-02-02 11:11 ` [PATCH 5/5] perf/probe: Change MAX_CMDLEN Ravi Bangoria
2017-02-07 1:40 ` Masami Hiramatsu
2017-02-07 5:45 ` [PATCH v2] " Ravi Bangoria
2017-03-21 5:19 ` Masami Hiramatsu
2017-03-21 13:37 ` Arnaldo Carvalho de Melo
2017-03-24 18:43 ` [tip:perf/core] perf probe: " tip-bot for Ravi Bangoria
2017-02-07 2:55 ` [PATCH 0/5] perf/sdt: Argument support for x86 and powepc Masami Hiramatsu
2017-03-06 7:53 ` Ravi Bangoria
2017-03-06 13:42 ` Masami Hiramatsu
2017-03-21 5:08 ` [PATCH v5 0/2] perf probe: add sdt probes arguments into the uprobe cmd string Masami Hiramatsu
2016-12-14 0:07 ` [PATCH v5 1/2] perf sdt: add scanning of sdt probles arguments Alexis Berlemont
2017-03-06 13:39 ` Masami Hiramatsu
2017-03-21 13:52 ` Arnaldo Carvalho de Melo
2017-03-24 18:44 ` [tip:perf/core] perf sdt: Add scanning of sdt probes arguments tip-bot for Alexis Berlemont
2016-12-14 0:07 ` [PATCH v5 2/2] perf probe: add sdt probes arguments into the uprobe cmd string Alexis Berlemont
2017-01-24 8:50 ` Ravi Bangoria
2017-03-06 17:23 ` Masami Hiramatsu
2017-03-24 18:44 ` [tip:perf/core] perf probe: Add " tip-bot for Alexis Berlemont
2016-11-18 23:56 ` [PATCH v2 2/2] perf probe: add " Alexis Berlemont
2016-11-21 10:25 ` Hemant Kumar [this message]
2016-11-24 23:13 ` [PATCH v3 0/2] " Alexis Berlemont
2016-11-24 23:13 ` [PATCH v3 1/2] perf sdt: add scanning of sdt probles arguments Alexis Berlemont
2016-11-24 23:13 ` [PATCH v3 2/2] perf probe: add sdt probes arguments into the uprobe cmd string Alexis Berlemont
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=81ed2f2c-9569-c5bb-246d-1a4da7ec4607@linux.vnet.ibm.com \
--to=hemant@linux.vnet.ibm.com \
--cc=acme@kernel.org \
--cc=alexander.shishkin@linux.intel.com \
--cc=alexis.berlemont@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=peterz@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).