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

[Hemant Kumar <hkshaw at linux dot vnet dot ibm dot 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