Re: [PATCH v7 7/8] perf tools: Check write_backward during evlist config

[Arnaldo Carvalho de Melo <acme@kernel.org>]

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:

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 9b2e3e624efe..4e8e6d8795d0 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -1389,8 +1389,11 @@ 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 (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;
@@ -1453,12 +1456,6 @@ retry_open:
 				err = -EINVAL;
 				goto out_close;
 			}
-
-			if (evsel->overwrite &&
-			    perf_missing_features.write_backward) {
-				err = -EINVAL;
-				goto out_close;
-			}
 		}
 	}
 
@@ -1496,7 +1493,10 @@ try_fallback:
 	 * Must probe features in the order they were added to the
 	 * perf_event_attr interface.
 	 */
-	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)