[PATCH v2 0/5] kretprobe fixes

[PATCH v2 0/5] kretprobe fixes

[Naveen N. Rao]

I'm including all patches (generic and powerpc changes) in
this series as suggested by Masami.

v1 patches:
https://marc.info/?l=linux-kernel&m=148718276424380
https://marc.info/?l=linux-kernel&m=148723314105453&w=2

Patches 1 and 2 are the same as v1.
Patch 3 is updated to include a line in ftrace README.
Patch 4 is new.
Patch 5 is updated to consider ftrace README.


Thanks,
Naveen

Naveen N. Rao (5):
  kretprobes: ensure probe location is at function entry
  powerpc: kretprobes: override default function entry offset
  trace/kprobes: allow return probes with offsets and absolute addresses
  perf: kretprobes: offset from reloc_sym if kernel supports it
  perf: powerpc: choose local entry point with kretprobes

 arch/powerpc/kernel/kprobes.c               |  9 ++++++
 include/linux/kprobes.h                     |  1 +
 kernel/kprobes.c                            | 13 ++++++++
 kernel/trace/trace.c                        |  1 +
 kernel/trace/trace_kprobe.c                 |  8 -----
 tools/perf/arch/powerpc/util/sym-handling.c |  9 +++---
 tools/perf/util/probe-event.c               | 47 ++++++++++++++++++++++++-----
 tools/perf/util/probe-event.h               |  2 ++
 8 files changed, 71 insertions(+), 19 deletions(-)

-- 
2.11.0

[PATCH v2 1/5] kretprobes: ensure probe location is at function entry

[Naveen N. Rao]

kretprobes can be registered by specifying an absolute address or by
specifying offset to a symbol. However, we need to ensure this falls at
function entry so as to be able to determine the return address.

Validate the same during kretprobe registration. By default, there
should not be any offset from a function entry, as determined through a
kallsyms_lookup(). Introduce arch_function_offset_within_entry() as a
way for architectures to override this.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 include/linux/kprobes.h |  1 +
 kernel/kprobes.c        | 13 +++++++++++++
 2 files changed, 14 insertions(+)

diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index 16ddfb8b304a..862f87adcbb4 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -266,6 +266,7 @@ extern int arch_init_kprobes(void);
 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 within_kprobe_blacklist(unsigned long addr);
 
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index ebb4dadca66b..524766563896 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1869,12 +1869,25 @@ static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)
 }
 NOKPROBE_SYMBOL(pre_handler_kretprobe);
 
+bool __weak arch_function_offset_within_entry(unsigned long offset)
+{
+	return !offset;
+}
+
 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))
+		return -EINVAL;
 
 	if (kretprobe_blacklist_size) {
 		addr = kprobe_addr(&rp->kp);
-- 
2.11.0

[tip:perf/core] kretprobes: Ensure probe location is at function entry

[tip-bot for Naveen N. Rao]

Commit-ID:  90ec5e89e393c76e19afc845d8f88a5dc8315919
Gitweb:     http://git.kernel.org/tip/90ec5e89e393c76e19afc845d8f88a5dc8315919
Author:     Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
AuthorDate: Wed, 22 Feb 2017 19:23:37 +0530
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Fri, 3 Mar 2017 19:07:17 -0300

kretprobes: Ensure probe location is at function entry

kretprobes can be registered by specifying an absolute address or by
specifying offset to a symbol. However, we need to ensure this falls at
function entry so as to be able to determine the return address.

Validate the same during kretprobe registration. By default, there
should not be any offset from a function entry, as determined through a
kallsyms_lookup(). Introduce arch_function_offset_within_entry() as a
way for architectures to override this.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: linuxppc-dev@lists.ozlabs.org
Link: http://lkml.kernel.org/r/f1583bc4839a3862cfc2acefcc56f9c8837fa2ba.1487770934.git.naveen.n.rao@linux.vnet.ibm.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 include/linux/kprobes.h |  1 +
 kernel/kprobes.c        | 13 +++++++++++++
 2 files changed, 14 insertions(+)

diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index c328e4f..177bdf6 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -267,6 +267,7 @@ extern int arch_init_kprobes(void);
 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 within_kprobe_blacklist(unsigned long addr);
 
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 699c5bc..448759d 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1875,12 +1875,25 @@ static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)
 }
 NOKPROBE_SYMBOL(pre_handler_kretprobe);
 
+bool __weak arch_function_offset_within_entry(unsigned long offset)
+{
+	return !offset;
+}
+
 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))
+		return -EINVAL;
 
 	if (kretprobe_blacklist_size) {
 		addr = kprobe_addr(&rp->kp);

[PATCH v2 2/5] powerpc: kretprobes: override default function entry offset

[Naveen N. Rao]

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

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 735ff3d3f77d..e37b76b8b6b2 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.0

Re: [PATCH v2 2/5] powerpc: kretprobes: override default function entry offset

[Arnaldo Carvalho de Melo]

Em Wed, Feb 22, 2017 at 07:23:38PM +0530, Naveen N. Rao escreveu:
> With ABIv2, we offset 8 bytes into a function to get at the local entry
> point.

So, I think I can carry the first one via Ingo, etc, what about this
one?

Is it ok for me to process it?

Seems simple enough, has been thru a lot of discussion, but would be
better if it was Reviewed-by the PPC maintainers or else just processed
by them.

Please advise,

- Arnaldo
 
> 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 735ff3d3f77d..e37b76b8b6b2 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.0

Re: [PATCH v2 2/5] powerpc: kretprobes: override default function entry offset

[Michael Ellerman]

Arnaldo Carvalho de Melo <acme@kernel.org> writes:

> Em Wed, Feb 22, 2017 at 07:23:38PM +0530, Naveen N. Rao escreveu:
>> With ABIv2, we offset 8 bytes into a function to get at the local entry
>> point.
>
> So, I think I can carry the first one via Ingo, etc, what about this
> one?
>
> Is it ok for me to process it?

Yes please.

> Seems simple enough, has been thru a lot of discussion, but would be
> better if it was Reviewed-by the PPC maintainers or else just processed
> by them.
>
> Please advise,

I think it's best if you take it with patch 1. I realise they could go
separately, but they make more sense together I think.

Acked-by: Michael Ellerman <mpe@ellerman.id.au>

cheers

Re: [PATCH v2 2/5] powerpc: kretprobes: override default function entry offset

[Ananth N Mavinakayanahalli]

On Wed, Feb 22, 2017 at 07:23:38PM +0530, Naveen N. Rao wrote:
> With ABIv2, we offset 8 bytes into a function to get at the local entry
> point.
> 

Looks good.

> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>

Acked-by: Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>

[PATCH v2 3/5] trace/kprobes: allow return probes with offsets and absolute addresses

[Naveen N. Rao]

Since the kernel includes many non-global functions with same names, we
will need to use offsets from other symbols (typically _text/_stext) or
absolute addresses to place return probes on specific functions. Also,
the core register_kretprobe() API never forbid use of offsets or
absolute addresses with kretprobes.

Allow its use with the trace infrastructure. To distinguish kernels that
support this, update ftrace README to explicitly call this out.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 kernel/trace/trace.c        | 1 +
 kernel/trace/trace_kprobe.c | 8 --------
 2 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index d7449783987a..ababe56b3e8f 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -4362,6 +4362,7 @@ static const char readme_msg[] =
 	"\t           -:[<group>/]<event>\n"
 #ifdef CONFIG_KPROBE_EVENT
 	"\t    place: [<module>:]<symbol>[+<offset>]|<memaddr>\n"
+  "place (kretprobe): [<module>:]<symbol>[+<offset>]|<memaddr>\n"
 #endif
 #ifdef CONFIG_UPROBE_EVENT
 	"\t    place: <path>:<offset>\n"
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 7ad9e53ad174..2768cb60ebd7 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -679,10 +679,6 @@ static int create_trace_kprobe(int argc, char **argv)
 		return -EINVAL;
 	}
 	if (isdigit(argv[1][0])) {
-		if (is_return) {
-			pr_info("Return probe point must be a symbol.\n");
-			return -EINVAL;
-		}
 		/* an address specified */
 		ret = kstrtoul(&argv[1][0], 0, (unsigned long *)&addr);
 		if (ret) {
@@ -698,10 +694,6 @@ static int create_trace_kprobe(int argc, char **argv)
 			pr_info("Failed to parse symbol.\n");
 			return ret;
 		}
-		if (offset && is_return) {
-			pr_info("Return probe must be used without offset.\n");
-			return -EINVAL;
-		}
 	}
 	argc -= 2; argv += 2;
 
