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

[Ravi Bangoria]

On 03/14/2018 07:39 PM, Masami Hiramatsu wrote:
> Hi Ravi,
>
> This code logic looks good. I just have several small comments for style.
>
> On Tue, 13 Mar 2018 18:26:02 +0530
> Ravi Bangoria <ravi.bangoria@xxxxxxxxxxxxxxxxxx> wrote:
>
>> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
>> index e1dbc98..2cbe68a 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,22 @@ char *synthesize_probe_trace_command(struct probe_trace_event *tev)
>>  	}
>>  
>>  	/* Use the tp->address for uprobes */
>> -	if (tev->uprobes)
>> -		err = strbuf_addf(&buf, "%s:0x%lx", tp->module, tp->address);
>> -	else if (!strncmp(tp->symbol, "0x", 2))
>> +	if (tev->uprobes) {
>> +		if (tp->ref_ctr_offset)
>> +			err = strbuf_addf(&buf, "%s:0x%lx(0x%lx)", tp->module,
>> +					  tp->address, tp->ref_ctr_offset);
>> +		else
>> +			err = strbuf_addf(&buf, "%s:0x%lx", tp->module,
>> +					  tp->address);
>> +	} else if (!strncmp(tp->symbol, "0x", 2)) {
>>  		/* Absolute address. See try_to_find_absolute_address() */
>>  		err = strbuf_addf(&buf, "%s%s0x%lx", tp->module ?: "",
>>  				  tp->module ? ":" : "", tp->address);
>> -	else
>> +	} else {
>>  		err = strbuf_addf(&buf, "%s%s%s+%lu", tp->module ?: "",
>>  				tp->module ? ":" : "", tp->symbol, tp->offset);
>> +	}
> What the purpose of this {}?

The starting if has multiple statements and thus it needs braces. So I added
braces is all other conditions.

>> +
>>  	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 */
>>  	bool		retprobe;	/* Return probe flag */
>>  };
>> diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
>> index 4ae1123..08ba3a6 100644
>> --- a/tools/perf/util/probe-file.c
>> +++ b/tools/perf/util/probe-file.c
>> @@ -701,6 +701,12 @@ static unsigned long long sdt_note__get_addr(struct sdt_note *note)
>>  		 : (unsigned long long)note->addr.a64[0];
>>  }
>>  
>> +static unsigned long long sdt_note__get_ref_ctr_offset(struct sdt_note *note)
>> +{
>> +	return note->bit32 ? (unsigned long long)note->addr.a32[2]
>> +		: (unsigned long long)note->addr.a64[2];
>> +}
> Could you please introduce an enum for specifying the index by name?
>
> e.g.
> enum {
> 	SDT_NOTE_IDX_ADDR = 0,
> 	SDT_NOTE_IDX_REFCTR = 2,
> };

That will be good. Will change it.

>> +
>>  static const char * const type_to_suffix[] = {
>>  	":s64", "", "", "", ":s32", "", ":s16", ":s8",
>>  	"", ":u8", ":u16", "", ":u32", "", "", "", ":u64"
>> @@ -776,14 +782,24 @@ 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",
>> +	ref_ctr_offset = sdt_note__get_ref_ctr_offset(note);
>> +
>> +	if (ref_ctr_offset)
>> +		err = strbuf_addf(&buf, "p:%s/%s %s:0x%llx(0x%llx)",
>>  				sdtgrp, note->name, pathname,
>> -				sdt_note__get_addr(note)) < 0)
>> +				sdt_note__get_addr(note), ref_ctr_offset);
>> +	else
>> +		err = strbuf_addf(&buf, "p:%s/%s %s:0x%llx",
>> +				sdtgrp, note->name, pathname,
>> +				sdt_note__get_addr(note));
> This can be minimized (and avoid repeating) by using 2 strbuf_addf()s, like
>
> 	err = strbuf_addf(&buf, "p:%s/%s %s:0x%llx",
> 			sdtgrp, note->name, pathname,
> 			sdt_note__get_addr(note));
> 	if (ref_ctr_offset && !err < 0)
> 		err = strbuf_addf("(0x%llx)", ref_ctr_offset);

Sure. Will change it.

>
>> +
>> +	if (err < 0)
>>  		goto error;
>>  
>>  	if (!note->args)
>> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
>> index 2de7705..76c7b54 100644
>> --- a/tools/perf/util/symbol-elf.c
>> +++ b/tools/perf/util/symbol-elf.c
>> @@ -1928,6 +1928,16 @@ static int populate_sdt_note(Elf **elf, const char *data, size_t len,
>>  		}
>>  	}
>>  
>> +	/* Adjust reference counter offset */
>> +	if (elf_section_by_name(*elf, &ehdr, &shdr, SDT_PROBES_SCN, NULL)) {
>> +		if (shdr.sh_offset) {
>> +			if (tmp->bit32)
>> +				tmp->addr.a32[2] -= (shdr.sh_addr - shdr.sh_offset);
>> +			else
>> +				tmp->addr.a64[2] -= (shdr.sh_addr - shdr.sh_offset);
> Here we should use enum above too.

Sure.

Thanks for the review :)
Ravi