Message ID | 20200220181747.12898-1-lamm@linux.ibm.com |
---|---|
State | New |
Headers | show |
Series | [v3] Fix time/tst-cpuclock1 intermitent failures | expand |
Hi Lucas, I believe the main idea behind Carlos' initial suggestion was to make something more generic that could also be used in other places. The patch is following in this direction alright but I think there are still a few details that could be improved in this regard. Comments below. On 2/20/20 3:17 PM, Lucas A. M. Magalhaes wrote: > This test fails intermittently in systems with heavy load as > CLOCK_PROCESS_CPUTIME_ID is subject to scheduler pressure. Thus the > test boundaries where relaxed to keep it from fail on this systems. "Thus the test boundaries were relaxed to keep it from failing on such systems." > > A refactor of the spent time checking was made with some support > functions. With the advantage to represent time jitter in percent > of the target. > > The values used by the test boundaries are all empirical. > > --- > > Hi, > > changes on V3: > - refactor support functions > - use existing timespec-sub function > > changes on V2: > - Add support functions > > support/Makefile | 1 + > support/support_cpuclock.c | 58 ++++++++++++++++++++++++++++++++++++++ > support/support_cpuclock.h | 28 ++++++++++++++++++ > time/tst-cpuclock1.c | 47 +++++++++++------------------- > 4 files changed, 103 insertions(+), 31 deletions(-) > create mode 100644 support/support_cpuclock.c > create mode 100644 support/support_cpuclock.h > > diff --git a/support/Makefile b/support/Makefile > index a0304e6def..1e45d99b27 100644 > --- a/support/Makefile > +++ b/support/Makefile > @@ -31,6 +31,7 @@ libsupport-routines = \ > check_dns_packet \ > check_hostent \ > check_netent \ > + support_cpuclock \ > delayed_exit \ > ignore_stderr \ > next_to_fault \ > diff --git a/support/support_cpuclock.c b/support/support_cpuclock.c > new file mode 100644 > index 0000000000..cd5212bc32 > --- /dev/null > +++ b/support/support_cpuclock.c > @@ -0,0 +1,58 @@ > +/* Support functions for cpuclock tests. > + Copyright (C) 2020 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + <https://www.gnu.org/licenses/>. */ > + > +#include "support_cpuclock.h" > +#include <stdlib.h> > +#include <assert.h> > + > +#define TIMESPEC_HZ 1000000000.0 > + > +/* Returns t normalized timespec with .tv_nsec < TIMESPEC_HZ > + and the overflows added to .tv_sec. */ > +struct timespec > +support_timespec_normalize (struct timespec t) > +{ > + int diff; > + diff = (t.tv_nsec / TIMESPEC_HZ); > + t.tv_sec += diff; > + t.tv_nsec += -(diff * TIMESPEC_HZ); > + return t; > +} Wouldn't something like: struct timespec support_timespec_normalize (struct timespec t) { t.tv_sec += (t.tv_nsec / TIMESPEC_HZ); t.tv_nsec = (t.tv_nsec % TIMESPEC_HZ); return t; } be more straightforward? Also, since this is a fairly generic function for timespec, I think it could be added to support/timespec.h, like timespec_add and timespec_sub. > + > +/* Returns TRUE if diff to base ratio is within the specified bounds, and > +FALSE otherwise. > +For example the call > + > +support_timespec_check_ratio(base, diff, 1.2, .5); > + > +will check if > + > +.5 <= diff/base <= 1.2 > + > +In other words it will check if diff time is within 50% to 120% of > +the base time. */ > +int > +support_timespec_check_ratio (struct timespec base, struct timespec diff, > + double upper_bound, double lower_bound) I believe support_timespec_check_in_range (as used somewhere previously on this thread) or support_timespec_check_within_bounds (maybe too long) would be more appropriate names, since the ratio is only part of the calculation, not what the function actually does. A minor detail, but my brain finds it confusing that the upper_bound comes before the lower_bound in the arguments list when the final check will be like: lower_bound <= diff/base <= upper_bound. At last, diff seems to be a leftover from the current use case for this function (in time/tst-cpuclock1.c). Since this is a generic function that can be used in other places, I suggest changing the name to 'value' (or similar). > +{ > + assert (upper_bound >= lower_bound); > + double base_norm = base.tv_sec + base.tv_nsec / TIMESPEC_HZ; > + double diff_norm = diff.tv_sec + diff.tv_nsec / TIMESPEC_HZ; > + double ratio = diff_norm / base_norm; > + return (ratio <= upper_bound && ratio >= lower_bound); > +} When Adhermeval suggested to normalize this to nanoseconds instead of seconds, you replied that this would not be used to check really small values. But once it enters support/*, I believe it is fair game to be reused in other places that might as well deal with small values, so it's something to consider. > diff --git a/support/support_cpuclock.h b/support/support_cpuclock.h > new file mode 100644 > index 0000000000..a14bc91e3f > --- /dev/null > +++ b/support/support_cpuclock.h > @@ -0,0 +1,28 @@ > +/* Support functions for cpuclock tests. > + Copyright (C) 2020 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + <https://www.gnu.org/licenses/>. */ > + > +#ifndef SUPPORT_CPUCLOCK_H > +#define SUPPORT_CPUCLOCK_H > + > +#include <time.h> > + > +struct timespec support_timespec_normalize (struct timespec t); > +int support_timespec_check_ratio (struct timespec base, struct timespec diff, > + double upper_bound, double lower_bound); > + > +#endif /* SUPPORT_CPUCLOCK_H */ > diff --git a/time/tst-cpuclock1.c b/time/tst-cpuclock1.c > index 0120906f23..72450f71ee 100644 > --- a/time/tst-cpuclock1.c > +++ b/time/tst-cpuclock1.c > @@ -26,6 +26,8 @@ > #include <signal.h> > #include <stdint.h> > #include <sys/wait.h> > +#include <support/support_cpuclock.h> > +#include <support/timespec.h> > > /* This function is intended to rack up both user and system time. */ > static void > @@ -155,19 +157,13 @@ do_test (void) > printf ("live PID %d after sleep => %ju.%.9ju\n", > child, (uintmax_t) after.tv_sec, (uintmax_t) after.tv_nsec); > > - struct timespec diff = { .tv_sec = after.tv_sec - before.tv_sec, > - .tv_nsec = after.tv_nsec - before.tv_nsec }; > - if (diff.tv_nsec < 0) > - { > - --diff.tv_sec; > - diff.tv_nsec += 1000000000; > - } > - if (diff.tv_sec != 0 > - || diff.tv_nsec > 600000000 > - || diff.tv_nsec < 100000000) > + support_timespec_normalize(after); > + support_timespec_normalize(before); > + struct timespec diff = timespec_sub(after, before); > + if (!support_timespec_check_ratio(sleeptime, diff, 1.3, .0025)) > { > printf ("before - after %ju.%.9ju outside reasonable range\n", Ok. > - (uintmax_t) diff.tv_sec, (uintmax_t) diff.tv_nsec); > + (uintmax_t) diff.tv_sec, (uintmax_t) diff.tv_nsec); Unintended change? > result = 1; > } > > @@ -194,19 +190,12 @@ do_test (void) > } > else > { > - struct timespec d = { .tv_sec = afterns.tv_sec - after.tv_sec, > - .tv_nsec = afterns.tv_nsec - after.tv_nsec }; > - if (d.tv_nsec < 0) > - { > - --d.tv_sec; > - d.tv_nsec += 1000000000; > - } > - if (d.tv_sec > 0 > - || d.tv_nsec < sleeptime.tv_nsec > - || d.tv_nsec > sleeptime.tv_nsec * 2) > + support_timespec_normalize(afterns); > + diff = timespec_sub(afterns, after); > + if (!support_timespec_check_ratio(sleeptime, diff, 1.6, .71)) > { > printf ("nanosleep time %ju.%.9ju outside reasonable range\n", > - (uintmax_t) d.tv_sec, (uintmax_t) d.tv_nsec); > + (uintmax_t) diff.tv_sec, (uintmax_t) diff.tv_nsec); > result = 1; > } > } Ok. > @@ -241,17 +230,13 @@ do_test (void) > printf ("dead PID %d => %ju.%.9ju\n", > child, (uintmax_t) dead.tv_sec, (uintmax_t) dead.tv_nsec); > > - diff.tv_sec = dead.tv_sec - after.tv_sec; > - diff.tv_nsec = dead.tv_nsec - after.tv_nsec; > - if (diff.tv_nsec < 0) > - { > - --diff.tv_sec; > - diff.tv_nsec += 1000000000; > - } > - if (diff.tv_sec != 0 || diff.tv_nsec > 200000000) > + support_timespec_normalize(dead); > + diff = timespec_sub(dead, after); > + sleeptime.tv_nsec = 100000000; > + if (!support_timespec_check_ratio(sleeptime, diff, 1.6, .7)) > { Ok. > printf ("dead - after %ju.%.9ju outside reasonable range\n", > - (uintmax_t) diff.tv_sec, (uintmax_t) diff.tv_nsec); > + (uintmax_t) diff.tv_sec, (uintmax_t) diff.tv_nsec); Unintended change? > result = 1; > } > > Thanks, Matheus Castanho
Quoting Matheus Castanho (2020-03-04 16:24:14) > Hi Lucas, > > I believe the main idea behind Carlos' initial suggestion was to make > something more generic that could also be used in other places. The > patch is following in this direction alright but I think there are still > a few details that could be improved in this regard. Comments below. > Hi Matheus, Thanks for the review. AFAIU Carlos was specifically pointing out that this could be reused on tst-cpuclock2. So, this was my target during the development of this patch. From what I got you believe that the timespec functions could be completely generic. I agree so I will rewrite the patch for this. > > +#include "support_cpuclock.h" > > +#include <stdlib.h> > > +#include <assert.h> > > + > > +#define TIMESPEC_HZ 1000000000.0 > > + > > +/* Returns t normalized timespec with .tv_nsec < TIMESPEC_HZ > > + and the overflows added to .tv_sec. */ > > +struct timespec > > +support_timespec_normalize (struct timespec t) > > +{ > > + int diff; > > + diff = (t.tv_nsec / TIMESPEC_HZ); > > + t.tv_sec += diff; > > + t.tv_nsec += -(diff * TIMESPEC_HZ); > > + return t; > > +} > > Wouldn't something like: > > struct timespec > support_timespec_normalize (struct timespec t) > { > t.tv_sec += (t.tv_nsec / TIMESPEC_HZ); > t.tv_nsec = (t.tv_nsec % TIMESPEC_HZ); > return t; > } > > be more straightforward? > Yes, Nice. Thanks to point this out.
diff --git a/support/Makefile b/support/Makefile index a0304e6def..1e45d99b27 100644 --- a/support/Makefile +++ b/support/Makefile @@ -31,6 +31,7 @@ libsupport-routines = \ check_dns_packet \ check_hostent \ check_netent \ + support_cpuclock \ delayed_exit \ ignore_stderr \ next_to_fault \ diff --git a/support/support_cpuclock.c b/support/support_cpuclock.c new file mode 100644 index 0000000000..cd5212bc32 --- /dev/null +++ b/support/support_cpuclock.c @@ -0,0 +1,58 @@ +/* Support functions for cpuclock tests. + Copyright (C) 2020 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <https://www.gnu.org/licenses/>. */ + +#include "support_cpuclock.h" +#include <stdlib.h> +#include <assert.h> + +#define TIMESPEC_HZ 1000000000.0 + +/* Returns t normalized timespec with .tv_nsec < TIMESPEC_HZ + and the overflows added to .tv_sec. */ +struct timespec +support_timespec_normalize (struct timespec t) +{ + int diff; + diff = (t.tv_nsec / TIMESPEC_HZ); + t.tv_sec += diff; + t.tv_nsec += -(diff * TIMESPEC_HZ); + return t; +} + +/* Returns TRUE if diff to base ratio is within the specified bounds, and +FALSE otherwise. +For example the call + +support_timespec_check_ratio(base, diff, 1.2, .5); + +will check if + +.5 <= diff/base <= 1.2 + +In other words it will check if diff time is within 50% to 120% of +the base time. */ +int +support_timespec_check_ratio (struct timespec base, struct timespec diff, + double upper_bound, double lower_bound) +{ + assert (upper_bound >= lower_bound); + double base_norm = base.tv_sec + base.tv_nsec / TIMESPEC_HZ; + double diff_norm = diff.tv_sec + diff.tv_nsec / TIMESPEC_HZ; + double ratio = diff_norm / base_norm; + return (ratio <= upper_bound && ratio >= lower_bound); +} diff --git a/support/support_cpuclock.h b/support/support_cpuclock.h new file mode 100644 index 0000000000..a14bc91e3f --- /dev/null +++ b/support/support_cpuclock.h @@ -0,0 +1,28 @@ +/* Support functions for cpuclock tests. + Copyright (C) 2020 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <https://www.gnu.org/licenses/>. */ + +#ifndef SUPPORT_CPUCLOCK_H +#define SUPPORT_CPUCLOCK_H + +#include <time.h> + +struct timespec support_timespec_normalize (struct timespec t); +int support_timespec_check_ratio (struct timespec base, struct timespec diff, + double upper_bound, double lower_bound); + +#endif /* SUPPORT_CPUCLOCK_H */ diff --git a/time/tst-cpuclock1.c b/time/tst-cpuclock1.c index 0120906f23..72450f71ee 100644 --- a/time/tst-cpuclock1.c +++ b/time/tst-cpuclock1.c @@ -26,6 +26,8 @@ #include <signal.h> #include <stdint.h> #include <sys/wait.h> +#include <support/support_cpuclock.h> +#include <support/timespec.h> /* This function is intended to rack up both user and system time. */ static void @@ -155,19 +157,13 @@ do_test (void) printf ("live PID %d after sleep => %ju.%.9ju\n", child, (uintmax_t) after.tv_sec, (uintmax_t) after.tv_nsec); - struct timespec diff = { .tv_sec = after.tv_sec - before.tv_sec, - .tv_nsec = after.tv_nsec - before.tv_nsec }; - if (diff.tv_nsec < 0) - { - --diff.tv_sec; - diff.tv_nsec += 1000000000; - } - if (diff.tv_sec != 0 - || diff.tv_nsec > 600000000 - || diff.tv_nsec < 100000000) + support_timespec_normalize(after); + support_timespec_normalize(before); + struct timespec diff = timespec_sub(after, before); + if (!support_timespec_check_ratio(sleeptime, diff, 1.3, .0025)) { printf ("before - after %ju.%.9ju outside reasonable range\n", - (uintmax_t) diff.tv_sec, (uintmax_t) diff.tv_nsec); + (uintmax_t) diff.tv_sec, (uintmax_t) diff.tv_nsec); result = 1; } @@ -194,19 +190,12 @@ do_test (void) } else { - struct timespec d = { .tv_sec = afterns.tv_sec - after.tv_sec, - .tv_nsec = afterns.tv_nsec - after.tv_nsec }; - if (d.tv_nsec < 0) - { - --d.tv_sec; - d.tv_nsec += 1000000000; - } - if (d.tv_sec > 0 - || d.tv_nsec < sleeptime.tv_nsec - || d.tv_nsec > sleeptime.tv_nsec * 2) + support_timespec_normalize(afterns); + diff = timespec_sub(afterns, after); + if (!support_timespec_check_ratio(sleeptime, diff, 1.6, .71)) { printf ("nanosleep time %ju.%.9ju outside reasonable range\n", - (uintmax_t) d.tv_sec, (uintmax_t) d.tv_nsec); + (uintmax_t) diff.tv_sec, (uintmax_t) diff.tv_nsec); result = 1; } } @@ -241,17 +230,13 @@ do_test (void) printf ("dead PID %d => %ju.%.9ju\n", child, (uintmax_t) dead.tv_sec, (uintmax_t) dead.tv_nsec); - diff.tv_sec = dead.tv_sec - after.tv_sec; - diff.tv_nsec = dead.tv_nsec - after.tv_nsec; - if (diff.tv_nsec < 0) - { - --diff.tv_sec; - diff.tv_nsec += 1000000000; - } - if (diff.tv_sec != 0 || diff.tv_nsec > 200000000) + support_timespec_normalize(dead); + diff = timespec_sub(dead, after); + sleeptime.tv_nsec = 100000000; + if (!support_timespec_check_ratio(sleeptime, diff, 1.6, .7)) { printf ("dead - after %ju.%.9ju outside reasonable range\n", - (uintmax_t) diff.tv_sec, (uintmax_t) diff.tv_nsec); + (uintmax_t) diff.tv_sec, (uintmax_t) diff.tv_nsec); result = 1; }