Messages in this thread | | | From | Ian Rogers <> | Date | Mon, 11 May 2020 13:29:42 -0700 | Subject | Re: [PATCH v3] perf c2c: fix '-e list' |
| |
On Mon, May 11, 2020 at 12:36 PM Jiri Olsa <jolsa@redhat.com> wrote: > > On Thu, May 07, 2020 at 03:06:04PM -0700, Ian Rogers wrote: > > When the event is passed as list, the default events should be listed as > > per 'perf mem record -e list'. Previous behavior is: > > > > $ perf c2c record -e list > > failed: event 'list' not found, use '-e list' to get list of available events > > > > Usage: perf c2c record [<options>] [<command>] > > or: perf c2c record [<options>] -- <command> [<options>] > > > > -e, --event <event> event selector. Use 'perf mem record -e list' to list available events > > > > New behavior: > > > > $ perf c2c record -e list > > ldlat-loads : available > > ldlat-stores : available > > > > v3: is a rebase. > > v2: addresses review comments by Jiri Olsa. > > https://lore.kernel.org/lkml/20191127081844.GH32367@krava/ > > Signed-off-by: Ian Rogers <irogers@google.com> > > --- > > tools/perf/Documentation/perf-c2c.txt | 2 +- > > tools/perf/builtin-c2c.c | 9 ++++++++- > > tools/perf/builtin-mem.c | 24 +++++++----------------- > > tools/perf/util/mem-events.c | 15 +++++++++++++++ > > tools/perf/util/mem-events.h | 2 ++ > > 5 files changed, 33 insertions(+), 19 deletions(-) > > > > diff --git a/tools/perf/Documentation/perf-c2c.txt b/tools/perf/Documentation/perf-c2c.txt > > index 2133eb320cb0..98efdab5fbd4 100644 > > --- a/tools/perf/Documentation/perf-c2c.txt > > +++ b/tools/perf/Documentation/perf-c2c.txt > > @@ -40,7 +40,7 @@ RECORD OPTIONS > > -------------- > > -e:: > > --event=:: > > - Select the PMU event. Use 'perf mem record -e list' > > + Select the PMU event. Use 'perf c2c record -e list' > > to list available events. > > > > -v:: > > diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c > > index 1baf4cae086f..d617d5682c68 100644 > > --- a/tools/perf/builtin-c2c.c > > +++ b/tools/perf/builtin-c2c.c > > @@ -2887,8 +2887,15 @@ static int parse_record_events(const struct option *opt, > > { > > bool *event_set = (bool *) opt->value; > > > > + if (!strcmp(str, "list")) { > > + perf_mem_events__list(); > > + exit(0); > > + } > > + if (perf_mem_events__parse(str)) > > + exit(-1); > > won't this exit(-1) callsbreak the parsing stuff? > like displaying the option values on error or such?
The previous code was: - if (strcmp(str, "list")) { This is handled explicitly in the code above this.
- if (!perf_mem_events__parse(str)) { - mem->operation = 0; - return 0; - } - exit(-1); This is the code where the exit -1 happens, I inverted the comparison so that exit was more the exceptional code path. The behavior should be identical.
Thanks, Ian
> other than that it looks ok to me > > jirka >
| |