[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Two problems with result of num_processors (NPROC_CURRENT_OVERRIDABL
From: |
Pádraig Brady |
Subject: |
Re: Two problems with result of num_processors (NPROC_CURRENT_OVERRIDABLE) |
Date: |
Sun, 26 Feb 2017 09:42:17 -0800 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 |
On 26/02/17 09:05, Bruno Haible wrote:
> Hi Pádraig,
>
> The function 'num_processors' is now a bit large, and in particular contains
> the multiplied complexity of
> - the number of systems which each require a different approach,
> - the query parameter which in one MUST consider omp_env_limit and in the
> other case MAY but NEED NOT consider omp_env_limit.
>
> To make this code more maintainable, I would propose to split it into
> - a function which contains only the complexity of the various systems,
> - a function which contains only the complexity of OMP handling.
>
> Like this:
>
>
> 2017-02-26 Bruno Haible <address@hidden>
>
> nproc: Refactor large function.
> * lib/nproc.c (num_processors_ignoring_omp): New function, extracted
> from num_processors.
> (num_processors): In this function, only deal with OMP.
>
> diff --git a/lib/nproc.c b/lib/nproc.c
> index db3ca9b..7880e62 100644
> --- a/lib/nproc.c
> +++ b/lib/nproc.c
> @@ -199,65 +199,12 @@ num_processors_via_affinity_mask (void)
> return 0;
> }
>
> -/* Parse OMP environment variables without dependence on OMP.
> - Return 0 for invalid values. */
> -static unsigned long int
> -parse_omp_threads (char const* threads)
> -{
> - unsigned long int ret = 0;
> -
> - if (threads == NULL)
> - return ret;
> -
> - /* The OpenMP spec says that the value assigned to the environment
> variables
> - "may have leading and trailing white space". */
> - while (*threads != '\0' && c_isspace (*threads))
> - threads++;
>
> - /* Convert it from positive decimal to 'unsigned long'. */
> - if (c_isdigit (*threads))
> - {
> - char *endptr = NULL;
> - unsigned long int value = strtoul (threads, &endptr, 10);
> -
> - if (endptr != NULL)
> - {
> - while (*endptr != '\0' && c_isspace (*endptr))
> - endptr++;
> - if (*endptr == '\0')
> - return value;
> - /* Also accept the first value in a nesting level,
> - since we can't determine the nesting level from env vars. */
> - else if (*endptr == ',')
> - return value;
> - }
> - }
> -
> - return ret;
> -}
> -
> -unsigned long int
> -num_processors (enum nproc_query query)
> +/* Return the total number of processors. Here QUERY must be one of
> + NPROC_ALL, NPROC_CURRENT. The result is guaranteed to be at least 1. */
> +static unsigned long int
> +num_processors_ignoring_omp (enum nproc_query query)
> {
> - unsigned long int omp_env_limit = ULONG_MAX;
> -
> - if (query == NPROC_CURRENT_OVERRIDABLE)
> - {
> - unsigned long int omp_env_threads;
> - /* Honor the OpenMP environment variables, recognized also by all
> - programs that are based on OpenMP. */
> - omp_env_threads = parse_omp_threads (getenv ("OMP_NUM_THREADS"));
> - omp_env_limit = parse_omp_threads (getenv ("OMP_THREAD_LIMIT"));
> - if (! omp_env_limit)
> - omp_env_limit = ULONG_MAX;
> -
> - if (omp_env_threads)
> - return MIN (omp_env_threads, omp_env_limit);
> -
> - query = NPROC_CURRENT;
> - }
> - /* Here query is one of NPROC_ALL, NPROC_CURRENT. */
> -
> /* On systems with a modern affinity mask system call, we have
> sysconf (_SC_NPROCESSORS_CONF)
> >= sysconf (_SC_NPROCESSORS_ONLN)
> @@ -279,7 +226,7 @@ num_processors (enum nproc_query query)
> unsigned long nprocs = num_processors_via_affinity_mask ();
>
> if (nprocs > 0)
> - return MIN (nprocs, omp_env_limit);
> + return nprocs;
> }
>
> #if defined _SC_NPROCESSORS_ONLN
> @@ -287,7 +234,7 @@ num_processors (enum nproc_query query)
> Cygwin, Haiku. */
> long int nprocs = sysconf (_SC_NPROCESSORS_ONLN);
> if (nprocs > 0)
> - return MIN (nprocs, omp_env_limit);
> + return nprocs;
> }
> #endif
> }
> @@ -330,7 +277,7 @@ num_processors (enum nproc_query query)
> if (query == NPROC_CURRENT)
> {
> if (psd.psd_proc_cnt > 0)
> - return MIN (psd.psd_proc_cnt, omp_env_limit);
> + return psd.psd_proc_cnt;
> }
> else
> {
> @@ -351,7 +298,7 @@ num_processors (enum nproc_query query)
> ? MP_NAPROCS
> : MP_NPROCS);
> if (nprocs > 0)
> - return MIN (nprocs, omp_env_limit);
> + return nprocs;
> }
> #endif
>
> @@ -367,7 +314,7 @@ num_processors (enum nproc_query query)
> if (sysctl (mib, ARRAY_SIZE (mib), &nprocs, &len, NULL, 0) == 0
> && len == sizeof (nprocs)
> && 0 < nprocs)
> - return MIN (nprocs, omp_env_limit);
> + return nprocs;
> }
> #endif
>
> @@ -376,9 +323,73 @@ num_processors (enum nproc_query query)
> SYSTEM_INFO system_info;
> GetSystemInfo (&system_info);
> if (0 < system_info.dwNumberOfProcessors)
> - return MIN (system_info.dwNumberOfProcessors, omp_env_limit);
> + return system_info.dwNumberOfProcessors;
> }
> #endif
>
> return 1;
> }
> +
> +/* Parse OMP environment variables without dependence on OMP.
> + Return 0 for invalid values. */
> +static unsigned long int
> +parse_omp_threads (char const* threads)
> +{
> + unsigned long int ret = 0;
> +
> + if (threads == NULL)
> + return ret;
> +
> + /* The OpenMP spec says that the value assigned to the environment
> variables
> + "may have leading and trailing white space". */
> + while (*threads != '\0' && c_isspace (*threads))
> + threads++;
> +
> + /* Convert it from positive decimal to 'unsigned long'. */
> + if (c_isdigit (*threads))
> + {
> + char *endptr = NULL;
> + unsigned long int value = strtoul (threads, &endptr, 10);
> +
> + if (endptr != NULL)
> + {
> + while (*endptr != '\0' && c_isspace (*endptr))
> + endptr++;
> + if (*endptr == '\0')
> + return value;
> + /* Also accept the first value in a nesting level,
> + since we can't determine the nesting level from env vars. */
> + else if (*endptr == ',')
> + return value;
> + }
> + }
> +
> + return ret;
> +}
> +
> +unsigned long int
> +num_processors (enum nproc_query query)
> +{
> + unsigned long int omp_env_limit = ULONG_MAX;
> +
> + if (query == NPROC_CURRENT_OVERRIDABLE)
> + {
> + unsigned long int omp_env_threads;
> + /* Honor the OpenMP environment variables, recognized also by all
> + programs that are based on OpenMP. */
> + omp_env_threads = parse_omp_threads (getenv ("OMP_NUM_THREADS"));
> + omp_env_limit = parse_omp_threads (getenv ("OMP_THREAD_LIMIT"));
> + if (! omp_env_limit)
> + omp_env_limit = ULONG_MAX;
> +
> + if (omp_env_threads)
> + return MIN (omp_env_threads, omp_env_limit);
> +
> + query = NPROC_CURRENT;
> + }
> + /* Here query is one of NPROC_ALL, NPROC_CURRENT. */
> + {
> + unsigned long nprocs = num_processors_ignoring_omp (query);
> + return MIN (nprocs, omp_env_limit);
> + }
> +}
Yes much better with that cleanup.
I presume you'll apply.
thanks!
Pádraig