Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1591538 > unrolled thread

[PATCH v4 2/3] perf: kretprobes: offset from reloc_sym if kernel supports it

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v4 2/3] perf: kretprobes: offset from reloc_sym if kernel supports it "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> - 2017-03-03 00:10 +0100
    Re: [PATCH v4 2/3] perf: kretprobes: offset from reloc_sym if  kernel supports it Masami Hiramatsu <mhiramat@kernel.org> - 2017-03-04 02:00 +0100
      Re: [PATCH v4 2/3] perf: kretprobes: offset from reloc_sym if  kernel supports it Masami Hiramatsu <mhiramat@kernel.org> - 2017-03-04 03:50 +0100
        Re: [PATCH v4 2/3] perf: kretprobes: offset from reloc_sym if  kernel supports it Masami Hiramatsu <mhiramat@kernel.org> - 2017-03-04 03:50 +0100
        Re: [PATCH v4 2/3] perf: kretprobes: offset from reloc_sym if  kernel supports it Masami Hiramatsu <mhiramat@kernel.org> - 2017-03-04 05:50 +0100
          Re: [PATCH v4 2/3] perf: kretprobes: offset from reloc_sym if kernel  supports it "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> - 2017-03-06 20:10 +0100
          Re: [PATCH v4 2/3] perf: kretprobes: offset from reloc_sym if  kernel supports it Masami Hiramatsu <mhiramat@kernel.org> - 2017-03-06 22:10 +0100
            Re: [PATCH v4 2/3] perf: kretprobes: offset from reloc_sym if kernel "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> - 2017-03-07 12:00 +0100
              [RESEND PATCH 6/6] perf: powerpc: choose local entry point with kretprobes "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> - 2017-03-07 12:00 +0100
                [PATCH v2 6/6] perf: powerpc: choose local entry point with kretprobes "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> - 2017-03-07 22:00 +0100
              [RESEND PATCH 2/6] powerpc: kretprobes: override default function entry offset "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> - 2017-03-07 12:00 +0100
              [PATCH 5/6] perf: probes: move ftrace README parsing logic into trace-event-parse.c "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> - 2017-03-07 12:00 +0100
                Re: [PATCH 5/6] perf: probes: move ftrace README parsing logic into  trace-event-parse.c Masami Hiramatsu <mhiramat@kernel.org> - 2017-03-07 17:00 +0100
                  Re: [PATCH 5/6] perf: probes: move ftrace README parsing logic into  trace-event-parse.c "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> - 2017-03-07 20:50 +0100
                Re: [PATCH 5/6] perf: probes: move ftrace README parsing logic into  trace-event-parse.c "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> - 2017-03-07 17:40 +0100
                Re: [PATCH 5/6] perf: probes: move ftrace README parsing logic into  trace-event-parse.c Masami Hiramatsu <mhiramat@kernel.org> - 2017-03-07 18:50 +0100
          Re: [PATCH v4 2/3] perf: kretprobes: offset from reloc_sym if kernel supports it "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> - 2017-03-07 01:50 +0100
      Re: [PATCH v4 2/3] perf: kretprobes: offset from reloc_sym if kernel  supports it "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> - 2017-03-06 16:10 +0100
        Re: [PATCH v4 2/3] perf: kretprobes: offset from reloc_sym if  kernel supports it Masami Hiramatsu <mhiramat@kernel.org> - 2017-03-06 22:30 +0100

#1591538 — [PATCH v4 2/3] perf: kretprobes: offset from reloc_sym if kernel supports it

