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

[Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>]

(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 :)


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

[...]
> +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.

> +	}
> +
> +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.

> +	if (!ret && list_empty(&sdt_notes))
> +		ret = -ENOENT;

I think this can be removed, because it always be false (caught by previous !found).

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

Thank you,



-- 
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com