[PATCH] selftests: powerpc: Fix online CPU selection

[PATCH] selftests: powerpc: Fix online CPU selection

[Sandipan Das]

The size of the cpu set must be large enough for systems
with a very large number of CPUs. Otherwise, tests which
try to determine the first online CPU by calling
sched_getaffinity() will fail. This makes sure that the
size of the allocated cpu set is dependent on the number
of CPUs as reported by get_nprocs().

Fixes: 3752e453f6ba ("selftests/powerpc: Add tests of PMU EBBs")
Reported-by: Shirisha Ganta <shiganta@in.ibm.com>
Signed-off-by: Sandipan Das <sandipan@linux.ibm.com>
---
 tools/testing/selftests/powerpc/utils.c | 33 ++++++++++++++++---------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/tools/testing/selftests/powerpc/utils.c b/tools/testing/selftests/powerpc/utils.c
index 933678f1ed0a..bb8e402752c0 100644
--- a/tools/testing/selftests/powerpc/utils.c
+++ b/tools/testing/selftests/powerpc/utils.c
@@ -16,6 +16,7 @@
 #include <string.h>
 #include <sys/ioctl.h>
 #include <sys/stat.h>
+#include <sys/sysinfo.h>
 #include <sys/types.h>
 #include <sys/utsname.h>
 #include <unistd.h>
