Re: [PATCH][BZ #16996] get_nprocs: Only return explictly set cache values

[Siddhesh Poyarekar <siddhesh at redhat dot com>]

On Thu, Jun 12, 2014 at 11:37:16AM -0500, Meador Inge wrote:
> The implementation of __get_nprocs uses a stactic variable to cache
> the value of the current number of processors.  The caching breaks when
> 'time (NULL) == 0':
> 
>   $ cat nproc.c
>   #include <stdio.h>
>   #include <time.h>
>   #include <sys/time.h>
> 
>   int main(int argc, char *argv[])
>   {
>     time_t t;
>     struct timeval tv = {0, 0};
>     printf("settimeofday({0, 0}, NULL) = %d\n", settimeofday(&tv, NULL));
>     t = time(NULL);
>     printf("Time: %d, CPUs: %d\n", (unsigned int)t, get_nprocs());
>     return 0;
>   }
>   $ gcc -O3 nproc.c
>   $ ./a.out
>   settimeofday({0, 0}, NULL) = -1
>   Time: 1401311578, CPUs: 4
>   $ sudo ./a.out
>   settimeofday({0, 0}, NULL) = 0
>   Time: 0, CPUs: 0
> 
> The problem is with the condition used to check whether a cached
> value should be returned or not:
> 
>   static int cached_result;
>   static time_t timestamp;
> 
>   time_t now = time (NULL);
>   time_t prev = timestamp;
>   atomic_read_barrier ();
>   if (now == prev)
>     return cached_result;
> 
> When 'timestamp == 0', as in the above test case, an unitialized
> cache value is returned.
> 
> This patch fixes the problem by ensuring that 'cached_result' has
> been set at least once before returning it.
> 
> I checked for regressions with 'make check' and verified that the
> described test case works, but couldn't find a good way to automate
> the test in the GLIBC testsuite.
> 
> OK?
> 
> 2014-06-12  Meador Inge  <meadori@codesourcery.com>
> 
> 	[BZ #16996]
> 	sysdeps/unix/sysv/linux/getsysstats.c (__get_nprocs): Ensure that
> 	the cached result has been set before returning it.
> 
> ---
>  sysdeps/unix/sysv/linux/getsysstats.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/getsysstats.c b/sysdeps/unix/sysv/linux/getsysstats.c
> index b6a6fe3..755c259 100644
> --- a/sysdeps/unix/sysv/linux/getsysstats.c
> +++ b/sysdeps/unix/sysv/linux/getsysstats.c
> @@ -126,13 +126,13 @@ next_line (int fd, char *const buffer, char **cp, char **re,
>  int
>  __get_nprocs (void)
>  {
> -  static int cached_result;
> +  static int cached_result = -1;
>    static time_t timestamp;
>  
>    time_t now = time (NULL);
>    time_t prev = timestamp;
>    atomic_read_barrier ();
> -  if (now == prev)
> +  if (now == prev && cached_result > -1)

Wouldn't (now == prev && now > 0) be nicer?

Siddhesh

>      return cached_result;
>  
>    /* XXX Here will come a test for the new system call.  */
> -- 
> 1.7.9.5
>

Attachment: pgpiQAAirp31f.pgp
Description: PGP signature