We indicate support for accepting sym+offset with kretprobes through a
line in ftrace README. Parse the same to identify support and choose the
appropriate format for kprobe_events.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 tools/perf/util/probe-event.c | 12 +++++-------
 tools/perf/util/probe-file.c  |  7 +++++++
 tools/perf/util/probe-file.h  |  1 +
 3 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 35f5b7b7715c..faf5789902f5 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -757,7 +757,9 @@ post_process_kernel_probe_trace_events(struct probe_trace_event *tevs,
 	}
 
 	for (i = 0; i < ntevs; i++) {
-		if (!tevs[i].point.address || tevs[i].point.retprobe)
+		if (!tevs[i].point.address)
+			continue;
+		if (tevs[i].point.retprobe && !kretprobe_offset_is_supported())
 			continue;
 		/* If we found a wrong one, mark it by NULL symbol */
 		if (kprobe_warn_out_range(tevs[i].point.symbol,
@@ -1528,11 +1530,6 @@ static int parse_perf_probe_point(char *arg, struct perf_probe_event *pev)
 		return -EINVAL;
 	}
 
-	if (pp->retprobe && !pp->function) {
-		semantic_error("Return probe requires an entry function.\n");
-		return -EINVAL;
-	}
-
 	if ((pp->offset || pp->line || pp->lazy_line) && pp->retprobe) {
 		semantic_error("Offset/Line/Lazy pattern can't be used with "
 			       "return probe.\n");
@@ -2841,7 +2838,8 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
 	}
 
 	/* Note that the symbols in the kmodule are not relocated */
-	if (!pev->uprobes && !pp->retprobe && !pev->target) {
+	if (!pev->uprobes && !pev->target &&
+			(!pp->retprobe || kretprobe_offset_is_supported())) {
 		reloc_sym = kernel_get_ref_reloc_sym();
 		if (!reloc_sym) {
 			pr_warning("Relocated base symbol is not found!\n");
diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
index 8a219cd831b7..1542cd0d6799 100644
--- a/tools/perf/util/probe-file.c
+++ b/tools/perf/util/probe-file.c
@@ -879,6 +879,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_END,
 };
 
@@ -889,6 +890,7 @@ static struct {
 #define DEFINE_TYPE(idx, pat)			\
 	[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): *"),
 };
 
 static bool scan_ftrace_readme(enum ftrace_readme type)
@@ -939,3 +941,8 @@ bool probe_type_is_available(enum probe_type type)
 
 	return true;
 }
+
+bool kretprobe_offset_is_supported(void)
+{
+	return scan_ftrace_readme(FTRACE_README_KRETPROBE_OFFSET);
+}
diff --git a/tools/perf/util/probe-file.h b/tools/perf/util/probe-file.h
index a17a82eff8a0..dbf95a00864a 100644
--- a/tools/perf/util/probe-file.h
+++ b/tools/perf/util/probe-file.h
@@ -65,6 +65,7 @@ struct probe_cache_entry *probe_cache__find_by_name(struct probe_cache *pcache,
 					const char *group, const char *event);
 int probe_cache__show_all_caches(struct strfilter *filter);
 bool probe_type_is_available(enum probe_type type);
+bool kretprobe_offset_is_supported(void);
 #else	/* ! HAVE_LIBELF_SUPPORT */
 static inline struct probe_cache *probe_cache__new(const char *tgt __maybe_unused)
 {
-- 
2.11.1

[toc] | [next] | [standalone]

#1592367 — Re: [PATCH v4 2/3] perf: kretprobes: offset from reloc_sym if kernel supports it

On Thu,  2 Mar 2017 23:25:06 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> We indicate support for accepting sym+offset with kretprobes through a
> line in ftrace README. Parse the same to identify support and choose the
> appropriate format for kprobe_events.

Could you give us an example of this change here? :)
for example, comment of commit 613f050d68a8 .

I think the code is OK, but we need actual example of result.

Thanks,

> 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  tools/perf/util/probe-event.c | 12 +++++-------
>  tools/perf/util/probe-file.c  |  7 +++++++
>  tools/perf/util/probe-file.h  |  1 +
>  3 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 35f5b7b7715c..faf5789902f5 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -757,7 +757,9 @@ post_process_kernel_probe_trace_events(struct probe_trace_event *tevs,
>  	}
>  
>  	for (i = 0; i < ntevs; i++) {
> -		if (!tevs[i].point.address || tevs[i].point.retprobe)
> +		if (!tevs[i].point.address)
> +			continue;
> +		if (tevs[i].point.retprobe && !kretprobe_offset_is_supported())
>  			continue;
>  		/* If we found a wrong one, mark it by NULL symbol */
>  		if (kprobe_warn_out_range(tevs[i].point.symbol,
> @@ -1528,11 +1530,6 @@ static int parse_perf_probe_point(char *arg, struct perf_probe_event *pev)
>  		return -EINVAL;
>  	}
>  
> -	if (pp->retprobe && !pp->function) {
> -		semantic_error("Return probe requires an entry function.\n");
> -		return -EINVAL;
> -	}
> -
>  	if ((pp->offset || pp->line || pp->lazy_line) && pp->retprobe) {
>  		semantic_error("Offset/Line/Lazy pattern can't be used with "
>  			       "return probe.\n");
> @@ -2841,7 +2838,8 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
>  	}
>  
>  	/* Note that the symbols in the kmodule are not relocated */
> -	if (!pev->uprobes && !pp->retprobe && !pev->target) {
> +	if (!pev->uprobes && !pev->target &&
> +			(!pp->retprobe || kretprobe_offset_is_supported())) {
>  		reloc_sym = kernel_get_ref_reloc_sym();
>  		if (!reloc_sym) {
>  			pr_warning("Relocated base symbol is not found!\n");
> diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
> index 8a219cd831b7..1542cd0d6799 100644
> --- a/tools/perf/util/probe-file.c
> +++ b/tools/perf/util/probe-file.c
> @@ -879,6 +879,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_END,
>  };
>  
> @@ -889,6 +890,7 @@ static struct {
>  #define DEFINE_TYPE(idx, pat)			\
>  	[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): *"),
>  };
>  
>  static bool scan_ftrace_readme(enum ftrace_readme type)
> @@ -939,3 +941,8 @@ bool probe_type_is_available(enum probe_type type)
>  
>  	return true;
>  }
> +
> +bool kretprobe_offset_is_supported(void)
> +{
> +	return scan_ftrace_readme(FTRACE_README_KRETPROBE_OFFSET);
> +}
> diff --git a/tools/perf/util/probe-file.h b/tools/perf/util/probe-file.h
> index a17a82eff8a0..dbf95a00864a 100644
> --- a/tools/perf/util/probe-file.h
> +++ b/tools/perf/util/probe-file.h
> @@ -65,6 +65,7 @@ struct probe_cache_entry *probe_cache__find_by_name(struct probe_cache *pcache,
>  					const char *group, const char *event);
>  int probe_cache__show_all_caches(struct strfilter *filter);
>  bool probe_type_is_available(enum probe_type type);
> +bool kretprobe_offset_is_supported(void);
>  #else	/* ! HAVE_LIBELF_SUPPORT */
>  static inline struct probe_cache *probe_cache__new(const char *tgt __maybe_unused)
>  {
> -- 
> 2.11.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

[toc] | [prev] | [next] | [standalone]

#1592380 — Re: [PATCH v4 2/3] perf: kretprobes: offset from reloc_sym if kernel supports it

On Sat, 4 Mar 2017 09:49:11 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> On Thu,  2 Mar 2017 23:25:06 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 
> > We indicate support for accepting sym+offset with kretprobes through a
> > line in ftrace README. Parse the same to identify support and choose the
> > appropriate format for kprobe_events.
> 
> Could you give us an example of this change here? :)
> for example, comment of commit 613f050d68a8 .
> 
> I think the code is OK, but we need actual example of result.

Hi Naveen,

I've tried following commands

$ grep "[Tt] user_read$" /proc/kallsyms  
0000000000000000 T user_read
0000000000000000 t user_read
$ sudo ./perf probe -D user_read%return
r:probe/user_read _text+3539616
r:probe/user_read_1 _text+3653408

OK, looks good. However, when I set the retprobes, I got an error.

$ sudo ./perf probe -a user_read%return
Failed to write event: Invalid argument
  Error: Failed to add events.

And kernel rejected that.

$ dmesg -k | tail -n 1
[  850.315068] Given offset is not valid for return probe.

Hmm, curious..

I tried normal probes

$ sudo ./perf probe -D user_read
p:probe/user_read _text+3539616
p:probe/user_read_1 _text+3653408
$ sudo ./perf probe -a user_read
Added new events:
  probe:user_read      (on user_read)
  probe:user_read_1    (on user_read)

You can now use it in all perf tools, such as:

	perf record -e probe:user_read_1 -aR sleep 1

It works!

$ sudo ./perf probe -l
  probe:user_read      (on user_read@security/keys/user_defined.c)
  probe:user_read_1    (on user_read@selinux/ss/policydb.c)
$ sudo cat /sys/kernel/debug/kprobes/list
ffffffff9237bf20  k  user_read+0x0    [DISABLED][FTRACE]
ffffffff923602a0  k  user_read+0x0    [DISABLED][FTRACE]

So, the both "_text+3539616" and "_text+3653408" are correctly located
on the entry address of user_read functions. It seems kernel-side
symbol+offset check is wrong.

Thank you,


> 
> Thanks,
> 
> > 
> > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> > ---
> >  tools/perf/util/probe-event.c | 12 +++++-------
> >  tools/perf/util/probe-file.c  |  7 +++++++
> >  tools/perf/util/probe-file.h  |  1 +
> >  3 files changed, 13 insertions(+), 7 deletions(-)
> > 
> > diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> > index 35f5b7b7715c..faf5789902f5 100644
> > --- a/tools/perf/util/probe-event.c
> > +++ b/tools/perf/util/probe-event.c
> > @@ -757,7 +757,9 @@ post_process_kernel_probe_trace_events(struct probe_trace_event *tevs,
> >  	}
> >  
> >  	for (i = 0; i < ntevs; i++) {
> > -		if (!tevs[i].point.address || tevs[i].point.retprobe)
> > +		if (!tevs[i].point.address)
> > +			continue;
> > +		if (tevs[i].point.retprobe && !kretprobe_offset_is_supported())
> >  			continue;
> >  		/* If we found a wrong one, mark it by NULL symbol */
> >  		if (kprobe_warn_out_range(tevs[i].point.symbol,
> > @@ -1528,11 +1530,6 @@ static int parse_perf_probe_point(char *arg, struct perf_probe_event *pev)
> >  		return -EINVAL;
> >  	}
> >  
> > -	if (pp->retprobe && !pp->function) {
> > -		semantic_error("Return probe requires an entry function.\n");
> > -		return -EINVAL;
> > -	}
> > -
> >  	if ((pp->offset || pp->line || pp->lazy_line) && pp->retprobe) {
> >  		semantic_error("Offset/Line/Lazy pattern can't be used with "
> >  			       "return probe.\n");
> > @@ -2841,7 +2838,8 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
> >  	}
> >  
> >  	/* Note that the symbols in the kmodule are not relocated */
> > -	if (!pev->uprobes && !pp->retprobe && !pev->target) {
> > +	if (!pev->uprobes && !pev->target &&
> > +			(!pp->retprobe || kretprobe_offset_is_supported())) {
> >  		reloc_sym = kernel_get_ref_reloc_sym();
> >  		if (!reloc_sym) {
> >  			pr_warning("Relocated base symbol is not found!\n");
> > diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
> > index 8a219cd831b7..1542cd0d6799 100644
> > --- a/tools/perf/util/probe-file.c
> > +++ b/tools/perf/util/probe-file.c
> > @@ -879,6 +879,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_END,
> >  };
> >  
> > @@ -889,6 +890,7 @@ static struct {
> >  #define DEFINE_TYPE(idx, pat)			\
> >  	[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): *"),
> >  };
> >  
> >  static bool scan_ftrace_readme(enum ftrace_readme type)
> > @@ -939,3 +941,8 @@ bool probe_type_is_available(enum probe_type type)
> >  
> >  	return true;
> >  }
> > +
> > +bool kretprobe_offset_is_supported(void)
> > +{
> > +	return scan_ftrace_readme(FTRACE_README_KRETPROBE_OFFSET);
> > +}
> > diff --git a/tools/perf/util/probe-file.h b/tools/perf/util/probe-file.h
> > index a17a82eff8a0..dbf95a00864a 100644
> > --- a/tools/perf/util/probe-file.h
> > +++ b/tools/perf/util/probe-file.h
> > @@ -65,6 +65,7 @@ struct probe_cache_entry *probe_cache__find_by_name(struct probe_cache *pcache,
> >  					const char *group, const char *event);
> >  int probe_cache__show_all_caches(struct strfilter *filter);
> >  bool probe_type_is_available(enum probe_type type);
> > +bool kretprobe_offset_is_supported(void);
> >  #else	/* ! HAVE_LIBELF_SUPPORT */
> >  static inline struct probe_cache *probe_cache__new(const char *tgt __maybe_unused)
> >  {
> > -- 
> > 2.11.1
> > 
> 
> 
> -- 
> Masami Hiramatsu <mhiramat@kernel.org>


-- 
Masami Hiramatsu <mhiramat@kernel.org>

[toc] | [prev] | [next] | [standalone]

#1592385 — Re: [PATCH v4 2/3] perf: kretprobes: offset from reloc_sym if kernel supports it

On Sat, 4 Mar 2017 11:35:51 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> On Sat, 4 Mar 2017 09:49:11 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > On Thu,  2 Mar 2017 23:25:06 +0530
> > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> > 
> > > We indicate support for accepting sym+offset with kretprobes through a
> > > line in ftrace README. Parse the same to identify support and choose the
> > > appropriate format for kprobe_events.
> > 
> > Could you give us an example of this change here? :)
> > for example, comment of commit 613f050d68a8 .
> > 
> > I think the code is OK, but we need actual example of result.
> 
> Hi Naveen,
> 
> I've tried following commands
> 
> $ grep "[Tt] user_read$" /proc/kallsyms  
> 0000000000000000 T user_read
> 0000000000000000 t user_read
> $ sudo ./perf probe -D user_read%return
> r:probe/user_read _text+3539616
> r:probe/user_read_1 _text+3653408
> 
> OK, looks good. However, when I set the retprobes, I got an error.
> 
> $ sudo ./perf probe -a user_read%return
> Failed to write event: Invalid argument
>   Error: Failed to add events.
> 
> And kernel rejected that.
> 
> $ dmesg -k | tail -n 1
> [  850.315068] Given offset is not valid for return probe.
> 
> Hmm, curious..
> 
> I tried normal probes
> 
> $ sudo ./perf probe -D user_read
> p:probe/user_read _text+3539616
> p:probe/user_read_1 _text+3653408
> $ sudo ./perf probe -a user_read
> Added new events:
>   probe:user_read      (on user_read)
>   probe:user_read_1    (on user_read)
> 
> You can now use it in all perf tools, such as:
> 
> 	perf record -e probe:user_read_1 -aR sleep 1
> 
> It works!
> 
> $ sudo ./perf probe -l
>   probe:user_read      (on user_read@security/keys/user_defined.c)
>   probe:user_read_1    (on user_read@selinux/ss/policydb.c)
> $ sudo cat /sys/kernel/debug/kprobes/list
> ffffffff9237bf20  k  user_read+0x0    [DISABLED][FTRACE]
> ffffffff923602a0  k  user_read+0x0    [DISABLED][FTRACE]
> 
> So, the both "_text+3539616" and "_text+3653408" are correctly located
> on the entry address of user_read functions. It seems kernel-side
> symbol+offset check is wrong.

FYI, without this patch, perf probe returns same place for same-name
functions. So this patch itself looks good.

$ sudo ./perf probe -D user_read%return
r:probe/user_read user_read+0
r:probe/user_read_1 user_read+0


Thanks,


-- 
Masami Hiramatsu <mhiramat@kernel.org>

[toc] | [prev] | [next] | [standalone]

#1592392 — Re: [PATCH v4 2/3] perf: kretprobes: offset from reloc_sym if kernel supports it

[Multipart message — attachments visible in raw view] — view raw

On Sat, 4 Mar 2017 11:35:51 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> On Sat, 4 Mar 2017 09:49:11 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > On Thu,  2 Mar 2017 23:25:06 +0530
> > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> > 
> > > We indicate support for accepting sym+offset with kretprobes through a
> > > line in ftrace README. Parse the same to identify support and choose the
> > > appropriate format for kprobe_events.
> > 
> > Could you give us an example of this change here? :)
> > for example, comment of commit 613f050d68a8 .
> > 
> > I think the code is OK, but we need actual example of result.
> 
> Hi Naveen,
> 
> I've tried following commands
> 
> $ grep "[Tt] user_read$" /proc/kallsyms  
> 0000000000000000 T user_read
> 0000000000000000 t user_read
> $ sudo ./perf probe -D user_read%return
> r:probe/user_read _text+3539616
> r:probe/user_read_1 _text+3653408
> 
> OK, looks good. However, when I set the retprobes, I got an error.
> 
> $ sudo ./perf probe -a user_read%return
> Failed to write event: Invalid argument
>   Error: Failed to add events.
> 
> And kernel rejected that.
> 
> $ dmesg -k | tail -n 1
> [  850.315068] Given offset is not valid for return probe.
> 
> Hmm, curious..

Ah, I see.

static int create_trace_kprobe(int argc, char **argv)
...
        } else {
                /* a symbol specified */
                symbol = argv[1];
                /* TODO: support .init module functions */
                ret = traceprobe_split_symbol_offset(symbol, &offset);
                if (ret) {
                        pr_info("Failed to parse symbol.\n");
                        return ret;
                }
                if (offset && is_return &&
                    !arch_function_offset_within_entry(offset)) {
                        pr_info("Given offset is not valid for return probe.\n");
                        return -EINVAL;
                }
        }

So, actually, traceprobe_split_symbol_offset() just split out symbol
and offset from symbol string (e.g. "_text+3539616").
So, you should use kallsyms_lookup_size_offset() here again to check
offset.

Please try attached patch (I've already tested on x86-64).

$ sudo ./perf probe -a user_read%return
Added new events:
  probe:user_read      (on user_read%return)
  probe:user_read_1    (on user_read%return)

You can now use it in all perf tools, such as:

	perf record -e probe:user_read_1 -aR sleep 1

$ sudo ./perf probe -l
  probe:user_read      (on user_read%return@security/keys/user_defined.c)
  probe:user_read_1    (on user_read%return@selinux/ss/policydb.c)
$ sudo cat /sys/kernel/debug/kprobes/list
ffffffff9637bf70  r  user_read+0x0    [DISABLED][FTRACE]
ffffffff963602f0  r  user_read+0x0    [DISABLED][FTRACE]

Thank you,


-- 
Masami Hiramatsu <mhiramat@kernel.org>

[toc] | [prev] | [next] | [standalone]

#1593601 — Re: [PATCH v4 2/3] perf: kretprobes: offset from reloc_sym if kernel supports it

On 2017/03/04 01:34PM, Masami Hiramatsu wrote:
> On Sat, 4 Mar 2017 11:35:51 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > On Sat, 4 Mar 2017 09:49:11 +0900
> > Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > 
> > > On Thu,  2 Mar 2017 23:25:06 +0530
> > > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> > > 
> > > > We indicate support for accepting sym+offset with kretprobes through a
> > > > line in ftrace README. Parse the same to identify support and choose the
> > > > appropriate format for kprobe_events.
> > > 
> > > Could you give us an example of this change here? :)
> > > for example, comment of commit 613f050d68a8 .
> > > 
> > > I think the code is OK, but we need actual example of result.
> > 
> > Hi Naveen,
> > 
> > I've tried following commands
> > 
> > $ grep "[Tt] user_read$" /proc/kallsyms  
> > 0000000000000000 T user_read
> > 0000000000000000 t user_read
> > $ sudo ./perf probe -D user_read%return
> > r:probe/user_read _text+3539616
> > r:probe/user_read_1 _text+3653408
> > 
> > OK, looks good. However, when I set the retprobes, I got an error.
> > 
> > $ sudo ./perf probe -a user_read%return
> > Failed to write event: Invalid argument
> >   Error: Failed to add events.
> > 
> > And kernel rejected that.
> > 
> > $ dmesg -k | tail -n 1
> > [  850.315068] Given offset is not valid for return probe.
> > 
> > Hmm, curious..
> 
> Ah, I see.
> 
> static int create_trace_kprobe(int argc, char **argv)
> ...
>         } else {
>                 /* a symbol specified */
>                 symbol = argv[1];
>                 /* TODO: support .init module functions */
>                 ret = traceprobe_split_symbol_offset(symbol, &offset);
>                 if (ret) {
>                         pr_info("Failed to parse symbol.\n");
>                         return ret;
>                 }
>                 if (offset && is_return &&
>                     !arch_function_offset_within_entry(offset)) {
>                         pr_info("Given offset is not valid for return probe.\n");
>                         return -EINVAL;
>                 }
>         }
> 
> So, actually, traceprobe_split_symbol_offset() just split out symbol
> and offset from symbol string (e.g. "_text+3539616").
> So, you should use kallsyms_lookup_size_offset() here again to check
> offset.

Ah, nice catch! I should have tested Steven's patch...

> 
> Please try attached patch (I've already tested on x86-64).
> 
> $ sudo ./perf probe -a user_read%return
> Added new events:
>   probe:user_read      (on user_read%return)
>   probe:user_read_1    (on user_read%return)
> 
> You can now use it in all perf tools, such as:
> 
> 	perf record -e probe:user_read_1 -aR sleep 1
> 
> $ sudo ./perf probe -l
>   probe:user_read      (on user_read%return@security/keys/user_defined.c)
>   probe:user_read_1    (on user_read%return@selinux/ss/policydb.c)
> $ sudo cat /sys/kernel/debug/kprobes/list
> ffffffff9637bf70  r  user_read+0x0    [DISABLED][FTRACE]
> ffffffff963602f0  r  user_read+0x0    [DISABLED][FTRACE]

Thanks, I will test this.


- Naveen

[toc] | [prev] | [next] | [standalone]

#1593734 — Re: [PATCH v4 2/3] perf: kretprobes: offset from reloc_sym if kernel supports it

On Mon,  6 Mar 2017 23:19:09 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> Masami,
> Your patch works, thanks! However, I felt we could refactor and reuse
> some of the code across kprobes.c for this purpose. Can you please see
> if the below patch is fine?

OK, looks good to me:)

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

Thanks!

> 
> Thanks,
> Naveen
> 
> --
> trace/kprobes: fix check for kretprobe offset within function entry
> 
> perf specifies an offset from _text and since this offset is fed
> directly into the arch-specific helper, kprobes tracer rejects
> installation of kretprobes through perf. Fix this by looking up the
> actual offset from a function for the specified sym+offset.
> 
> Refactor and reuse existing routines to limit code duplication -- we
> repurpose kprobe_addr() for determining final kprobe address and we
> split out the function entry offset determination into a separate
> generic helper.
> 
> Before patch:
> 
>   naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe -v do_open%return
>   probe-definition(0): do_open%return
>   symbol:do_open file:(null) line:0 offset:0 return:1 lazy:(null)
>   0 arguments
>   Looking at the vmlinux_path (8 entries long)
>   Using /boot/vmlinux for symbols
>   Open Debuginfo file: /boot/vmlinux
>   Try to find probe point from debuginfo.
>   Matched function: do_open [2d0c7ff]
>   Probe point found: do_open+0
>   Matched function: do_open [35d76dc]
>   found inline addr: 0xc0000000004ba9c4
>   Failed to find "do_open%return",
>    because do_open is an inlined function and has no return point.
>   An error occurred in debuginfo analysis (-22).
>   Trying to use symbols.
>   Opening /sys/kernel/debug/tracing//README write=0
>   Opening /sys/kernel/debug/tracing//kprobe_events write=1
>   Writing event: r:probe/do_open _text+4469776
>   Failed to write event: Invalid argument
>     Error: Failed to add events. Reason: Invalid argument (Code: -22)
>   naveen@ubuntu:~/linux/tools/perf$ dmesg | tail
>   <snip>
>   [   33.568656] Given offset is not valid for return probe.
> 
> After patch:
> 
>   naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe -v do_open%return
>   probe-definition(0): do_open%return
>   symbol:do_open file:(null) line:0 offset:0 return:1 lazy:(null)
>   0 arguments
>   Looking at the vmlinux_path (8 entries long)
>   Using /boot/vmlinux for symbols
>   Open Debuginfo file: /boot/vmlinux
>   Try to find probe point from debuginfo.
>   Matched function: do_open [2d0c7d6]
>   Probe point found: do_open+0
>   Matched function: do_open [35d76b3]
>   found inline addr: 0xc0000000004ba9e4
>   Failed to find "do_open%return",
>    because do_open is an inlined function and has no return point.
>   An error occurred in debuginfo analysis (-22).
>   Trying to use symbols.
>   Opening /sys/kernel/debug/tracing//README write=0
>   Opening /sys/kernel/debug/tracing//kprobe_events write=1
>   Writing event: r:probe/do_open _text+4469808
>   Writing event: r:probe/do_open_1 _text+4956344
>   Added new events:
>     probe:do_open        (on do_open%return)
>     probe:do_open_1      (on do_open%return)
> 
>   You can now use it in all perf tools, such as:
> 
> 	  perf record -e probe:do_open_1 -aR sleep 1
> 
>   naveen@ubuntu:~/linux/tools/perf$ sudo cat /sys/kernel/debug/kprobes/list
>   c000000000041370  k  kretprobe_trampoline+0x0    [OPTIMIZED]
>   c0000000004ba0b8  r  do_open+0x8    [DISABLED]
>   c000000000443430  r  do_open+0x0    [DISABLED]
> 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  include/linux/kprobes.h     |  1 +
>  kernel/kprobes.c            | 40 ++++++++++++++++++++++++++--------------
>  kernel/trace/trace_kprobe.c |  2 +-
>  3 files changed, 28 insertions(+), 15 deletions(-)
> 
> diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
> index 862f87adcbb4..6888c9f29cb6 100644
> --- a/include/linux/kprobes.h
> +++ b/include/linux/kprobes.h
> @@ -267,6 +267,7 @@ extern void show_registers(struct pt_regs *regs);
>  extern void kprobes_inc_nmissed_count(struct kprobe *p);
>  extern bool arch_within_kprobe_blacklist(unsigned long addr);
>  extern bool arch_function_offset_within_entry(unsigned long offset);
> +extern bool function_offset_within_entry(kprobe_opcode_t *addr, const char *sym, unsigned long offset);
>  
>  extern bool within_kprobe_blacklist(unsigned long addr);
>  
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 524766563896..49f870ea4070 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1391,21 +1391,19 @@ bool within_kprobe_blacklist(unsigned long addr)
>   * This returns encoded errors if it fails to look up symbol or invalid
>   * combination of parameters.
>   */
> -static kprobe_opcode_t *kprobe_addr(struct kprobe *p)
> +static kprobe_opcode_t *_kprobe_addr(kprobe_opcode_t *addr,
> +			const char *symbol_name, unsigned int offset)
>  {
> -	kprobe_opcode_t *addr = p->addr;
> -
> -	if ((p->symbol_name && p->addr) ||
> -	    (!p->symbol_name && !p->addr))
> +	if ((symbol_name && addr) || (!symbol_name && !addr))
>  		goto invalid;
>  
> -	if (p->symbol_name) {
> -		kprobe_lookup_name(p->symbol_name, addr);
> +	if (symbol_name) {
> +		kprobe_lookup_name(symbol_name, addr);
>  		if (!addr)
>  			return ERR_PTR(-ENOENT);
>  	}
>  
> -	addr = (kprobe_opcode_t *)(((char *)addr) + p->offset);
> +	addr = (kprobe_opcode_t *)(((char *)addr) + offset);
>  	if (addr)
>  		return addr;
>  
> @@ -1413,6 +1411,11 @@ static kprobe_opcode_t *kprobe_addr(struct kprobe *p)
>  	return ERR_PTR(-EINVAL);
>  }
>  
> +static kprobe_opcode_t *kprobe_addr(struct kprobe *p)
> +{
> +	return _kprobe_addr(p->addr, p->symbol_name, p->offset);
> +}
> +
>  /* Check passed kprobe is valid and return kprobe in kprobe_table. */
>  static struct kprobe *__get_valid_kprobe(struct kprobe *p)
>  {
> @@ -1874,19 +1877,28 @@ bool __weak arch_function_offset_within_entry(unsigned long offset)
>      return !offset;
>  }
>  
> +bool function_offset_within_entry(kprobe_opcode_t *addr, const char *sym, unsigned long offset)
> +{
> +	kprobe_opcode_t *kp_addr = _kprobe_addr(addr, sym, offset);
> +
> +	if (IS_ERR(kp_addr))
> +		return false;
> +
> +	if (!kallsyms_lookup_size_offset((unsigned long)kp_addr, NULL, &offset) ||
> +						!arch_function_offset_within_entry(offset))
> +		return false;
> +
> +	return true;
> +}
> +
>  int register_kretprobe(struct kretprobe *rp)
>  {
>  	int ret = 0;
>  	struct kretprobe_instance *inst;
>  	int i;
>  	void *addr;
> -	unsigned long offset;
> -
> -	addr = kprobe_addr(&rp->kp);
> -	if (!kallsyms_lookup_size_offset((unsigned long)addr, NULL, &offset))
> -		return -EINVAL;
>  
> -	if (!arch_function_offset_within_entry(offset))
> +	if (!function_offset_within_entry(rp->kp.addr, rp->kp.symbol_name, rp->kp.offset))
>  		return -EINVAL;
>  
>  	if (kretprobe_blacklist_size) {
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index c60f9dc4610e..828ef31a1c27 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -695,7 +695,7 @@ static int create_trace_kprobe(int argc, char **argv)
>  			return ret;
>  		}
>  		if (offset && is_return &&
> -		    !arch_function_offset_within_entry(offset)) {
> +		    !function_offset_within_entry(NULL, symbol, offset)) {
>  			pr_info("Given offset is not valid for return probe.\n");
>  			return -EINVAL;
>  		}
> -- 
> 2.11.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

[toc] | [prev] | [next] | [standalone]

#1594109 — Re: [PATCH v4 2/3] perf: kretprobes: offset from reloc_sym if kernel

On 2017/03/06 10:06PM, Masami Hiramatsu wrote:
> On Mon,  6 Mar 2017 23:19:09 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 
> > Masami,
> > Your patch works, thanks! However, I felt we could refactor and reuse
> > some of the code across kprobes.c for this purpose. Can you please see
> > if the below patch is fine?
> 
> OK, looks good to me:)
> 
> Acked-by: Masami Hiramatsu <mhiramat@kernel.org>

Thanks for the review, Masami!
I ended up adding one more patch to this series (patch 5/6) to move the
ftrace README scanning out of probe-file.c, as it doesn't need libelf.
Patch 6 fails to build without libelf otherwise. Please take a look.

Arnaldo,
I am re-sending the remaining patches in this series which apply on top
of the 4 patches you sent to Ingo, so as to keep this simple. All the
patches have been acked, except the new patch 5/6. Kindly take a look.

Thanks,
Naveen

--
Naveen N. Rao (6):
  trace/kprobes: fix check for kretprobe offset within function entry
  powerpc: kretprobes: override default function entry offset
  perf: probe: factor out the ftrace README scanning
  perf: kretprobes: offset from reloc_sym if kernel supports it
  perf: probes: move ftrace README parsing logic into
    trace-event-parse.c
  perf: powerpc: choose local entry point with kretprobes

 arch/powerpc/kernel/kprobes.c               |  9 +++
 include/linux/kprobes.h                     |  1 +
 kernel/kprobes.c                            | 40 ++++++++-----
 kernel/trace/trace_kprobe.c                 |  2 +-
 tools/perf/arch/powerpc/util/sym-handling.c | 10 ++--
 tools/perf/util/probe-event.c               | 12 ++--
 tools/perf/util/probe-file.c                | 80 +++-----------------------
 tools/perf/util/probe-file.h                |  1 -
 tools/perf/util/trace-event-parse.c         | 89 +++++++++++++++++++++++++++++
 tools/perf/util/trace-event.h               |  4 ++
 10 files changed, 149 insertions(+), 99 deletions(-)

-- 
2.11.1

[toc] | [prev] | [next] | [standalone]

#1594112 — [RESEND PATCH 6/6] perf: powerpc: choose local entry point with kretprobes

perf now uses an offset from _text/_stext for kretprobes if the kernel
supports it, rather than the actual function name. As such, let's choose
the LEP for powerpc ABIv2 so as to ensure the probe gets hit. Do it only
if the kernel supports specifying offsets with kretprobes.

Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 tools/perf/arch/powerpc/util/sym-handling.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/tools/perf/arch/powerpc/util/sym-handling.c b/tools/perf/arch/powerpc/util/sym-handling.c
index 1030a6e504bb..e93b3db25012 100644
--- a/tools/perf/arch/powerpc/util/sym-handling.c
+++ b/tools/perf/arch/powerpc/util/sym-handling.c
@@ -10,6 +10,7 @@
 #include "symbol.h"
 #include "map.h"
 #include "probe-event.h"
+#include "trace-event.h"
 
 #ifdef HAVE_LIBELF_SUPPORT
 bool elf__needs_adjust_symbols(GElf_Ehdr ehdr)
@@ -79,11 +80,12 @@ void arch__fix_tev_from_maps(struct perf_probe_event *pev,
 	 * However, if the user specifies an offset, we fall back to using the
 	 * GEP since all userspace applications (objdump/readelf) show function
 	 * disassembly with offsets from the GEP.
-	 *
-	 * In addition, we shouldn't specify an offset for kretprobes.
 	 */
-	if (pev->point.offset || (!pev->uprobes && pev->point.retprobe) ||
-	    !map || !sym)
+	if (pev->point.offset || !map || !sym)
+		return;
+
+	/* For kretprobes, add an offset only if the kernel supports it */
+	if (!pev->uprobes && pev->point.retprobe && !kretprobe_offset_is_supported())
 		return;
 
 	lep_offset = PPC64_LOCAL_ENTRY_OFFSET(sym->arch_sym);
-- 
2.11.1

[toc] | [prev] | [next] | [standalone]

#1594642 — [PATCH v2 6/6] perf: powerpc: choose local entry point with kretprobes

perf now uses an offset from _text/_stext for kretprobes if the kernel
supports it, rather than the actual function name. As such, let's choose
the LEP for powerpc ABIv2 so as to ensure the probe gets hit. Do it only
if the kernel supports specifying offsets with kretprobes.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
Changes:
- updated to address build issues due to dropping patch 5/6.

 tools/perf/arch/powerpc/util/sym-handling.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/tools/perf/arch/powerpc/util/sym-handling.c b/tools/perf/arch/powerpc/util/sym-handling.c
index 1030a6e504bb..b8cbefc49aca 100644
--- a/tools/perf/arch/powerpc/util/sym-handling.c
+++ b/tools/perf/arch/powerpc/util/sym-handling.c
@@ -10,6 +10,7 @@
 #include "symbol.h"
 #include "map.h"
 #include "probe-event.h"
+#include "probe-file.h"
 
 #ifdef HAVE_LIBELF_SUPPORT
 bool elf__needs_adjust_symbols(GElf_Ehdr ehdr)
@@ -79,11 +80,16 @@ void arch__fix_tev_from_maps(struct perf_probe_event *pev,
 	 * However, if the user specifies an offset, we fall back to using the
 	 * GEP since all userspace applications (objdump/readelf) show function
 	 * disassembly with offsets from the GEP.
-	 *
-	 * In addition, we shouldn't specify an offset for kretprobes.
 	 */
-	if (pev->point.offset || (!pev->uprobes && pev->point.retprobe) ||
-	    !map || !sym)
+	if (pev->point.offset || !map || !sym)
+		return;
+
+	/* For kretprobes, add an offset only if the kernel supports it */
+	if (!pev->uprobes && pev->point.retprobe
+#ifdef HAVE_LIBELF_SUPPORT
+			&& !kretprobe_offset_is_supported()
+#endif
+			)
 		return;
 
 	lep_offset = PPC64_LOCAL_ENTRY_OFFSET(sym->arch_sym);
-- 
2.11.1

[toc] | [prev] | [next] | [standalone]

#1594113 — [RESEND PATCH 2/6] powerpc: kretprobes: override default function entry offset

With ABIv2, we offset 8 bytes into a function to get at the local entry
point.

Acked-by: Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>
Acked-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/kprobes.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index fce05a38851c..331751701fed 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -131,6 +131,15 @@ static void __kprobes set_current_kprobe(struct kprobe *p, struct pt_regs *regs,
 	kcb->kprobe_saved_msr = regs->msr;
 }
 
+bool arch_function_offset_within_entry(unsigned long offset)
+{
+#ifdef PPC64_ELF_ABI_v2
+	return offset <= 8;
+#else
+	return !offset;
+#endif
+}
+
 void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri,
 				      struct pt_regs *regs)
 {
-- 
2.11.1

[toc] | [prev] | [next] | [standalone]

#1594118 — [PATCH 5/6] perf: probes: move ftrace README parsing logic into trace-event-parse.c

probe-file.c needs libelf, but scanning ftrace README does not require
that. As such, move the ftrace README scanning logic out of probe-file.c
and into trace-event-parse.c.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 tools/perf/util/probe-file.c        | 87 +++---------------------------------
 tools/perf/util/probe-file.h        |  2 -
 tools/perf/util/trace-event-parse.c | 89 +++++++++++++++++++++++++++++++++++++
 tools/perf/util/trace-event.h       |  4 ++
 4 files changed, 99 insertions(+), 83 deletions(-)

diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
index 1542cd0d6799..ff872fa30cdb 100644
--- a/tools/perf/util/probe-file.c
+++ b/tools/perf/util/probe-file.c
@@ -26,6 +26,7 @@
 #include <api/fs/tracing_path.h>
 #include "probe-event.h"
 #include "probe-file.h"
+#include "trace-event.h"
 #include "session.h"
 
 #define MAX_CMDLEN 256
@@ -70,33 +71,17 @@ static void print_both_open_warning(int kerr, int uerr)
 	}
 }
 
-int open_trace_file(const char *trace_file, bool readwrite)
-{
-	char buf[PATH_MAX];
-	int ret;
-
-	ret = e_snprintf(buf, PATH_MAX, "%s/%s",
-			 tracing_path, trace_file);
-	if (ret >= 0) {
-		pr_debug("Opening %s write=%d\n", buf, readwrite);
-		if (readwrite && !probe_event_dry_run)
-			ret = open(buf, O_RDWR | O_APPEND, 0);
-		else
-			ret = open(buf, O_RDONLY, 0);
-
-		if (ret < 0)
-			ret = -errno;
-	}
-	return ret;
-}
-
 static int open_kprobe_events(bool readwrite)
 {
+	if (probe_event_dry_run)
+		readwrite = false;
 	return open_trace_file("kprobe_events", readwrite);
 }
 
 static int open_uprobe_events(bool readwrite)
 {
+	if (probe_event_dry_run)
+		readwrite = false;
 	return open_trace_file("uprobe_events", readwrite);
 }
 
@@ -877,72 +862,12 @@ int probe_cache__show_all_caches(struct strfilter *filter)
 	return 0;
 }
 
-enum ftrace_readme {
-	FTRACE_README_PROBE_TYPE_X = 0,
-	FTRACE_README_KRETPROBE_OFFSET,
-	FTRACE_README_END,
-};
-
-static struct {
-	const char *pattern;
-	bool avail;
-} ftrace_readme_table[] = {
-#define DEFINE_TYPE(idx, pat)			\
-	[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): *"),
-};
-
-static bool scan_ftrace_readme(enum ftrace_readme type)
-{
-	int fd;
-	FILE *fp;
-	char *buf = NULL;
-	size_t len = 0;
-	bool ret = false;
-	static bool scanned = false;
-
-	if (scanned)
-		goto result;
-
-	fd = open_trace_file("README", false);
-	if (fd < 0)
-		return ret;
-
-	fp = fdopen(fd, "r");
-	if (!fp) {
-		close(fd);
-		return ret;
-	}
-
-	while (getline(&buf, &len, fp) > 0)
-		for (enum ftrace_readme i = 0; i < FTRACE_README_END; i++)
-			if (!ftrace_readme_table[i].avail)
-				ftrace_readme_table[i].avail =
-					strglobmatch(buf, ftrace_readme_table[i].pattern);
-	scanned = true;
-
-	fclose(fp);
-	free(buf);
-
-result:
-	if (type >= FTRACE_README_END)
-		return false;
-
-	return ftrace_readme_table[type].avail;
-}
-
 bool probe_type_is_available(enum probe_type type)
 {
 	if (type >= PROBE_TYPE_END)
 		return false;
 	else if (type == PROBE_TYPE_X)
-		return scan_ftrace_readme(FTRACE_README_PROBE_TYPE_X);
+		return probe_type_x_is_supported();
 
 	return true;
 }
-
-bool kretprobe_offset_is_supported(void)
-{
-	return scan_ftrace_readme(FTRACE_README_KRETPROBE_OFFSET);
-}
diff --git a/tools/perf/util/probe-file.h b/tools/perf/util/probe-file.h
index dbf95a00864a..eba44c3e9dca 100644
--- a/tools/perf/util/probe-file.h
+++ b/tools/perf/util/probe-file.h
@@ -35,7 +35,6 @@ enum probe_type {
 
 /* probe-file.c depends on libelf */
 #ifdef HAVE_LIBELF_SUPPORT
-int open_trace_file(const char *trace_file, bool readwrite);
 int probe_file__open(int flag);
 int probe_file__open_both(int *kfd, int *ufd, int flag);
 struct strlist *probe_file__get_namelist(int fd);
@@ -65,7 +64,6 @@ struct probe_cache_entry *probe_cache__find_by_name(struct probe_cache *pcache,
 					const char *group, const char *event);
 int probe_cache__show_all_caches(struct strfilter *filter);
 bool probe_type_is_available(enum probe_type type);
-bool kretprobe_offset_is_supported(void);
 #else	/* ! HAVE_LIBELF_SUPPORT */
 static inline struct probe_cache *probe_cache__new(const char *tgt __maybe_unused)
 {
diff --git a/tools/perf/util/trace-event-parse.c b/tools/perf/util/trace-event-parse.c
index de0078e21408..77697c446cbd 100644
--- a/tools/perf/util/trace-event-parse.c
+++ b/tools/perf/util/trace-event-parse.c
@@ -23,10 +23,12 @@
 #include <string.h>
 #include <ctype.h>
 #include <errno.h>
+#include <api/fs/tracing_path.h>
 
 #include "../perf.h"
 #include "util.h"
 #include "trace-event.h"
+#include "debug.h"
 
 static int get_common_field(struct scripting_context *context,
 			    int *offset, int *size, const char *type)
@@ -254,3 +256,90 @@ unsigned long long eval_flag(const char *flag)
 
 	return 0;
 }
+
+int open_trace_file(const char *trace_file, bool readwrite)
+{
+	char buf[PATH_MAX];
+	int ret;
+
+	ret = snprintf(buf, PATH_MAX, "%s/%s",
+			 tracing_path, trace_file);
+	if (ret >= PATH_MAX)
+		ret = -E2BIG;
+	if (ret >= 0) {
+		pr_debug("Opening %s write=%d\n", buf, readwrite);
+		if (readwrite)
+			ret = open(buf, O_RDWR | O_APPEND, 0);
+		else
+			ret = open(buf, O_RDONLY, 0);
+
+		if (ret < 0)
+			ret = -errno;
+	}
+	return ret;
+}
+
+enum ftrace_readme {
+	FTRACE_README_PROBE_TYPE_X = 0,
+	FTRACE_README_KRETPROBE_OFFSET,
+	FTRACE_README_END,
+};
+
+static struct {
+	const char *pattern;
+	bool avail;
+} ftrace_readme_table[] = {
+#define DEFINE_TYPE(idx, pat)			\
+	[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): *"),
+};
+
+static bool scan_ftrace_readme(enum ftrace_readme type)
+{
+	int fd;
+	FILE *fp;
+	char *buf = NULL;
+	size_t len = 0;
+	bool ret = false;
+	static bool scanned = false;
+
+	if (scanned)
+		goto result;
+
+	fd = open_trace_file("README", false);
+	if (fd < 0)
+		return ret;
+
+	fp = fdopen(fd, "r");
+	if (!fp) {
+		close(fd);
+		return ret;
+	}
+
+	while (getline(&buf, &len, fp) > 0)
+		for (enum ftrace_readme i = 0; i < FTRACE_README_END; i++)
+			if (!ftrace_readme_table[i].avail)
+				ftrace_readme_table[i].avail =
+					strglobmatch(buf, ftrace_readme_table[i].pattern);
+	scanned = true;
+
+	fclose(fp);
+	free(buf);
+
+result:
+	if (type >= FTRACE_README_END)
+		return false;
+
+	return ftrace_readme_table[type].avail;
+}
+
+bool kretprobe_offset_is_supported(void)
+{
+	return scan_ftrace_readme(FTRACE_README_KRETPROBE_OFFSET);
+}
+
+bool probe_type_x_is_supported(void)
+{
+	return scan_ftrace_readme(FTRACE_README_PROBE_TYPE_X);
+}
diff --git a/tools/perf/util/trace-event.h b/tools/perf/util/trace-event.h
index 1fbc044f9eb0..ecda2d822daf 100644
--- a/tools/perf/util/trace-event.h
+++ b/tools/perf/util/trace-event.h
@@ -37,6 +37,10 @@ int parse_ftrace_file(struct pevent *pevent, char *buf, unsigned long size);
 int parse_event_file(struct pevent *pevent,
 		     char *buf, unsigned long size, char *sys);
 
+int open_trace_file(const char *trace_file, bool readwrite);
+bool kretprobe_offset_is_supported(void);
+bool probe_type_x_is_supported(void);
+
 unsigned long long
 raw_field_value(struct event_format *event, const char *name, void *data);
 
-- 
2.11.1

[toc] | [prev] | [next] | [standalone]

#1594376 — Re: [PATCH 5/6] perf: probes: move ftrace README parsing logic into trace-event-parse.c

On Tue,  7 Mar 2017 16:17:40 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> probe-file.c needs libelf, but scanning ftrace README does not require
> that. As such, move the ftrace README scanning logic out of probe-file.c
> and into trace-event-parse.c.

As far as I can see, there is no reason to push this out from probe-file.c
because anyway this API using code requires libelf. Without this, I can
still build perf with NO_LIBELF=1. So I wouldn't like to pick this.
(I think we can drop this from this series)
Thank you,

> 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  tools/perf/util/probe-file.c        | 87 +++---------------------------------
>  tools/perf/util/probe-file.h        |  2 -
>  tools/perf/util/trace-event-parse.c | 89 +++++++++++++++++++++++++++++++++++++
>  tools/perf/util/trace-event.h       |  4 ++
>  4 files changed, 99 insertions(+), 83 deletions(-)
> 
> diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
> index 1542cd0d6799..ff872fa30cdb 100644
> --- a/tools/perf/util/probe-file.c
> +++ b/tools/perf/util/probe-file.c
> @@ -26,6 +26,7 @@
>  #include <api/fs/tracing_path.h>
>  #include "probe-event.h"
>  #include "probe-file.h"
> +#include "trace-event.h"
>  #include "session.h"
>  
>  #define MAX_CMDLEN 256
> @@ -70,33 +71,17 @@ static void print_both_open_warning(int kerr, int uerr)
>  	}
>  }
>  
> -int open_trace_file(const char *trace_file, bool readwrite)
> -{
> -	char buf[PATH_MAX];
> -	int ret;
> -
> -	ret = e_snprintf(buf, PATH_MAX, "%s/%s",
> -			 tracing_path, trace_file);
> -	if (ret >= 0) {
> -		pr_debug("Opening %s write=%d\n", buf, readwrite);
> -		if (readwrite && !probe_event_dry_run)
> -			ret = open(buf, O_RDWR | O_APPEND, 0);
> -		else
> -			ret = open(buf, O_RDONLY, 0);
> -
> -		if (ret < 0)
> -			ret = -errno;
> -	}
> -	return ret;
> -}
> -
>  static int open_kprobe_events(bool readwrite)
>  {
> +	if (probe_event_dry_run)
> +		readwrite = false;
>  	return open_trace_file("kprobe_events", readwrite);
>  }
>  
>  static int open_uprobe_events(bool readwrite)
>  {
> +	if (probe_event_dry_run)
> +		readwrite = false;
>  	return open_trace_file("uprobe_events", readwrite);
>  }
>  
> @@ -877,72 +862,12 @@ int probe_cache__show_all_caches(struct strfilter *filter)
>  	return 0;
>  }
>  
> -enum ftrace_readme {
> -	FTRACE_README_PROBE_TYPE_X = 0,
> -	FTRACE_README_KRETPROBE_OFFSET,
> -	FTRACE_README_END,
> -};
> -
> -static struct {
> -	const char *pattern;
> -	bool avail;
> -} ftrace_readme_table[] = {
> -#define DEFINE_TYPE(idx, pat)			\
> -	[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): *"),
> -};
> -
> -static bool scan_ftrace_readme(enum ftrace_readme type)
> -{
> -	int fd;
> -	FILE *fp;
> -	char *buf = NULL;
> -	size_t len = 0;
> -	bool ret = false;
> -	static bool scanned = false;
> -
> -	if (scanned)
> -		goto result;
> -
> -	fd = open_trace_file("README", false);
> -	if (fd < 0)
> -		return ret;
> -
> -	fp = fdopen(fd, "r");
> -	if (!fp) {
> -		close(fd);
> -		return ret;
> -	}
> -
> -	while (getline(&buf, &len, fp) > 0)
> -		for (enum ftrace_readme i = 0; i < FTRACE_README_END; i++)
> -			if (!ftrace_readme_table[i].avail)
> -				ftrace_readme_table[i].avail =
> -					strglobmatch(buf, ftrace_readme_table[i].pattern);
> -	scanned = true;
> -
> -	fclose(fp);
> -	free(buf);
> -
> -result:
> -	if (type >= FTRACE_README_END)
> -		return false;
> -
> -	return ftrace_readme_table[type].avail;
> -}
> -
>  bool probe_type_is_available(enum probe_type type)
>  {
>  	if (type >= PROBE_TYPE_END)
>  		return false;
>  	else if (type == PROBE_TYPE_X)
> -		return scan_ftrace_readme(FTRACE_README_PROBE_TYPE_X);
> +		return probe_type_x_is_supported();
>  
>  	return true;
>  }
> -
> -bool kretprobe_offset_is_supported(void)
> -{
> -	return scan_ftrace_readme(FTRACE_README_KRETPROBE_OFFSET);
> -}
> diff --git a/tools/perf/util/probe-file.h b/tools/perf/util/probe-file.h
> index dbf95a00864a..eba44c3e9dca 100644
> --- a/tools/perf/util/probe-file.h
> +++ b/tools/perf/util/probe-file.h
> @@ -35,7 +35,6 @@ enum probe_type {
>  
>  /* probe-file.c depends on libelf */
>  #ifdef HAVE_LIBELF_SUPPORT
> -int open_trace_file(const char *trace_file, bool readwrite);
>  int probe_file__open(int flag);
>  int probe_file__open_both(int *kfd, int *ufd, int flag);
>  struct strlist *probe_file__get_namelist(int fd);
> @@ -65,7 +64,6 @@ struct probe_cache_entry *probe_cache__find_by_name(struct probe_cache *pcache,
>  					const char *group, const char *event);
>  int probe_cache__show_all_caches(struct strfilter *filter);
>  bool probe_type_is_available(enum probe_type type);
> -bool kretprobe_offset_is_supported(void);
>  #else	/* ! HAVE_LIBELF_SUPPORT */
>  static inline struct probe_cache *probe_cache__new(const char *tgt __maybe_unused)
>  {
> diff --git a/tools/perf/util/trace-event-parse.c b/tools/perf/util/trace-event-parse.c
> index de0078e21408..77697c446cbd 100644
> --- a/tools/perf/util/trace-event-parse.c
> +++ b/tools/perf/util/trace-event-parse.c
> @@ -23,10 +23,12 @@
>  #include <string.h>
>  #include <ctype.h>
>  #include <errno.h>
> +#include <api/fs/tracing_path.h>
>  
>  #include "../perf.h"
>  #include "util.h"
>  #include "trace-event.h"
> +#include "debug.h"
>  
>  static int get_common_field(struct scripting_context *context,
>  			    int *offset, int *size, const char *type)
> @@ -254,3 +256,90 @@ unsigned long long eval_flag(const char *flag)
>  
>  	return 0;
>  }
> +
> +int open_trace_file(const char *trace_file, bool readwrite)
> +{
> +	char buf[PATH_MAX];
> +	int ret;
> +
> +	ret = snprintf(buf, PATH_MAX, "%s/%s",
> +			 tracing_path, trace_file);
> +	if (ret >= PATH_MAX)
> +		ret = -E2BIG;
> +	if (ret >= 0) {
> +		pr_debug("Opening %s write=%d\n", buf, readwrite);
> +		if (readwrite)
> +			ret = open(buf, O_RDWR | O_APPEND, 0);
> +		else
> +			ret = open(buf, O_RDONLY, 0);
> +
> +		if (ret < 0)
> +			ret = -errno;
> +	}
> +	return ret;
> +}
> +
> +enum ftrace_readme {
> +	FTRACE_README_PROBE_TYPE_X = 0,
> +	FTRACE_README_KRETPROBE_OFFSET,
> +	FTRACE_README_END,
> +};
> +
> +static struct {
> +	const char *pattern;
> +	bool avail;
> +} ftrace_readme_table[] = {
> +#define DEFINE_TYPE(idx, pat)			\
> +	[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): *"),
> +};
> +
> +static bool scan_ftrace_readme(enum ftrace_readme type)
> +{
> +	int fd;
> +	FILE *fp;
> +	char *buf = NULL;
> +	size_t len = 0;
> +	bool ret = false;
> +	static bool scanned = false;
> +
> +	if (scanned)
> +		goto result;
> +
> +	fd = open_trace_file("README", false);
> +	if (fd < 0)
> +		return ret;
> +
> +	fp = fdopen(fd, "r");
> +	if (!fp) {
> +		close(fd);
> +		return ret;
> +	}
> +
> +	while (getline(&buf, &len, fp) > 0)
> +		for (enum ftrace_readme i = 0; i < FTRACE_README_END; i++)
> +			if (!ftrace_readme_table[i].avail)
> +				ftrace_readme_table[i].avail =
> +					strglobmatch(buf, ftrace_readme_table[i].pattern);
> +	scanned = true;
> +
> +	fclose(fp);
> +	free(buf);
> +
> +result:
> +	if (type >= FTRACE_README_END)
> +		return false;
> +
> +	return ftrace_readme_table[type].avail;
> +}
> +
> +bool kretprobe_offset_is_supported(void)
> +{
> +	return scan_ftrace_readme(FTRACE_README_KRETPROBE_OFFSET);
> +}
> +
> +bool probe_type_x_is_supported(void)
> +{
> +	return scan_ftrace_readme(FTRACE_README_PROBE_TYPE_X);
> +}
> diff --git a/tools/perf/util/trace-event.h b/tools/perf/util/trace-event.h
> index 1fbc044f9eb0..ecda2d822daf 100644
> --- a/tools/perf/util/trace-event.h
> +++ b/tools/perf/util/trace-event.h
> @@ -37,6 +37,10 @@ int parse_ftrace_file(struct pevent *pevent, char *buf, unsigned long size);
>  int parse_event_file(struct pevent *pevent,
>  		     char *buf, unsigned long size, char *sys);
>  
> +int open_trace_file(const char *trace_file, bool readwrite);
> +bool kretprobe_offset_is_supported(void);
> +bool probe_type_x_is_supported(void);
> +
>  unsigned long long
>  raw_field_value(struct event_format *event, const char *name, void *data);
>  
> -- 
> 2.11.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

[toc] | [prev] | [next] | [standalone]

#1594590 — Re: [PATCH 5/6] perf: probes: move ftrace README parsing logic into trace-event-parse.c

On 2017/03/07 04:51PM, Masami Hiramatsu wrote:
> On Tue,  7 Mar 2017 16:17:40 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 
> > probe-file.c needs libelf, but scanning ftrace README does not require
> > that. As such, move the ftrace README scanning logic out of probe-file.c
> > and into trace-event-parse.c.
> 
> As far as I can see, there is no reason to push this out from probe-file.c
> because anyway this API using code requires libelf. Without this, I can
> still build perf with NO_LIBELF=1. So I wouldn't like to pick this.
> (I think we can drop this from this series)

Ok. We can drop this. I'll rework patch 6/6.

Thanks,
Naveen

[toc] | [prev] | [next] | [standalone]

#1594409 — Re: [PATCH 5/6] perf: probes: move ftrace README parsing logic into trace-event-parse.c

On 2017/03/07 03:03PM, Masami Hiramatsu wrote:
> On Tue,  7 Mar 2017 16:17:40 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 
> > probe-file.c needs libelf, but scanning ftrace README does not require
> > that. As such, move the ftrace README scanning logic out of probe-file.c
> > and into trace-event-parse.c.
> 
> Hmm, it seems probe-file.c doesn't require libelf at all...
> I would like to keep ftrace related things in probe-file.c.

Not sure I understand. probe-file.h explicitly calls out a need for 
libelf due to the probe cache and related routines - commit 
40218daea1db1 ("perf list: Show SDT and pre-cached events").

However, if you prefer to retain the ftrace README scanning here, we can 
drop this patch and I can update patch 6 to check for libelf.

Thanks,
Naveen

[toc] | [prev] | [next] | [standalone]

#1594474 — Re: [PATCH 5/6] perf: probes: move ftrace README parsing logic into trace-event-parse.c

On Tue,  7 Mar 2017 16:17:40 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> probe-file.c needs libelf, but scanning ftrace README does not require
> that. As such, move the ftrace README scanning logic out of probe-file.c
> and into trace-event-parse.c.

Hmm, it seems probe-file.c doesn't require libelf at all...
I would like to keep ftrace related things in probe-file.c.

Thanks,

> 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  tools/perf/util/probe-file.c        | 87 +++---------------------------------
>  tools/perf/util/probe-file.h        |  2 -
>  tools/perf/util/trace-event-parse.c | 89 +++++++++++++++++++++++++++++++++++++
>  tools/perf/util/trace-event.h       |  4 ++
>  4 files changed, 99 insertions(+), 83 deletions(-)
> 
> diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
> index 1542cd0d6799..ff872fa30cdb 100644
> --- a/tools/perf/util/probe-file.c
> +++ b/tools/perf/util/probe-file.c
> @@ -26,6 +26,7 @@
>  #include <api/fs/tracing_path.h>
>  #include "probe-event.h"
>  #include "probe-file.h"
> +#include "trace-event.h"
>  #include "session.h"
>  
>  #define MAX_CMDLEN 256
> @@ -70,33 +71,17 @@ static void print_both_open_warning(int kerr, int uerr)
>  	}
>  }
>  
> -int open_trace_file(const char *trace_file, bool readwrite)
> -{
> -	char buf[PATH_MAX];
> -	int ret;
> -
> -	ret = e_snprintf(buf, PATH_MAX, "%s/%s",
> -			 tracing_path, trace_file);
> -	if (ret >= 0) {
> -		pr_debug("Opening %s write=%d\n", buf, readwrite);
> -		if (readwrite && !probe_event_dry_run)
> -			ret = open(buf, O_RDWR | O_APPEND, 0);
> -		else
> -			ret = open(buf, O_RDONLY, 0);
> -
> -		if (ret < 0)
> -			ret = -errno;
> -	}
> -	return ret;
> -}
> -
>  static int open_kprobe_events(bool readwrite)
>  {
> +	if (probe_event_dry_run)
> +		readwrite = false;
>  	return open_trace_file("kprobe_events", readwrite);
>  }
>  
>  static int open_uprobe_events(bool readwrite)
>  {
> +	if (probe_event_dry_run)
> +		readwrite = false;
>  	return open_trace_file("uprobe_events", readwrite);
>  }
>  
> @@ -877,72 +862,12 @@ int probe_cache__show_all_caches(struct strfilter *filter)
>  	return 0;
>  }
>  
> -enum ftrace_readme {
> -	FTRACE_README_PROBE_TYPE_X = 0,
> -	FTRACE_README_KRETPROBE_OFFSET,
> -	FTRACE_README_END,
> -};
> -
> -static struct {
> -	const char *pattern;
> -	bool avail;
> -} ftrace_readme_table[] = {
> -#define DEFINE_TYPE(idx, pat)			\
> -	[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): *"),
> -};
> -
> -static bool scan_ftrace_readme(enum ftrace_readme type)
> -{
> -	int fd;
> -	FILE *fp;
> -	char *buf = NULL;
> -	size_t len = 0;
> -	bool ret = false;
> -	static bool scanned = false;
> -
> -	if (scanned)
> -		goto result;
> -
> -	fd = open_trace_file("README", false);
> -	if (fd < 0)
> -		return ret;
> -
> -	fp = fdopen(fd, "r");
> -	if (!fp) {
> -		close(fd);
> -		return ret;
> -	}
> -
> -	while (getline(&buf, &len, fp) > 0)
> -		for (enum ftrace_readme i = 0; i < FTRACE_README_END; i++)
> -			if (!ftrace_readme_table[i].avail)
> -				ftrace_readme_table[i].avail =
> -					strglobmatch(buf, ftrace_readme_table[i].pattern);
> -	scanned = true;
> -
> -	fclose(fp);
> -	free(buf);
> -
> -result:
> -	if (type >= FTRACE_README_END)
> -		return false;
> -
> -	return ftrace_readme_table[type].avail;
> -}
> -
>  bool probe_type_is_available(enum probe_type type)
>  {
>  	if (type >= PROBE_TYPE_END)
>  		return false;
>  	else if (type == PROBE_TYPE_X)
> -		return scan_ftrace_readme(FTRACE_README_PROBE_TYPE_X);
> +		return probe_type_x_is_supported();
>  
>  	return true;
>  }
> -
> -bool kretprobe_offset_is_supported(void)
> -{
> -	return scan_ftrace_readme(FTRACE_README_KRETPROBE_OFFSET);
> -}
> diff --git a/tools/perf/util/probe-file.h b/tools/perf/util/probe-file.h
> index dbf95a00864a..eba44c3e9dca 100644
> --- a/tools/perf/util/probe-file.h
> +++ b/tools/perf/util/probe-file.h
> @@ -35,7 +35,6 @@ enum probe_type {
>  
>  /* probe-file.c depends on libelf */
>  #ifdef HAVE_LIBELF_SUPPORT
> -int open_trace_file(const char *trace_file, bool readwrite);
>  int probe_file__open(int flag);
>  int probe_file__open_both(int *kfd, int *ufd, int flag);
>  struct strlist *probe_file__get_namelist(int fd);
> @@ -65,7 +64,6 @@ struct probe_cache_entry *probe_cache__find_by_name(struct probe_cache *pcache,
>  					const char *group, const char *event);
>  int probe_cache__show_all_caches(struct strfilter *filter);
>  bool probe_type_is_available(enum probe_type type);
> -bool kretprobe_offset_is_supported(void);
>  #else	/* ! HAVE_LIBELF_SUPPORT */
>  static inline struct probe_cache *probe_cache__new(const char *tgt __maybe_unused)
>  {
> diff --git a/tools/perf/util/trace-event-parse.c b/tools/perf/util/trace-event-parse.c
> index de0078e21408..77697c446cbd 100644
> --- a/tools/perf/util/trace-event-parse.c
> +++ b/tools/perf/util/trace-event-parse.c
> @@ -23,10 +23,12 @@
>  #include <string.h>
>  #include <ctype.h>
>  #include <errno.h>
> +#include <api/fs/tracing_path.h>
>  
>  #include "../perf.h"
>  #include "util.h"
>  #include "trace-event.h"
> +#include "debug.h"
>  
>  static int get_common_field(struct scripting_context *context,
>  			    int *offset, int *size, const char *type)
> @@ -254,3 +256,90 @@ unsigned long long eval_flag(const char *flag)
>  
>  	return 0;
>  }
> +
> +int open_trace_file(const char *trace_file, bool readwrite)
> +{
> +	char buf[PATH_MAX];
> +	int ret;
> +
> +	ret = snprintf(buf, PATH_MAX, "%s/%s",
> +			 tracing_path, trace_file);
> +	if (ret >= PATH_MAX)
> +		ret = -E2BIG;
> +	if (ret >= 0) {
> +		pr_debug("Opening %s write=%d\n", buf, readwrite);
> +		if (readwrite)
> +			ret = open(buf, O_RDWR | O_APPEND, 0);
> +		else
> +			ret = open(buf, O_RDONLY, 0);
> +
> +		if (ret < 0)
> +			ret = -errno;
> +	}
> +	return ret;
> +}
> +
> +enum ftrace_readme {
> +	FTRACE_README_PROBE_TYPE_X = 0,
> +	FTRACE_README_KRETPROBE_OFFSET,
> +	FTRACE_README_END,
> +};
> +
> +static struct {
> +	const char *pattern;
> +	bool avail;
> +} ftrace_readme_table[] = {
> +#define DEFINE_TYPE(idx, pat)			\
> +	[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): *"),
> +};
> +
> +static bool scan_ftrace_readme(enum ftrace_readme type)
> +{
> +	int fd;
> +	FILE *fp;
> +	char *buf = NULL;
> +	size_t len = 0;
> +	bool ret = false;
> +	static bool scanned = false;
> +
> +	if (scanned)
> +		goto result;
> +
> +	fd = open_trace_file("README", false);
> +	if (fd < 0)
> +		return ret;
> +
> +	fp = fdopen(fd, "r");
> +	if (!fp) {
> +		close(fd);
> +		return ret;
> +	}
> +
> +	while (getline(&buf, &len, fp) > 0)
> +		for (enum ftrace_readme i = 0; i < FTRACE_README_END; i++)
> +			if (!ftrace_readme_table[i].avail)
> +				ftrace_readme_table[i].avail =
> +					strglobmatch(buf, ftrace_readme_table[i].pattern);
> +	scanned = true;
> +
> +	fclose(fp);
> +	free(buf);
> +
> +result:
> +	if (type >= FTRACE_README_END)
> +		return false;
> +
> +	return ftrace_readme_table[type].avail;
> +}
> +
> +bool kretprobe_offset_is_supported(void)
> +{
> +	return scan_ftrace_readme(FTRACE_README_KRETPROBE_OFFSET);
> +}
> +
> +bool probe_type_x_is_supported(void)
> +{
> +	return scan_ftrace_readme(FTRACE_README_PROBE_TYPE_X);
> +}
> diff --git a/tools/perf/util/trace-event.h b/tools/perf/util/trace-event.h
> index 1fbc044f9eb0..ecda2d822daf 100644
> --- a/tools/perf/util/trace-event.h
> +++ b/tools/perf/util/trace-event.h
> @@ -37,6 +37,10 @@ int parse_ftrace_file(struct pevent *pevent, char *buf, unsigned long size);
>  int parse_event_file(struct pevent *pevent,
>  		     char *buf, unsigned long size, char *sys);
>  
> +int open_trace_file(const char *trace_file, bool readwrite);
> +bool kretprobe_offset_is_supported(void);
> +bool probe_type_x_is_supported(void);
> +
>  unsigned long long
>  raw_field_value(struct event_format *event, const char *name, void *data);
>  
> -- 
> 2.11.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

[toc] | [prev] | [next] | [standalone]

#1593820

Masami,
Your patch works, thanks! However, I felt we could refactor and reuse
some of the code across kprobes.c for this purpose. Can you please see
if the below patch is fine?

Thanks,
Naveen

--
trace/kprobes: fix check for kretprobe offset within function entry

perf specifies an offset from _text and since this offset is fed
directly into the arch-specific helper, kprobes tracer rejects
installation of kretprobes through perf. Fix this by looking up the
actual offset from a function for the specified sym+offset.

Refactor and reuse existing routines to limit code duplication -- we
repurpose kprobe_addr() for determining final kprobe address and we
split out the function entry offset determination into a separate
generic helper.

Before patch:

  naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe -v do_open%return
  probe-definition(0): do_open%return
  symbol:do_open file:(null) line:0 offset:0 return:1 lazy:(null)
  0 arguments
  Looking at the vmlinux_path (8 entries long)
  Using /boot/vmlinux for symbols
  Open Debuginfo file: /boot/vmlinux
  Try to find probe point from debuginfo.
  Matched function: do_open [2d0c7ff]
  Probe point found: do_open+0
  Matched function: do_open [35d76dc]
  found inline addr: 0xc0000000004ba9c4
  Failed to find "do_open%return",
   because do_open is an inlined function and has no return point.
  An error occurred in debuginfo analysis (-22).
  Trying to use symbols.
  Opening /sys/kernel/debug/tracing//README write=0
  Opening /sys/kernel/debug/tracing//kprobe_events write=1
  Writing event: r:probe/do_open _text+4469776
  Failed to write event: Invalid argument
    Error: Failed to add events. Reason: Invalid argument (Code: -22)
  naveen@ubuntu:~/linux/tools/perf$ dmesg | tail
  <snip>
  [   33.568656] Given offset is not valid for return probe.

After patch:

  naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe -v do_open%return
  probe-definition(0): do_open%return
  symbol:do_open file:(null) line:0 offset:0 return:1 lazy:(null)
  0 arguments
  Looking at the vmlinux_path (8 entries long)
  Using /boot/vmlinux for symbols
  Open Debuginfo file: /boot/vmlinux
  Try to find probe point from debuginfo.
  Matched function: do_open [2d0c7d6]
  Probe point found: do_open+0
  Matched function: do_open [35d76b3]
  found inline addr: 0xc0000000004ba9e4
  Failed to find "do_open%return",
   because do_open is an inlined function and has no return point.
  An error occurred in debuginfo analysis (-22).
  Trying to use symbols.
  Opening /sys/kernel/debug/tracing//README write=0
  Opening /sys/kernel/debug/tracing//kprobe_events write=1
  Writing event: r:probe/do_open _text+4469808
  Writing event: r:probe/do_open_1 _text+4956344
  Added new events:
    probe:do_open        (on do_open%return)
    probe:do_open_1      (on do_open%return)

  You can now use it in all perf tools, such as:

	  perf record -e probe:do_open_1 -aR sleep 1

  naveen@ubuntu:~/linux/tools/perf$ sudo cat /sys/kernel/debug/kprobes/list
  c000000000041370  k  kretprobe_trampoline+0x0    [OPTIMIZED]
  c0000000004ba0b8  r  do_open+0x8    [DISABLED]
  c000000000443430  r  do_open+0x0    [DISABLED]

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 include/linux/kprobes.h     |  1 +
 kernel/kprobes.c            | 40 ++++++++++++++++++++++++++--------------
 kernel/trace/trace_kprobe.c |  2 +-
 3 files changed, 28 insertions(+), 15 deletions(-)

diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index 862f87adcbb4..6888c9f29cb6 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -267,6 +267,7 @@ extern void show_registers(struct pt_regs *regs);
 extern void kprobes_inc_nmissed_count(struct kprobe *p);
 extern bool arch_within_kprobe_blacklist(unsigned long addr);
 extern bool arch_function_offset_within_entry(unsigned long offset);
+extern bool function_offset_within_entry(kprobe_opcode_t *addr, const char *sym, unsigned long offset);
 
 extern bool within_kprobe_blacklist(unsigned long addr);
 
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 524766563896..49f870ea4070 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1391,21 +1391,19 @@ bool within_kprobe_blacklist(unsigned long addr)
  * This returns encoded errors if it fails to look up symbol or invalid
  * combination of parameters.
  */
-static kprobe_opcode_t *kprobe_addr(struct kprobe *p)
+static kprobe_opcode_t *_kprobe_addr(kprobe_opcode_t *addr,
+			const char *symbol_name, unsigned int offset)
 {
-	kprobe_opcode_t *addr = p->addr;
-
-	if ((p->symbol_name && p->addr) ||
-	    (!p->symbol_name && !p->addr))
+	if ((symbol_name && addr) || (!symbol_name && !addr))
 		goto invalid;
 
-	if (p->symbol_name) {
-		kprobe_lookup_name(p->symbol_name, addr);
+	if (symbol_name) {
+		kprobe_lookup_name(symbol_name, addr);
 		if (!addr)
 			return ERR_PTR(-ENOENT);
 	}
 
-	addr = (kprobe_opcode_t *)(((char *)addr) + p->offset);
+	addr = (kprobe_opcode_t *)(((char *)addr) + offset);
 	if (addr)
 		return addr;
 
@@ -1413,6 +1411,11 @@ static kprobe_opcode_t *kprobe_addr(struct kprobe *p)
 	return ERR_PTR(-EINVAL);
 }
 
+static kprobe_opcode_t *kprobe_addr(struct kprobe *p)
+{
+	return _kprobe_addr(p->addr, p->symbol_name, p->offset);
+}
+
 /* Check passed kprobe is valid and return kprobe in kprobe_table. */
 static struct kprobe *__get_valid_kprobe(struct kprobe *p)
 {
@@ -1874,19 +1877,28 @@ bool __weak arch_function_offset_within_entry(unsigned long offset)
     return !offset;
 }
 
+bool function_offset_within_entry(kprobe_opcode_t *addr, const char *sym, unsigned long offset)
+{
+	kprobe_opcode_t *kp_addr = _kprobe_addr(addr, sym, offset);
+
+	if (IS_ERR(kp_addr))
+		return false;
+
+	if (!kallsyms_lookup_size_offset((unsigned long)kp_addr, NULL, &offset) ||
+						!arch_function_offset_within_entry(offset))
+		return false;
+
+	return true;
+}
+
 int register_kretprobe(struct kretprobe *rp)
 {
 	int ret = 0;
 	struct kretprobe_instance *inst;
 	int i;
 	void *addr;
-	unsigned long offset;
-
-	addr = kprobe_addr(&rp->kp);
-	if (!kallsyms_lookup_size_offset((unsigned long)addr, NULL, &offset))
-		return -EINVAL;
 
-	if (!arch_function_offset_within_entry(offset))
+	if (!function_offset_within_entry(rp->kp.addr, rp->kp.symbol_name, rp->kp.offset))
 		return -EINVAL;
 
 	if (kretprobe_blacklist_size) {
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index c60f9dc4610e..828ef31a1c27 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -695,7 +695,7 @@ static int create_trace_kprobe(int argc, char **argv)
 			return ret;
 		}
 		if (offset && is_return &&
-		    !arch_function_offset_within_entry(offset)) {
+		    !function_offset_within_entry(NULL, symbol, offset)) {
 			pr_info("Given offset is not valid for return probe.\n");
 			return -EINVAL;
 		}
-- 
2.11.1

[toc] | [prev] | [next] | [standalone]

#1593464 — Re: [PATCH v4 2/3] perf: kretprobes: offset from reloc_sym if kernel supports it

On 2017/03/04 09:49AM, Masami Hiramatsu wrote:
> On Thu,  2 Mar 2017 23:25:06 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 
> > We indicate support for accepting sym+offset with kretprobes through a
> > line in ftrace README. Parse the same to identify support and choose the
> > appropriate format for kprobe_events.
> 
> Could you give us an example of this change here? :)
> for example, comment of commit 613f050d68a8 .
> 
> I think the code is OK, but we need actual example of result.

Sure :)
As an example, without this perf patch, but with the ftrace changes:

  naveen@ubuntu:~/linux/tools/perf$ sudo cat /sys/kernel/debug/tracing/README | grep kretprobe
  place (kretprobe): [<module>:]<symbol>[+<offset>]|<memaddr>
  naveen@ubuntu:~/linux/tools/perf$ 
  naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe -v do_open%return
  probe-definition(0): do_open%return 
  symbol:do_open file:(null) line:0 offset:0 return:1 lazy:(null)
  0 arguments
  Looking at the vmlinux_path (8 entries long)
  Using /boot/vmlinux for symbols
  Open Debuginfo file: /boot/vmlinux
  Try to find probe point from debuginfo.
  Matched function: do_open [2d0c7d8]
  Probe point found: do_open+0
  Matched function: do_open [35d76b5]
  found inline addr: 0xc0000000004ba984
  Failed to find "do_open%return",
   because do_open is an inlined function and has no return point.
  An error occurred in debuginfo analysis (-22).
  Trying to use symbols.
  Opening /sys/kernel/debug/tracing//kprobe_events write=1
  Writing event: r:probe/do_open do_open+0
  Writing event: r:probe/do_open_1 do_open+0
  Added new events:
    probe:do_open        (on do_open%return)
    probe:do_open_1      (on do_open%return)

  You can now use it in all perf tools, such as:

	  perf record -e probe:do_open_1 -aR sleep 1

  naveen@ubuntu:~/linux/tools/perf$ sudo cat /sys/kernel/debug/kprobes/list
  c000000000041370  k  kretprobe_trampoline+0x0    [OPTIMIZED]
  c0000000004433d0  r  do_open+0x0    [DISABLED]
  c0000000004433d0  r  do_open+0x0    [DISABLED]

And after this patch (and the subsequent powerpc patch):

  naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe -v do_open%return
  probe-definition(0): do_open%return 
  symbol:do_open file:(null) line:0 offset:0 return:1 lazy:(null)
  0 arguments
  Looking at the vmlinux_path (8 entries long)
  Using /boot/vmlinux for symbols
  Open Debuginfo file: /boot/vmlinux
  Try to find probe point from debuginfo.
  Matched function: do_open [2d0c7d8]
  Probe point found: do_open+0
  Matched function: do_open [35d76b5]
  found inline addr: 0xc0000000004ba984
  Failed to find "do_open%return",
   because do_open is an inlined function and has no return point.
  An error occurred in debuginfo analysis (-22).
  Trying to use symbols.
  Opening /sys/kernel/debug/tracing//README write=0
  Opening /sys/kernel/debug/tracing//kprobe_events write=1
  Writing event: r:probe/do_open _text+4469712
  Writing event: r:probe/do_open_1 _text+4956248
  Added new events:
    probe:do_open        (on do_open%return)
    probe:do_open_1      (on do_open%return)

  You can now use it in all perf tools, such as:

	  perf record -e probe:do_open_1 -aR sleep 1

  naveen@ubuntu:~/linux/tools/perf$ sudo cat /sys/kernel/debug/kprobes/list
  c000000000041370  k  kretprobe_trampoline+0x0    [OPTIMIZED]
  c0000000004433d0  r  do_open+0x0    [DISABLED]
  c0000000004ba058  r  do_open+0x8    [DISABLED]


Thanks,
- Naveen

[toc] | [prev] | [next] | [standalone]

#1593743 — Re: [PATCH v4 2/3] perf: kretprobes: offset from reloc_sym if kernel supports it

On Mon, 6 Mar 2017 20:34:10 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> On 2017/03/04 09:49AM, Masami Hiramatsu wrote:
> > On Thu,  2 Mar 2017 23:25:06 +0530
> > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> > 
> > > We indicate support for accepting sym+offset with kretprobes through a
> > > line in ftrace README. Parse the same to identify support and choose the
> > > appropriate format for kprobe_events.
> > 
> > Could you give us an example of this change here? :)
> > for example, comment of commit 613f050d68a8 .
> > 
> > I think the code is OK, but we need actual example of result.
> 
> Sure :)
> As an example, without this perf patch, but with the ftrace changes:
> 
>   naveen@ubuntu:~/linux/tools/perf$ sudo cat /sys/kernel/debug/tracing/README | grep kretprobe
>   place (kretprobe): [<module>:]<symbol>[+<offset>]|<memaddr>
>   naveen@ubuntu:~/linux/tools/perf$ 
>   naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe -v do_open%return
>   probe-definition(0): do_open%return 
>   symbol:do_open file:(null) line:0 offset:0 return:1 lazy:(null)
>   0 arguments
>   Looking at the vmlinux_path (8 entries long)
>   Using /boot/vmlinux for symbols
>   Open Debuginfo file: /boot/vmlinux
>   Try to find probe point from debuginfo.
>   Matched function: do_open [2d0c7d8]
>   Probe point found: do_open+0
>   Matched function: do_open [35d76b5]
>   found inline addr: 0xc0000000004ba984
>   Failed to find "do_open%return",
>    because do_open is an inlined function and has no return point.
>   An error occurred in debuginfo analysis (-22).
>   Trying to use symbols.
>   Opening /sys/kernel/debug/tracing//kprobe_events write=1
>   Writing event: r:probe/do_open do_open+0
>   Writing event: r:probe/do_open_1 do_open+0
>   Added new events:
>     probe:do_open        (on do_open%return)
>     probe:do_open_1      (on do_open%return)
> 
>   You can now use it in all perf tools, such as:
> 
> 	  perf record -e probe:do_open_1 -aR sleep 1
> 
>   naveen@ubuntu:~/linux/tools/perf$ sudo cat /sys/kernel/debug/kprobes/list
>   c000000000041370  k  kretprobe_trampoline+0x0    [OPTIMIZED]
>   c0000000004433d0  r  do_open+0x0    [DISABLED]
>   c0000000004433d0  r  do_open+0x0    [DISABLED]
> 
> And after this patch (and the subsequent powerpc patch):
> 
>   naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe -v do_open%return
>   probe-definition(0): do_open%return 
>   symbol:do_open file:(null) line:0 offset:0 return:1 lazy:(null)
>   0 arguments
>   Looking at the vmlinux_path (8 entries long)
>   Using /boot/vmlinux for symbols
>   Open Debuginfo file: /boot/vmlinux
>   Try to find probe point from debuginfo.
>   Matched function: do_open [2d0c7d8]
>   Probe point found: do_open+0
>   Matched function: do_open [35d76b5]
>   found inline addr: 0xc0000000004ba984
>   Failed to find "do_open%return",
>    because do_open is an inlined function and has no return point.
>   An error occurred in debuginfo analysis (-22).
>   Trying to use symbols.
>   Opening /sys/kernel/debug/tracing//README write=0
>   Opening /sys/kernel/debug/tracing//kprobe_events write=1
>   Writing event: r:probe/do_open _text+4469712
>   Writing event: r:probe/do_open_1 _text+4956248
>   Added new events:
>     probe:do_open        (on do_open%return)
>     probe:do_open_1      (on do_open%return)
> 
>   You can now use it in all perf tools, such as:
> 
> 	  perf record -e probe:do_open_1 -aR sleep 1
> 
>   naveen@ubuntu:~/linux/tools/perf$ sudo cat /sys/kernel/debug/kprobes/list
>   c000000000041370  k  kretprobe_trampoline+0x0    [OPTIMIZED]
>   c0000000004433d0  r  do_open+0x0    [DISABLED]
>   c0000000004ba058  r  do_open+0x8    [DISABLED]

Ok, with this usage example.

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

Thanks,

> 
> 
> Thanks,
> - Naveen
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web