| Message ID | 1480343646-5423-1-git-send-email-balakrishna.garapati@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
ping /Krishna On 28 November 2016 at 15:34, Balakrishna Garapati < balakrishna.garapati@linaro.org> 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.garapati@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) > { > - 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); > > - /* 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 > >
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.garapati@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 >
The original code reads whole system cpuset, and provides no command line parameters to specify cpuset resources while launching ODP application. Thus can be an obstacle for the coexisting of multiple ODP applications. New code implies that ODP application launcher should prepare resource arrangements (cpuset etc) before launching the ODP applications, favours multiple ODP applications' coexistence, this patch looks good for me. Agree with Brian's 2 comments: Should this function be renamed to something like get_available_cpus? In documentation, do we have a chapter covers "Run ODP Applications" topic? Can be a separate task. Thanks and Best Regards, Yi On 2 December 2016 at 03:38, Brian Brooks <brian.brooks@linaro.org> wrote: > 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.garapati@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 > > >
On 2 December 2016 at 04:20, Yi He <yi.he@linaro.org> wrote: > The original code reads whole system cpuset, and provides no command line > parameters to specify cpuset resources while launching ODP application. > Thus can be an obstacle for the coexisting of multiple ODP applications. > > New code implies that ODP application launcher should prepare resource > arrangements (cpuset etc) before launching the ODP applications, favours > multiple ODP applications' coexistence, this patch looks good for me. > > Agree with Brian's 2 comments: > > Should this function be renamed to something like get_available_cpus? > sure, will rename it. Send you an other version > > In documentation, do we have a chapter covers "Run ODP Applications" > topic? Can be a separate task. > I couldn't find any section like that. It can be a separate task. /Krishna > > Thanks and Best Regards, Yi > > > > On 2 December 2016 at 03:38, Brian Brooks <brian.brooks@linaro.org> wrote: > >> 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.garapati@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 >> > >> > >
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) { - 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); - /* 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; } /*
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.garapati@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(-) -- 1.9.1