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

>> @@ -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 for the review :).
Ravi