-- 
2.11.0

Re: [PATCH v2 3/5] trace/kprobes: allow return probes with offsets and absolute addresses

[Steven Rostedt]

On Wed, 22 Feb 2017 19:23:39 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> Since the kernel includes many non-global functions with same names, we
> will need to use offsets from other symbols (typically _text/_stext) or
> absolute addresses to place return probes on specific functions. Also,
> the core register_kretprobe() API never forbid use of offsets or
> absolute addresses with kretprobes.
> 
> Allow its use with the trace infrastructure. To distinguish kernels that
> support this, update ftrace README to explicitly call this out.
> 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  kernel/trace/trace.c        | 1 +
>  kernel/trace/trace_kprobe.c | 8 --------
>  2 files changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index d7449783987a..ababe56b3e8f 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -4362,6 +4362,7 @@ static const char readme_msg[] =
>  	"\t           -:[<group>/]<event>\n"
>  #ifdef CONFIG_KPROBE_EVENT
>  	"\t    place: [<module>:]<symbol>[+<offset>]|<memaddr>\n"
> +  "place (kretprobe): [<module>:]<symbol>[+<offset>]|<memaddr>\n"
>  #endif
>  #ifdef CONFIG_UPROBE_EVENT
>  	"\t    place: <path>:<offset>\n"
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 7ad9e53ad174..2768cb60ebd7 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -679,10 +679,6 @@ static int create_trace_kprobe(int argc, char **argv)
>  		return -EINVAL;
>  	}
>  	if (isdigit(argv[1][0])) {
> -		if (is_return) {
> -			pr_info("Return probe point must be a symbol.\n");
> -			return -EINVAL;
> -		}
>  		/* an address specified */
>  		ret = kstrtoul(&argv[1][0], 0, (unsigned long *)&addr);
>  		if (ret) {
> @@ -698,10 +694,6 @@ static int create_trace_kprobe(int argc, char **argv)
>  			pr_info("Failed to parse symbol.\n");
>  			return ret;
>  		}
> -		if (offset && is_return) {
> -			pr_info("Return probe must be used without offset.\n");
> -			return -EINVAL;
> -		}

I understand that your retprobes will now have an offset, but I'm
worried we are removing informative errors. For those archs that don't
allow an offset, will we still get the error telling users that offsets
are not allowed?

I don't want to lose informative error handling.

-- Steve


>  	}
>  	argc -= 2; argv += 2;
>  

[PATCH v2 3.5/5] trace/kprobes: Add back warning about offset in return probes

[Steven Rostedt (VMware)]

Let's not remove the warning about offsets and return probes when the
offset is invalid.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 3f4f788..f626235 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -695,6 +695,11 @@ static int create_trace_kprobe(int argc, char **argv)
 			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;
+		}
 	}
 	argc -= 2; argv += 2;
 

Re: [PATCH v2 3.5/5] trace/kprobes: Add back warning about offset in return probes

[Masami Hiramatsu]

On Mon, 27 Feb 2017 11:52:04 -0500
"Steven Rostedt (VMware)" <rostedt@goodmis.org> wrote:

> Let's not remove the warning about offsets and return probes when the
> offset is invalid.

Agreed, This looks good to me.

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

Thanks!

> 
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 3f4f788..f626235 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -695,6 +695,11 @@ static int create_trace_kprobe(int argc, char **argv)
>  			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;
> +		}
>  	}
>  	argc -= 2; argv += 2;
>  


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v2 3.5/5] trace/kprobes: Add back warning about offset in return probes

[Naveen N. Rao]

On 2017/02/27 11:52AM, Steven Rostedt (VMware) wrote:
> Let's not remove the warning about offsets and return probes when the
> offset is invalid.

Good point!

Thanks, Steve!

> 
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

Acked-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>

> ---
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 3f4f788..f626235 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -695,6 +695,11 @@ static int create_trace_kprobe(int argc, char **argv)
>  			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;
> +		}
>  	}
>  	argc -= 2; argv += 2;
> 
> 

[tip:perf/core] trace/kprobes: Add back warning about offset in return probes

[tip-bot for Steven Rostedt (VMware)]

Commit-ID:  d0e02579c282ccf34c79818045ec2d2934b56c19
Gitweb:     http://git.kernel.org/tip/d0e02579c282ccf34c79818045ec2d2934b56c19
Author:     Steven Rostedt (VMware) <rostedt@goodmis.org>
AuthorDate: Mon, 27 Feb 2017 11:52:04 -0500
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Fri, 3 Mar 2017 19:07:19 -0300

trace/kprobes: Add back warning about offset in return probes

Let's not remove the warning about offsets and return probes when the
offset is invalid.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
Acked-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
Cc: Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: linuxppc-dev@lists.ozlabs.org
Link: http://lkml.kernel.org/r/20170227115204.00f92846@gandalf.local.home
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 kernel/trace/trace_kprobe.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 18775ef..2b7d0dd 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -695,6 +695,11 @@ static int create_trace_kprobe(int argc, char **argv)
 			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;
+		}
 	}
 	argc -= 2; argv += 2;
 

[tip:perf/core] trace/kprobes: Allow return probes with offsets and absolute addresses

[tip-bot for Naveen N. Rao]

Commit-ID:  35b6f55aa9ba65141f2def0997e23aab13715d3f
Gitweb:     http://git.kernel.org/tip/35b6f55aa9ba65141f2def0997e23aab13715d3f
Author:     Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
AuthorDate: Wed, 22 Feb 2017 19:23:39 +0530
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Fri, 3 Mar 2017 19:07:18 -0300

trace/kprobes: Allow return probes with offsets and absolute addresses

Since the kernel includes many non-global functions with same names, we
will need to use offsets from other symbols (typically _text/_stext) or
absolute addresses to place return probes on specific functions. Also,
the core register_kretprobe() API never forbid use of offsets or
absolute addresses with kretprobes.

Allow its use with the trace infrastructure. To distinguish kernels that
support this, update ftrace README to explicitly call this out.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: linuxppc-dev@lists.ozlabs.org
Link: http://lkml.kernel.org/r/183e7ce2921a08c9c755ee9a5da3134febc6695b.1487770934.git.naveen.n.rao@linux.vnet.ibm.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 kernel/trace/trace.c        | 1 +
 kernel/trace/trace_kprobe.c | 8 --------
 2 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index f351095..0ed834d 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -4355,6 +4355,7 @@ static const char readme_msg[] =
 	"\t           -:[<group>/]<event>\n"
 #ifdef CONFIG_KPROBE_EVENTS
 	"\t    place: [<module>:]<symbol>[+<offset>]|<memaddr>\n"
+  "place (kretprobe): [<module>:]<symbol>[+<offset>]|<memaddr>\n"
 #endif
 #ifdef CONFIG_UPROBE_EVENTS
 	"\t    place: <path>:<offset>\n"
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index eadd96e..18775ef 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -680,10 +680,6 @@ static int create_trace_kprobe(int argc, char **argv)
 		return -EINVAL;
 	}
 	if (isdigit(argv[1][0])) {
-		if (is_return) {
-			pr_info("Return probe point must be a symbol.\n");
-			return -EINVAL;
-		}
 		/* an address specified */
 		ret = kstrtoul(&argv[1][0], 0, (unsigned long *)&addr);
 		if (ret) {
@@ -699,10 +695,6 @@ static int create_trace_kprobe(int argc, char **argv)
 			pr_info("Failed to parse symbol.\n");
 			return ret;
 		}
-		if (offset && is_return) {
-			pr_info("Return probe must be used without offset.\n");
-			return -EINVAL;
-		}
 	}
 	argc -= 2; argv += 2;
 

[PATCH v2 4/5] perf: kretprobes: offset from reloc_sym if kernel supports it

[Naveen N. Rao]

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 | 47 ++++++++++++++++++++++++++++++++++++-------
 tools/perf/util/probe-event.h |  2 ++
 2 files changed, 42 insertions(+), 7 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 35f5b7b7715c..f6bc61c47271 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -737,6 +737,41 @@ post_process_module_probe_trace_events(struct probe_trace_event *tevs,
 	return ret;
 }
 
