Message ID | 20200609073733.997643-1-sandipan@linux.ibm.com (mailing list archive) |
---|---|
State | Changes Requested |
Headers | show |
Series | [v2] selftests: powerpc: Fix online CPU selection | expand |
Context | Check | Description |
---|---|---|
snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (ec7b8eb9bc7a519047485c95f7292b48f5b73fe6) |
snowpatch_ozlabs/build-ppc64le | success | Build succeeded |
snowpatch_ozlabs/build-ppc64be | success | Build succeeded |
snowpatch_ozlabs/build-ppc64e | success | Build succeeded |
snowpatch_ozlabs/build-pmac32 | success | Build succeeded |
snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 59 lines checked |
snowpatch_ozlabs/needsstable | warning | Please consider tagging this patch for stable! |
Sandipan Das <sandipan@linux.ibm.com> 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 <shiganta@in.ibm.com> > Signed-off-by: Sandipan Das <sandipan@linux.ibm.com> > Reviewed-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com> > --- > 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 <sandipan@linux.ibm.com> [2020-06-09 13:07:33]: > 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 <shiganta@in.ibm.com> > Signed-off-by: Sandipan Das <sandipan@linux.ibm.com> > Reviewed-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com> > --- > 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. > + size = CPU_ALLOC_SIZE(ncpus); > + mask = CPU_ALLOC(ncpus); > + if (!mask) { > + perror("malloc"); > + return -1; > + } >
Hi Srikar, Michael, On 29/07/20 9:33 pm, Srikar Dronamraju wrote: > * Sandipan Das <sandipan@linux.ibm.com> [2020-06-09 13:07:33]: > >> 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 <shiganta@in.ibm.com> >> Signed-off-by: Sandipan Das <sandipan@linux.ibm.com> >> Reviewed-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com> >> --- >> 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
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)