[PATCH v2 0/2] perf tests: Check for ARM [vectors] page

[PATCH v2 0/2] perf tests: Check for ARM [vectors] page

[Florian Fainelli]

Hi all,

I just painfully learned that perf would segfault when
CONFIG_KUSER_HELPERS is disabled because it unconditionally makes use of
it. This patch series adds an ARM test for that by leveraging the
existing find_vdso_map() function and making it more generic and capable
of location any map within /proc/self/maps.

Changes in v2:

- use strlen() instead of sizeof() -1 since we made the page name a
  parameter
- use TEST_OK/TEST_FAIL in lieu of 0/-1
- added an error message indicating CONFIG_KUSER_HELPERS might be
  disabled

Florian Fainelli (2):
  perf tools: Make find_vdso_map() more modular
  perf tests: Add a test for the ARM 32-bit [vectors] page

 tools/perf/arch/arm/tests/Build          |  1 +
 tools/perf/arch/arm/tests/arch-tests.c   |  4 +++
 tools/perf/arch/arm/tests/vectors-page.c | 24 ++++++++++++++++++
 tools/perf/tests/tests.h                 |  5 ++++
 tools/perf/util/find-map.c               | 31 ++++++++++++++++++++++++
 tools/perf/util/find-vdso-map.c          | 30 +++--------------------
 6 files changed, 68 insertions(+), 27 deletions(-)
 create mode 100644 tools/perf/arch/arm/tests/vectors-page.c
 create mode 100644 tools/perf/util/find-map.c

-- 
2.17.1

[PATCH v2 1/2] perf tools: Make find_vdso_map() more modular

[Florian Fainelli]

In preparation for checking that the vectors page on the ARM
architecture, refactor the find_vdso_map() function to accept finding an
arbitrary string and create a dedicated helper function for that under
util/find-map.c and update find_vdso_map() to use it.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 tools/perf/util/find-map.c      | 31 +++++++++++++++++++++++++++++++
 tools/perf/util/find-vdso-map.c | 30 +++---------------------------
 2 files changed, 34 insertions(+), 27 deletions(-)
 create mode 100644 tools/perf/util/find-map.c

diff --git a/tools/perf/util/find-map.c b/tools/perf/util/find-map.c
new file mode 100644
index 000000000000..19a3431a7b2a
--- /dev/null
+++ b/tools/perf/util/find-map.c
@@ -0,0 +1,31 @@
+// SPDX-License-Identifier: GPL-2.0
+static int find_map(void **start, void **end, const char *name)
+{
+	FILE *maps;
+	char line[128];
+	int found = 0;
+
+	maps = fopen("/proc/self/maps", "r");
+	if (!maps) {
+		fprintf(stderr, "vdso: cannot open maps\n");
+		return -1;
+	}
+
+	while (!found && fgets(line, sizeof(line), maps)) {
+		int m = -1;
+
+		/* We care only about private r-x mappings. */
+		if (2 != sscanf(line, "%p-%p r-xp %*x %*x:%*x %*u %n",
+				start, end, &m))
+			continue;
+		if (m < 0)
+			continue;
+
+		if (!strncmp(&line[m], name, strlen(name)))
+			found = 1;
+	}
+
+	fclose(maps);
+	return !found;
+}
+
diff --git a/tools/perf/util/find-vdso-map.c b/tools/perf/util/find-vdso-map.c
index d7823e3508fc..840d7d6e29e2 100644
--- a/tools/perf/util/find-vdso-map.c
+++ b/tools/perf/util/find-vdso-map.c
@@ -1,31 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
+#include "find-map.c"
+
 static int find_vdso_map(void **start, void **end)
 {
-	FILE *maps;
-	char line[128];
-	int found = 0;
-
-	maps = fopen("/proc/self/maps", "r");
-	if (!maps) {
-		fprintf(stderr, "vdso: cannot open maps\n");
-		return -1;
-	}
-
-	while (!found && fgets(line, sizeof(line), maps)) {
-		int m = -1;
-
-		/* We care only about private r-x mappings. */
-		if (2 != sscanf(line, "%p-%p r-xp %*x %*x:%*x %*u %n",
-				start, end, &m))
-			continue;
-		if (m < 0)
-			continue;
-
-		if (!strncmp(&line[m], VDSO__MAP_NAME,
-			     sizeof(VDSO__MAP_NAME) - 1))
-			found = 1;
-	}
-
-	fclose(maps);
-	return !found;
+	return find_map(start, end, VDSO__MAP_NAME);
 }