+bool is_kretprobe_offset_supported(void)
+{
+	FILE *fp;
+	char *buf = NULL;
+	size_t len = 0;
+	bool target_line = false;
+	static int supported = -1;
+
+	if (supported >= 0)
+		return !!supported;
+
+	if (asprintf(&buf, "%s/README", tracing_path) < 0)
+		return false;
+
+	fp = fopen(buf, "r");
+	if (!fp)
+		goto end;
+
+	zfree(&buf);
+	while (getline(&buf, &len, fp) > 0) {
+		target_line = !!strstr(buf, "place (kretprobe): ");
+		if (!target_line)
+			continue;
+		supported = 1;
+	}
+	if (supported == -1)
+		supported = 0;
+
+	fclose(fp);
+end:
+	free(buf);
+
+	return !!supported;
+}
+
 static int
 post_process_kernel_probe_trace_events(struct probe_trace_event *tevs,
 				       int ntevs)
@@ -757,7 +792,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 && !is_kretprobe_offset_supported())
 			continue;
 		/* If we found a wrong one, mark it by NULL symbol */
 		if (kprobe_warn_out_range(tevs[i].point.symbol,
@@ -1528,11 +1565,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 +2873,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 || is_kretprobe_offset_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-event.h b/tools/perf/util/probe-event.h
index 5d4e94061402..449d4f311355 100644
--- a/tools/perf/util/probe-event.h
+++ b/tools/perf/util/probe-event.h
@@ -135,6 +135,8 @@ bool perf_probe_with_var(struct perf_probe_event *pev);
 /* Check the perf_probe_event needs debuginfo */
 bool perf_probe_event_need_dwarf(struct perf_probe_event *pev);
 
+bool is_kretprobe_offset_supported(void);
+
 /* Release event contents */
 void clear_perf_probe_event(struct perf_probe_event *pev);
 void clear_probe_trace_event(struct probe_trace_event *tev);
-- 
2.11.0

Re: [PATCH v2 4/5] perf: kretprobes: offset from reloc_sym if kernel supports it

[Masami Hiramatsu]

On Wed, 22 Feb 2017 19:23:40 +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.
> 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  tools/perf/util/probe-event.c | 47 ++++++++++++++++++++++++++++++++++++-------
>  tools/perf/util/probe-event.h |  2 ++
>  2 files changed, 42 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 35f5b7b7715c..f6bc61c47271 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -737,6 +737,41 @@ post_process_module_probe_trace_events(struct probe_trace_event *tevs,
>  	return ret;
>  }
>  
> +bool is_kretprobe_offset_supported(void)
> +{
> +	FILE *fp;
> +	char *buf = NULL;
> +	size_t len = 0;
> +	bool target_line = false;
> +	static int supported = -1;
> +
> +	if (supported >= 0)
> +		return !!supported;
> +
> +	if (asprintf(&buf, "%s/README", tracing_path) < 0)
> +		return false;
> +
> +	fp = fopen(buf, "r");
> +	if (!fp)
> +		goto end;
> +
> +	zfree(&buf);
> +	while (getline(&buf, &len, fp) > 0) {
> +		target_line = !!strstr(buf, "place (kretprobe): ");
> +		if (!target_line)
> +			continue;
> +		supported = 1;
> +	}
> +	if (supported == -1)
> +		supported = 0;
> +
> +	fclose(fp);
> +end:
> +	free(buf);
> +
> +	return !!supported;
> +}

Could you reuse (refactoring) probe_type_is_available() in probe-file.c to share
opening README file?

Others looks good to me :)

Thank you,

> +
>  static int
>  post_process_kernel_probe_trace_events(struct probe_trace_event *tevs,
>  				       int ntevs)
> @@ -757,7 +792,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 && !is_kretprobe_offset_supported())
>  			continue;
>  		/* If we found a wrong one, mark it by NULL symbol */
>  		if (kprobe_warn_out_range(tevs[i].point.symbol,
> @@ -1528,11 +1565,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 +2873,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 || is_kretprobe_offset_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-event.h b/tools/perf/util/probe-event.h
> index 5d4e94061402..449d4f311355 100644
> --- a/tools/perf/util/probe-event.h
> +++ b/tools/perf/util/probe-event.h
> @@ -135,6 +135,8 @@ bool perf_probe_with_var(struct perf_probe_event *pev);
>  /* Check the perf_probe_event needs debuginfo */
>  bool perf_probe_event_need_dwarf(struct perf_probe_event *pev);
>  
> +bool is_kretprobe_offset_supported(void);
> +
>  /* Release event contents */
>  void clear_perf_probe_event(struct perf_probe_event *pev);
>  void clear_probe_trace_event(struct probe_trace_event *tev);
> -- 
> 2.11.0
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

[PATCH v3 1/2] perf: probe: generalize probe event file open routine

[Naveen N. Rao]

...into a generic function for opening trace files.

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

diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
index 436b64731f65..1a62daceb028 100644
--- a/tools/perf/util/probe-file.c
+++ b/tools/perf/util/probe-file.c
@@ -70,7 +70,7 @@ static void print_both_open_warning(int kerr, int uerr)
 	}
 }
 