@@ -88,28 +89,36 @@ void *get_auxv_entry(int type)
 
 int pick_online_cpu(void)
 {
-	cpu_set_t mask;
-	int cpu;
+	int ncpus, cpu = -1;
+	cpu_set_t *mask;
+	size_t size;
 
-	CPU_ZERO(&mask);
+	ncpus = get_nprocs();
+	size = CPU_ALLOC_SIZE(ncpus);
+	mask = CPU_ALLOC(ncpus);
 
-	if (sched_getaffinity(0, sizeof(mask), &mask)) {
+	CPU_ZERO_S(size, mask);
+
+	if (sched_getaffinity(0, size, mask)) {
 		perror("sched_getaffinity");
-		return -1;
+		goto done;
 	}
 
 	/* We prefer a primary thread, but skip 0 */
-	for (cpu = 8; cpu < CPU_SETSIZE; cpu += 8)
-		if (CPU_ISSET(cpu, &mask))
-			return cpu;
+	for (cpu = 8; cpu < ncpus; cpu += 8)
+		if (CPU_ISSET_S(cpu, size, mask))
+			goto done;
 
 	/* Search for anything, but in reverse */
-	for (cpu = CPU_SETSIZE - 1; cpu >= 0; cpu--)
-		if (CPU_ISSET(cpu, &mask))
-			return cpu;
+	for (cpu = ncpus - 1; cpu >= 0; cpu--)
+		if (CPU_ISSET_S(cpu, size, mask))
+			goto done;
 
 	printf("No cpus in affinity mask?!\n");
-	return -1;
+
+done:
+	CPU_FREE(mask);
+	return cpu;
 }
 
 bool is_ppc64le(void)
-- 
2.25.1

Re: [PATCH] selftests: powerpc: Fix online CPU selection

[Kamalesh Babulal]

On 6/8/20 8:12 PM, Sandipan Das wrote:
> The size of the cpu set must be large enough for systems
> with a very large number of CPUs. Otherwise, tests which
> try to determine the first online CPU by calling
> sched_getaffinity() will fail. This makes sure that the
> size of the allocated cpu set is dependent on the number
> of CPUs as reported by get_nprocs().
> 
> Fixes: 3752e453f6ba ("selftests/powerpc: Add tests of PMU EBBs")
> Reported-by: Shirisha Ganta <shiganta@in.ibm.com>
> Signed-off-by: Sandipan Das <sandipan@linux.ibm.com>

LGTM,

Reviewed-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>

-- 
Kamalesh

Re: [PATCH] selftests: powerpc: Fix online CPU selection

[Sandipan Das]

On 08/06/20 8:12 pm, Sandipan Das wrote:
> The size of the cpu set must be large enough for systems
> with a very large number of CPUs. Otherwise, tests which
> try to determine the first online CPU by calling
> sched_getaffinity() will fail. This makes sure that the
> size of the allocated cpu set is dependent on the number
> of CPUs as reported by get_nprocs().
> 
> Fixes: 3752e453f6ba ("selftests/powerpc: Add tests of PMU EBBs")
> Reported-by: Shirisha Ganta <shiganta@in.ibm.com>
> Signed-off-by: Sandipan Das <sandipan@linux.ibm.com>
> ---
>  tools/testing/selftests/powerpc/utils.c | 33 ++++++++++++++++---------
>  1 file changed, 21 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/testing/selftests/powerpc/utils.c b/tools/testing/selftests/powerpc/utils.c
> index 933678f1ed0a..bb8e402752c0 100644
> --- a/tools/testing/selftests/powerpc/utils.c
> +++ b/tools/testing/selftests/powerpc/utils.c
> @@ -16,6 +16,7 @@
> @@ -88,28 +89,36 @@ void *get_auxv_entry(int type)
> [...] 
>  int pick_online_cpu(void)
>  {
> -	cpu_set_t mask;
> -	int cpu;
> +	int ncpus, cpu = -1;
> +	cpu_set_t *mask;
> +	size_t size;
>  
> -	CPU_ZERO(&mask);
> +	ncpus = get_nprocs();
> +	size = CPU_ALLOC_SIZE(ncpus);
> +	mask = CPU_ALLOC(ncpus);
>  
> -	if (sched_getaffinity(0, sizeof(mask), &mask)) {
> +	CPU_ZERO_S(size, mask);
> +
> +	if (sched_getaffinity(0, size, mask)) {
>  		perror("sched_getaffinity");
> -		return -1;
> +		goto done;
>  	}
>  
>  	/* We prefer a primary thread, but skip 0 */
> -	for (cpu = 8; cpu < CPU_SETSIZE; cpu += 8)
> -		if (CPU_ISSET(cpu, &mask))
> -			return cpu;
> +	for (cpu = 8; cpu < ncpus; cpu += 8)
> +		if (CPU_ISSET_S(cpu, size, mask))
> +			goto done;
>  
>  	/* Search for anything, but in reverse */
> -	for (cpu = CPU_SETSIZE - 1; cpu >= 0; cpu--)
> -		if (CPU_ISSET(cpu, &mask))
> -			return cpu;
> +	for (cpu = ncpus - 1; cpu >= 0; cpu--)
> +		if (CPU_ISSET_S(cpu, size, mask))
> +			goto done;
>  
>  	printf("No cpus in affinity mask?!\n");
> -	return -1;

There's a bug here as cpu should have been set to -1.
Will send v2 with this fix.

> +
> +done:
> +	CPU_FREE(mask);
> +	return cpu;
>  }
>  
>  bool is_ppc64le(void)
> 

- Sandipan

Re: [PATCH] selftests: powerpc: Fix online CPU selection

[Sandipan Das]

On 08/06/20 8:12 pm, Sandipan Das wrote:
> The size of the cpu set must be large enough for systems
> with a very large number of CPUs. Otherwise, tests which
> try to determine the first online CPU by calling
> sched_getaffinity() will fail. This makes sure that the
> size of the allocated cpu set is dependent on the number
> of CPUs as reported by get_nprocs().
> 
> Fixes: 3752e453f6ba ("selftests/powerpc: Add tests of PMU EBBs")
> Reported-by: Shirisha Ganta <shiganta@in.ibm.com>
> Signed-off-by: Sandipan Das <sandipan@linux.ibm.com>
> ---
>  tools/testing/selftests/powerpc/utils.c | 33 ++++++++++++++++---------
>  1 file changed, 21 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/testing/selftests/powerpc/utils.c b/tools/testing/selftests/powerpc/utils.c
> index 933678f1ed0a..bb8e402752c0 100644
> --- a/tools/testing/selftests/powerpc/utils.c
> +++ b/tools/testing/selftests/powerpc/utils.c
> @@ -16,6 +16,7 @@
> [...]
>  
>  int pick_online_cpu(void)
>  {
> -	cpu_set_t mask;
> -	int cpu;
> +	int ncpus, cpu = -1;
> +	cpu_set_t *mask;
> +	size_t size;
>  
> -	CPU_ZERO(&mask);
> +	ncpus = get_nprocs();
> +	size = CPU_ALLOC_SIZE(ncpus);
> +	mask = CPU_ALLOC(ncpus);
>  
> -	if (sched_getaffinity(0, sizeof(mask), &mask)) {
> +	CPU_ZERO_S(size, mask);
> +
> +	if (sched_getaffinity(0, size, mask)) {
>  		perror("sched_getaffinity");
> -		return -1;
> +		goto done;
>  	}
>  
>  	/* We prefer a primary thread, but skip 0 */
> -	for (cpu = 8; cpu < CPU_SETSIZE; cpu += 8)
> -		if (CPU_ISSET(cpu, &mask))
> -			return cpu;
> +	for (cpu = 8; cpu < ncpus; cpu += 8)
> +		if (CPU_ISSET_S(cpu, size, mask))
> +			goto done;
>  
>  	/* Search for anything, but in reverse */
> -	for (cpu = CPU_SETSIZE - 1; cpu >= 0; cpu--)
> -		if (CPU_ISSET(cpu, &mask))
> -			return cpu;
> +	for (cpu = ncpus - 1; cpu >= 0; cpu--)
> +		if (CPU_ISSET_S(cpu, size, mask))
> +			goto done;
>  
>  	printf("No cpus in affinity mask?!\n");
> -	return -1;

Missed the fact that the for loop before this would anyway make 'cpu'
count down to -1 if no online CPU is found. Please ignore the previous
message.

> +
> +done:
> +	CPU_FREE(mask);
> +	return cpu;
>  }
>  
>  bool is_ppc64le(void)
> 

- Sandipan

[PATCH] selftests: powerpc: Fix online CPU selection

[Harish]

On systems with large number of cpus, test fails trying to set
affinity by calling sched_setaffinity() with smaller size for
cpuset. This patch fixes it by making sure that the size of
allocated cpu set is dependent on the number of CPUs as reported
by get_nprocs().

Reported-by: Shirisha Ganta <shiganta@in.ibm.com>
Signed-off-by: Harish <harish@linux.ibm.com>
Signed-off-by: Sandipan Das <sandipan@linux.ibm.com>
---
 .../powerpc/benchmarks/context_switch.c        | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/powerpc/benchmarks/context_switch.c b/tools/testing/selftests/powerpc/benchmarks/context_switch.c
index a2e8c9da7fa5..de6c49d6f88f 100644
--- a/tools/testing/selftests/powerpc/benchmarks/context_switch.c
+++ b/tools/testing/selftests/powerpc/benchmarks/context_switch.c
@@ -19,6 +19,7 @@
 #include <limits.h>
 #include <sys/time.h>
 #include <sys/syscall.h>
+#include <sys/sysinfo.h>
 #include <sys/types.h>
 #include <sys/shm.h>
 #include <linux/futex.h>
@@ -104,8 +105,9 @@ static void start_thread_on(void *(*fn)(void *), void *arg, unsigned long cpu)
 
 static void start_process_on(void *(*fn)(void *), void *arg, unsigned long cpu)
 {
-	int pid;
-	cpu_set_t cpuset;
+	int pid, ncpus;
+	cpu_set_t *cpuset;
+	size_t size;
 
 	pid = fork();
 	if (pid == -1) {
@@ -116,12 +118,16 @@ static void start_process_on(void *(*fn)(void *), void *arg, unsigned long cpu)
 	if (pid)
 		return;
 
-	CPU_ZERO(&cpuset);
-	CPU_SET(cpu, &cpuset);
+	size = CPU_ALLOC_SIZE(ncpus);
+	ncpus = get_nprocs();
+	cpuset = CPU_ALLOC(ncpus);
+	CPU_ZERO_S(size, cpuset);
+	CPU_SET_S(cpu, size, cpuset);
 
-	if (sched_setaffinity(0, sizeof(cpuset), &cpuset)) {
+	if (sched_setaffinity(0, size, cpuset)) {
 		perror("sched_setaffinity");
-		exit(1);
+		CPU_FREE(cpuset);
+		exit(-1);
 	}
 
 	fn(arg);
-- 
2.24.1