[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <57676C88.2080908@huawei.com>
Date: Mon, 20 Jun 2016 12:09:44 +0800
From: "Wangnan (F)" <wangnan0@...wei.com>
To: Arnaldo Carvalho de Melo <acme@...nel.org>
CC: <pi3orama@....com>, <linux-kernel@...r.kernel.org>,
He Kuang <hekuang@...wei.com>,
Arnaldo Carvalho de Melo <acme@...hat.com>,
Jiri Olsa <jolsa@...nel.org>,
Masami Hiramatsu <mhiramat@...nel.org>,
Namhyung Kim <namhyung@...nel.org>,
Zefan Li <lizefan@...wei.com>
Subject: Re: [PATCH v7 7/8] perf tools: Check write_backward during evlist
config
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)
Powered by blists - more mailing lists