Re: [PATCH v4 2/3] Support for perf to probe into SDT markers:

[Hemant Kumar <hkshaw@linux.vnet.ibm.com>]

Hi,

On 10/24/2013 11:15 AM, Masami Hiramatsu wrote:
> (2013/10/23 14:05), Hemant Kumar wrote:
>> This allows perf to probe into the sdt markers/notes present in
>> the libraries and executables. We try to find the associated location
>> and handle prelinking (since, stapsdt notes section is not allocated
>> during runtime). Prelinking is handled with the help of base
>> section which is allocated during runtime. This address can be compared
>> with the address retrieved from the notes' description. If its different,
>> we can take this difference and then add to the note's location.
>>
>> We can use existing '-a/--add' option to add events for sdt markers.
>> Also, we can add multiple events at once using the same '-a' option.
>>
>> Usage:
>> perf probe -x /lib64/libc.so.6 -a 'my_event=%libc:setjmp'
>>
>> Output:
>> Added new event:
>>    libc:my_event        (on 0x35981)
>>
>> You can now use it in all perf tools, such as:
>>
>>      perf record -e libc:my_event -aR sleep 1
>>
>>
>> Signed-off-by: Hemant Kumar Shaw <hkshaw@linux.vnet.ibm.com>
> Almost! please check below comments :)

:) Thanks again for reviewing the patches.

>
>>   static int convert_to_probe_trace_events(struct perf_probe_event *pev,
>>   					  struct probe_trace_event **tevs,
>>   					  int max_tevs, const char *target)
>> @@ -1916,11 +1975,23 @@ static int convert_to_probe_trace_events(struct perf_probe_event *pev,
>>   	struct symbol *sym;
>>   	int ret = 0, i;
>>   	struct probe_trace_event *tev;
>> +	char *buf;
>> +	unsigned long long addr;
>>   
>> -	/* Convert perf_probe_event with debuginfo */
>> -	ret = try_to_find_probe_trace_events(pev, tevs, max_tevs, target);
>> -	if (ret != 0)
>> -		return ret;	/* Found in debuginfo or got an error */
>> +	if (pev->sdt) {
>> +		ret = -EBADF;
>> +		if (pev->uprobes)
>> +			ret = try_to_find_sdt_notes(pev, target);
>> +		if (ret)
>> +			return ret;
>> +	} else {
>> +		/* Convert perf_probe_event with debuginfo */
>> +		ret = try_to_find_probe_trace_events(pev, tevs, max_tevs,
>> +						     target);
>> +		/* Found in debuginfo or got an error */
>> +		if (ret != 0)
>> +			return ret;
> These "ret != 0" checkers can be merged.
>
> [...]

Indeed, they can be merged.

>> +int search_sdt_note(struct sdt_note *key, const char *target)
>> +{
>> +	Elf *elf;
>> +	int fd, ret;
>> +	bool found = false;
>> +	struct sdt_note *pos = NULL;
>> +	LIST_HEAD(sdt_notes);
>> +
>> +	fd = open(target, O_RDONLY);
>> +	if (fd < 0) {
>> +		pr_err("%s : %s\n", target, strerror(errno));
>> +		return -errno;
>> +	}
>> +
>> +	symbol__elf_init();
>> +	elf = elf_begin(fd, PERF_ELF_C_READ_MMAP, NULL);
>> +	if (!elf) {
>> +		ret = -EBADF;
>> +		pr_debug("Can't read the elf of %s\n", target);
>> +		goto out_close;
>> +	}
>> +
>> +	ret = construct_sdt_notes_list(elf, &sdt_notes);
>> +	if (ret)
>> +		goto out_end;
>> +
>> +	/* Iterate through the notes and retrieve the required note */
>> +	list_for_each_entry(pos, &sdt_notes, note_list) {
>> +		if (!strcmp(key->name, pos->name) &&
>> +		    !strcmp(key->provider, pos->provider)) {
>> +			adjust_note_addr(pos, key, elf);
>> +			found = true;
>> +			break;
>> +		}
>> +	}
>> +	if (!found) {
>> +		printf("%%%s:%s not found in %s!\n", key->provider, key->name,
>> +		       target);
>> +		return -ENOENT;
> Here, you skipped the closing process. maybe ret = -ENOENT is enough here.

Yeah, I have missed this. But I think ret = -ENOENT is not enough. We 
shouldn't go to sdt_err() for this case. We can return -ENOENT and print 
the error.

>> +	}
>> +
>> +out_end:
>> +	elf_end(elf);
>> +out_close:
>> +	close(fd);
>> +	ret = sdt_err(ret, target);
> It seems the sdt_err is only for the return value of contruct_sdt_notes_list(),
> thus it is better to integrate it.

Yeah it'll be better to integrate it.

>
>> +	if (!ret && list_empty(&sdt_notes))
>> +		ret = -ENOENT;
> I think this can be removed, because it always be false (caught by previous !found).

I wrote this check for 'no markers present' case. Anyways, the result is 
the same, so,
I guess we can eliminate this (since, its covered by the 'found' check).

>
>> +	cleanup_sdt_note_list(&sdt_notes);
>> +	return ret;
>> +}
>

I think it'll be better to move the check for found : if (!found) {} 
after cleanup_sdt_notes().

-- 
Thanks
Hemant Kumar