-static int open_probe_events(const char *trace_file, bool readwrite)
+int open_trace_file(const char *trace_file, bool readwrite)
 {
 	char buf[PATH_MAX];
 	int ret;
@@ -92,12 +92,12 @@ static int open_probe_events(const char *trace_file, bool readwrite)
 
 static int open_kprobe_events(bool readwrite)
 {
-	return open_probe_events("kprobe_events", readwrite);
+	return open_trace_file("kprobe_events", readwrite);
 }
 
 static int open_uprobe_events(bool readwrite)
 {
-	return open_probe_events("uprobe_events", readwrite);
+	return open_trace_file("uprobe_events", readwrite);
 }
 
 int probe_file__open(int flag)
@@ -899,6 +899,7 @@ bool probe_type_is_available(enum probe_type type)
 	size_t len = 0;
 	bool target_line = false;
 	bool ret = probe_type_table[type].avail;
+	int fd;
 
 	if (type >= PROBE_TYPE_END)
 		return false;
@@ -906,14 +907,16 @@ bool probe_type_is_available(enum probe_type type)
 	if (ret || probe_type_table[type].checked)
 		return ret;
 
-	if (asprintf(&buf, "%s/README", tracing_path) < 0)
+	fd = open_trace_file("README", false);
+	if (fd < 0)
 		return ret;
 
-	fp = fopen(buf, "r");
-	if (!fp)
-		goto end;
+	fp = fdopen(fd, "r");
+	if (!fp) {
+		close(fd);
+		return ret;
+	}
 
-	zfree(&buf);
 	while (getline(&buf, &len, fp) > 0 && !ret) {
 		if (!target_line) {
 			target_line = !!strstr(buf, " type: ");
@@ -928,7 +931,6 @@ bool probe_type_is_available(enum probe_type type)
 	probe_type_table[type].avail = ret;
 
 	fclose(fp);
-end:
 	free(buf);
 
 	return ret;
diff --git a/tools/perf/util/probe-file.h b/tools/perf/util/probe-file.h
index eba44c3e9dca..a17a82eff8a0 100644
--- a/tools/perf/util/probe-file.h
+++ b/tools/perf/util/probe-file.h
@@ -35,6 +35,7 @@ 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);
-- 
2.11.1

Re: [PATCH v3 1/2] perf: probe: generalize probe event file open routine

[Masami Hiramatsu]

On Thu, 23 Feb 2017 17:07:23 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> ...into a generic function for opening trace files.

Even if it repeats subject, please write complete description...

Patch itself is OK to me.

Thanks,

> 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  tools/perf/util/probe-file.c | 20 +++++++++++---------
>  tools/perf/util/probe-file.h |  1 +
>  2 files changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
> index 436b64731f65..1a62daceb028 100644
> --- a/tools/perf/util/probe-file.c
> +++ b/tools/perf/util/probe-file.c
> @@ -70,7 +70,7 @@ static void print_both_open_warning(int kerr, int uerr)
>  	}
>  }
>  
> -static int open_probe_events(const char *trace_file, bool readwrite)
> +int open_trace_file(const char *trace_file, bool readwrite)
>  {
>  	char buf[PATH_MAX];
>  	int ret;
> @@ -92,12 +92,12 @@ static int open_probe_events(const char *trace_file, bool readwrite)
>  
>  static int open_kprobe_events(bool readwrite)
>  {
> -	return open_probe_events("kprobe_events", readwrite);
> +	return open_trace_file("kprobe_events", readwrite);
>  }
>  
>  static int open_uprobe_events(bool readwrite)
>  {
> -	return open_probe_events("uprobe_events", readwrite);
> +	return open_trace_file("uprobe_events", readwrite);
>  }
>  
>  int probe_file__open(int flag)
> @@ -899,6 +899,7 @@ bool probe_type_is_available(enum probe_type type)
>  	size_t len = 0;
>  	bool target_line = false;
>  	bool ret = probe_type_table[type].avail;
> +	int fd;
>  
>  	if (type >= PROBE_TYPE_END)
>  		return false;
> @@ -906,14 +907,16 @@ bool probe_type_is_available(enum probe_type type)
>  	if (ret || probe_type_table[type].checked)
>  		return ret;
>  
> -	if (asprintf(&buf, "%s/README", tracing_path) < 0)
> +	fd = open_trace_file("README", false);
> +	if (fd < 0)
>  		return ret;
>  
> -	fp = fopen(buf, "r");
> -	if (!fp)
> -		goto end;
> +	fp = fdopen(fd, "r");
> +	if (!fp) {
> +		close(fd);
> +		return ret;
> +	}
>  
> -	zfree(&buf);
>  	while (getline(&buf, &len, fp) > 0 && !ret) {
>  		if (!target_line) {
>  			target_line = !!strstr(buf, " type: ");
> @@ -928,7 +931,6 @@ bool probe_type_is_available(enum probe_type type)
>  	probe_type_table[type].avail = ret;
>  
>  	fclose(fp);
> -end:
>  	free(buf);
>  
>  	return ret;
> diff --git a/tools/perf/util/probe-file.h b/tools/perf/util/probe-file.h
> index eba44c3e9dca..a17a82eff8a0 100644
> --- a/tools/perf/util/probe-file.h
> +++ b/tools/perf/util/probe-file.h
> @@ -35,6 +35,7 @@ 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);
> -- 
> 2.11.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v3 1/2] perf: probe: generalize probe event file open routine

[Arnaldo Carvalho de Melo]

Em Sat, Feb 25, 2017 at 01:46:01AM +0900, Masami Hiramatsu escreveu:
> On Thu, 23 Feb 2017 17:07:23 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 
> > ...into a generic function for opening trace files.
> 
> Even if it repeats subject, please write complete description...
> 
> Patch itself is OK to me.

Did it and added your Acked-by as per your OK above.

- arnaldo
 
> Thanks,
> 
> > 
> > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> > ---
> >  tools/perf/util/probe-file.c | 20 +++++++++++---------
> >  tools/perf/util/probe-file.h |  1 +
> >  2 files changed, 12 insertions(+), 9 deletions(-)
> > 
> > diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
> > index 436b64731f65..1a62daceb028 100644
> > --- a/tools/perf/util/probe-file.c
> > +++ b/tools/perf/util/probe-file.c
> > @@ -70,7 +70,7 @@ static void print_both_open_warning(int kerr, int uerr)
> >  	}
> >  }
> >  
> > -static int open_probe_events(const char *trace_file, bool readwrite)
> > +int open_trace_file(const char *trace_file, bool readwrite)
> >  {
> >  	char buf[PATH_MAX];
> >  	int ret;
> > @@ -92,12 +92,12 @@ static int open_probe_events(const char *trace_file, bool readwrite)
> >  
> >  static int open_kprobe_events(bool readwrite)
> >  {
> > -	return open_probe_events("kprobe_events", readwrite);
> > +	return open_trace_file("kprobe_events", readwrite);
> >  }
> >  
> >  static int open_uprobe_events(bool readwrite)
> >  {
> > -	return open_probe_events("uprobe_events", readwrite);
> > +	return open_trace_file("uprobe_events", readwrite);
> >  }
> >  
> >  int probe_file__open(int flag)
> > @@ -899,6 +899,7 @@ bool probe_type_is_available(enum probe_type type)
> >  	size_t len = 0;
> >  	bool target_line = false;
> >  	bool ret = probe_type_table[type].avail;
> > +	int fd;
> >  
> >  	if (type >= PROBE_TYPE_END)
> >  		return false;
> > @@ -906,14 +907,16 @@ bool probe_type_is_available(enum probe_type type)
> >  	if (ret || probe_type_table[type].checked)
> >  		return ret;
> >  
> > -	if (asprintf(&buf, "%s/README", tracing_path) < 0)
> > +	fd = open_trace_file("README", false);
> > +	if (fd < 0)
> >  		return ret;
> >  
> > -	fp = fopen(buf, "r");
> > -	if (!fp)
> > -		goto end;
> > +	fp = fdopen(fd, "r");
> > +	if (!fp) {
> > +		close(fd);
> > +		return ret;
> > +	}
> >  
> > -	zfree(&buf);
> >  	while (getline(&buf, &len, fp) > 0 && !ret) {
> >  		if (!target_line) {
> >  			target_line = !!strstr(buf, " type: ");
> > @@ -928,7 +931,6 @@ bool probe_type_is_available(enum probe_type type)
> >  	probe_type_table[type].avail = ret;
> >  
> >  	fclose(fp);
> > -end:
> >  	free(buf);
> >  
> >  	return ret;
> > diff --git a/tools/perf/util/probe-file.h b/tools/perf/util/probe-file.h
> > index eba44c3e9dca..a17a82eff8a0 100644
> > --- a/tools/perf/util/probe-file.h
> > +++ b/tools/perf/util/probe-file.h
> > @@ -35,6 +35,7 @@ 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);
> > -- 
> > 2.11.1
> > 
> 
> 
> -- 
> Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v3 1/2] perf: probe: generalize probe event file open routine

[Naveen N. Rao]

On 2017/02/25 01:46AM, Masami Hiramatsu wrote:
> On Thu, 23 Feb 2017 17:07:23 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 
> > ...into a generic function for opening trace files.
> 
> Even if it repeats subject, please write complete description...

Agh, ok sure. I will try to curb my urge to not type too much :)

> 
> Patch itself is OK to me.

Thanks for the review on this series!

- Naveen

[tip:perf/core] perf probe: Generalize probe event file open routine

[tip-bot for Naveen N. Rao]

Commit-ID:  e491bc2f0dd9f1b4a23ba6f3da88f6b695c4a4c9
Gitweb:     http://git.kernel.org/tip/e491bc2f0dd9f1b4a23ba6f3da88f6b695c4a4c9
Author:     Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
AuthorDate: Thu, 23 Feb 2017 17:07:23 +0530
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Fri, 3 Mar 2017 19:07:18 -0300

perf probe: Generalize probe event file open routine

Generalize probe event file open routine into a generic function for opening
trace files.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: linuxppc-dev@lists.ozlabs.org
Link: http://lkml.kernel.org/r/b580465c7a4dcd5d3b40fdf8568e6be45d0a6333.1487849577.git.naveen.n.rao@linux.vnet.ibm.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/probe-file.c | 20 +++++++++++---------
 tools/perf/util/probe-file.h |  1 +
 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
index 436b647..1a62dac 100644
--- a/tools/perf/util/probe-file.c
+++ b/tools/perf/util/probe-file.c
@@ -70,7 +70,7 @@ static void print_both_open_warning(int kerr, int uerr)
 	}
 }
 
