bug-gnulib
[Top][All Lists]
Advanced

[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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]