Re: [PATCH v2 9/9] perf probe: Support SDT markers having reference counter (semaphore)

[Masami Hiramatsu]

On Mon, 9 Apr 2018 13:59:16 +0530
Ravi Bangoria  wrote:

> Hi Masami,
> 
> On 04/09/2018 12:58 PM, Masami Hiramatsu wrote:
> > Hi Ravi,
> >
> > On Wed,  4 Apr 2018 14:01:10 +0530
> > Ravi Bangoria  wrote:
> >
> >> @@ -2054,15 +2060,21 @@ char *synthesize_probe_trace_command(struct 
> >> probe_trace_event *tev)
> >>}
> >>  
> >>/* Use the tp->address for uprobes */
> >> -  if (tev->uprobes)
> >> +  if (tev->uprobes) {
> >>err = strbuf_addf(, "%s:0x%lx", tp->module, tp->address);
> >> -  else if (!strncmp(tp->symbol, "0x", 2))
> >> +  if (uprobe_ref_ctr_is_supported() &&
> >> +  tp->ref_ctr_offset &&
> >> +  err >= 0)
> >> +  err = strbuf_addf(, "(0x%lx)", tp->ref_ctr_offset);
> > If the kernel doesn't support uprobe_ref_ctr but the event requires
> > to increment uprobe_ref_ctr, I think we should (at least) warn user here.
> 
> pr_debug("A semaphore is associated with %s:%s and seems your kernel doesn't 
> support it.\n"
>  tev->group, tev->event);
> 
> Looks good?

I think it should be pr_warning() and return NULL, since user may not be able to
trace the event even if it is enabled.

> 
> >> @@ -776,14 +784,21 @@ static char *synthesize_sdt_probe_command(struct 
> >> sdt_note *note,
> >>  {
> >>struct strbuf buf;
> >>char *ret = NULL, **args;
> >> -  int i, args_count;
> >> +  int i, args_count, err;
> >> +  unsigned long long ref_ctr_offset;
> >>  
> >>if (strbuf_init(, 32) < 0)
> >>return NULL;
> >>  
> >> -  if (strbuf_addf(, "p:%s/%s %s:0x%llx",
> >> -  sdtgrp, note->name, pathname,
> >> -  sdt_note__get_addr(note)) < 0)
> >> +  err = strbuf_addf(, "p:%s/%s %s:0x%llx",
> >> +  sdtgrp, note->name, pathname,
> >> +  sdt_note__get_addr(note));
> >> +
> >> +  ref_ctr_offset = sdt_note__get_ref_ctr_offset(note);
> >> +  if (uprobe_ref_ctr_is_supported() && ref_ctr_offset && err >= 0)
> >> +  err = strbuf_addf(, "(0x%llx)", ref_ctr_offset);
> > We don't have to care about uprobe_ref_ctr support here, because
> > this information will be just cached, not directly written to
> > uprobe_events.
> 
> Sure, will remove the check.

Thanks!

> 
> Thanks for the review :).
> Ravi
> 


-- 
Masami Hiramatsu 

Re: [PATCH v2 9/9] perf probe: Support SDT markers having reference counter (semaphore)

[Masami Hiramatsu]

On Mon, 9 Apr 2018 13:59:16 +0530
Ravi Bangoria  wrote:

> Hi Masami,
> 
> On 04/09/2018 12:58 PM, Masami Hiramatsu wrote:
> > Hi Ravi,
> >
> > On Wed,  4 Apr 2018 14:01:10 +0530
> > Ravi Bangoria  wrote:
> >
> >> @@ -2054,15 +2060,21 @@ char *synthesize_probe_trace_command(struct 
> >> probe_trace_event *tev)
> >>}
> >>  
> >>/* Use the tp->address for uprobes */
> >> -  if (tev->uprobes)
> >> +  if (tev->uprobes) {
> >>err = strbuf_addf(, "%s:0x%lx", tp->module, tp->address);
> >> -  else if (!strncmp(tp->symbol, "0x", 2))
> >> +  if (uprobe_ref_ctr_is_supported() &&
> >> +  tp->ref_ctr_offset &&
> >> +  err >= 0)
> >> +  err = strbuf_addf(, "(0x%lx)", tp->ref_ctr_offset);
> > If the kernel doesn't support uprobe_ref_ctr but the event requires
> > to increment uprobe_ref_ctr, I think we should (at least) warn user here.
> 
> pr_debug("A semaphore is associated with %s:%s and seems your kernel doesn't 
> support it.\n"
>  tev->group, tev->event);
> 
> Looks good?

I think it should be pr_warning() and return NULL, since user may not be able to
trace the event even if it is enabled.

> 
> >> @@ -776,14 +784,21 @@ static char *synthesize_sdt_probe_command(struct 
> >> sdt_note *note,
> >>  {
> >>struct strbuf buf;
> >>char *ret = NULL, **args;
> >> -  int i, args_count;
> >> +  int i, args_count, err;
> >> +  unsigned long long ref_ctr_offset;
> >>  
> >>if (strbuf_init(, 32) < 0)
> >>return NULL;
> >>  
> >> -  if (strbuf_addf(, "p:%s/%s %s:0x%llx",
> >> -  sdtgrp, note->name, pathname,
> >> -  sdt_note__get_addr(note)) < 0)
> >> +  err = strbuf_addf(, "p:%s/%s %s:0x%llx",
> >> +  sdtgrp, note->name, pathname,
> >> +  sdt_note__get_addr(note));
> >> +
> >> +  ref_ctr_offset = sdt_note__get_ref_ctr_offset(note);
> >> +  if (uprobe_ref_ctr_is_supported() && ref_ctr_offset && err >= 0)
> >> +  err = strbuf_addf(, "(0x%llx)", ref_ctr_offset);
> > We don't have to care about uprobe_ref_ctr support here, because
> > this information will be just cached, not directly written to
> > uprobe_events.
> 
> Sure, will remove the check.

Thanks!

> 
> Thanks for the review :).
> Ravi
> 


-- 
Masami Hiramatsu 

Re: [PATCH v2 9/9] perf probe: Support SDT markers having reference counter (semaphore)

[Ravi Bangoria]

Hi Masami,

On 04/09/2018 12:58 PM, Masami Hiramatsu wrote:
> Hi Ravi,
>
> On Wed,  4 Apr 2018 14:01:10 +0530
> Ravi Bangoria  wrote:
>
>> @@ -2054,15 +2060,21 @@ char *synthesize_probe_trace_command(struct 
>> probe_trace_event *tev)
>>  }
>>  
>>  /* Use the tp->address for uprobes */
>> -if (tev->uprobes)
>> +if (tev->uprobes) {
>>  err = strbuf_addf(, "%s:0x%lx", tp->module, tp->address);
>> -else if (!strncmp(tp->symbol, "0x", 2))
>> +if (uprobe_ref_ctr_is_supported() &&
>> +tp->ref_ctr_offset &&
>> +err >= 0)
>> +err = strbuf_addf(, "(0x%lx)", tp->ref_ctr_offset);
> If the kernel doesn't support uprobe_ref_ctr but the event requires
> to increment uprobe_ref_ctr, I think we should (at least) warn user here.

pr_debug("A semaphore is associated with %s:%s and seems your kernel doesn't 
support it.\n"
 tev->group, tev->event);

Looks good?

>> @@ -776,14 +784,21 @@ static char *synthesize_sdt_probe_command(struct 
>> sdt_note *note,
>>  {
>>  struct strbuf buf;
>>  char *ret = NULL, **args;
>> -int i, args_count;
>> +int i, args_count, err;
>> +unsigned long long ref_ctr_offset;
>>  
>>  if (strbuf_init(, 32) < 0)
>>  return NULL;
>>  
>> -if (strbuf_addf(, "p:%s/%s %s:0x%llx",
>> -sdtgrp, note->name, pathname,
>> -sdt_note__get_addr(note)) < 0)
>> +err = strbuf_addf(, "p:%s/%s %s:0x%llx",
>> +sdtgrp, note->name, pathname,
>> +sdt_note__get_addr(note));
>> +
>> +ref_ctr_offset = sdt_note__get_ref_ctr_offset(note);
>> +if (uprobe_ref_ctr_is_supported() && ref_ctr_offset && err >= 0)
>> +err = strbuf_addf(, "(0x%llx)", ref_ctr_offset);
> We don't have to care about uprobe_ref_ctr support here, because
> this information will be just cached, not directly written to
> uprobe_events.

Sure, will remove the check.

Thanks for the review :).
Ravi

Re: [PATCH v2 9/9] perf probe: Support SDT markers having reference counter (semaphore)

[Ravi Bangoria]

Hi Masami,

On 04/09/2018 12:58 PM, Masami Hiramatsu wrote:
> Hi Ravi,
>
> On Wed,  4 Apr 2018 14:01:10 +0530
> Ravi Bangoria  wrote:
>
>> @@ -2054,15 +2060,21 @@ char *synthesize_probe_trace_command(struct 
>> probe_trace_event *tev)
>>  }
>>  
>>  /* Use the tp->address for uprobes */
>> -if (tev->uprobes)
>> +if (tev->uprobes) {
>>  err = strbuf_addf(, "%s:0x%lx", tp->module, tp->address);
>> -else if (!strncmp(tp->symbol, "0x", 2))
>> +if (uprobe_ref_ctr_is_supported() &&
>> +tp->ref_ctr_offset &&
>> +err >= 0)
>> +err = strbuf_addf(, "(0x%lx)", tp->ref_ctr_offset);
> If the kernel doesn't support uprobe_ref_ctr but the event requires
> to increment uprobe_ref_ctr, I think we should (at least) warn user here.

pr_debug("A semaphore is associated with %s:%s and seems your kernel doesn't 
support it.\n"
 tev->group, tev->event);

Looks good?

>> @@ -776,14 +784,21 @@ static char *synthesize_sdt_probe_command(struct 
>> sdt_note *note,
>>  {
>>  struct strbuf buf;
>>  char *ret = NULL, **args;
>> -int i, args_count;
>> +int i, args_count, err;
>> +unsigned long long ref_ctr_offset;
>>  
>>  if (strbuf_init(, 32) < 0)
>>  return NULL;
>>  
>> -if (strbuf_addf(, "p:%s/%s %s:0x%llx",
>> -sdtgrp, note->name, pathname,
>> -sdt_note__get_addr(note)) < 0)
>> +err = strbuf_addf(, "p:%s/%s %s:0x%llx",
>> +sdtgrp, note->name, pathname,
>> +sdt_note__get_addr(note));
>> +
>> +ref_ctr_offset = sdt_note__get_ref_ctr_offset(note);
>> +if (uprobe_ref_ctr_is_supported() && ref_ctr_offset && err >= 0)
>> +err = strbuf_addf(, "(0x%llx)", ref_ctr_offset);
> We don't have to care about uprobe_ref_ctr support here, because
> this information will be just cached, not directly written to
> uprobe_events.

Sure, will remove the check.

Thanks for the review :).
Ravi

Re: [PATCH v2 9/9] perf probe: Support SDT markers having reference counter (semaphore)

[Masami Hiramatsu]

Hi Ravi,

On Wed,  4 Apr 2018 14:01:10 +0530
Ravi Bangoria  wrote:

> With this, perf buildid-cache will save SDT markers with reference
> counter in probe cache. Perf probe will be able to probe markers
> having reference counter. Ex,
> 
>   # readelf -n /tmp/tick | grep -A1 loop2
> Name: loop2
> ... Semaphore: 0x10020036
> 
>   # ./perf buildid-cache --add /tmp/tick
>   # ./perf probe sdt_tick:loop2
>   # ./perf stat -e sdt_tick:loop2 /tmp/tick
> hi: 0
> hi: 1
> hi: 2
> ^C
>  Performance counter stats for '/tmp/tick':
>  3  sdt_tick:loop2
>2.561851452 seconds time elapsed
> 
> Signed-off-by: Ravi Bangoria 
> ---
>  tools/perf/util/probe-event.c | 18 ++---
>  tools/perf/util/probe-event.h |  1 +
>  tools/perf/util/probe-file.c  | 34 ++--
>  tools/perf/util/probe-file.h  |  1 +
>  tools/perf/util/symbol-elf.c  | 46 
> ---
>  tools/perf/util/symbol.h  |  7 +++
>  6 files changed, 86 insertions(+), 21 deletions(-)
> 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index e1dbc98..b3a1330 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -1832,6 +1832,12 @@ int parse_probe_trace_command(const char *cmd, struct 
> probe_trace_event *tev)
>   tp->offset = strtoul(fmt2_str, NULL, 10);
>   }
>  
> + if (tev->uprobes) {
> + fmt2_str = strchr(p, '(');
> + if (fmt2_str)
> + tp->ref_ctr_offset = strtoul(fmt2_str + 1, NULL, 0);
> + }
> +
>   tev->nargs = argc - 2;
>   tev->args = zalloc(sizeof(struct probe_trace_arg) * tev->nargs);
>   if (tev->args == NULL) {
> @@ -2054,15 +2060,21 @@ char *synthesize_probe_trace_command(struct 
> probe_trace_event *tev)
>   }
>  
>   /* Use the tp->address for uprobes */
> - if (tev->uprobes)
> + if (tev->uprobes) {
>   err = strbuf_addf(, "%s:0x%lx", tp->module, tp->address);
> - else if (!strncmp(tp->symbol, "0x", 2))
> + if (uprobe_ref_ctr_is_supported() &&
> + tp->ref_ctr_offset &&
> + err >= 0)
> + err = strbuf_addf(, "(0x%lx)", tp->ref_ctr_offset);

If the kernel doesn't support uprobe_ref_ctr but the event requires
to increment uprobe_ref_ctr, I think we should (at least) warn user here.

> + } else if (!strncmp(tp->symbol, "0x", 2)) {
>   /* Absolute address. See try_to_find_absolute_address() */
>   err = strbuf_addf(, "%s%s0x%lx", tp->module ?: "",
> tp->module ? ":" : "", tp->address);
> - else
> + } else {
>   err = strbuf_addf(, "%s%s%s+%lu", tp->module ?: "",
>   tp->module ? ":" : "", tp->symbol, tp->offset);
> + }
> +
>   if (err)
>   goto error;
>  
> diff --git a/tools/perf/util/probe-event.h b/tools/perf/util/probe-event.h
> index 45b14f0..15a98c3 100644
> --- a/tools/perf/util/probe-event.h
> +++ b/tools/perf/util/probe-event.h
> @@ -27,6 +27,7 @@ struct probe_trace_point {
>   char*symbol;/* Base symbol */
>   char*module;/* Module name */
>   unsigned long   offset; /* Offset from symbol */
> + unsigned long   ref_ctr_offset; /* SDT reference counter offset */
>   unsigned long   address;/* Actual address of the trace point */
>   boolretprobe;   /* Return probe flag */
>  };
> diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
> index 4ae1123..ca0e524 100644
> --- a/tools/perf/util/probe-file.c
> +++ b/tools/perf/util/probe-file.c
> @@ -697,8 +697,16 @@ int probe_cache__add_entry(struct probe_cache *pcache,
>  #ifdef HAVE_GELF_GETNOTE_SUPPORT
>  static unsigned long long sdt_note__get_addr(struct sdt_note *note)
>  {
> - return note->bit32 ? (unsigned long long)note->addr.a32[0]
> -  : (unsigned long long)note->addr.a64[0];
> + return note->bit32 ?
> + (unsigned long long)note->addr.a32[SDT_NOTE_IDX_LOC] :
> + (unsigned long long)note->addr.a64[SDT_NOTE_IDX_LOC];
> +}
> +
> +static unsigned long long sdt_note__get_ref_ctr_offset(struct sdt_note *note)
> +{
> + return note->bit32 ?
> + (unsigned long long)note->addr.a32[SDT_NOTE_IDX_REFCTR] :
> + (unsigned long long)note->addr.a64[SDT_NOTE_IDX_REFCTR];
>  }
>  
>  static const char * const type_to_suffix[] = {
> @@ -776,14 +784,21 @@ static char *synthesize_sdt_probe_command(struct 
> sdt_note *note,
>  {
>   struct strbuf buf;
>   char *ret = NULL, **args;
> - int i, args_count;
> + int i, args_count, err;
> + unsigned long long ref_ctr_offset;
>  
>   if (strbuf_init(, 32) < 0)
>   return NULL;
>  

Re: [PATCH v2 9/9] perf probe: Support SDT markers having reference counter (semaphore)

[Masami Hiramatsu]

Hi Ravi,

On Wed,  4 Apr 2018 14:01:10 +0530
Ravi Bangoria  wrote:

> With this, perf buildid-cache will save SDT markers with reference
> counter in probe cache. Perf probe will be able to probe markers
> having reference counter. Ex,
> 
>   # readelf -n /tmp/tick | grep -A1 loop2
> Name: loop2
> ... Semaphore: 0x10020036
> 
>   # ./perf buildid-cache --add /tmp/tick
>   # ./perf probe sdt_tick:loop2
>   # ./perf stat -e sdt_tick:loop2 /tmp/tick
> hi: 0
> hi: 1
> hi: 2
> ^C
>  Performance counter stats for '/tmp/tick':
>  3  sdt_tick:loop2
>2.561851452 seconds time elapsed
> 
> Signed-off-by: Ravi Bangoria 
> ---
>  tools/perf/util/probe-event.c | 18 ++---
>  tools/perf/util/probe-event.h |  1 +
>  tools/perf/util/probe-file.c  | 34 ++--
>  tools/perf/util/probe-file.h  |  1 +
>  tools/perf/util/symbol-elf.c  | 46 
> ---
>  tools/perf/util/symbol.h  |  7 +++
>  6 files changed, 86 insertions(+), 21 deletions(-)
> 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index e1dbc98..b3a1330 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -1832,6 +1832,12 @@ int parse_probe_trace_command(const char *cmd, struct 
> probe_trace_event *tev)
>   tp->offset = strtoul(fmt2_str, NULL, 10);
>   }
>  
> + if (tev->uprobes) {
> + fmt2_str = strchr(p, '(');
> + if (fmt2_str)
> + tp->ref_ctr_offset = strtoul(fmt2_str + 1, NULL, 0);
> + }
> +
>   tev->nargs = argc - 2;
>   tev->args = zalloc(sizeof(struct probe_trace_arg) * tev->nargs);
>   if (tev->args == NULL) {
> @@ -2054,15 +2060,21 @@ char *synthesize_probe_trace_command(struct 
> probe_trace_event *tev)
>   }
>  
>   /* Use the tp->address for uprobes */
> - if (tev->uprobes)
> + if (tev->uprobes) {
>   err = strbuf_addf(, "%s:0x%lx", tp->module, tp->address);
> - else if (!strncmp(tp->symbol, "0x", 2))
> + if (uprobe_ref_ctr_is_supported() &&
> + tp->ref_ctr_offset &&
> + err >= 0)
> + err = strbuf_addf(, "(0x%lx)", tp->ref_ctr_offset);

If the kernel doesn't support uprobe_ref_ctr but the event requires
to increment uprobe_ref_ctr, I think we should (at least) warn user here.

> + } else if (!strncmp(tp->symbol, "0x", 2)) {
>   /* Absolute address. See try_to_find_absolute_address() */
>   err = strbuf_addf(, "%s%s0x%lx", tp->module ?: "",
> tp->module ? ":" : "", tp->address);
> - else
> + } else {
>   err = strbuf_addf(, "%s%s%s+%lu", tp->module ?: "",
>   tp->module ? ":" : "", tp->symbol, tp->offset);
> + }
> +
>   if (err)
>   goto error;
>  
> diff --git a/tools/perf/util/probe-event.h b/tools/perf/util/probe-event.h
> index 45b14f0..15a98c3 100644
> --- a/tools/perf/util/probe-event.h
> +++ b/tools/perf/util/probe-event.h
> @@ -27,6 +27,7 @@ struct probe_trace_point {
>   char*symbol;/* Base symbol */
>   char*module;/* Module name */
>   unsigned long   offset; /* Offset from symbol */
> + unsigned long   ref_ctr_offset; /* SDT reference counter offset */
>   unsigned long   address;/* Actual address of the trace point */
>   boolretprobe;   /* Return probe flag */
>  };
> diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
> index 4ae1123..ca0e524 100644
> --- a/tools/perf/util/probe-file.c
> +++ b/tools/perf/util/probe-file.c
> @@ -697,8 +697,16 @@ int probe_cache__add_entry(struct probe_cache *pcache,
>  #ifdef HAVE_GELF_GETNOTE_SUPPORT
>  static unsigned long long sdt_note__get_addr(struct sdt_note *note)
>  {
> - return note->bit32 ? (unsigned long long)note->addr.a32[0]
> -  : (unsigned long long)note->addr.a64[0];
> + return note->bit32 ?
> + (unsigned long long)note->addr.a32[SDT_NOTE_IDX_LOC] :
> + (unsigned long long)note->addr.a64[SDT_NOTE_IDX_LOC];
> +}
> +
> +static unsigned long long sdt_note__get_ref_ctr_offset(struct sdt_note *note)
> +{
> + return note->bit32 ?
> + (unsigned long long)note->addr.a32[SDT_NOTE_IDX_REFCTR] :
> + (unsigned long long)note->addr.a64[SDT_NOTE_IDX_REFCTR];
>  }
>  
>  static const char * const type_to_suffix[] = {
> @@ -776,14 +784,21 @@ static char *synthesize_sdt_probe_command(struct 
> sdt_note *note,
>  {
>   struct strbuf buf;
>   char *ret = NULL, **args;
> - int i, args_count;
> + int i, args_count, err;
> + unsigned long long ref_ctr_offset;
>  
>   if (strbuf_init(, 32) < 0)
>   return NULL;
>  
> - if (strbuf_addf(, "p:%s/%s %s:0x%llx",
> -