-static int open_probe_events(const char *trace_file, bool readwrite)
+int open_trace_file(const char *trace_file, bool readwrite)
 {
 	char buf[PATH_MAX];
 	int ret;
@@ -92,12 +92,12 @@ static int open_probe_events(const char *trace_file, bool readwrite)
 
 static int open_kprobe_events(bool readwrite)
 {
-	return open_probe_events("kprobe_events", readwrite);
+	return open_trace_file("kprobe_events", readwrite);
 }
 
 static int open_uprobe_events(bool readwrite)
 {
-	return open_probe_events("uprobe_events", readwrite);
+	return open_trace_file("uprobe_events", readwrite);
 }
 
 int probe_file__open(int flag)
@@ -899,6 +899,7 @@ bool probe_type_is_available(enum probe_type type)
 	size_t len = 0;
 	bool target_line = false;
 	bool ret = probe_type_table[type].avail;
+	int fd;
 
 	if (type >= PROBE_TYPE_END)
 		return false;
@@ -906,14 +907,16 @@ bool probe_type_is_available(enum probe_type type)
 	if (ret || probe_type_table[type].checked)
 		return ret;
 
-	if (asprintf(&buf, "%s/README", tracing_path) < 0)
+	fd = open_trace_file("README", false);
+	if (fd < 0)
 		return ret;
 
-	fp = fopen(buf, "r");
-	if (!fp)
-		goto end;
+	fp = fdopen(fd, "r");
+	if (!fp) {
+		close(fd);
+		return ret;
+	}
 
-	zfree(&buf);
 	while (getline(&buf, &len, fp) > 0 && !ret) {
 		if (!target_line) {
 			target_line = !!strstr(buf, " type: ");
@@ -928,7 +931,6 @@ bool probe_type_is_available(enum probe_type type)
 	probe_type_table[type].avail = ret;
 
 	fclose(fp);
-end:
 	free(buf);
 
 	return ret;
diff --git a/tools/perf/util/probe-file.h b/tools/perf/util/probe-file.h
index eba44c3..a17a82e 100644
--- a/tools/perf/util/probe-file.h
+++ b/tools/perf/util/probe-file.h
@@ -35,6 +35,7 @@ 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);

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

[Naveen N. Rao]

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 | 49 ++++++++++++++++++++++++++++++++++++-------
 tools/perf/util/probe-event.h |  2 ++
 2 files changed, 44 insertions(+), 7 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 35f5b7b7715c..dd6b9ce0eef3 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -737,6 +737,43 @@ post_process_module_probe_trace_events(struct probe_trace_event *tevs,
 	return ret;
 }
 
+bool is_kretprobe_offset_supported(void)
+{
+	FILE *fp;
+	char *buf = NULL;
+	size_t len = 0;
+	bool target_line = false;
+	static int supported = -1;
+	int fd;
+
+	if (supported >= 0)
+		return !!supported;
+
+	fd = open_trace_file("README", false);
+	if (fd < 0)
+		return false;
+
+	fp = fdopen(fd, "r");
+	if (!fp) {
+		close(fd);
+		return false;
+	}
+
+	while (getline(&buf, &len, fp) > 0) {
+		target_line = !!strstr(buf, "place (kretprobe): ");
+		if (!target_line)
+			continue;
+		supported = 1;
+	}
+	if (supported == -1)
+		supported = 0;
+
+	fclose(fp);
+	free(buf);
+
+	return !!supported;
+}
+
 static int
 post_process_kernel_probe_trace_events(struct probe_trace_event *tevs,
 				       int ntevs)
@@ -757,7 +794,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 && !is_kretprobe_offset_supported())
 			continue;
 		/* If we found a wrong one, mark it by NULL symbol */
 		if (kprobe_warn_out_range(tevs[i].point.symbol,
@@ -1528,11 +1567,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 +2875,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 || is_kretprobe_offset_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-event.h b/tools/perf/util/probe-event.h
index 5d4e94061402..449d4f311355 100644
--- a/tools/perf/util/probe-event.h
+++ b/tools/perf/util/probe-event.h
@@ -135,6 +135,8 @@ bool perf_probe_with_var(struct perf_probe_event *pev);
 /* Check the perf_probe_event needs debuginfo */
 bool perf_probe_event_need_dwarf(struct perf_probe_event *pev);
 
+bool is_kretprobe_offset_supported(void);
+
 /* Release event contents */
 void clear_perf_probe_event(struct perf_probe_event *pev);
 void clear_probe_trace_event(struct probe_trace_event *tev);
-- 
2.11.1

Re: [PATCH v3 2/2] perf: kretprobes: offset from reloc_sym if kernel supports it

[Masami Hiramatsu]

On Thu, 23 Feb 2017 17:07:24 +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.
> 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  tools/perf/util/probe-event.c | 49 ++++++++++++++++++++++++++++++++++++-------
>  tools/perf/util/probe-event.h |  2 ++
>  2 files changed, 44 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 35f5b7b7715c..dd6b9ce0eef3 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -737,6 +737,43 @@ post_process_module_probe_trace_events(struct probe_trace_event *tevs,
>  	return ret;
>  }
>  
> +bool is_kretprobe_offset_supported(void)
> +{
> +	FILE *fp;
> +	char *buf = NULL;
> +	size_t len = 0;
> +	bool target_line = false;
> +	static int supported = -1;
> +	int fd;
> +
> +	if (supported >= 0)
> +		return !!supported;
> +
> +	fd = open_trace_file("README", false);
> +	if (fd < 0)
> +		return false;
> +
> +	fp = fdopen(fd, "r");
> +	if (!fp) {
> +		close(fd);
> +		return false;
> +	}
> +
> +	while (getline(&buf, &len, fp) > 0) {
> +		target_line = !!strstr(buf, "place (kretprobe): ");
> +		if (!target_line)
> +			continue;
> +		supported = 1;
> +	}
> +	if (supported == -1)
> +		supported = 0;
> +
> +	fclose(fp);
> +	free(buf);
> +
> +	return !!supported;
> +}

Hmm, I think you can do more than that. 
Can you reuse probe_type_is_available() to scan README?
I think we can have something like scan_ftrace_readme() in probe-file.c
to scan all the options and cache the results. 

probe_type_is_available() and kreprobe_offset_is_available()
just returns cached result or scan it in first call.(I would like to
ask you to do it in probe-file.c too)

Thank you,


> +
>  static int
>  post_process_kernel_probe_trace_events(struct probe_trace_event *tevs,
>  				       int ntevs)
> @@ -757,7 +794,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 && !is_kretprobe_offset_supported())
>  			continue;
>  		/* If we found a wrong one, mark it by NULL symbol */
>  		if (kprobe_warn_out_range(tevs[i].point.symbol,
> @@ -1528,11 +1567,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 +2875,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 || is_kretprobe_offset_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-event.h b/tools/perf/util/probe-event.h
> index 5d4e94061402..449d4f311355 100644
> --- a/tools/perf/util/probe-event.h
> +++ b/tools/perf/util/probe-event.h
> @@ -135,6 +135,8 @@ bool perf_probe_with_var(struct perf_probe_event *pev);
>  /* Check the perf_probe_event needs debuginfo */
>  bool perf_probe_event_need_dwarf(struct perf_probe_event *pev);
>  
> +bool is_kretprobe_offset_supported(void);
> +
>  /* Release event contents */
>  void clear_perf_probe_event(struct perf_probe_event *pev);
>  void clear_probe_trace_event(struct probe_trace_event *tev);
> -- 
> 2.11.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v3 2/2] perf: kretprobes: offset from reloc_sym if kernel supports it

[Naveen N. Rao]

On 2017/02/25 02:12AM, Masami Hiramatsu wrote:
> On Thu, 23 Feb 2017 17:07:24 +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.
> > 
> > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> > ---
> >  tools/perf/util/probe-event.c | 49 ++++++++++++++++++++++++++++++++++++-------
> >  tools/perf/util/probe-event.h |  2 ++
> >  2 files changed, 44 insertions(+), 7 deletions(-)
> > 
> > diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> > index 35f5b7b7715c..dd6b9ce0eef3 100644
> > --- a/tools/perf/util/probe-event.c
> > +++ b/tools/perf/util/probe-event.c
> > @@ -737,6 +737,43 @@ post_process_module_probe_trace_events(struct probe_trace_event *tevs,
> >  	return ret;
> >  }
> >  
> > +bool is_kretprobe_offset_supported(void)
> > +{
> > +	FILE *fp;
> > +	char *buf = NULL;
> > +	size_t len = 0;
> > +	bool target_line = false;
> > +	static int supported = -1;
> > +	int fd;
> > +
> > +	if (supported >= 0)
> > +		return !!supported;
> > +
> > +	fd = open_trace_file("README", false);
> > +	if (fd < 0)
> > +		return false;
> > +
> > +	fp = fdopen(fd, "r");
> > +	if (!fp) {
> > +		close(fd);
> > +		return false;
> > +	}
> > +
> > +	while (getline(&buf, &len, fp) > 0) {
> > +		target_line = !!strstr(buf, "place (kretprobe): ");
> > +		if (!target_line)
> > +			continue;
> > +		supported = 1;
> > +	}
> > +	if (supported == -1)
> > +		supported = 0;
> > +
> > +	fclose(fp);
> > +	free(buf);
> > +
> > +	return !!supported;
> > +}
> 
> Hmm, I think you can do more than that. 
> Can you reuse probe_type_is_available() to scan README?
> I think we can have something like scan_ftrace_readme() in probe-file.c
> to scan all the options and cache the results. 
> 
> probe_type_is_available() and kreprobe_offset_is_available()
> just returns cached result or scan it in first call.(I would like to
> ask you to do it in probe-file.c too)

Ok sure, that makes sense. I see that we only ever care about support 
for hex type, so I will add a separate routine to only look for that and 
the newly added kretprobe offset support.

- Naveen

Re: [PATCH v2 4/5] perf: kretprobes: offset from reloc_sym if kernel supports it

[Naveen N. Rao]

On 2017/02/23 06:10PM, Masami Hiramatsu wrote:
> On Wed, 22 Feb 2017 19:23:40 +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.
> > 
> > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> > ---
> >  tools/perf/util/probe-event.c | 47 ++++++++++++++++++++++++++++++++++++-------
> >  tools/perf/util/probe-event.h |  2 ++
> >  2 files changed, 42 insertions(+), 7 deletions(-)
> > 

[snip]

> 
> Could you reuse (refactoring) probe_type_is_available() in probe-file.c to share
> opening README file?

Done. I've sent patches to do that, please review.

> 
> Others looks good to me :)

Thanks. I hope that's an Ack for this patchset?

If so, and if Ingo/Michael agree, would it be ok to take the kernel bits 
through the powerpc tree like we did for kprobe_exceptions_notify() 
cleanup?


Regards,
Naveen

Re: [PATCH v2 4/5] perf: kretprobes: offset from reloc_sym if kernel supports it

[Masami Hiramatsu]

On Fri, 24 Feb 2017 00:46:08 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> On 2017/02/23 06:10PM, Masami Hiramatsu wrote:
> > On Wed, 22 Feb 2017 19:23:40 +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.
> > > 
> > > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> > > ---
> > >  tools/perf/util/probe-event.c | 47 ++++++++++++++++++++++++++++++++++++-------
> > >  tools/perf/util/probe-event.h |  2 ++
> > >  2 files changed, 42 insertions(+), 7 deletions(-)
> > > 
> 
> [snip]
> 
> > 
> > Could you reuse (refactoring) probe_type_is_available() in probe-file.c to share
> > opening README file?
> 
> Done. I've sent patches to do that, please review.

OK.

> 
> > 
> > Others looks good to me :)
> 
> Thanks. I hope that's an Ack for this patchset?

OK, for 1/5, 2/5, 3/5, and 5/5;

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

And could you make v4 series including all patches? (Not only updates)

> 
> If so, and if Ingo/Michael agree, would it be ok to take the kernel bits 
> through the powerpc tree like we did for kprobe_exceptions_notify() 
> cleanup?

If it is not urgent (yes, it seems) and since it changes arch independent
parts, I think this series should finally go through Ingo's tree.


Thank you,


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v2 4/5] perf: kretprobes: offset from reloc_sym if kernel supports it

