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 <ravi.bangoria@xxxxxxxxxxxxxxxxxx> 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 <ravi.bangoria@xxxxxxxxxxxxxxxxxx> 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(&buf, "%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(&buf, "(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(&buf, 32) < 0)
> >>  		return NULL;
> >>  
> >> -	if (strbuf_addf(&buf, "p:%s/%s %s:0x%llx",
> >> -				sdtgrp, note->name, pathname,
> >> -				sdt_note__get_addr(note)) < 0)
> >> +	err = strbuf_addf(&buf, "p:%s/%s %s:0x%llx",
> >> +			sdtgrp, note->name, pathname,
> >> +			sdt_note__get_addr(note));
> >> +
> >> +	ref_ctr_offset = sdt_note__get_ref_ctr_offset(note);
> >> +	if (uprobe_ref_ctr_is_supported() && ref_ctr_offset && err >= 0)
> >> +		err = strbuf_addf(&buf, "(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 <mhiramat@xxxxxxxxxx>