This is the mail archive of the
systemtap@sourceware.org
mailing list for the systemtap project.
Re: [PATCH v4 2/3] Support for perf to probe into SDT markers:
- From: Masami Hiramatsu <masami dot hiramatsu dot pt at hitachi dot com>
- To: Hemant Kumar <hkshaw at linux dot vnet dot ibm dot com>
- Cc: linux-kernel at vger dot kernel dot org, srikar at linux dot vnet dot ibm dot com, peterz at infradead dot org, oleg at redhat dot com, hegdevasant at linux dot vnet dot ibm dot com, mingo at redhat dot com, anton at redhat dot com, systemtap at sourceware dot org, namhyung at kernel dot org, aravinda at linux dot vnet dot ibm dot com
- Date: Thu, 24 Oct 2013 14:45:18 +0900
- Subject: Re: [PATCH v4 2/3] Support for perf to probe into SDT markers:
- Authentication-results: sourceware.org; auth=none
- References: <20131023044511 dot 1886 dot 82571 dot stgit at hemant-fedora> <20131023050502 dot 1886 dot 15779 dot stgit at hemant-fedora>
(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