| Message ID | 1465957415-83376-8-git-send-email-wangnan0@huawei.com |
|---|---|
| State | New |
| Headers | show |
On 2016/6/17 5:47, Arnaldo Carvalho de Melo wrote: > Em Wed, Jun 15, 2016 at 02:23:34AM +0000, Wang Nan escreveu: >> Before this patch, when using overwritable ring buffer on an old >> kernel, error message is misleading: >> >> # ~/perf record -m 1 -e raw_syscalls:*/overwrite/ -a >> Error: >> The raw_syscalls:sys_enter event is not supported. >> >> This patch output clear error message to tell user his/her kernel >> is too old: >> >> # ~/perf record -m 1 -e raw_syscalls:*/overwrite/ -a >> Reading from overwrite event is not supported by this kernel >> Error: >> The raw_syscalls:sys_enter event is not supported. > So I went to see if exposing that missing_features struct outside > evsel.c was strictly needed and found that we already have fallbacking > for this feature (attr.write_backwards) i.e. if we set it and > sys_perf_event_open() fails, we will check if we are asking the kernel > for some attr. field that it doesn't supports, set that missing_features > and try again. > > But the way this was done for attr.write_backwards was buggy, as we need > to check features in the inverse order of their introduction to the > kernel, so that a newer tool checks first the newest perf_event_attr > fields, detecting that the older kernel doesn't have support for them. > The patch that introduced write_backwards support ([1]) in perf_evsel__open() > did this checking after all the other older attributes, wrongly. > > [1]: b90dc17a5d14 ("perf evsel: Add overwrite attribute and check write_backward") > > Also we shouldn't even try to call sys_perf_event_open if > perf_missing_features.write_backward is true and evsel->overwrite is > also true, the old code would check this only after successfully opening > the fd, do it before the open loop. > > Please take a look at the following patch, see if it is sufficient for > handling older kernels, probably we need to emit a message to the user, > but that has to be done at the builtin- level, i.e. at the tool, i.e. > perf_evsel_open__strerror() should have what it takes to figure out this > extra error and provide/ a proper string, lemme add this to the patch... > done, please check: > > write_backwards_fallback.patch: [SNIP] > > @@ -1496,7 +1493,10 @@ try_fallback: > * Must probe features in the order they were added to the > * perf_event_attr interface. > */ I read this comment but misunderstand. I thought 'order' means newest last. Will try your patch. Thank you. > - if (!perf_missing_features.clockid_wrong && evsel->attr.use_clockid) { > + if (!perf_missing_features.write_backward && evsel->attr.write_backward) { > + perf_missing_features.write_backward = true; > + goto fallback_missing_features; > + } else if (!perf_missing_features.clockid_wrong && evsel->attr.use_clockid) { > perf_missing_features.clockid_wrong = true; > goto fallback_missing_features; > } else if (!perf_missing_features.clockid && evsel->attr.use_clockid) { > @@ -1521,10 +1521,6 @@ try_fallback: > PERF_SAMPLE_BRANCH_NO_FLAGS))) { > perf_missing_features.lbr_flags = true; > goto fallback_missing_features; > - } else if (!perf_missing_features.write_backward && > - evsel->attr.write_backward) { > - perf_missing_features.write_backward = true; > - goto fallback_missing_features; > } > > out_close: > @@ -2409,6 +2405,8 @@ int perf_evsel__open_strerror(struct perf_evsel *evsel, struct target *target, > "We found oprofile daemon running, please stop it and try again."); > break; > case EINVAL: > + if (evsel->overwrite && perf_missing_features.write_backward) > + return scnprintf(msg, size, "Reading from overwrite event is not supported by this kernel."); > if (perf_missing_features.clockid) > return scnprintf(msg, size, "clockid feature not supported."); > if (perf_missing_features.clockid_wrong)
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c index ef223c7..59a69bb 100644 --- a/tools/perf/util/evsel.c +++ b/tools/perf/util/evsel.c @@ -29,17 +29,7 @@ #include "trace-event.h" #include "stat.h" -static struct { - bool sample_id_all; - bool exclude_guest; - bool mmap2; - bool cloexec; - bool clockid; - bool clockid_wrong; - bool lbr_flags; - bool write_backward; -} perf_missing_features; - +struct perf_missing_features perf_missing_features; static clockid_t clockid; static int perf_evsel__no_extra_init(struct perf_evsel *evsel __maybe_unused) @@ -690,8 +680,11 @@ static void apply_config_terms(struct perf_evsel *evsel, * possible to set overwrite globally, without config * terms. */ - if (evsel->overwrite) + if (evsel->overwrite) { + WARN_ONCE(perf_missing_features.write_backward, + "Reading from overwrite event is not supported by this kernel\n"); attr->write_backward = 1; + } /* User explicitly set per-event callgraph, clear the old setting and reset. */ if ((callgraph_buf != NULL) || (dump_size > 0) || max_stack) { diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h index 55ff958..17e506a 100644 --- a/tools/perf/util/evsel.h +++ b/tools/perf/util/evsel.h @@ -11,6 +11,19 @@ #include "cpumap.h" #include "counts.h" +struct perf_missing_features { + bool sample_id_all; + bool exclude_guest; + bool mmap2; + bool cloexec; + bool clockid; + bool clockid_wrong; + bool lbr_flags; + bool write_backward; +}; + +extern struct perf_missing_features perf_missing_features; + struct perf_evsel; /* diff --git a/tools/perf/util/record.c b/tools/perf/util/record.c index 481792c..e3ab812 100644 --- a/tools/perf/util/record.c +++ b/tools/perf/util/record.c @@ -90,6 +90,11 @@ static void perf_probe_context_switch(struct perf_evsel *evsel) evsel->attr.context_switch = 1; } +static void perf_probe_write_backward(struct perf_evsel *evsel) +{ + evsel->attr.write_backward = 1; +} + bool perf_can_sample_identifier(void) { return perf_probe_api(perf_probe_sample_identifier); @@ -129,6 +134,17 @@ bool perf_can_record_cpu_wide(void) return true; } +static void perf_check_write_backward(void) +{ + static bool checked = false; + + if (!checked) { + perf_missing_features.write_backward = + !perf_probe_api(perf_probe_write_backward); + checked = true; + } +} + void perf_evlist__config(struct perf_evlist *evlist, struct record_opts *opts, struct callchain_param *callchain) { @@ -136,6 +152,7 @@ void perf_evlist__config(struct perf_evlist *evlist, struct record_opts *opts, bool use_sample_identifier = false; bool use_comm_exec; + perf_check_write_backward(); /* * Set the evsel leader links before we configure attributes, * since some might depend on this info.
Before this patch, when using overwritable ring buffer on an old kernel, error message is misleading: # ~/perf record -m 1 -e raw_syscalls:*/overwrite/ -a Error: The raw_syscalls:sys_enter event is not supported. This patch output clear error message to tell user his/her kernel is too old: # ~/perf record -m 1 -e raw_syscalls:*/overwrite/ -a Reading from overwrite event is not supported by this kernel Error: The raw_syscalls:sys_enter event is not supported. Signed-off-by: Wang Nan <wangnan0@huawei.com> Cc: He Kuang <hekuang@huawei.com> Cc: Arnaldo Carvalho de Melo <acme@redhat.com> Cc: Jiri Olsa <jolsa@kernel.org> Cc: Masami Hiramatsu <mhiramat@kernel.org> Cc: Namhyung Kim <namhyung@kernel.org> Cc: Zefan Li <lizefan@huawei.com> Cc: pi3orama@163.com --- tools/perf/util/evsel.c | 17 +++++------------ tools/perf/util/evsel.h | 13 +++++++++++++ tools/perf/util/record.c | 17 +++++++++++++++++ 3 files changed, 35 insertions(+), 12 deletions(-) -- 1.8.3.4