Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932134AbbLOHxp (ORCPT ); Tue, 15 Dec 2015 02:53:45 -0500 Received: from mail-pf0-f178.google.com ([209.85.192.178]:34216 "EHLO mail-pf0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752751AbbLOHxY (ORCPT ); Tue, 15 Dec 2015 02:53:24 -0500 Date: Tue, 15 Dec 2015 16:52:42 +0900 From: Namhyung Kim To: Wang Nan Cc: acme@kernel.org, jolsa@kernel.org, adrian.hunter@intel.com, a.p.zijlstra@chello.nl, masami.hiramatsu.pt@hitachi.com, linux-kernel@vger.kernel.org, Arnaldo Carvalho de Melo Subject: Re: [PATCH] perf tests: Fix false TEST_OK result for 'perf test hist' Message-ID: <20151215075242.GA1821@danjae.kornet> References: <1450153124-130381-1-git-send-email-wangnan0@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1450153124-130381-1-git-send-email-wangnan0@huawei.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6875 Lines: 213 Hi, On Tue, Dec 15, 2015 at 04:18:44AM +0000, Wang Nan wrote: > Commit 71d6de64feddd4b455555326fba2111b3006d9e0 ('perf test: Fix hist > testcases when kptr_restrict is on') solves a double free problem when > 'perf test hist' calling setup_fake_machine(). However, the result is > still incorrect. For example: > > $ ./perf test -v 'filtering hist entries' > 25: Test filtering hist entries : > --- start --- > test child forked, pid 4186 > Cannot create kernel maps > test child finished with 0 > ---- end ---- > Test filtering hist entries: Ok > > In this case the body of this test is not get executed at all, but the > result is 'Ok'. I think 'Skip' should be better. Right, I missed this part. But on second thought, I think it should not create real kernel maps. What setup_fake_machine() does is to create/insert fake dso/mmap/symbols for testing hist functions. So it has to use the predefined info not real ones. > > The reason is that *ALL* hists test cases forget to reset err after > using it to hold an error code. > > This patch ensure err is reset. > > In case when kptr_restrict prevent normal user get kernel address, this > test should be skipped, not fail. This patch uses linux/err.h to store > error code in return value of setup_fake_machine(), and let 'EACCES' to > indicate this problem. Please just remove the machine__create_kernel_maps() in setup_fake_machine() and simply return TEST_FAIL if there's an error. Thanks, Namhyung > > Signed-off-by: Wang Nan > Cc: Arnaldo Carvalho de Melo > Cc: Jiri Olsa > Cc: Masami Hiramatsu > Cc: Namhyung Kim > --- > tools/perf/tests/hists_common.c | 12 +++++++++--- > tools/perf/tests/hists_common.h | 1 + > tools/perf/tests/hists_cumulate.c | 6 +++++- > tools/perf/tests/hists_filter.c | 6 +++++- > tools/perf/tests/hists_link.c | 6 +++++- > tools/perf/tests/hists_output.c | 6 +++++- > 6 files changed, 30 insertions(+), 7 deletions(-) > > diff --git a/tools/perf/tests/hists_common.c b/tools/perf/tests/hists_common.c > index bcfd081..2eca1b2 100644 > --- a/tools/perf/tests/hists_common.c > +++ b/tools/perf/tests/hists_common.c > @@ -81,15 +81,20 @@ struct machine *setup_fake_machine(struct machines *machines) > { > struct machine *machine = machines__find(machines, HOST_KERNEL_ID); > size_t i; > + int err = -ENOMEM; > > if (machine == NULL) { > pr_debug("Not enough memory for machine setup\n"); > - return NULL; > + return ERR_PTR(-ENOMEM); > } > > if (machine__create_kernel_maps(machine)) { > pr_debug("Cannot create kernel maps\n"); > - return NULL; > + if (symbol_conf.kptr_restrict) { > + pr_debug("Hint: Check /proc/sys/kernel/kptr_restrict.\n"); > + err = -EACCES; > + } > + return ERR_PTR(err); > } > > for (i = 0; i < ARRAY_SIZE(fake_threads); i++) { > @@ -155,7 +160,8 @@ struct machine *setup_fake_machine(struct machines *machines) > out: > pr_debug("Not enough memory for machine setup\n"); > machine__delete_threads(machine); > - return NULL; > + machine__exit(machine); > + return ERR_PTR(err); > } > > void print_hists_in(struct hists *hists) > diff --git a/tools/perf/tests/hists_common.h b/tools/perf/tests/hists_common.h > index 888254e..0252eae 100644 > --- a/tools/perf/tests/hists_common.h > +++ b/tools/perf/tests/hists_common.h > @@ -1,5 +1,6 @@ > #ifndef __PERF_TESTS__HISTS_COMMON_H__ > #define __PERF_TESTS__HISTS_COMMON_H__ > +#include > > struct machine; > struct machines; > diff --git a/tools/perf/tests/hists_cumulate.c b/tools/perf/tests/hists_cumulate.c > index 8292948..c211075 100644 > --- a/tools/perf/tests/hists_cumulate.c > +++ b/tools/perf/tests/hists_cumulate.c > @@ -706,13 +706,17 @@ int test__hists_cumulate(int subtest __maybe_unused) > err = parse_events(evlist, "cpu-clock", NULL); > if (err) > goto out; > + err = TEST_FAIL; > > machines__init(&machines); > > /* setup threads/dso/map/symbols also */ > machine = setup_fake_machine(&machines); > - if (!machine) > + if (IS_ERR(machine)) { > + if (PTR_ERR(machine) == -EACCES) > + err = TEST_SKIP; > goto out; > + } > > if (verbose > 1) > machine__fprintf(machine, stderr); > diff --git a/tools/perf/tests/hists_filter.c b/tools/perf/tests/hists_filter.c > index ccb5b49..795a04d 100644 > --- a/tools/perf/tests/hists_filter.c > +++ b/tools/perf/tests/hists_filter.c > @@ -120,6 +120,7 @@ int test__hists_filter(int subtest __maybe_unused) > err = parse_events(evlist, "task-clock", NULL); > if (err) > goto out; > + err = TEST_FAIL; > > /* default sort order (comm,dso,sym) will be used */ > if (setup_sorting() < 0) > @@ -129,8 +130,11 @@ int test__hists_filter(int subtest __maybe_unused) > > /* setup threads/dso/map/symbols also */ > machine = setup_fake_machine(&machines); > - if (!machine) > + if (IS_ERR(machine)) { > + if (PTR_ERR(machine) == -EACCES) > + err = TEST_SKIP; > goto out; > + } > > if (verbose > 1) > machine__fprintf(machine, stderr); > diff --git a/tools/perf/tests/hists_link.c b/tools/perf/tests/hists_link.c > index 6243e2b..f5f37ef9 100644 > --- a/tools/perf/tests/hists_link.c > +++ b/tools/perf/tests/hists_link.c > @@ -293,6 +293,7 @@ int test__hists_link(int subtest __maybe_unused) > if (err) > goto out; > > + err = TEST_FAIL; > /* default sort order (comm,dso,sym) will be used */ > if (setup_sorting() < 0) > goto out; > @@ -301,8 +302,11 @@ int test__hists_link(int subtest __maybe_unused) > > /* setup threads/dso/map/symbols also */ > machine = setup_fake_machine(&machines); > - if (!machine) > + if (IS_ERR(machine)) { > + if (PTR_ERR(machine) == -EACCES) > + err = TEST_SKIP; > goto out; > + } > > if (verbose > 1) > machine__fprintf(machine, stderr); > diff --git a/tools/perf/tests/hists_output.c b/tools/perf/tests/hists_output.c > index 248beec..cc570cf 100644 > --- a/tools/perf/tests/hists_output.c > +++ b/tools/perf/tests/hists_output.c > @@ -597,13 +597,17 @@ int test__hists_output(int subtest __maybe_unused) > err = parse_events(evlist, "cpu-clock", NULL); > if (err) > goto out; > + err = TEST_FAIL; > > machines__init(&machines); > > /* setup threads/dso/map/symbols also */ > machine = setup_fake_machine(&machines); > - if (!machine) > + if (IS_ERR(machine)) { > + if (PTR_ERR(machine) == -EACCES) > + err = TEST_SKIP; > goto out; > + } > > if (verbose > 1) > machine__fprintf(machine, stderr); > -- > 1.8.3.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/