From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751757AbcFTELT (ORCPT ); Mon, 20 Jun 2016 00:11:19 -0400 Received: from szxga01-in.huawei.com ([58.251.152.64]:11536 "EHLO szxga01-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751524AbcFTELI (ORCPT ); Mon, 20 Jun 2016 00:11:08 -0400 Subject: Re: [PATCH v7 7/8] perf tools: Check write_backward during evlist config To: Arnaldo Carvalho de Melo References: <1465957415-83376-1-git-send-email-wangnan0@huawei.com> <1465957415-83376-8-git-send-email-wangnan0@huawei.com> <20160616214724.GI13337@kernel.org> CC: , , He Kuang , Arnaldo Carvalho de Melo , Jiri Olsa , Masami Hiramatsu , Namhyung Kim , Zefan Li From: "Wangnan (F)" Message-ID: <57676C88.2080908@huawei.com> Date: Mon, 20 Jun 2016 12:09:44 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.5.0 MIME-Version: 1.0 In-Reply-To: <20160616214724.GI13337@kernel.org> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.111.66.109] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A090203.57676CD7.0030,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2013-06-18 04:22:30, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: 88180b02489919f4234bb1b9658d6711 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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)