From: He Zhe <[email protected]>
Since commit 72363540c009 ("perf probe: Support multiprobe event") and its
series, if there are multiple probes for one event,
__probe_file__get_namelist would return -EEXIST and cause the following
failure without proper hint, due to adding existing entry to output list.
root@qemuarm64:~# perf probe -x /lib64/libc.so.6 free
Added new events:
probe_libc:free (on free in /lib64/libc-2.31.so)
probe_libc:free (on free in /lib64/libc-2.31.so)
You can now use it in all perf tools, such as:
perf record -e probe_libc:free -aR sleep 1
root@qemuarm64:~# perf probe -l
probe_libc:free (on free@plt in /lib64/libc-2.31.so)
probe_libc:free (on cfree in /lib64/libc-2.31.so)
root@qemuarm64:~# perf probe -x /lib64/libc.so.6 free
Error: Failed to add events.
As we just want to check if there is any probe with the same name, -EEXIST
can be ignored, so we can have the right hint as before.
root@qemuarm64:~# perf probe -x /lib64/libc.so.6 free
Error: event "free" already exists.
Hint: Remove existing event by 'perf probe -d'
or force duplicates by 'perf probe -f'
or set 'force=yes' in BPF source.
Error: Failed to add events.
Signed-off-by: He Zhe <[email protected]>
---
tools/perf/util/probe-file.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
index 5003ba403345..cf44c05f89c1 100644
--- a/tools/perf/util/probe-file.c
+++ b/tools/perf/util/probe-file.c
@@ -201,10 +201,16 @@ static struct strlist *__probe_file__get_namelist(int fd, bool include_group)
if (include_group) {
ret = e_snprintf(buf, 128, "%s:%s", tev.group,
tev.event);
- if (ret >= 0)
+ if (ret >= 0) {
ret = strlist__add(sl, buf);
- } else
+ if (ret == -EEXIST)
+ ret = 0;
+ }
+ } else {
ret = strlist__add(sl, tev.event);
+ if (ret == -EEXIST)
+ ret = 0;
+ }
clear_probe_trace_event(&tev);
if (ret < 0)
break;
--
2.24.1
From: He Zhe <[email protected]>
strlist__add may fail with -ENOMEM or -EEXIST. Check it and give debugging
hint when necessary.
Signed-off-by: He Zhe <[email protected]>
---
tools/perf/builtin-probe.c | 30 ++++++++++++++++--------------
tools/perf/util/probe-file.c | 26 +++++++++++++++++++++-----
2 files changed, 37 insertions(+), 19 deletions(-)
diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
index 26bc5923e6b5..8b4511c70fed 100644
--- a/tools/perf/builtin-probe.c
+++ b/tools/perf/builtin-probe.c
@@ -442,24 +442,26 @@ static int perf_del_probe_events(struct strfilter *filter)
}
ret = probe_file__get_events(kfd, filter, klist);
- if (ret == 0) {
- strlist__for_each_entry(ent, klist)
- pr_info("Removed event: %s\n", ent->s);
+ if (ret < 0)
+ goto out;
- ret = probe_file__del_strlist(kfd, klist);
- if (ret < 0)
- goto error;
- }
+ strlist__for_each_entry(ent, klist)
+ pr_info("Removed event: %s\n", ent->s);
+
+ ret = probe_file__del_strlist(kfd, klist);
+ if (ret < 0)
+ goto error;
ret2 = probe_file__get_events(ufd, filter, ulist);
- if (ret2 == 0) {
- strlist__for_each_entry(ent, ulist)
- pr_info("Removed event: %s\n", ent->s);
+ if (ret2 < 0)
+ goto out;
- ret2 = probe_file__del_strlist(ufd, ulist);
- if (ret2 < 0)
- goto error;
- }
+ strlist__for_each_entry(ent, ulist)
+ pr_info("Removed event: %s\n", ent->s);
+
+ ret2 = probe_file__del_strlist(ufd, ulist);
+ if (ret2 < 0)
+ goto error;
if (ret == -ENOENT && ret2 == -ENOENT)
pr_warning("\"%s\" does not hit any event.\n", str);
diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
index cf44c05f89c1..00f086cba88f 100644
--- a/tools/perf/util/probe-file.c
+++ b/tools/perf/util/probe-file.c
@@ -307,10 +307,14 @@ int probe_file__get_events(int fd, struct strfilter *filter,
p = strchr(ent->s, ':');
if ((p && strfilter__compare(filter, p + 1)) ||
strfilter__compare(filter, ent->s)) {
- strlist__add(plist, ent->s);
- ret = 0;
+ ret = strlist__add(plist, ent->s);
+ if (ret < 0) {
+ pr_debug("strlist__add failed (%d)\n", ret);
+ goto out;
+ }
}
}
+out:
strlist__delete(namelist);
return ret;
@@ -517,7 +521,11 @@ static int probe_cache__load(struct probe_cache *pcache)
ret = -EINVAL;
goto out;
}
- strlist__add(entry->tevlist, buf);
+ ret = strlist__add(entry->tevlist, buf);
+ if (ret < 0) {
+ pr_debug("strlist__add failed (%d)\n", ret);
+ goto out;
+ }
}
}
out:
@@ -678,7 +686,12 @@ int probe_cache__add_entry(struct probe_cache *pcache,
command = synthesize_probe_trace_command(&tevs[i]);
if (!command)
goto out_err;
- strlist__add(entry->tevlist, command);
+ ret = strlist__add(entry->tevlist, command);
+ if (ret < 0) {
+ pr_debug("strlist__add failed (%d)\n", ret);
+ goto out_err;
+ }
+
free(command);
}
list_add_tail(&entry->node, &pcache->entries);
@@ -859,7 +872,10 @@ int probe_cache__scan_sdt(struct probe_cache *pcache, const char *pathname)
break;
}
- strlist__add(entry->tevlist, buf);
+ ret = strlist__add(entry->tevlist, buf);
+ if (ret < 0)
+ pr_debug("strlist__add failed (%d)\n", ret);
+
free(buf);
entry = NULL;
}
--
2.24.1
Hi,
Thanks for reporting. This bug has been reported and I should fixed last year...
https://lkml.org/lkml/2019/12/3/136
Arnaldo, haven't you picked it yet?
Thank you,
On Tue, 25 Feb 2020 22:41:42 +0800
<[email protected]> wrote:
> From: He Zhe <[email protected]>
>
> Since commit 72363540c009 ("perf probe: Support multiprobe event") and its
> series, if there are multiple probes for one event,
> __probe_file__get_namelist would return -EEXIST and cause the following
> failure without proper hint, due to adding existing entry to output list.
>
> root@qemuarm64:~# perf probe -x /lib64/libc.so.6 free
> Added new events:
> probe_libc:free (on free in /lib64/libc-2.31.so)
> probe_libc:free (on free in /lib64/libc-2.31.so)
>
> You can now use it in all perf tools, such as:
>
> perf record -e probe_libc:free -aR sleep 1
>
> root@qemuarm64:~# perf probe -l
> probe_libc:free (on free@plt in /lib64/libc-2.31.so)
> probe_libc:free (on cfree in /lib64/libc-2.31.so)
> root@qemuarm64:~# perf probe -x /lib64/libc.so.6 free
> Error: Failed to add events.
>
> As we just want to check if there is any probe with the same name, -EEXIST
> can be ignored, so we can have the right hint as before.
>
> root@qemuarm64:~# perf probe -x /lib64/libc.so.6 free
> Error: event "free" already exists.
> Hint: Remove existing event by 'perf probe -d'
> or force duplicates by 'perf probe -f'
> or set 'force=yes' in BPF source.
> Error: Failed to add events.
>
> Signed-off-by: He Zhe <[email protected]>
> ---
> tools/perf/util/probe-file.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
> index 5003ba403345..cf44c05f89c1 100644
> --- a/tools/perf/util/probe-file.c
> +++ b/tools/perf/util/probe-file.c
> @@ -201,10 +201,16 @@ static struct strlist *__probe_file__get_namelist(int fd, bool include_group)
> if (include_group) {
> ret = e_snprintf(buf, 128, "%s:%s", tev.group,
> tev.event);
> - if (ret >= 0)
> + if (ret >= 0) {
> ret = strlist__add(sl, buf);
> - } else
> + if (ret == -EEXIST)
> + ret = 0;
> + }
> + } else {
> ret = strlist__add(sl, tev.event);
> + if (ret == -EEXIST)
> + ret = 0;
> + }
> clear_probe_trace_event(&tev);
> if (ret < 0)
> break;
> --
> 2.24.1
>
--
Masami Hiramatsu <[email protected]>
On Tue, 25 Feb 2020 22:41:43 +0800
<[email protected]> wrote:
> From: He Zhe <[email protected]>
>
> strlist__add may fail with -ENOMEM or -EEXIST. Check it and give debugging
> hint when necessary.
>
> Signed-off-by: He Zhe <[email protected]>
> ---
> tools/perf/builtin-probe.c | 30 ++++++++++++++++--------------
> tools/perf/util/probe-file.c | 26 +++++++++++++++++++++-----
> 2 files changed, 37 insertions(+), 19 deletions(-)
>
> diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
> index 26bc5923e6b5..8b4511c70fed 100644
> --- a/tools/perf/builtin-probe.c
> +++ b/tools/perf/builtin-probe.c
> @@ -442,24 +442,26 @@ static int perf_del_probe_events(struct strfilter *filter)
> }
>
> ret = probe_file__get_events(kfd, filter, klist);
> - if (ret == 0) {
> - strlist__for_each_entry(ent, klist)
> - pr_info("Removed event: %s\n", ent->s);
> + if (ret < 0)
> + goto out;
No, this is ignored by design.
Since probe_file__get_events() returns -ENOENT when no event is matched,
this should be just ignored, and goto uprobe event matching.
>
> - ret = probe_file__del_strlist(kfd, klist);
> - if (ret < 0)
> - goto error;
> - }
> + strlist__for_each_entry(ent, klist)
> + pr_info("Removed event: %s\n", ent->s);
> +
> + ret = probe_file__del_strlist(kfd, klist);
> + if (ret < 0)
> + goto error;
>
> ret2 = probe_file__get_events(ufd, filter, ulist);
> - if (ret2 == 0) {
> - strlist__for_each_entry(ent, ulist)
> - pr_info("Removed event: %s\n", ent->s);
> + if (ret2 < 0)
> + goto out;
Ditto.
Thank you,
>
> - ret2 = probe_file__del_strlist(ufd, ulist);
> - if (ret2 < 0)
> - goto error;
> - }
> + strlist__for_each_entry(ent, ulist)
> + pr_info("Removed event: %s\n", ent->s);
> +
> + ret2 = probe_file__del_strlist(ufd, ulist);
> + if (ret2 < 0)
> + goto error;
>
> if (ret == -ENOENT && ret2 == -ENOENT)
> pr_warning("\"%s\" does not hit any event.\n", str);
> diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
> index cf44c05f89c1..00f086cba88f 100644
> --- a/tools/perf/util/probe-file.c
> +++ b/tools/perf/util/probe-file.c
> @@ -307,10 +307,14 @@ int probe_file__get_events(int fd, struct strfilter *filter,
> p = strchr(ent->s, ':');
> if ((p && strfilter__compare(filter, p + 1)) ||
> strfilter__compare(filter, ent->s)) {
> - strlist__add(plist, ent->s);
> - ret = 0;
> + ret = strlist__add(plist, ent->s);
> + if (ret < 0) {
> + pr_debug("strlist__add failed (%d)\n", ret);
> + goto out;
> + }
> }
> }
> +out:
> strlist__delete(namelist);
>
> return ret;
> @@ -517,7 +521,11 @@ static int probe_cache__load(struct probe_cache *pcache)
> ret = -EINVAL;
> goto out;
> }
> - strlist__add(entry->tevlist, buf);
> + ret = strlist__add(entry->tevlist, buf);
> + if (ret < 0) {
> + pr_debug("strlist__add failed (%d)\n", ret);
> + goto out;
> + }
> }
> }
> out:
> @@ -678,7 +686,12 @@ int probe_cache__add_entry(struct probe_cache *pcache,
> command = synthesize_probe_trace_command(&tevs[i]);
> if (!command)
> goto out_err;
> - strlist__add(entry->tevlist, command);
> + ret = strlist__add(entry->tevlist, command);
> + if (ret < 0) {
> + pr_debug("strlist__add failed (%d)\n", ret);
> + goto out_err;
> + }
> +
> free(command);
> }
> list_add_tail(&entry->node, &pcache->entries);
> @@ -859,7 +872,10 @@ int probe_cache__scan_sdt(struct probe_cache *pcache, const char *pathname)
> break;
> }
>
> - strlist__add(entry->tevlist, buf);
> + ret = strlist__add(entry->tevlist, buf);
> + if (ret < 0)
> + pr_debug("strlist__add failed (%d)\n", ret);
> +
> free(buf);
> entry = NULL;
> }
> --
> 2.24.1
>
--
Masami Hiramatsu <[email protected]>
On 2/26/20 6:49 AM, Masami Hiramatsu wrote:
> On Tue, 25 Feb 2020 22:41:43 +0800
> <[email protected]> wrote:
>
>> From: He Zhe <[email protected]>
>>
>> strlist__add may fail with -ENOMEM or -EEXIST. Check it and give debugging
>> hint when necessary.
>>
>> Signed-off-by: He Zhe <[email protected]>
>> ---
>> tools/perf/builtin-probe.c | 30 ++++++++++++++++--------------
>> tools/perf/util/probe-file.c | 26 +++++++++++++++++++++-----
>> 2 files changed, 37 insertions(+), 19 deletions(-)
>>
>> diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
>> index 26bc5923e6b5..8b4511c70fed 100644
>> --- a/tools/perf/builtin-probe.c
>> +++ b/tools/perf/builtin-probe.c
>> @@ -442,24 +442,26 @@ static int perf_del_probe_events(struct strfilter *filter)
>> }
>>
>> ret = probe_file__get_events(kfd, filter, klist);
>> - if (ret == 0) {
>> - strlist__for_each_entry(ent, klist)
>> - pr_info("Removed event: %s\n", ent->s);
>> + if (ret < 0)
>> + goto out;
> No, this is ignored by design.
> Since probe_file__get_events() returns -ENOENT when no event is matched,
> this should be just ignored, and goto uprobe event matching.
Thanks for pointing it out. However when strlist__add in probe_file__get_events
returns a -ENOMEM and we ignore that, though it happens not very likely, we
would miss some entries. So I add checks here and in probe_file__get_events to
give a heads-up in advance.
And the same reason is for the checks below for probe_cache__load,
probe_cache__add_entry and probe_cache__scan_sdt.
Regards,
Zhe
>
>>
>> - ret = probe_file__del_strlist(kfd, klist);
>> - if (ret < 0)
>> - goto error;
>> - }
>> + strlist__for_each_entry(ent, klist)
>> + pr_info("Removed event: %s\n", ent->s);
>> +
>> + ret = probe_file__del_strlist(kfd, klist);
>> + if (ret < 0)
>> + goto error;
>>
>> ret2 = probe_file__get_events(ufd, filter, ulist);
>> - if (ret2 == 0) {
>> - strlist__for_each_entry(ent, ulist)
>> - pr_info("Removed event: %s\n", ent->s);
>> + if (ret2 < 0)
>> + goto out;
> Ditto.
>
> Thank you,
>
>>
>> - ret2 = probe_file__del_strlist(ufd, ulist);
>> - if (ret2 < 0)
>> - goto error;
>> - }
>> + strlist__for_each_entry(ent, ulist)
>> + pr_info("Removed event: %s\n", ent->s);
>> +
>> + ret2 = probe_file__del_strlist(ufd, ulist);
>> + if (ret2 < 0)
>> + goto error;
>>
>> if (ret == -ENOENT && ret2 == -ENOENT)
>> pr_warning("\"%s\" does not hit any event.\n", str);
>> diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
>> index cf44c05f89c1..00f086cba88f 100644
>> --- a/tools/perf/util/probe-file.c
>> +++ b/tools/perf/util/probe-file.c
>> @@ -307,10 +307,14 @@ int probe_file__get_events(int fd, struct strfilter *filter,
>> p = strchr(ent->s, ':');
>> if ((p && strfilter__compare(filter, p + 1)) ||
>> strfilter__compare(filter, ent->s)) {
>> - strlist__add(plist, ent->s);
>> - ret = 0;
>> + ret = strlist__add(plist, ent->s);
>> + if (ret < 0) {
>> + pr_debug("strlist__add failed (%d)\n", ret);
>> + goto out;
>> + }
>> }
>> }
>> +out:
>> strlist__delete(namelist);
>>
>> return ret;
>> @@ -517,7 +521,11 @@ static int probe_cache__load(struct probe_cache *pcache)
>> ret = -EINVAL;
>> goto out;
>> }
>> - strlist__add(entry->tevlist, buf);
>> + ret = strlist__add(entry->tevlist, buf);
>> + if (ret < 0) {
>> + pr_debug("strlist__add failed (%d)\n", ret);
>> + goto out;
>> + }
>> }
>> }
>> out:
>> @@ -678,7 +686,12 @@ int probe_cache__add_entry(struct probe_cache *pcache,
>> command = synthesize_probe_trace_command(&tevs[i]);
>> if (!command)
>> goto out_err;
>> - strlist__add(entry->tevlist, command);
>> + ret = strlist__add(entry->tevlist, command);
>> + if (ret < 0) {
>> + pr_debug("strlist__add failed (%d)\n", ret);
>> + goto out_err;
>> + }
>> +
>> free(command);
>> }
>> list_add_tail(&entry->node, &pcache->entries);
>> @@ -859,7 +872,10 @@ int probe_cache__scan_sdt(struct probe_cache *pcache, const char *pathname)
>> break;
>> }
>>
>> - strlist__add(entry->tevlist, buf);
>> + ret = strlist__add(entry->tevlist, buf);
>> + if (ret < 0)
>> + pr_debug("strlist__add failed (%d)\n", ret);
>> +
>> free(buf);
>> entry = NULL;
>> }
>> --
>> 2.24.1
>>
>
On Wed, 26 Feb 2020 10:49:19 +0800
He Zhe <[email protected]> wrote:
>
>
> On 2/26/20 6:49 AM, Masami Hiramatsu wrote:
> > On Tue, 25 Feb 2020 22:41:43 +0800
> > <[email protected]> wrote:
> >
> >> From: He Zhe <[email protected]>
> >>
> >> strlist__add may fail with -ENOMEM or -EEXIST. Check it and give debugging
> >> hint when necessary.
> >>
> >> Signed-off-by: He Zhe <[email protected]>
> >> ---
> >> tools/perf/builtin-probe.c | 30 ++++++++++++++++--------------
> >> tools/perf/util/probe-file.c | 26 +++++++++++++++++++++-----
> >> 2 files changed, 37 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
> >> index 26bc5923e6b5..8b4511c70fed 100644
> >> --- a/tools/perf/builtin-probe.c
> >> +++ b/tools/perf/builtin-probe.c
> >> @@ -442,24 +442,26 @@ static int perf_del_probe_events(struct strfilter *filter)
> >> }
> >>
> >> ret = probe_file__get_events(kfd, filter, klist);
> >> - if (ret == 0) {
> >> - strlist__for_each_entry(ent, klist)
> >> - pr_info("Removed event: %s\n", ent->s);
> >> + if (ret < 0)
> >> + goto out;
> > No, this is ignored by design.
> > Since probe_file__get_events() returns -ENOENT when no event is matched,
> > this should be just ignored, and goto uprobe event matching.
>
> Thanks for pointing it out. However when strlist__add in probe_file__get_events
> returns a -ENOMEM and we ignore that, though it happens not very likely, we
> would miss some entries. So I add checks here and in probe_file__get_events to
> give a heads-up in advance.
If you are aware of -ENOMEM ( I guess in such case you'll see OOM killer
sooner or later ), please just catch it. I mean
if (ret == -ENOMEM)
goto out;
will be good.
Thank you,
>
> And the same reason is for the checks below for probe_cache__load,
> probe_cache__add_entry and probe_cache__scan_sdt.
>
>
> Regards,
> Zhe
>
> >
> >>
> >> - ret = probe_file__del_strlist(kfd, klist);
> >> - if (ret < 0)
> >> - goto error;
> >> - }
> >> + strlist__for_each_entry(ent, klist)
> >> + pr_info("Removed event: %s\n", ent->s);
> >> +
> >> + ret = probe_file__del_strlist(kfd, klist);
> >> + if (ret < 0)
> >> + goto error;
> >>
> >> ret2 = probe_file__get_events(ufd, filter, ulist);
> >> - if (ret2 == 0) {
> >> - strlist__for_each_entry(ent, ulist)
> >> - pr_info("Removed event: %s\n", ent->s);
> >> + if (ret2 < 0)
> >> + goto out;
> > Ditto.
> >
> > Thank you,
> >
> >>
> >> - ret2 = probe_file__del_strlist(ufd, ulist);
> >> - if (ret2 < 0)
> >> - goto error;
> >> - }
> >> + strlist__for_each_entry(ent, ulist)
> >> + pr_info("Removed event: %s\n", ent->s);
> >> +
> >> + ret2 = probe_file__del_strlist(ufd, ulist);
> >> + if (ret2 < 0)
> >> + goto error;
> >>
> >> if (ret == -ENOENT && ret2 == -ENOENT)
> >> pr_warning("\"%s\" does not hit any event.\n", str);
> >> diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
> >> index cf44c05f89c1..00f086cba88f 100644
> >> --- a/tools/perf/util/probe-file.c
> >> +++ b/tools/perf/util/probe-file.c
> >> @@ -307,10 +307,14 @@ int probe_file__get_events(int fd, struct strfilter *filter,
> >> p = strchr(ent->s, ':');
> >> if ((p && strfilter__compare(filter, p + 1)) ||
> >> strfilter__compare(filter, ent->s)) {
> >> - strlist__add(plist, ent->s);
> >> - ret = 0;
> >> + ret = strlist__add(plist, ent->s);
> >> + if (ret < 0) {
> >> + pr_debug("strlist__add failed (%d)\n", ret);
> >> + goto out;
> >> + }
> >> }
> >> }
> >> +out:
> >> strlist__delete(namelist);
> >>
> >> return ret;
> >> @@ -517,7 +521,11 @@ static int probe_cache__load(struct probe_cache *pcache)
> >> ret = -EINVAL;
> >> goto out;
> >> }
> >> - strlist__add(entry->tevlist, buf);
> >> + ret = strlist__add(entry->tevlist, buf);
> >> + if (ret < 0) {
> >> + pr_debug("strlist__add failed (%d)\n", ret);
> >> + goto out;
> >> + }
> >> }
> >> }
> >> out:
> >> @@ -678,7 +686,12 @@ int probe_cache__add_entry(struct probe_cache *pcache,
> >> command = synthesize_probe_trace_command(&tevs[i]);
> >> if (!command)
> >> goto out_err;
> >> - strlist__add(entry->tevlist, command);
> >> + ret = strlist__add(entry->tevlist, command);
> >> + if (ret < 0) {
> >> + pr_debug("strlist__add failed (%d)\n", ret);
> >> + goto out_err;
> >> + }
> >> +
> >> free(command);
> >> }
> >> list_add_tail(&entry->node, &pcache->entries);
> >> @@ -859,7 +872,10 @@ int probe_cache__scan_sdt(struct probe_cache *pcache, const char *pathname)
> >> break;
> >> }
> >>
> >> - strlist__add(entry->tevlist, buf);
> >> + ret = strlist__add(entry->tevlist, buf);
> >> + if (ret < 0)
> >> + pr_debug("strlist__add failed (%d)\n", ret);
> >> +
> >> free(buf);
> >> entry = NULL;
> >> }
> >> --
> >> 2.24.1
> >>
> >
>
--
Masami Hiramatsu <[email protected]>