| Message ID | 1402591036-29454-1-git-send-email-meadori@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
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.
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 >
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.
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).
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
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 --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. */
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(-)