[Arnaldo Carvalho de Melo]

Em Sat, Feb 25, 2017 at 02:29:17AM +0900, Masami Hiramatsu escreveu:
> On Fri, 24 Feb 2017 00:46:08 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> > Thanks. I hope that's an Ack for this patchset?
> 
> OK, for 1/5, 2/5, 3/5, and 5/5;
> 
> Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
> 
> And could you make v4 series including all patches? (Not only updates)

So, to make progress I processed these:

[acme@jouet linux]$ git log --oneline -3
eb55608340b7 perf probe: Generalize probe event file open routine
859d718fac06 trace/kprobes: Allow return probes with offsets and absolute addresses
a10489121c81 kretprobes: Ensure probe location is at function entry
[acme@jouet linux]$

Waiting for Naveen to react to these last minute considerations from
Masami and for the Ack from the PPC guys about "[PATCH v2 2/5] powerpc:
kretprobes: override default function entry offset".

- Arnaldo

Re: [PATCH v2 4/5] perf: kretprobes: offset from reloc_sym if kernel supports it

[Masami Hiramatsu]

On Fri, 24 Feb 2017 17:11:03 -0300
Arnaldo Carvalho de Melo <acme@kernel.org> wrote:

> Em Sat, Feb 25, 2017 at 02:29:17AM +0900, Masami Hiramatsu escreveu:
> > On Fri, 24 Feb 2017 00:46:08 +0530
> > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> > > Thanks. I hope that's an Ack for this patchset?
> > 
> > OK, for 1/5, 2/5, 3/5, and 5/5;
> > 
> > Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
> > 
> > And could you make v4 series including all patches? (Not only updates)
> 
> So, to make progress I processed these:
> 
> [acme@jouet linux]$ git log --oneline -3
> eb55608340b7 perf probe: Generalize probe event file open routine
> 859d718fac06 trace/kprobes: Allow return probes with offsets and absolute addresses
> a10489121c81 kretprobes: Ensure probe location is at function entry
> [acme@jouet linux]$
> 
> Waiting for Naveen to react to these last minute considerations from
> Masami and for the Ack from the PPC guys about "[PATCH v2 2/5] powerpc:
> kretprobes: override default function entry offset".

Thanks Arnaldo!!

Naveen, please update your ppc and perf patches and send it to Arnaldo.
I'm happy to review it.

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v2 4/5] perf: kretprobes: offset from reloc_sym if kernel supports it

[Naveen N. Rao]

On 2017/02/25 08:55AM, Masami Hiramatsu wrote:
> On Fri, 24 Feb 2017 17:11:03 -0300
> Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> 
> > Em Sat, Feb 25, 2017 at 02:29:17AM +0900, Masami Hiramatsu escreveu:
> > > On Fri, 24 Feb 2017 00:46:08 +0530
> > > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> > > > Thanks. I hope that's an Ack for this patchset?
> > > 
> > > OK, for 1/5, 2/5, 3/5, and 5/5;
> > > 
> > > Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
> > > 
> > > And could you make v4 series including all patches? (Not only updates)
> > 
> > So, to make progress I processed these:
> > 
> > [acme@jouet linux]$ git log --oneline -3
> > eb55608340b7 perf probe: Generalize probe event file open routine
> > 859d718fac06 trace/kprobes: Allow return probes with offsets and absolute addresses
> > a10489121c81 kretprobes: Ensure probe location is at function entry
> > [acme@jouet linux]$
> > 
> > Waiting for Naveen to react to these last minute considerations from
> > Masami and for the Ack from the PPC guys about "[PATCH v2 2/5] powerpc:
> > kretprobes: override default function entry offset".

Thanks Arnaldo!
Sorry, couldn't get to this sooner as I was off for a day...
I see that you've picked up 3 of the patches and Ananth/Michael have 
acked the powerpc patch.

I will post the remaining ones tonight/tomorrow.

> 
> Thanks Arnaldo!!
> 
> Naveen, please update your ppc and perf patches and send it to Arnaldo.
> I'm happy to review it.

Sure thanks, I'll work on those tonight/tomorrow.


- Naveen

Re: [PATCH v2 4/5] perf: kretprobes: offset from reloc_sym if kernel supports it

[Naveen N. Rao]

On 2017/02/24 05:11PM, Arnaldo Carvalho de Melo wrote:
> Em Sat, Feb 25, 2017 at 02:29:17AM +0900, Masami Hiramatsu escreveu:
> > On Fri, 24 Feb 2017 00:46:08 +0530
> > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> > > Thanks. I hope that's an Ack for this patchset?
> > 
> > OK, for 1/5, 2/5, 3/5, and 5/5;
> > 
> > Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
> > 
> > And could you make v4 series including all patches? (Not only updates)
> 
> So, to make progress I processed these:
> 
> [acme@jouet linux]$ git log --oneline -3
> eb55608340b7 perf probe: Generalize probe event file open routine
> 859d718fac06 trace/kprobes: Allow return probes with offsets and absolute addresses
> a10489121c81 kretprobes: Ensure probe location is at function entry
> [acme@jouet linux]$
> 
> Waiting for Naveen to react to these last minute considerations from
> Masami and for the Ack from the PPC guys about "[PATCH v2 2/5] powerpc:
> kretprobes: override default function entry offset".

Arnaldo,
I am posting the remaining three patches in this series. These three
patches are on top of the above 3 patches you have processed and the
other powerpc kretprobes patch (v2 2/5).

Masami,
Kindly review and let me know if this is fine.


Thanks,
Naveen

---
Naveen N. Rao (3):
  perf: probe: factor out the ftrace README scanning
  perf: kretprobes: offset from reloc_sym if kernel supports it
  perf: powerpc: choose local entry point with kretprobes

 tools/perf/arch/powerpc/util/sym-handling.c | 10 ++--
 tools/perf/util/probe-event.c               | 12 ++---
 tools/perf/util/probe-file.c                | 77 ++++++++++++++++-------------
 tools/perf/util/probe-file.h                |  1 +
 4 files changed, 56 insertions(+), 44 deletions(-)

-- 
2.11.1

Re: [PATCH v2 4/5] perf: kretprobes: offset from reloc_sym if kernel supports it

[Arnaldo Carvalho de Melo]

