Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 3 May 2018 23:57:13 +0900
From:   Masami Hiramatsu <mhiramat@kernel.org>
To:     Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
Cc:     oleg@redhat.com, peterz@infradead.org, srikar@linux.vnet.ibm.com,
        rostedt@goodmis.org, acme@kernel.org, ananth@linux.vnet.ibm.com,
        akpm@linux-foundation.org, alexander.shishkin@linux.intel.com,
        alexis.berlemont@gmail.com, corbet@lwn.net,
        dan.j.williams@intel.com, jolsa@redhat.com, kan.liang@intel.com,
        kjlx@templeofstupid.com, kstewart@linuxfoundation.org,
        linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org, milian.wolff@kdab.com, mingo@redhat.com,
        namhyung@kernel.org, naveen.n.rao@linux.vnet.ibm.com,
        pc@us.ibm.com, tglx@linutronix.de, yao.jin@linux.intel.com,
        fengguang.wu@intel.com, jglisse@redhat.com,
        Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Subject: Re: [PATCH v3 9/9] perf probe: Support SDT markers having reference
 counter (semaphore)
Message-Id: <20180503235713.6dff5b19b3a26b888abdb08b@kernel.org>
In-Reply-To: <20180417043244.7501-10-ravi.bangoria@linux.vnet.ibm.com>
References: <20180417043244.7501-1-ravi.bangoria@linux.vnet.ibm.com>
        <20180417043244.7501-10-ravi.bangoria@linux.vnet.ibm.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, 17 Apr 2018 10:02:44 +0530
Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:

> From: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> 
> With this, perf buildid-cache will save SDT markers with reference
> counter in probe cache. Perf probe will be able to probe markers
> having reference counter. Ex,
> 
>   # readelf -n /tmp/tick | grep -A1 loop2
>     Name: loop2
>     ... Semaphore: 0x0000000010020036
> 
>   # ./perf buildid-cache --add /tmp/tick
>   # ./perf probe sdt_tick:loop2
>   # ./perf stat -e sdt_tick:loop2 /tmp/tick
>     hi: 0
>     hi: 1
>     hi: 2
>     ^C
>      Performance counter stats for '/tmp/tick':
>                  3      sdt_tick:loop2
>        2.561851452 seconds time elapsed
> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>

Looks good to me.

Acked-by: Masami Hiramatsu <mhiramat@kernel.org>

Thanks!