-- 
2.17.1

[PATCH v2 2/2] perf tests: Add a test for the ARM 32-bit [vectors] page

[Florian Fainelli]

perf on ARM requires CONFIG_KUSER_HELPERS to be turned on to allow some
independance with respect to the ARM CPU being used. Add a test which
tries to locate the [vectors] page, created when CONFIG_KUSER_HELPERS is
turned on to help asses the system's health.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 tools/perf/arch/arm/tests/Build          |  1 +
 tools/perf/arch/arm/tests/arch-tests.c   |  4 ++++
 tools/perf/arch/arm/tests/vectors-page.c | 24 ++++++++++++++++++++++++
 tools/perf/tests/tests.h                 |  5 +++++
 4 files changed, 34 insertions(+)
 create mode 100644 tools/perf/arch/arm/tests/vectors-page.c

diff --git a/tools/perf/arch/arm/tests/Build b/tools/perf/arch/arm/tests/Build
index 883c57ff0c08..d9ae2733f9cc 100644
--- a/tools/perf/arch/arm/tests/Build
+++ b/tools/perf/arch/arm/tests/Build
@@ -1,4 +1,5 @@
 libperf-y += regs_load.o
 libperf-y += dwarf-unwind.o
+libperf-y += vectors-page.o
 
 libperf-y += arch-tests.o
diff --git a/tools/perf/arch/arm/tests/arch-tests.c b/tools/perf/arch/arm/tests/arch-tests.c
index 5b1543c98022..6848101a855f 100644
--- a/tools/perf/arch/arm/tests/arch-tests.c
+++ b/tools/perf/arch/arm/tests/arch-tests.c
@@ -10,6 +10,10 @@ struct test arch_tests[] = {
 		.func = test__dwarf_unwind,
 	},
 #endif
+	{
+		.desc = "Vectors page",
+		.func = test__vectors_page,
+	},
 	{
 		.func = NULL,
 	},
diff --git a/tools/perf/arch/arm/tests/vectors-page.c b/tools/perf/arch/arm/tests/vectors-page.c
new file mode 100644
index 000000000000..7ffdd79971c8
--- /dev/null
+++ b/tools/perf/arch/arm/tests/vectors-page.c
@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <stdio.h>
+#include <string.h>
+#include <linux/compiler.h>
+
+#include "debug.h"
+#include "tests/tests.h"
+#include "util/find-map.c"
+
+#define VECTORS__MAP_NAME "[vectors]"
+
+int test__vectors_page(struct test *test __maybe_unused,
+		       int subtest __maybe_unused)
+{
+	void *start, *end;
+
+	if (find_map(&start, &end, VECTORS__MAP_NAME)) {
+		pr_err("%s not found, is CONFIG_KUSER_HELPERS enabled?\n",
+		       VECTORS__MAP_NAME);
+		return TEST_FAIL;
+	}
+
+	return TEST_OK;
+}
diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
index b82f55fcc294..399f18ca71a3 100644
--- a/tools/perf/tests/tests.h
+++ b/tools/perf/tests/tests.h
@@ -119,4 +119,9 @@ int test__arch_unwind_sample(struct perf_sample *sample,
 			     struct thread *thread);
 #endif
 #endif
+
+#if defined(__arm__)
+int test__vectors_page(struct test *test, int subtest);
+#endif
+
 #endif /* TESTS_H */
-- 
2.17.1

Re: [PATCH v2 0/2] perf tests: Check for ARM [vectors] page

[Florian Fainelli]

On 10/25/18 10:55 AM, Florian Fainelli wrote:
> Hi all,
> 
> I just painfully learned that perf would segfault when
> CONFIG_KUSER_HELPERS is disabled because it unconditionally makes use of
> it. This patch series adds an ARM test for that by leveraging the
> existing find_vdso_map() function and making it more generic and capable
> of location any map within /proc/self/maps.

Did not get much feedback, you are all probably busy attending LPC
conferences, but I was wondering if this did make sense or if there is a
better approach that should be looked at?

I am starting to see additional tests failing that require some ARM
(32-bit) specific changes, and not accumulating too many of these fixes
on top of that series would be neat.

Thank you!

