lkml.org 
[lkml]   [2013]   [Oct]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v4 2/3] Support for perf to probe into SDT markers:
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



\
 
 \ /
  Last update: 2013-10-24 12:41    [W:0.163 / U:0.808 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site