On 11/28 15:34:06, Balakrishna Garapati wrote:
> With this new proposal cpu affinity is read correctly especially
> when using cgroups otherwise wrong cpu mask is set.
>
> Fixes bug: https://bugs.linaro.org/show_bug.cgi?id=2472
>
> Signed-off-by: Balakrishna Garapati <balakrishna.garap...@linaro.org>
> ---
>
> v1 to v2: added Description of the issue to the patch commit log.
> v2 to v3: Resending the patch adding the log change from v1 to v2
>
> platform/linux-generic/odp_cpumask.c | 69
> +++++++++---------------------------
> 1 file changed, 16 insertions(+), 53 deletions(-)
>
> diff --git a/platform/linux-generic/odp_cpumask.c
> b/platform/linux-generic/odp_cpumask.c
> index 6bf2632..7b0d80a 100644
> --- a/platform/linux-generic/odp_cpumask.c
> +++ b/platform/linux-generic/odp_cpumask.c
> @@ -227,71 +227,34 @@ int odp_cpumask_next(const odp_cpumask_t *mask, int cpu)
> */
> static int get_installed_cpus(void)
Should this function be renamed to something like get_available_cpus
since it returns the set of CPUs on which the calling thread is eligible
to run on instead of the set of CPUs in the entire system?
> {
> - char *numptr;
> - char *endptr;
> - long int cpu_idnum;
> - DIR *d;
> - struct dirent *dir;
> + int cpu_idnum;
> + cpu_set_t cpuset;
> + int ret;
>
> /* Clear the global cpumasks for control and worker CPUs */
> odp_cpumask_zero(&odp_global_data.control_cpus);
> odp_cpumask_zero(&odp_global_data.worker_cpus);
>
> - /*
> - * Scan the /sysfs pseudo-filesystem for CPU info directories.
> - * There should be one subdirectory for each installed logical CPU
> - */
> - d = opendir("/sys/devices/system/cpu");
> - if (d) {
> - while ((dir = readdir(d)) != NULL) {
> - cpu_idnum = CPU_SETSIZE;
> -
> - /*
> - * If the current directory entry doesn't represent
> - * a CPU info subdirectory then skip to the next entry.
> - */
> - if (dir->d_type == DT_DIR) {
> - if (!strncmp(dir->d_name, "cpu", 3)) {
> - /*
> - * Directory name starts with "cpu"...
> - * Try to extract a CPU ID number
> - * from the remainder of the dirname.
> - */
> - errno = 0;
> - numptr = dir->d_name;
> - numptr += 3;
> - cpu_idnum = strtol(numptr, &endptr,
> - 10);
> - if (errno || (endptr == numptr))
> - continue;
> - } else {
> - continue;
> - }
> - } else {
> - continue;
> - }
> - /*
> - * If we get here the current directory entry specifies
> - * a CPU info subdir for the CPU indexed by cpu_idnum.
> - */
> + CPU_ZERO(&cpuset);
> + ret = sched_getaffinity(0, sizeof(cpuset), &cpuset);
It would be great to add a note in the ODP spec that the application thread
calling odp_global_init() must double check its cpuset. E.g. if called from
a control plane thread that has already affinitized to control plane CPUs,
once odp_global_init() is called will the underlying ODP implementation
(and ODP helper) only know about the cpuset of the control plane cores?
> - /* Track number of logical CPUs discovered */
> - if (odp_global_data.num_cpus_installed <
> - (int)(cpu_idnum + 1))
> - odp_global_data.num_cpus_installed =
> - (int)(cpu_idnum + 1);
> + if (ret < 0) {
> + ODP_ERR("Failed to get cpu affinity");
> + return -1;
> + }
>
> + for (cpu_idnum = 0; cpu_idnum < CPU_SETSIZE - 1; cpu_idnum++) {
> + if (CPU_ISSET(cpu_idnum, &cpuset)) {
> + odp_global_data.num_cpus_installed++;
> /* Add the CPU to our default cpumasks */
> odp_cpumask_set(&odp_global_data.control_cpus,
> - (int)cpu_idnum);
> + (int)cpu_idnum);
> odp_cpumask_set(&odp_global_data.worker_cpus,
> - (int)cpu_idnum);
> + (int)cpu_idnum);
> }
> - closedir(d);
> - return 0;
> - } else {
> - return -1;
> }
> +
> + return 0;
> }
>
> /*
> --
> 1.9.1
>