> 
> Changes in v2:
> 
> - use strlen() instead of sizeof() -1 since we made the page name a
>   parameter
> - use TEST_OK/TEST_FAIL in lieu of 0/-1
> - added an error message indicating CONFIG_KUSER_HELPERS might be
>   disabled
> 
> Florian Fainelli (2):
>   perf tools: Make find_vdso_map() more modular
>   perf tests: Add a test for the ARM 32-bit [vectors] page
> 
>  tools/perf/arch/arm/tests/Build          |  1 +
>  tools/perf/arch/arm/tests/arch-tests.c   |  4 +++
>  tools/perf/arch/arm/tests/vectors-page.c | 24 ++++++++++++++++++
>  tools/perf/tests/tests.h                 |  5 ++++
>  tools/perf/util/find-map.c               | 31 ++++++++++++++++++++++++
>  tools/perf/util/find-vdso-map.c          | 30 +++--------------------
>  6 files changed, 68 insertions(+), 27 deletions(-)
>  create mode 100644 tools/perf/arch/arm/tests/vectors-page.c
>  create mode 100644 tools/perf/util/find-map.c
> 


-- 
Florian

Re: [PATCH v2 0/2] perf tests: Check for ARM [vectors] page

[Florian Fainelli]

On 11/13/18 2:44 PM, Florian Fainelli wrote:
> On 10/25/18 10:55 AM, Florian Fainelli wrote:
>> Hi all,
>>
>> I just painfully learned that perf would segfault when
>> CONFIG_KUSER_HELPERS is disabled because it unconditionally makes use of
>> it. This patch series adds an ARM test for that by leveraging the
>> existing find_vdso_map() function and making it more generic and capable
>> of location any map within /proc/self/maps.
> 
> Did not get much feedback, you are all probably busy attending LPC
> conferences, but I was wondering if this did make sense or if there is a
> better approach that should be looked at?
> 
> I am starting to see additional tests failing that require some ARM
> (32-bit) specific changes, and not accumulating too many of these fixes
> on top of that series would be neat.
> 
> Thank you!

Ping? Would you prefer a resend to avoid any possible conflicts? Thanks

> 
>>
>> Changes in v2:
>>
>> - use strlen() instead of sizeof() -1 since we made the page name a
>>   parameter
>> - use TEST_OK/TEST_FAIL in lieu of 0/-1
>> - added an error message indicating CONFIG_KUSER_HELPERS might be
>>   disabled
>>
>> Florian Fainelli (2):
>>   perf tools: Make find_vdso_map() more modular
>>   perf tests: Add a test for the ARM 32-bit [vectors] page
>>
>>  tools/perf/arch/arm/tests/Build          |  1 +
>>  tools/perf/arch/arm/tests/arch-tests.c   |  4 +++
>>  tools/perf/arch/arm/tests/vectors-page.c | 24 ++++++++++++++++++
>>  tools/perf/tests/tests.h                 |  5 ++++
>>  tools/perf/util/find-map.c               | 31 ++++++++++++++++++++++++
>>  tools/perf/util/find-vdso-map.c          | 30 +++--------------------
>>  6 files changed, 68 insertions(+), 27 deletions(-)
>>  create mode 100644 tools/perf/arch/arm/tests/vectors-page.c
>>  create mode 100644 tools/perf/util/find-map.c
>>
> 
> 


-- 
Florian

Re: [PATCH v2 1/2] perf tools: Make find_vdso_map() more modular

[Jiri Olsa]

On Thu, Oct 25, 2018 at 10:55:07AM -0700, Florian Fainelli wrote:
> In preparation for checking that the vectors page on the ARM
> architecture, refactor the find_vdso_map() function to accept finding an
> arbitrary string and create a dedicated helper function for that under
> util/find-map.c and update find_vdso_map() to use it.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  tools/perf/util/find-map.c      | 31 +++++++++++++++++++++++++++++++
>  tools/perf/util/find-vdso-map.c | 30 +++---------------------------
>  2 files changed, 34 insertions(+), 27 deletions(-)
>  create mode 100644 tools/perf/util/find-map.c
> 
> diff --git a/tools/perf/util/find-map.c b/tools/perf/util/find-map.c
> new file mode 100644
> index 000000000000..19a3431a7b2a
> --- /dev/null
> +++ b/tools/perf/util/find-map.c
> @@ -0,0 +1,31 @@
> +// SPDX-License-Identifier: GPL-2.0
> +static int find_map(void **start, void **end, const char *name)
> +{
> +	FILE *maps;
> +	char line[128];
> +	int found = 0;
> +
> +	maps = fopen("/proc/self/maps", "r");
> +	if (!maps) {
> +		fprintf(stderr, "vdso: cannot open maps\n");
> +		return -1;
> +	}
> +
> +	while (!found && fgets(line, sizeof(line), maps)) {
> +		int m = -1;
> +
> +		/* We care only about private r-x mappings. */
> +		if (2 != sscanf(line, "%p-%p r-xp %*x %*x:%*x %*u %n",
> +				start, end, &m))
> +			continue;
> +		if (m < 0)
> +			continue;
> +
> +		if (!strncmp(&line[m], name, strlen(name)))
> +			found = 1;
> +	}
> +
> +	fclose(maps);
> +	return !found;
> +}

