linux-kernel - Re: [PATCH v12 1/8] perf tests: Skip backward ring buffer on old kernel

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160701134042.GL5324@kernel.org>
Date:	Fri, 1 Jul 2016 10:40:42 -0300
From:	Arnaldo Carvalho de Melo <acme@...nel.org>
To:	Wang Nan <wangnan0@...wei.com>
Cc:	Jiri Olsa <jolsa@...nel.org>, linux-kernel@...r.kernel.org,
	pi3orama@....com, Zefan Li <lizefan@...wei.com>,
	Arnaldo Carvalho de Melo <acme@...hat.com>
Subject: Re: [PATCH v12 1/8] perf tests: Skip backward ring buffer on old
 kernel

Em Fri, Jul 01, 2016 at 02:23:44AM +0000, Wang Nan escreveu:
> From: Arnaldo Carvalho de Melo <acme@...hat.com>
> 
> 'perf test backward' FAILED on old kernel:
> 
>  [root@...et ~]# perf test -v backward
>  45: Test backward reading from ring buffer                   :
>  --- start ---
>  <SNIP>
>  mmap size 1052672B
>  Unexpected counter: sample_count=0, comm_count=0
>  ---- end ----
>  Test backward reading from ring buffer: FAILED!
>  [root@...et ~]#
> 
> Reason: when kernel doesn't support 'attr.backward', perf_evsel__open() will
> trim this bit and try again, unless evsel->overwrite is true.
> 
> Set evsel->overwrite in this test.

This fixes it, but then, if we get attr.backward set and the kernel
doesn't support it, what whould be the point of trying without it? I.e.
its not like we can do what is being asked for by fallbacking to some
other method.

I.e. perhaps evsel->overwrite is completely unnecessary and places
checking for it should instead check for evsel->attr.backward, no?

Something like the patch below, with it I also get:

[root@...et perf]# perf test backward
45: Test backward reading from ring buffer                   : Skip
[root@...et perf]# perf test -v backward 2>&1 | grep -v regist
45: Test backward reading from ring buffer                   :
--- start ---
test child forked, pid 29997
perf_evlist__open: Invalid argument
test child finished with -2
---- end ----
Test backward reading from ring buffer: Skip
[root@...et perf]# 

- Arnaldo

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 113507716044..060c3b3d14db 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -1003,7 +1003,7 @@ static bool
 perf_evlist__should_poll(struct perf_evlist *evlist __maybe_unused,
 			 struct perf_evsel *evsel)
 {
-	if (evsel->overwrite)
+	if (evsel->attr.write_backward)
 		return false;
 	return true;
 }
@@ -1018,7 +1018,7 @@ static int perf_evlist__mmap_per_evsel(struct perf_evlist *evlist, int idx,
 	evlist__for_each_entry(evlist, evsel) {
 		int fd;
 
-		if (evsel->overwrite != (evlist->overwrite && evlist->backward))
+		if (evsel->attr.write_backward != (evlist->overwrite && evlist->backward))
 			continue;
 
 		if (evsel->system_wide && thread)
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 0fea724e735c..3abe5195fe80 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -1359,6 +1359,9 @@ static int __perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
 	int pid = -1, err;
 	enum { NO_CHANGE, SET_TO_MAX, INCREASED_MAX } set_rlimit = NO_CHANGE;
 
+	if (perf_missing_features.write_backward && evsel->attr.write_backward)
+		return -EINVAL;
+
 	if (evsel->system_wide)
 		nthreads = 1;
 	else
@@ -1389,11 +1392,6 @@ fallback_missing_features:
 	if (perf_missing_features.lbr_flags)
 		evsel->attr.branch_sample_type &= ~(PERF_SAMPLE_BRANCH_NO_FLAGS |
 				     PERF_SAMPLE_BRANCH_NO_CYCLES);
-	if (perf_missing_features.write_backward) {
-		if (evsel->overwrite)
-			return -EINVAL;
-		evsel->attr.write_backward = false;
-	}
 retry_sample_id:
 	if (perf_missing_features.sample_id_all)
 		evsel->attr.sample_id_all = 0;
@@ -1495,7 +1493,7 @@ try_fallback:
 	 */
 	if (!perf_missing_features.write_backward && evsel->attr.write_backward) {
 		perf_missing_features.write_backward = true;
-		goto fallback_missing_features;
+		goto out_close;
 	} else if (!perf_missing_features.clockid_wrong && evsel->attr.use_clockid) {
 		perf_missing_features.clockid_wrong = true;
 		goto fallback_missing_features;
@@ -2404,7 +2402,7 @@ 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)
+		if (evsel->attr.write_backward && 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.");
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 86fed7a2932b..a31ee2d5d2d3 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -114,7 +114,6 @@ struct perf_evsel {
 	bool			tracking;
 	bool			per_pkg;
 	bool			precise_max;
-	bool			overwrite;
 	/* parse modifier helper */
 	int			exclude_GH;
 	int			nr_members;