From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753709AbbIDBa2 (ORCPT ); Thu, 3 Sep 2015 21:30:28 -0400 Received: from mail9.hitachi.co.jp ([133.145.228.44]:39976 "EHLO mail9.hitachi.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751483AbbIDBa0 convert rfc822-to-8bit (ORCPT ); Thu, 3 Sep 2015 21:30:26 -0400 From: =?iso-2022-jp?B?GyRCSj8+PjJtTCYbKEIgLyBISVJBTUFUVRskQiEkGyhCTUFTQU1J?= To: "'Arnaldo Carvalho de Melo'" CC: Namhyung Kim , Wang Nan , "Kaixu Xia" , Peter Zijlstra , "Daniel Borkmann" , "linux-kernel@vger.kernel.org" , He Kuang , "lizefan@huawei.com" , Jiri Olsa , David Ahern , Brendan Gregg , "mingo@kernel.org" , "ast@plumgrid.com" Subject: RE: [PATCH perf/core ] perf-probe: Output the result of adding/deleting probe in buildin-probe Thread-Topic: [PATCH perf/core ] perf-probe: Output the result of adding/deleting probe in buildin-probe Thread-Index: AQHQ5kJThiNMlyqF2EuWJjhM0XXDo54qquCAgADqtYA= Date: Fri, 4 Sep 2015 01:30:22 +0000 Message-ID: <50399556C9727B4D88A595C8584AAB375250BC29@GSjpTKYDCembx32.service.hitachi.net> References: <20150903002021.GB31697@sejong> <20150903121034.19931.33144.stgit@localhost.localdomain> <20150903202847.GA3284@kernel.org> In-Reply-To: <20150903202847.GA3284@kernel.org> Accept-Language: ja-JP, en-US Content-Language: ja-JP X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.198.220.34] Content-Type: text/plain; charset="iso-2022-jp" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > From: Arnaldo Carvalho de Melo [mailto:acme@kernel.org] > > Em Thu, Sep 03, 2015 at 09:10:35PM +0900, Masami Hiramatsu escreveu: > > Output the normal result of adding/deleting probe in buildin-probe > > instead of showing it by add/del_perf_probe_events. > > All the result string is stored into "result" strbuf parameter. > > If you want to ignore the result string, pass a NULL to the "result". > > Note that all warning/debug strings are still in the > > add/del_perf_probe_events. > > Please provide the before and after output of the affected tools. > > But I'll wait for you to react to Namyung's RFC. Yeah, I think his series is much better than this add-hoc fix :) I'll reply him asap. Thanks! > > - Arnaldo > > > Suggested-by: Namhyung Kim > > Signed-off-by: Masami Hiramatsu > > --- > > tools/perf/builtin-probe.c | 9 +++++++-- > > tools/perf/util/probe-event.c | 33 ++++++++++++++++++++------------- > > tools/perf/util/probe-event.h | 6 ++++-- > > tools/perf/util/probe-file.c | 5 +++-- > > tools/perf/util/probe-file.h | 4 +++- > > 5 files changed, 37 insertions(+), 20 deletions(-) > > > > diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c > > index b81cec3..d11ad21 100644 > > --- a/tools/perf/builtin-probe.c > > +++ b/tools/perf/builtin-probe.c > > @@ -402,6 +402,7 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused) > > "Enable kernel symbol demangling"), > > OPT_END() > > }; > > + struct strbuf buf = STRBUF_INIT; > > int ret; > > > > set_option_flag(options, 'a', "add", PARSE_OPT_EXCLUSIVE); > > @@ -483,7 +484,9 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused) > > return ret; > > #endif > > case 'd': > > - ret = del_perf_probe_events(params.filter); > > + ret = del_perf_probe_events(params.filter, &buf); > > + /* Even if failed, we should show the result first */ > > + pr_info("%s", buf.buf); > > if (ret < 0) { > > pr_err_with_code(" Error: Failed to delete events.", ret); > > return ret; > > @@ -496,7 +499,9 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused) > > usage_with_options(probe_usage, options); > > } > > > > - ret = add_perf_probe_events(params.events, params.nevents); > > + ret = add_perf_probe_events(params.events, params.nevents, &buf); > > + /* Even if failed, we should show the result first */ > > + pr_info("%s", buf.buf); > > if (ret < 0) { > > pr_err_with_code(" Error: Failed to add events.", ret); > > return ret; > > diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c > > index eb5f18b..1a3ed7c 100644 > > --- a/tools/perf/util/probe-event.c > > +++ b/tools/perf/util/probe-event.c > > @@ -2395,7 +2395,8 @@ static int probe_trace_event__set_name(struct probe_trace_event *tev, > > > > static int __add_probe_trace_events(struct perf_probe_event *pev, > > struct probe_trace_event *tevs, > > - int ntevs, bool allow_suffix) > > + int ntevs, bool allow_suffix, > > + struct strbuf *buf) > > { > > int i, fd, ret; > > struct probe_trace_event *tev = NULL; > > @@ -2415,7 +2416,9 @@ static int __add_probe_trace_events(struct perf_probe_event *pev, > > } > > > > ret = 0; > > - pr_info("Added new event%s\n", (ntevs > 1) ? "s:" : ":"); > > + if (buf) > > + strbuf_addf(buf, "Added new event%s\n", > > + (ntevs > 1) ? "s:" : ":"); > > for (i = 0; i < ntevs; i++) { > > tev = &tevs[i]; > > /* Skip if the symbol is out of .text or blacklisted */ > > @@ -2432,9 +2435,12 @@ static int __add_probe_trace_events(struct perf_probe_event *pev, > > if (ret < 0) > > break; > > > > - /* We use tev's name for showing new events */ > > - show_perf_probe_event(tev->group, tev->event, pev, > > - tev->point.module, false); > > + if (buf) { > > + /* We use tev's name for showing new events */ > > + perf_probe_event__sprintf(tev->group, tev->event, > > + pev, tev->point.module, buf); > > + strbuf_addch(buf, '\n'); > > + } > > /* Save the last valid name */ > > event = tev->event; > > group = tev->group; > > @@ -2451,10 +2457,10 @@ static int __add_probe_trace_events(struct perf_probe_event *pev, > > warn_uprobe_event_compat(tev); > > > > /* Note that it is possible to skip all events because of blacklist */ > > - if (ret >= 0 && event) { > > + if (ret >= 0 && event && buf) { > > /* Show how to use the event. */ > > - pr_info("\nYou can now use it in all perf tools, such as:\n\n"); > > - pr_info("\tperf record -e %s:%s -aR sleep 1\n\n", group, event); > > + strbuf_addf(buf, "\nYou can now use it in all perf tools, such as:\n\n"); > > + strbuf_addf(buf, "\tperf record -e %s:%s -aR sleep 1\n\n", group, event); > > } > > > > strlist__delete(namelist); > > @@ -2765,7 +2771,8 @@ struct __event_package { > > int ntevs; > > }; > > > > -int add_perf_probe_events(struct perf_probe_event *pevs, int npevs) > > +int add_perf_probe_events(struct perf_probe_event *pevs, int npevs, > > + struct strbuf *result) > > { > > int i, j, ret; > > struct __event_package *pkgs; > > @@ -2802,7 +2809,7 @@ int add_perf_probe_events(struct perf_probe_event *pevs, int npevs) > > for (i = 0; i < npevs; i++) { > > ret = __add_probe_trace_events(pkgs[i].pev, pkgs[i].tevs, > > pkgs[i].ntevs, > > - probe_conf.force_add); > > + probe_conf.force_add, result); > > if (ret < 0) > > break; > > } > > @@ -2819,7 +2826,7 @@ end: > > return ret; > > } > > > > -int del_perf_probe_events(struct strfilter *filter) > > +int del_perf_probe_events(struct strfilter *filter, struct strbuf *result) > > { > > int ret, ret2, ufd = -1, kfd = -1; > > char *str = strfilter__string(filter); > > @@ -2834,11 +2841,11 @@ int del_perf_probe_events(struct strfilter *filter) > > if (ret < 0) > > goto out; > > > > - ret = probe_file__del_events(kfd, filter); > > + ret = probe_file__del_events(kfd, filter, result); > > if (ret < 0 && ret != -ENOENT) > > goto error; > > > > - ret2 = probe_file__del_events(ufd, filter); > > + ret2 = probe_file__del_events(ufd, filter, result); > > if (ret2 < 0 && ret2 != -ENOENT) { > > ret = ret2; > > goto error; > > diff --git a/tools/perf/util/probe-event.h b/tools/perf/util/probe-event.h > > index 6e7ec68..9855dbf 100644 > > --- a/tools/perf/util/probe-event.h > > +++ b/tools/perf/util/probe-event.h > > @@ -137,8 +137,10 @@ extern void line_range__clear(struct line_range *lr); > > /* Initialize line range */ > > extern int line_range__init(struct line_range *lr); > > > > -extern int add_perf_probe_events(struct perf_probe_event *pevs, int npevs); > > -extern int del_perf_probe_events(struct strfilter *filter); > > +extern int add_perf_probe_events(struct perf_probe_event *pevs, int npevs, > > + struct strbuf *result); > > +extern int del_perf_probe_events(struct strfilter *filter, > > + struct strbuf *result); > > extern int show_perf_probe_events(struct strfilter *filter); > > extern int show_line_range(struct line_range *lr, const char *module, > > bool user); > > diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c > > index bbb2437..e22fa12 100644 > > --- a/tools/perf/util/probe-file.c > > +++ b/tools/perf/util/probe-file.c > > @@ -267,7 +267,6 @@ static int __del_trace_probe_event(int fd, struct str_node *ent) > > goto error; > > } > > > > - pr_info("Removed event: %s\n", ent->s); > > return 0; > > error: > > pr_warning("Failed to delete event: %s\n", > > @@ -275,7 +274,7 @@ error: > > return ret; > > } > > > > -int probe_file__del_events(int fd, struct strfilter *filter) > > +int probe_file__del_events(int fd, struct strfilter *filter, struct strbuf *buf) > > { > > struct strlist *namelist; > > struct str_node *ent; > > @@ -293,6 +292,8 @@ int probe_file__del_events(int fd, struct strfilter *filter) > > ret = __del_trace_probe_event(fd, ent); > > if (ret < 0) > > break; > > + if (buf) > > + strbuf_addf(buf, "Removed event: %s\n", ent->s); > > } > > } > > strlist__delete(namelist); > > diff --git a/tools/perf/util/probe-file.h b/tools/perf/util/probe-file.h > > index ada94a2..ee89ef0 100644 > > --- a/tools/perf/util/probe-file.h > > +++ b/tools/perf/util/probe-file.h > > @@ -1,6 +1,7 @@ > > #ifndef __PROBE_FILE_H > > #define __PROBE_FILE_H > > > > +#include "strbuf.h" > > #include "strlist.h" > > #include "strfilter.h" > > #include "probe-event.h" > > @@ -13,6 +14,7 @@ int probe_file__open_both(int *kfd, int *ufd, int flag); > > struct strlist *probe_file__get_namelist(int fd); > > struct strlist *probe_file__get_rawlist(int fd); > > int probe_file__add_event(int fd, struct probe_trace_event *tev); > > -int probe_file__del_events(int fd, struct strfilter *filter); > > +int probe_file__del_events(int fd, struct strfilter *filter, > > + struct strbuf *buf); > > > > #endif > >