please keep just one object for both.. looks to me like
it coud go to util.c.. or just find-map.c if there's
a reason to have this separated

thanks,
jirka

Re: [PATCH v2 0/2] perf tests: Check for ARM [vectors] page

[Jiri Olsa]

On Thu, Oct 25, 2018 at 10:55:06AM -0700, Florian Fainelli wrote:
> Hi all,
> 
> I just painfully learned that perf would segfault when
> CONFIG_KUSER_HELPERS is disabled because it unconditionally makes use of
> it. This patch series adds an ARM test for that by leveraging the
> existing find_vdso_map() function and making it more generic and capable
> of location any map within /proc/self/maps.
> 
> Changes in v2:
> 
> - use strlen() instead of sizeof() -1 since we made the page name a
>   parameter
> - use TEST_OK/TEST_FAIL in lieu of 0/-1
> - added an error message indicating CONFIG_KUSER_HELPERS might be
>   disabled

other than the function placing I mentioned in
my previous email, this seems ok to me

jirka

> 
> Florian Fainelli (2):
>   perf tools: Make find_vdso_map() more modular
>   perf tests: Add a test for the ARM 32-bit [vectors] page
> 
>  tools/perf/arch/arm/tests/Build          |  1 +
>  tools/perf/arch/arm/tests/arch-tests.c   |  4 +++
>  tools/perf/arch/arm/tests/vectors-page.c | 24 ++++++++++++++++++
>  tools/perf/tests/tests.h                 |  5 ++++
>  tools/perf/util/find-map.c               | 31 ++++++++++++++++++++++++
>  tools/perf/util/find-vdso-map.c          | 30 +++--------------------
>  6 files changed, 68 insertions(+), 27 deletions(-)
>  create mode 100644 tools/perf/arch/arm/tests/vectors-page.c
>  create mode 100644 tools/perf/util/find-map.c
> 
> -- 
> 2.17.1
> 

Re: [PATCH v2 1/2] perf tools: Make find_vdso_map() more modular

[Jiri Olsa]

On Thu, Oct 25, 2018 at 10:55:07AM -0700, Florian Fainelli wrote:
> In preparation for checking that the vectors page on the ARM
> architecture, refactor the find_vdso_map() function to accept finding an
> arbitrary string and create a dedicated helper function for that under
> util/find-map.c and update find_vdso_map() to use it.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  tools/perf/util/find-map.c      | 31 +++++++++++++++++++++++++++++++
>  tools/perf/util/find-vdso-map.c | 30 +++---------------------------
>  2 files changed, 34 insertions(+), 27 deletions(-)
>  create mode 100644 tools/perf/util/find-map.c
> 
> diff --git a/tools/perf/util/find-map.c b/tools/perf/util/find-map.c
> new file mode 100644
> index 000000000000..19a3431a7b2a
> --- /dev/null
> +++ b/tools/perf/util/find-map.c
> @@ -0,0 +1,31 @@
> +// SPDX-License-Identifier: GPL-2.0
> +static int find_map(void **start, void **end, const char *name)
> +{
> +	FILE *maps;
> +	char line[128];
> +	int found = 0;
> +
> +	maps = fopen("/proc/self/maps", "r");
> +	if (!maps) {
> +		fprintf(stderr, "vdso: cannot open maps\n");
> +		return -1;
> +	}
> +
> +	while (!found && fgets(line, sizeof(line), maps)) {
> +		int m = -1;
> +
> +		/* We care only about private r-x mappings. */
> +		if (2 != sscanf(line, "%p-%p r-xp %*x %*x:%*x %*u %n",
> +				start, end, &m))
> +			continue;
> +		if (m < 0)
> +			continue;
> +
> +		if (!strncmp(&line[m], name, strlen(name)))
> +			found = 1;
> +	}
> +
> +	fclose(maps);
> +	return !found;
> +}
> +
> diff --git a/tools/perf/util/find-vdso-map.c b/tools/perf/util/find-vdso-map.c
> index d7823e3508fc..840d7d6e29e2 100644
> --- a/tools/perf/util/find-vdso-map.c
> +++ b/tools/perf/util/find-vdso-map.c
> @@ -1,31 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
> +#include "find-map.c"


