[PATCH] linux: fix accuracy of get_nprocs and get_nprocs_conf [BZ #28865]

[Adhemerval Zanella]

On 05/02/2022 18:24, Dmitry V. Levin wrote:
> get_nprocs() and get_nprocs_conf() use various methods to obtain an
> accurate number of processors.  Re-introduce __get_nprocs_sched() as
> a source of information, and fix the order in which these methods are
> used to return the most accurate information.  The primary source of
> information used in both functions remains unchanged.
> 
> This also changes __get_nprocs_sched() error return value from 2 to 0,
> but all its users are already prepared to handle that.
> 
> Old behavior:
>   get_nprocs:
>     /sys/devices/system/cpu/online -> /proc/stat -> 2
>   get_nprocs_conf:
>     /sys/devices/system/cpu/ -> /proc/stat -> 2
> 
> New behavior:
>   get_nprocs:
>     /sys/devices/system/cpu/online -> sched_getaffinity -> /proc/stat -> 2
>   get_nprocs_conf:
>     /sys/devices/system/cpu/ -> /proc/stat -> sched_getaffinity -> 2
> 
> Fixes: 342298278e ("linux: Revert the use of sched_getaffinity on get_nproc")
> Closes: BZ #28865

I think we are circling back on this, on BZ#27645 [1] we changed get_nprocs
to use sched_getaffinity and then we have to revert it with BZ#28310 [2] because
it introduced regression on some monitoring tools [3].

In fact from BZ#27645 and BZ#28624 [4] discussion I think we can't reliable use 
sched_getaffinity because since some container environment returns a synthetic
mask that might break some programs.  Also, sched_getaffinity returns a 
'per-process' mask instead of system-wide as we discussed in previous threads.
It should be ok to get adjusting internal tuning (as for malloc).

[1] https://sourceware.org/bugzilla/show_bug.cgi?id=27645
[2] https://sourceware.org/bugzilla/show_bug.cgi?id=28310
[3] https://sourceware.org/bugzilla/show_bug.cgi?id=27645#c5
[4] https://sourceware.org/bugzilla/show_bug.cgi?id=28624

> ---
>  sysdeps/unix/sysv/linux/getsysstats.c | 101 ++++++++++++++++++--------
>  1 file changed, 70 insertions(+), 31 deletions(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/getsysstats.c b/sysdeps/unix/sysv/linux/getsysstats.c
> index c98c8ce3d4..e1ed96070c 100644
> --- a/sysdeps/unix/sysv/linux/getsysstats.c
> +++ b/sysdeps/unix/sysv/linux/getsysstats.c
> @@ -50,9 +50,8 @@ __get_nprocs_sched (void)
>         is an arbitrary values assuming such systems should be rare and there
>         is no offline cpus.  */
>      return max_num_cpus;
> -  /* Some other error.  2 is conservative (not a uniprocessor system, so
> -     atomics are needed). */
> -  return 2;
> +  /* Some other error.  */
> +  return 0;
>  }
>  
>  static char *
> @@ -108,22 +107,19 @@ next_line (int fd, char *const buffer, char **cp, char **re,
>  }
>  
>  static int
> -get_nproc_stat (char *buffer, size_t buffer_size)
> +get_nproc_stat (void)
>  {
> +  enum { buffer_size = 1024 };
> +  char buffer[buffer_size];
>    char *buffer_end = buffer + buffer_size;
>    char *cp = buffer_end;
>    char *re = buffer_end;
> -
> -  /* Default to an SMP system in case we cannot obtain an accurate
> -     number.  */
> -  int result = 2;
> +  int result = 0;
>  
>    const int flags = O_RDONLY | O_CLOEXEC;
>    int fd = __open_nocancel ("/proc/stat", flags);
>    if (fd != -1)
>      {
> -      result = 0;
> -
>        char *l;
>        while ((l = next_line (fd, buffer, &cp, &re, buffer_end)) != NULL)
>  	/* The current format of /proc/stat has all the cpu* entries
> @@ -139,8 +135,8 @@ get_nproc_stat (char *buffer, size_t buffer_size)
>    return result;
>  }
>  
> -int
> -__get_nprocs (void)
> +static int
> +get_nprocs_cpu_online (void)
>  {
>    enum { buffer_size = 1024 };
>    char buffer[buffer_size];
> @@ -179,7 +175,8 @@ __get_nprocs (void)
>  		  }
>  	      }
>  
> -	    result += m - n + 1;
> +	    if (m >= n)
> +	      result += m - n + 1;
>  
>  	    l = endp;
>  	    if (l < re && *l == ',')
> @@ -188,28 +185,18 @@ __get_nprocs (void)
>  	while (l < re && *l != '\n');
>  
>        __close_nocancel_nostatus (fd);
> -
> -      if (result > 0)
> -	return result;
>      }
>  
> -  return get_nproc_stat (buffer, buffer_size);
> +  return result;
>  }
> -libc_hidden_def (__get_nprocs)
> -weak_alias (__get_nprocs, get_nprocs)
> -
>  
> -/* On some architectures it is possible to distinguish between configured
> -   and active cpus.  */
> -int
> -__get_nprocs_conf (void)
> +static int
> +get_nprocs_cpu (void)
>  {
> -  /* Try to use the sysfs filesystem.  It has actual information about
> -     online processors.  */
> +  int count = 0;
>    DIR *dir = __opendir ("/sys/devices/system/cpu");
>    if (dir != NULL)
>      {
> -      int count = 0;
>        struct dirent64 *d;
>  
>        while ((d = __readdir64 (dir)) != NULL)
> @@ -224,12 +211,64 @@ __get_nprocs_conf (void)
>  
>        __closedir (dir);
>  
> -      return count;
>      }
> +  return count;
> +}
>  
> -  enum { buffer_size = 1024 };
> -  char buffer[buffer_size];
> -  return get_nproc_stat (buffer, buffer_size);
> +int
> +__get_nprocs (void)
> +{
> +  int result;
> +
> +  /* Try /sys/devices/system/cpu/online first.  */
> +  result = get_nprocs_cpu_online ();
> +  if (result)
> +    return result;
> +
> +  /* Try sched_getaffinity(2).  */
> +  result = __get_nprocs_sched ();
> +  if (result)
> +    return result;
> +
> +  /* Try /proc/stat.  */
> +  result = get_nproc_stat ();
> +  if (result)
> +    return result;
> +
> +  /* We failed to obtain an accurate number.  Be conservative: return
> +     the smallest number meaning that this is not a uniprocessor system,
> +     so atomics are needed.  */
> +  return 2;
> +}
> +libc_hidden_def (__get_nprocs)
> +weak_alias (__get_nprocs, get_nprocs)
> +
> +/* On some architectures it is possible to distinguish between configured
> +   and active cpus.  */
> +int
> +__get_nprocs_conf (void)
> +{
> +  int result;
> +
> +  /* Try /sys/devices/system/cpu/ first.  */
> +  result = get_nprocs_cpu ();
> +  if (result)
> +    return result;
> +
> +  /* Try /proc/stat.  */
> +  result = get_nproc_stat ();
> +  if (result)
> +    return result;
> +
> +  /* Try sched_getaffinity(2).  */
> +  result = __get_nprocs_sched ();
> +  if (result)
> +    return result;
> +
> +  /* We failed to obtain an accurate number.  Be conservative: return
> +     the smallest number meaning that this is not a uniprocessor system,
> +     so atomics are needed.  */
> +  return 2;
>  }
>  libc_hidden_def (__get_nprocs_conf)
>  weak_alias (__get_nprocs_conf, get_nprocs_conf)
>