> ---
>  tools/perf/util/probe-event.c | 39 ++++++++++++++++++++++++++++++++----
>  tools/perf/util/probe-event.h |  1 +
>  tools/perf/util/probe-file.c  | 34 ++++++++++++++++++++++++++------
>  tools/perf/util/probe-file.h  |  1 +
>  tools/perf/util/symbol-elf.c  | 46 ++++++++++++++++++++++++++++++++-----------
>  tools/perf/util/symbol.h      |  7 +++++++
>  6 files changed, 106 insertions(+), 22 deletions(-)
> 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index e1dbc98..9b9c26e 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -1832,6 +1832,12 @@ int parse_probe_trace_command(const char *cmd, struct probe_trace_event *tev)
>  			tp->offset = strtoul(fmt2_str, NULL, 10);
>  	}
>  
> +	if (tev->uprobes) {
> +		fmt2_str = strchr(p, '(');
> +		if (fmt2_str)
> +			tp->ref_ctr_offset = strtoul(fmt2_str + 1, NULL, 0);
> +	}
> +
>  	tev->nargs = argc - 2;
>  	tev->args = zalloc(sizeof(struct probe_trace_arg) * tev->nargs);
>  	if (tev->args == NULL) {
> @@ -2025,6 +2031,22 @@ static int synthesize_probe_trace_arg(struct probe_trace_arg *arg,
>  	return err;
>  }
>  
> +static int
> +synthesize_uprobe_trace_def(struct probe_trace_event *tev, struct strbuf *buf)
> +{
> +	struct probe_trace_point *tp = &tev->point;
> +	int err;
> +
> +	err = strbuf_addf(buf, "%s:0x%lx", tp->module, tp->address);
> +
> +	if (err >= 0 && tp->ref_ctr_offset) {
> +		if (!uprobe_ref_ctr_is_supported())
> +			return -1;
> +		err = strbuf_addf(buf, "(0x%lx)", tp->ref_ctr_offset);
> +	}
> +	return err >= 0 ? 0 : -1;
> +}
> +
>  char *synthesize_probe_trace_command(struct probe_trace_event *tev)
>  {
>  	struct probe_trace_point *tp = &tev->point;
> @@ -2054,15 +2076,17 @@ char *synthesize_probe_trace_command(struct probe_trace_event *tev)
>  	}
>  
>  	/* Use the tp->address for uprobes */
> -	if (tev->uprobes)
> -		err = strbuf_addf(&buf, "%s:0x%lx", tp->module, tp->address);
> -	else if (!strncmp(tp->symbol, "0x", 2))
> +	if (tev->uprobes) {
> +		err = synthesize_uprobe_trace_def(tev, &buf);
> +	} else if (!strncmp(tp->symbol, "0x", 2)) {
>  		/* Absolute address. See try_to_find_absolute_address() */
>  		err = strbuf_addf(&buf, "%s%s0x%lx", tp->module ?: "",
>  				  tp->module ? ":" : "", tp->address);
> -	else
> +	} else {
>  		err = strbuf_addf(&buf, "%s%s%s+%lu", tp->module ?: "",
>  				tp->module ? ":" : "", tp->symbol, tp->offset);
> +	}
> +
>  	if (err)
>  		goto error;
>  
> @@ -2646,6 +2670,13 @@ static void warn_uprobe_event_compat(struct probe_trace_event *tev)
>  {
>  	int i;
>  	char *buf = synthesize_probe_trace_command(tev);
> +	struct probe_trace_point *tp = &tev->point;
> +
> +	if (tp->ref_ctr_offset && !uprobe_ref_ctr_is_supported()) {
> +		pr_warning("A semaphore is associated with %s:%s and "
> +			   "seems your kernel doesn't support it.\n",
> +			   tev->group, tev->event);
> +	}
>  
>  	/* Old uprobe event doesn't support memory dereference */
>  	if (!tev->uprobes || tev->nargs == 0 || !buf)
> diff --git a/tools/perf/util/probe-event.h b/tools/perf/util/probe-event.h
> index 45b14f0..15a98c3 100644
> --- a/tools/perf/util/probe-event.h
> +++ b/tools/perf/util/probe-event.h
> @@ -27,6 +27,7 @@ struct probe_trace_point {
>  	char		*symbol;	/* Base symbol */
>  	char		*module;	/* Module name */
>  	unsigned long	offset;		/* Offset from symbol */
> +	unsigned long	ref_ctr_offset;	/* SDT reference counter offset */
>  	unsigned long	address;	/* Actual address of the trace point */
>  	bool		retprobe;	/* Return probe flag */
>  };
> diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
> index 4ae1123..a17ba6a 100644
> --- a/tools/perf/util/probe-file.c
> +++ b/tools/perf/util/probe-file.c
> @@ -697,8 +697,16 @@ int probe_cache__add_entry(struct probe_cache *pcache,
>  #ifdef HAVE_GELF_GETNOTE_SUPPORT
>  static unsigned long long sdt_note__get_addr(struct sdt_note *note)
>  {
> -	return note->bit32 ? (unsigned long long)note->addr.a32[0]
> -		 : (unsigned long long)note->addr.a64[0];
> +	return note->bit32 ?
> +		(unsigned long long)note->addr.a32[SDT_NOTE_IDX_LOC] :
> +		(unsigned long long)note->addr.a64[SDT_NOTE_IDX_LOC];
> +}
> +
> +static unsigned long long sdt_note__get_ref_ctr_offset(struct sdt_note *note)
> +{
> +	return note->bit32 ?
> +		(unsigned long long)note->addr.a32[SDT_NOTE_IDX_REFCTR] :
> +		(unsigned long long)note->addr.a64[SDT_NOTE_IDX_REFCTR];
>  }
>  
>  static const char * const type_to_suffix[] = {
> @@ -776,14 +784,21 @@ static char *synthesize_sdt_probe_command(struct sdt_note *note,
>  {
>  	struct strbuf buf;
>  	char *ret = NULL, **args;
> -	int i, args_count;
> +	int i, args_count, err;
> +	unsigned long long ref_ctr_offset;
>  
>  	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)
> +	err = strbuf_addf(&buf, "p:%s/%s %s:0x%llx",
> +			sdtgrp, note->name, pathname,
> +			sdt_note__get_addr(note));
> +
> +	ref_ctr_offset = sdt_note__get_ref_ctr_offset(note);
> +	if (ref_ctr_offset && err >= 0)
> +		err = strbuf_addf(&buf, "(0x%llx)", ref_ctr_offset);
> +
> +	if (err < 0)
>  		goto error;
>  
>  	if (!note->args)
> @@ -999,6 +1014,7 @@ int probe_cache__show_all_caches(struct strfilter *filter)
>  enum ftrace_readme {
>  	FTRACE_README_PROBE_TYPE_X = 0,
>  	FTRACE_README_KRETPROBE_OFFSET,
> +	FTRACE_README_UPROBE_REF_CTR,
>  	FTRACE_README_END,
>  };
>  
> @@ -1010,6 +1026,7 @@ enum ftrace_readme {
>  	[idx] = {.pattern = pat, .avail = false}
>  	DEFINE_TYPE(FTRACE_README_PROBE_TYPE_X, "*type: * x8/16/32/64,*"),
>  	DEFINE_TYPE(FTRACE_README_KRETPROBE_OFFSET, "*place (kretprobe): *"),
> +	DEFINE_TYPE(FTRACE_README_UPROBE_REF_CTR, "*ref_ctr_offset*"),
>  };
>  
>  static bool scan_ftrace_readme(enum ftrace_readme type)
> @@ -1065,3 +1082,8 @@ bool kretprobe_offset_is_supported(void)
>  {
>  	return scan_ftrace_readme(FTRACE_README_KRETPROBE_OFFSET);
>  }
> +
> +bool uprobe_ref_ctr_is_supported(void)
> +{
> +	return scan_ftrace_readme(FTRACE_README_UPROBE_REF_CTR);
> +}
> diff --git a/tools/perf/util/probe-file.h b/tools/perf/util/probe-file.h
> index 63f29b1..2a24918 100644
> --- a/tools/perf/util/probe-file.h
> +++ b/tools/perf/util/probe-file.h
> @@ -69,6 +69,7 @@ struct probe_cache_entry *probe_cache__find_by_name(struct probe_cache *pcache,
>  int probe_cache__show_all_caches(struct strfilter *filter);
>  bool probe_type_is_available(enum probe_type type);
>  bool kretprobe_offset_is_supported(void);
> +bool uprobe_ref_ctr_is_supported(void);
>  #else	/* ! HAVE_LIBELF_SUPPORT */
>  static inline struct probe_cache *probe_cache__new(const char *tgt __maybe_unused, struct nsinfo *nsi __maybe_unused)
>  {
> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> index 2de7705..45b7dba 100644
> --- a/tools/perf/util/symbol-elf.c
> +++ b/tools/perf/util/symbol-elf.c
> @@ -1803,6 +1803,34 @@ void kcore_extract__delete(struct kcore_extract *kce)
>  }
>  
>  #ifdef HAVE_GELF_GETNOTE_SUPPORT
> +
> +static void sdt_adjust_loc(struct sdt_note *tmp, GElf_Addr base_off)
> +{
> +	if (!base_off)
> +		return;
> +
> +	if (tmp->bit32)
> +		tmp->addr.a32[SDT_NOTE_IDX_LOC] =
> +			tmp->addr.a32[SDT_NOTE_IDX_LOC] + base_off -
> +			tmp->addr.a32[SDT_NOTE_IDX_BASE];
> +	else
> +		tmp->addr.a64[SDT_NOTE_IDX_LOC] =
> +			tmp->addr.a64[SDT_NOTE_IDX_LOC] + base_off -
> +			tmp->addr.a64[SDT_NOTE_IDX_BASE];
> +}
> +
> +static void sdt_adjust_refctr(struct sdt_note *tmp, GElf_Addr base_addr,
> +			      GElf_Addr base_off)
> +{
> +	if (!base_off)
> +		return;
> +
> +	if (tmp->bit32)
> +		tmp->addr.a32[SDT_NOTE_IDX_REFCTR] -= (base_addr - base_off);
> +	else
> +		tmp->addr.a64[SDT_NOTE_IDX_REFCTR] -= (base_addr - base_off);
> +}
> +
>  /**
>   * populate_sdt_note : Parse raw data and identify SDT note
>   * @elf: elf of the opened file
> @@ -1820,7 +1848,6 @@ static int populate_sdt_note(Elf **elf, const char *data, size_t len,
>  	const char *provider, *name, *args;
>  	struct sdt_note *tmp = NULL;
>  	GElf_Ehdr ehdr;
> -	GElf_Addr base_off = 0;
>  	GElf_Shdr shdr;
>  	int ret = -EINVAL;
>  
> @@ -1916,17 +1943,12 @@ static int populate_sdt_note(Elf **elf, const char *data, size_t len,
>  	 * base address in the description of the SDT note. If its different,
>  	 * then accordingly, adjust the note location.
>  	 */
> -	if (elf_section_by_name(*elf, &ehdr, &shdr, SDT_BASE_SCN, NULL)) {
> -		base_off = shdr.sh_offset;
> -		if (base_off) {
> -			if (tmp->bit32)
> -				tmp->addr.a32[0] = tmp->addr.a32[0] + base_off -
> -					tmp->addr.a32[1];
> -			else
> -				tmp->addr.a64[0] = tmp->addr.a64[0] + base_off -
> -					tmp->addr.a64[1];
> -		}
> -	}
> +	if (elf_section_by_name(*elf, &ehdr, &shdr, SDT_BASE_SCN, NULL))
> +		sdt_adjust_loc(tmp, shdr.sh_offset);
> +
> +	/* Adjust reference counter offset */
> +	if (elf_section_by_name(*elf, &ehdr, &shdr, SDT_PROBES_SCN, NULL))
> +		sdt_adjust_refctr(tmp, shdr.sh_addr, shdr.sh_offset);
>  
>  	list_add_tail(&tmp->note_list, sdt_notes);
>  	return 0;
> diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
> index 70c16741..aa095bf 100644
> --- a/tools/perf/util/symbol.h
> +++ b/tools/perf/util/symbol.h
> @@ -384,12 +384,19 @@ struct sdt_note {
>  int cleanup_sdt_note_list(struct list_head *sdt_notes);
>  int sdt_notes__get_count(struct list_head *start);
>  
> +#define SDT_PROBES_SCN ".probes"
>  #define SDT_BASE_SCN ".stapsdt.base"
>  #define SDT_NOTE_SCN  ".note.stapsdt"
>  #define SDT_NOTE_TYPE 3
>  #define SDT_NOTE_NAME "stapsdt"
>  #define NR_ADDR 3
>  
> +enum {
> +	SDT_NOTE_IDX_LOC = 0,
> +	SDT_NOTE_IDX_BASE,
> +	SDT_NOTE_IDX_REFCTR,
> +};
> +
>  struct mem_info *mem_info__new(void);
>  struct mem_info *mem_info__get(struct mem_info *mi);
>  void   mem_info__put(struct mem_info *mi);
> -- 
> 1.8.3.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>