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

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

[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  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"  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 
> > > 
> > > 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

[Masami Hiramatsu]

On Fri, 24 Feb 2017 17:11:03 -0300
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"  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 
> > 
> > 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 

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"  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 
> 
> 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 00:46:08 +0530
"Naveen N. Rao"  wrote:

> On 2017/02/23 06:10PM, Masami Hiramatsu wrote:
> > On Wed, 22 Feb 2017 19:23:40 +0530
> > "Naveen N. Rao"  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 
> > > ---
> > >  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 

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 

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"  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 
> > ---
> >  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 Wed, 22 Feb 2017 19:23:40 +0530
"Naveen N. Rao"  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 
> ---
>  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(, "%s/README", tracing_path) < 0)
> + return false;
> +
> + fp = fopen(buf, "r");
> + if (!fp)
> + goto end;
> +
> + zfree();
> + while (getline(, , 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