aaah, I was wondering how this could build without 'Build' file record ;-)

jirka


> +
>  static int find_vdso_map(void **start, void **end)
>  {
> -	FILE *maps;
> -	char line[128];
> -	int found = 0;
> -
> -	maps = fopen("/proc/self/maps", "r");
> -	if (!maps) {
> -		fprintf(stderr, "vdso: cannot open maps\n");
> -		return -1;
> -	}
> -
> -	while (!found && fgets(line, sizeof(line), maps)) {
> -		int m = -1;
> -
> -		/* We care only about private r-x mappings. */
> -		if (2 != sscanf(line, "%p-%p r-xp %*x %*x:%*x %*u %n",
> -				start, end, &m))
> -			continue;
> -		if (m < 0)
> -			continue;
> -
> -		if (!strncmp(&line[m], VDSO__MAP_NAME,
> -			     sizeof(VDSO__MAP_NAME) - 1))
> -			found = 1;
> -	}
> -
> -	fclose(maps);
> -	return !found;
> +	return find_map(start, end, VDSO__MAP_NAME);
>  }
> -- 
> 2.17.1
> 

Re: [PATCH v2 1/2] perf tools: Make find_vdso_map() more modular

[Florian Fainelli]

On 11/27/18 1:31 AM, Jiri Olsa wrote:
> On Thu, Oct 25, 2018 at 10:55:07AM -0700, Florian Fainelli wrote:
>> In preparation for checking that the vectors page on the ARM
>> architecture, refactor the find_vdso_map() function to accept finding an
>> arbitrary string and create a dedicated helper function for that under
>> util/find-map.c and update find_vdso_map() to use it.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>  tools/perf/util/find-map.c      | 31 +++++++++++++++++++++++++++++++
>>  tools/perf/util/find-vdso-map.c | 30 +++---------------------------
>>  2 files changed, 34 insertions(+), 27 deletions(-)
>>  create mode 100644 tools/perf/util/find-map.c
>>
>> diff --git a/tools/perf/util/find-map.c b/tools/perf/util/find-map.c
>> new file mode 100644
>> index 000000000000..19a3431a7b2a
>> --- /dev/null
>> +++ b/tools/perf/util/find-map.c
>> @@ -0,0 +1,31 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +static int find_map(void **start, void **end, const char *name)
>> +{
>> +	FILE *maps;
>> +	char line[128];
>> +	int found = 0;
>> +
>> +	maps = fopen("/proc/self/maps", "r");
>> +	if (!maps) {
>> +		fprintf(stderr, "vdso: cannot open maps\n");
>> +		return -1;
>> +	}
>> +
>> +	while (!found && fgets(line, sizeof(line), maps)) {
>> +		int m = -1;
>> +
>> +		/* We care only about private r-x mappings. */
>> +		if (2 != sscanf(line, "%p-%p r-xp %*x %*x:%*x %*u %n",
>> +				start, end, &m))
>> +			continue;
>> +		if (m < 0)
>> +			continue;
>> +
>> +		if (!strncmp(&line[m], name, strlen(name)))
>> +			found = 1;
>> +	}
>> +
>> +	fclose(maps);
>> +	return !found;
>> +}
> 
> please keep just one object for both.. looks to me like
> it coud go to util.c.. or just find-map.c if there's
> a reason to have this separated

The reason for keeping both implementations separate is because
VDSO__MAP_NAME may not be defined/applicable for all architectures, so
instead of having them have to define VDSO__MAP_NAME just to get that
file to build, they would only include find-map.c to get access to
find_map() and then search for specific tokens.

Would you prefer if find_map() was moved to tools/perf/util/util.c?
-- 
Florian