Em Thu, Mar 02, 2017 at 11:25:04PM +0530, Naveen N. Rao escreveu:
> On 2017/02/24 05:11PM, Arnaldo Carvalho de Melo wrote:
> > Em Sat, Feb 25, 2017 at 02:29:17AM +0900, Masami Hiramatsu escreveu:
> > > On Fri, 24 Feb 2017 00:46:08 +0530
> > > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> > > > Thanks. I hope that's an Ack for this patchset?
> > > 
> > > OK, for 1/5, 2/5, 3/5, and 5/5;
> > > 
> > > Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
> > > 
> > > And could you make v4 series including all patches? (Not only updates)
> > 
> > So, to make progress I processed these:
> > 
> > [acme@jouet linux]$ git log --oneline -3
> > eb55608340b7 perf probe: Generalize probe event file open routine
> > 859d718fac06 trace/kprobes: Allow return probes with offsets and absolute addresses
> > a10489121c81 kretprobes: Ensure probe location is at function entry
> > [acme@jouet linux]$
> > 
> > Waiting for Naveen to react to these last minute considerations from
> > Masami and for the Ack from the PPC guys about "[PATCH v2 2/5] powerpc:
> > kretprobes: override default function entry offset".
> 
> Arnaldo,
> I am posting the remaining three patches in this series. These three
> patches are on top of the above 3 patches you have processed and the
> other powerpc kretprobes patch (v2 2/5).
> 
> Masami,
> Kindly review and let me know if this is fine.

Will process after Masami-san has time to review it,

Thanks,

- Arnaldo

[PATCH v4 1/3] perf: probe: factor out the ftrace README scanning

[Naveen N. Rao]

Simplify and separate out the ftrace README scanning logic into a
separate helper. This is used subsequently to scan for all patterns of
interest and to cache the result.

Since we are only interested in availability of probe argument type x,
we will only scan for that.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 tools/perf/util/probe-file.c | 70 +++++++++++++++++++++++---------------------
 1 file changed, 37 insertions(+), 33 deletions(-)

diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
index 1a62daceb028..8a219cd831b7 100644
--- a/tools/perf/util/probe-file.c
+++ b/tools/perf/util/probe-file.c
@@ -877,35 +877,31 @@ int probe_cache__show_all_caches(struct strfilter *filter)
 	return 0;
 }
 
+enum ftrace_readme {
+	FTRACE_README_PROBE_TYPE_X = 0,
+	FTRACE_README_END,
+};
+
 static struct {
 	const char *pattern;
-	bool	avail;
-	bool	checked;
-} probe_type_table[] = {
-#define DEFINE_TYPE(idx, pat, def_avail)	\
-	[idx] = {.pattern = pat, .avail = (def_avail)}
-	DEFINE_TYPE(PROBE_TYPE_U, "* u8/16/32/64,*", true),
-	DEFINE_TYPE(PROBE_TYPE_S, "* s8/16/32/64,*", true),
-	DEFINE_TYPE(PROBE_TYPE_X, "* x8/16/32/64,*", false),
-	DEFINE_TYPE(PROBE_TYPE_STRING, "* string,*", true),
-	DEFINE_TYPE(PROBE_TYPE_BITFIELD,
-		    "* b<bit-width>@<bit-offset>/<container-size>", true),
+	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,*"),
 };
 
-bool probe_type_is_available(enum probe_type type)
+static bool scan_ftrace_readme(enum ftrace_readme type)
 {
+	int fd;
 	FILE *fp;
 	char *buf = NULL;
 	size_t len = 0;
-	bool target_line = false;
-	bool ret = probe_type_table[type].avail;
-	int fd;
+	bool ret = false;
+	static bool scanned = false;
 
-	if (type >= PROBE_TYPE_END)
-		return false;
-	/* We don't have to check the type which supported by default */
-	if (ret || probe_type_table[type].checked)
-		return ret;
+	if (scanned)
+		goto result;
 
 	fd = open_trace_file("README", false);
 	if (fd < 0)
@@ -917,21 +913,29 @@ bool probe_type_is_available(enum probe_type type)
 		return ret;
 	}
 
-	while (getline(&buf, &len, fp) > 0 && !ret) {
-		if (!target_line) {
-			target_line = !!strstr(buf, " type: ");
-			if (!target_line)
-				continue;
-		} else if (strstr(buf, "\t          ") != buf)
-			break;
-		ret = strglobmatch(buf, probe_type_table[type].pattern);
-	}
-	/* Cache the result */
-	probe_type_table[type].checked = true;
-	probe_type_table[type].avail = 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);
 
-	return ret;
+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 true;
 }
-- 
2.11.1

Re: [PATCH v4 1/3] perf: probe: factor out the ftrace README scanning

[Masami Hiramatsu]

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

> Simplify and separate out the ftrace README scanning logic into a
> separate helper. This is used subsequently to scan for all patterns of
> interest and to cache the result.
> 
> Since we are only interested in availability of probe argument type x,
> we will only scan for that.

Ah, OK, this can simplify and shorten the actual scanning time.
If there are any needs for checking those in the future, we can
add it again at that moment.

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

Thank you!

> 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  tools/perf/util/probe-file.c | 70 +++++++++++++++++++++++---------------------
>  1 file changed, 37 insertions(+), 33 deletions(-)
> 
> diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
> index 1a62daceb028..8a219cd831b7 100644
> --- a/tools/perf/util/probe-file.c
> +++ b/tools/perf/util/probe-file.c
> @@ -877,35 +877,31 @@ int probe_cache__show_all_caches(struct strfilter *filter)
>  	return 0;
>  }
>  
> +enum ftrace_readme {
> +	FTRACE_README_PROBE_TYPE_X = 0,
> +	FTRACE_README_END,
> +};
> +
>  static struct {
>  	const char *pattern;
> -	bool	avail;
> -	bool	checked;
> -} probe_type_table[] = {
> -#define DEFINE_TYPE(idx, pat, def_avail)	\
> -	[idx] = {.pattern = pat, .avail = (def_avail)}
> -	DEFINE_TYPE(PROBE_TYPE_U, "* u8/16/32/64,*", true),
> -	DEFINE_TYPE(PROBE_TYPE_S, "* s8/16/32/64,*", true),
> -	DEFINE_TYPE(PROBE_TYPE_X, "* x8/16/32/64,*", false),
> -	DEFINE_TYPE(PROBE_TYPE_STRING, "* string,*", true),
> -	DEFINE_TYPE(PROBE_TYPE_BITFIELD,
> -		    "* b<bit-width>@<bit-offset>/<container-size>", true),
> +	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,*"),
>  };
>  
> -bool probe_type_is_available(enum probe_type type)
> +static bool scan_ftrace_readme(enum ftrace_readme type)
>  {
> +	int fd;
>  	FILE *fp;
>  	char *buf = NULL;
>  	size_t len = 0;
> -	bool target_line = false;
> -	bool ret = probe_type_table[type].avail;
> -	int fd;
> +	bool ret = false;
> +	static bool scanned = false;
>  
> -	if (type >= PROBE_TYPE_END)
> -		return false;
> -	/* We don't have to check the type which supported by default */
> -	if (ret || probe_type_table[type].checked)
> -		return ret;
> +	if (scanned)
> +		goto result;
>  
>  	fd = open_trace_file("README", false);
>  	if (fd < 0)
> @@ -917,21 +913,29 @@ bool probe_type_is_available(enum probe_type type)
>  		return ret;
>  	}
>  
> -	while (getline(&buf, &len, fp) > 0 && !ret) {
> -		if (!target_line) {
> -			target_line = !!strstr(buf, " type: ");
> -			if (!target_line)
> -				continue;
> -		} else if (strstr(buf, "\t          ") != buf)
> -			break;
> -		ret = strglobmatch(buf, probe_type_table[type].pattern);
> -	}
> -	/* Cache the result */
> -	probe_type_table[type].checked = true;
> -	probe_type_table[type].avail = 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);
>  
> -	return ret;
> +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 true;
>  }
> -- 
> 2.11.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v4 1/3] perf: probe: factor out the ftrace README scanning

[Steven Rostedt]

FYI,

When creating new patch series, please start a new thread, and don't
post a patch as a reply to another patch. It gets easily lost that way.

-- Steve


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

> Simplify and separate out the ftrace README scanning logic into a
> separate helper. This is used subsequently to scan for all patterns of
> interest and to cache the result.
> 
> Since we are only interested in availability of probe argument type x,
> we will only scan for that.
> 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---

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

[Naveen N. Rao]

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

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

[Masami Hiramatsu]

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>

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

[Masami Hiramatsu]

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>

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

[Masami Hiramatsu]

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>

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

[Masami Hiramatsu]

