diff mbox

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

Message ID 1402591036-29454-1-git-send-email-meadori@codesourcery.com
State New
Headers show

Commit Message

Meador Inge June 12, 2014, 4:37 p.m. UTC
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(-)

Comments

Meador Inge Jan. 1, 1970, 12:06 a.m. UTC | #1
On 06/12/2014 03:54 PM, Siddhesh Poyarekar wrote:

> Oh yes, so:
> 
>     (now == prev && now != 0)

That is indeed simpler and fixes the correctness issue, but now caching is
disabled for the 'time (NULL) == 0' case.  Therefore two close call to
'get_nprocs' at that time point will cause the value to be computed both times.
I prefer the first patch where correctness and the caching behavior is
maintained, but I understand if others value the simpler approach that
sacrifices a caching corner-case while still fixing correctness.
Siddhesh Poyarekar June 12, 2014, 5:15 p.m. UTC | #2
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
>
Meador Inge June 12, 2014, 8:22 p.m. UTC | #3
On 06/12/2014 12:15 PM, Siddhesh Poyarekar wrote:

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

Hmmm, good point, but is it OK to make assumptions about the underlying
representation of 'time_t'* (e.g. that we get the expected behavior from the > 0
compare)?  The 'now == prev' compare is at least 'time_t'-to-'time_t'.  I
thought about just doing 'cached_result > 0' too, but I suppose 'cached_result'
can actually be 0 in the case of an error.

* The C Standard generally says not to, but maybe we can make that assumption
within the implementation of the C Standard Library.
Roland McGrath June 12, 2014, 8:37 p.m. UTC | #4
time_t is always an integer type counting seconds since 1970 in glibc.
However, it can be signed and negative values can be valid timestamps (if
you set your computer's clock to 1969, for example).
Siddhesh Poyarekar June 12, 2014, 8:54 p.m. UTC | #5
On Thu, Jun 12, 2014 at 01:37:37PM -0700, Roland McGrath wrote:
> time_t is always an integer type counting seconds since 1970 in glibc.
> However, it can be signed and negative values can be valid timestamps (if
> you set your computer's clock to 1969, for example).

Oh yes, so:

    (now == prev && now != 0)

Siddhesh
Siddhesh Poyarekar June 12, 2014, 10:01 p.m. UTC | #6
On Wed, Dec 31, 1969 at 06:06:21PM -0600, Meador Inge wrote:
> Therefore two close call to 'get_nprocs' at that time point will
> cause the value to be computed both times.  I prefer the first patch
> where correctness and the caching behavior is maintained, but I
> understand if others value the simpler approach that sacrifices a
> caching corner-case while still fixing correctness.

I'd say correctness trumps here, even if it is an obscure corner case.
I'll push it tomorrow if nobody objects.

Siddhesh
diff mbox

Patch

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)
     return cached_result;
 
   /* XXX Here will come a test for the new system call.  */