Hi Tobias! This became commit 8af1592882509fbead16e30fd2d056330a9609e4 "OpenMP: Add omp_get_device_distances routine [PR125877]".
On 2026-06-19T15:35:06+0200, Tobias Burnus <[email protected]> wrote: > --- a/libgomp/config/linux/affinity.c > +++ b/libgomp/config/linux/affinity.c (Per commit 8ce473f8a26142a39487a0adac10570761f8ed90 "libgomp: Move internal NUMA function to a separate file [PR125940]", the following code moved to 'libgomp/config/linux/numa.c'.) These global variables: > +static int num_numa_nodes = 0; > +static int *numa_distances = NULL; > +int > +gomp_get_numa_distance (int node1, int node2) > +{ > + if (node1 < 0 || node2 < 0 || num_numa_nodes < 0) > + return -1; > + > + if (numa_distances == NULL) > + { > + num_numa_nodes = -1; ... are accessed in here in a non-thread-safe way, which means that bad things are bound to happen in case 'gomp_get_numa_distance' gets called (via 'libgomp/target.c:omp_get_device_distances') from multiple threads concurrently? (Also, why are those global variables, if only accessed in here?) > + DIR *dir = opendir ("/sys/devices/system/node"); > + if (!dir) > + return -1; > + struct dirent *dp; > + int cnt = 0; > + errno = 0; > + while ((dp = readdir(dir)) != NULL) > + if (strncmp ("node", dp->d_name, 4 /* strlen ("node") */) == 0) > + cnt++; > + else if (errno) > + { > + closedir (dir); > + return -1; > + } > + closedir (dir); > + numa_distances = (int *) gomp_malloc (sizeof (int) * cnt * cnt); > + > + constexpr int len = sizeof ("/sys/devices/system/node/node12345/" > + "distance"); > + char filename[len]; > + > + for (int i = 0; i < cnt; i++) > + { > + if (len < snprintf (filename, sizeof (filename), > + "/sys/devices/system/node/node%d/distance", i)) This code assumes that 'node[N]' in '/sys/devices/system/node/' appear for consecutive '[N]'s, which evidently isn't always the case, for example: nvidia5: /sys/devices/system/node/node0 /sys/devices/system/node/node8 nvidia8: /sys/devices/system/node/node0 /sys/devices/system/node/node8 /sys/devices/system/node/node252 /sys/devices/system/node/node253 /sys/devices/system/node/node254 /sys/devices/system/node/node255 I note that instead of 'readdir'ing '/sys/devices/system/node' and 'cnt++;'ing all 'node*' files, 'libgomp/config/linux/affinity.c:gomp_affinity_init_numa_domains' instead appears to parse '/sys/devices/system/node/online', for example: nvidia5: $ cat < /sys/devices/system/node/online 0,8 nvidia8: $ cat < /sys/devices/system/node/online 0,8,252-255 > + return -1; This error case doesn't 'free (numa_distances);'. > + int distance = -1; > + FILE *in = fopen (filename, "r"); If we fail to 'fopen' here (for example, attempting 'node1' for what should've been 'node8'), we get the null pointer in 'in', and then... > + for (int j = 0; j < cnt; j++) > + { > + fscanf (in, "%d", &distance); ... fault here. (I've patched just this one, and otherwise going to re-open PR125877 "[OpenMP] Implement omp_get_device_distances".) > + if (distance == -1) Shouldn't we always set 'distance = -1;' right before the 'fscanf' above, so that we can detect failure to 'fscanf' also in the second and following loop iterations? > + { > + fclose (in); > + free (numa_distances); > + return -1; > + } > + numa_distances[i * cnt + j] = distance; > + } > + fclose (in); > + } > + num_numa_nodes = cnt; > + } > + return numa_distances[node1 * num_numa_nodes + node2]; > +} Grüße Thomas