[-- Attachment #1: Type: text/plain, Size: 2838 bytes --]

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>

[-- Attachment #2: tracing-kprobe-check-kretprobe --]
[-- Type: application/octet-stream, Size: 2894 bytes --]

tracing: kprobe: Check kretprobe offset from symbol correctly

From: Masami Hiramatsu <mhiramat@kernel.org>

Check the kretprobe event offset from the nearest symbol correctly
instead of checking given offset value.
This will allow users to specify the relative offset from _text
or _stext so that they can put a kretprobe on one of same-name
functions.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 include/linux/kprobes.h     |    9 +++++++++
 kernel/kprobes.c            |    9 ---------
 kernel/trace/trace_kprobe.c |   18 +++++++++++++++++-
 3 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index 862f87a..6a92734 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -44,6 +44,15 @@
 #ifdef CONFIG_KPROBES
 #include <asm/kprobes.h>
 
+/*
+ * Some oddball architectures like 64bit powerpc have function descriptors
+ * so this must be overridable.
+ */
+#ifndef kprobe_lookup_name
+#define kprobe_lookup_name(name, addr) \
+	addr = ((kprobe_opcode_t *)(kallsyms_lookup_name(name)))
+#endif
+
 /* kprobe_status settings */
 #define KPROBE_HIT_ACTIVE	0x00000001
 #define KPROBE_HIT_SS		0x00000002
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index a77f9d7..9236f3f 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -58,15 +58,6 @@
 #define KPROBE_TABLE_SIZE (1 << KPROBE_HASH_BITS)
 
 
-/*
- * Some oddball architectures like 64bit powerpc have function descriptors
- * so this must be overridable.
- */
-#ifndef kprobe_lookup_name
-#define kprobe_lookup_name(name, addr) \
-	addr = ((kprobe_opcode_t *)(kallsyms_lookup_name(name)))
-#endif
-
 static int kprobes_initialized;
 static struct hlist_head kprobe_table[KPROBE_TABLE_SIZE];
 static struct hlist_head kretprobe_inst_table[KPROBE_TABLE_SIZE];
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index c60f9dc..59f2b2f 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -587,6 +587,22 @@ static int trace_kprobe_module_callback(struct notifier_block *nb,
 	return NOTIFY_DONE;
 }
 
+static bool function_offset_within_entry(const char *sym, unsigned long offs)
+{
+	void *addr = NULL;
+
+	kprobe_lookup_name(sym, addr);
+	if (!addr)
+		return false;
+	addr += offs;
+
+	if (kallsyms_lookup_size_offset((unsigned long)addr, NULL, &offs) &&
+	    offs == 0)
+		return true;
+
+	return false;
+}
+
 static struct notifier_block trace_kprobe_module_nb = {
 	.notifier_call = trace_kprobe_module_callback,
 	.priority = 1	/* Invoked after kprobe module callback */
@@ -695,7 +711,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(symbol, offset)) {
 			pr_info("Given offset is not valid for return probe.\n");
 			return -EINVAL;
 		}

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

[Naveen N. Rao]

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

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

[Naveen N. Rao]

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

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

[Masami Hiramatsu]

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>

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

[Naveen N. Rao]

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

[RESEND PATCH 1/6] trace/kprobes: fix check for kretprobe offset within function entry

[Naveen N. Rao]

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]

Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
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 177bdf6c6aeb..47e4da5b4fa2 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -268,6 +268,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 448759d4a263..32e6ac5131ed 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)
 {
@@ -1880,19 +1883,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 2b7d0dd938ba..861d98fee9ea 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -696,7 +696,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

Re: [RESEND PATCH 1/6] trace/kprobes: fix check for kretprobe offset within function entry

[Steven Rostedt]

Please start a new thread. When sending patches as replies to other
patch threads, especially this deep into the thread, they will most
likely get ignored.

-- Steve

Re: [RESEND PATCH 1/6] trace/kprobes: fix check for kretprobe offset within function entry

[Naveen N. Rao]

On 2017/03/07 03:47PM, Steven Rostedt wrote:
> 
> Please start a new thread. When sending patches as replies to other
> patch threads, especially this deep into the thread, they will most
> likely get ignored.

Sorry, got carried off. I will re-post in a new series.

- Naveen

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

[Naveen N. Rao]

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

[RESEND PATCH 3/6] perf: probe: factor out the ftrace README scanning

[Naveen N. Rao]

Simplify and separate out the ftrace README scanning logic into a
separate helper. This is used subsequently to scan for all patterns of
interest and to cache the result.

Since we are only interested in availability of probe argument type x,
we will only scan for that.

Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 tools/perf/util/probe-file.c | 70 +++++++++++++++++++++++---------------------
 1 file changed, 37 insertions(+), 33 deletions(-)

diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
index 1a62daceb028..8a219cd831b7 100644
--- a/tools/perf/util/probe-file.c
+++ b/tools/perf/util/probe-file.c
@@ -877,35 +877,31 @@ int probe_cache__show_all_caches(struct strfilter *filter)
 	return 0;
 }
 
+enum ftrace_readme {
+	FTRACE_README_PROBE_TYPE_X = 0,
+	FTRACE_README_END,
+};
+
 static struct {
 	const char *pattern;
-	bool	avail;
-	bool	checked;
-} probe_type_table[] = {
-#define DEFINE_TYPE(idx, pat, def_avail)	\
-	[idx] = {.pattern = pat, .avail = (def_avail)}
-	DEFINE_TYPE(PROBE_TYPE_U, "* u8/16/32/64,*", true),
-	DEFINE_TYPE(PROBE_TYPE_S, "* s8/16/32/64,*", true),
-	DEFINE_TYPE(PROBE_TYPE_X, "* x8/16/32/64,*", false),
-	DEFINE_TYPE(PROBE_TYPE_STRING, "* string,*", true),
-	DEFINE_TYPE(PROBE_TYPE_BITFIELD,
-		    "* b<bit-width>@<bit-offset>/<container-size>", true),
+	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,*"),
 };
 
-bool probe_type_is_available(enum probe_type type)
+static bool scan_ftrace_readme(enum ftrace_readme type)
 {
+	int fd;
 	FILE *fp;
 	char *buf = NULL;
 	size_t len = 0;
-	bool target_line = false;
-	bool ret = probe_type_table[type].avail;
-	int fd;
+	bool ret = false;
+	static bool scanned = false;
 
-	if (type >= PROBE_TYPE_END)
-		return false;
-	/* We don't have to check the type which supported by default */
-	if (ret || probe_type_table[type].checked)
-		return ret;
+	if (scanned)
+		goto result;
 
 	fd = open_trace_file("README", false);
 	if (fd < 0)
@@ -917,21 +913,29 @@ bool probe_type_is_available(enum probe_type type)
 		return ret;
 	}
 
-	while (getline(&buf, &len, fp) > 0 && !ret) {
-		if (!target_line) {
-			target_line = !!strstr(buf, " type: ");
-			if (!target_line)
-				continue;
-		} else if (strstr(buf, "\t          ") != buf)
-			break;
-		ret = strglobmatch(buf, probe_type_table[type].pattern);
-	}
-	/* Cache the result */
-	probe_type_table[type].checked = true;
-	probe_type_table[type].avail = 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);
 
-	return ret;
+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 true;
 }
-- 
2.11.1

[RESEND PATCH 4/6] perf: kretprobes: offset from reloc_sym if kernel supports it

[Naveen N. Rao]

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.

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]

Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
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 28fb62c32678..c9bdc9ded0c3 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

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

[Naveen N. Rao]

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

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

[Masami Hiramatsu]

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>

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

[Naveen N. Rao]

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

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

[Masami Hiramatsu]

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>

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

[Naveen N. Rao]

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

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

[Naveen N. Rao]

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

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

[Naveen N. Rao]

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

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

[Naveen N. Rao]

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

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

[Masami Hiramatsu]

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>

[PATCH v4 3/3] perf: powerpc: choose local entry point with kretprobes

[Naveen N. Rao]

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>
---
 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..cc7c2697c036 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,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

Re: [PATCH v4 3/3] perf: powerpc: choose local entry point with kretprobes

[Masami Hiramatsu]

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

> 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.

Looks good to me.

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

Thanks!

> 
> 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..cc7c2697c036 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,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
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

[PATCH v2 5/5] perf: powerpc: choose local entry point with kretprobes

[Naveen N. Rao]

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>
---
 tools/perf/arch/powerpc/util/sym-handling.c | 9 +++++----
 1 file changed, 5 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..73dbdc83286c 100644
--- a/tools/perf/arch/powerpc/util/sym-handling.c
+++ b/tools/perf/arch/powerpc/util/sym-handling.c
@@ -79,11 +79,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 && !is_kretprobe_offset_supported())
 		return;
 
 	lep_offset = PPC64_LOCAL_ENTRY_OFFSET(sym->arch_sym);
-- 
2.11.0