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

The size of the CPU affinity mask 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 affinity mask 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 <redacted>
Signed-off-by: Sandipan Das <redacted>
Reviewed-by: Kamalesh Babulal <redacted>
---
Previous versions can be found at:
v1: https://lore.kernel.org/linuxppc-dev/20200608144212.985144-1-sandipan@linux.ibm.com/

Changes in v2:
- Added NULL check for the affinity mask as suggested by Kamalesh.
- Changed "cpu set" to "CPU affinity mask" in the commit message.

---
 tools/testing/selftests/powerpc/utils.c | 37 +++++++++++++++++--------
 1 file changed, 25 insertions(+), 12 deletions(-)

diff --git a/tools/testing/selftests/powerpc/utils.c b/tools/testing/selftests/powerpc/utils.c
index 933678f1ed0a..798fa8fdd5f4 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,40 @@ 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;
+
+	ncpus = get_nprocs();
+	size = CPU_ALLOC_SIZE(ncpus);
+	mask = CPU_ALLOC(ncpus);
+	if (!mask) {
+		perror("malloc");
+		return -1;
+	}
 
-	CPU_ZERO(&mask);
+	CPU_ZERO_S(size, mask);
 
-	if (sched_getaffinity(0, sizeof(mask), &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

Sandipan Das [off-list ref] writes:

The size of the CPU affinity mask 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 affinity mask 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 <redacted>
Signed-off-by: Sandipan Das <redacted>
Reviewed-by: Kamalesh Babulal <redacted>
---
Previous versions can be found at:
v1: https://lore.kernel.org/linuxppc-dev/20200608144212.985144-1-sandipan@linux.ibm.com/

Changes in v2:
- Added NULL check for the affinity mask as suggested by Kamalesh.
- Changed "cpu set" to "CPU affinity mask" in the commit message.

This sometimes breaks, eg:

  # ./count_instructions 
  test: count_instructions
  tags: git_version:v5.8-rc2-327-g9a1d992a7eb7
  sched_getaffinity: Invalid argument
  [FAIL] Test FAILED on line 123
  failure: count_instructions


This system has a messed up SMT setup, but the old code was able to cope
with it:

  # ppc64_cpu --info
  Core   0:    0*    1*    2     3     4     5     6     7  
  Core   1:    8     9    10*   11*   12    13    14    15  
  Core   2:   16    17    18    19    20    21    22    23  
  Core   3:   24    25    26    27    28    29    30    31  
  Core   4:   32    33    34    35    36    37    38    39  
  Core   5:   40    41    42    43    44    45    46    47  
  Core   6:   48    49    50    51    52    53    54    55  
  Core   7:   56    57    58    59    60    61    62    63  
  Core   8:   64    65    66    67    68    69    70    71  
  Core   9:   72    73    74    75    76    77    78    79  
  Core  10:   80    81    82    83    84    85    86    87  
  Core  11:   88    89    90    91    92    93    94    95  
  Core  12:   96    97    98    99   100*  101*  102*  103* 
  Core  13:  104*  105*  106*  107*  108*  109*  110*  111* 
  Core  14:  112*  113*  114*  115*  116*  117*  118*  119* 
  Core  15:  120   121   122   123   124   125   126   127  
  Core  16:  128   129   130   131   132   133   134   135  
  Core  17:  136   137   138   139   140   141   142   143  
  Core  18:  144   145   146   147   148   149   150   151  
  Core  19:  152   153   154   155   156   157   158   159 

cheers

* Sandipan Das [off-list ref] [2020-06-09 13:07:33]:

Please use get_nprocs_conf or sysconf(_SC_NPROCESSORS_CONF). The manpage
seems to suggest the latter. Not sure how accurate the manpage is.

get_nprocs is returning online cpus and when smt is off, the cpu numbers
would be sparse and hence the result from get_nprocs wouldn't be ideal for
allocating cpumask. However get_nprocs_conf would return the max configured
cpus and would be able to handle it. 

I think this was the same situation hit by Michael Ellerman.

+	size = CPU_ALLOC_SIZE(ncpus);
+	mask = CPU_ALLOC(ncpus);
+	if (!mask) {
+		perror("malloc");
+		return -1;
+	}

-- 
Thanks and Regards
Srikar Dronamraju

Hi Srikar, Michael,

On 29/07/20 9:33 pm, Srikar Dronamraju wrote:

* Sandipan Das [off-list ref] [2020-06-09 13:07:33]:

quoted

The size of the CPU affinity mask 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 affinity mask 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 <redacted>
Signed-off-by: Sandipan Das <redacted>
Reviewed-by: Kamalesh Babulal <redacted>
---
Previous versions can be found at:
v1: https://lore.kernel.org/linuxppc-dev/20200608144212.985144-1-sandipan@linux.ibm.com/

@@ -88,28 +89,40 @@ 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;
+
+	ncpus = get_nprocs();

Please use get_nprocs_conf or sysconf(_SC_NPROCESSORS_CONF). The manpage
seems to suggest the latter. Not sure how accurate the manpage is.

get_nprocs is returning online cpus and when smt is off, the cpu numbers
would be sparse and hence the result from get_nprocs wouldn't be ideal for
allocating cpumask. However get_nprocs_conf would return the max configured
cpus and would be able to handle it. 

I think this was the same situation hit by Michael Ellerman.

Yes, that seems to be the case. Thanks for testing this.
Will fix this in v3.

- Sandipan