From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 83799 invoked by alias); 19 Feb 2020 16:42:23 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 83777 invoked by uid 89); 19 Feb 2020 16:42:22 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-27.6 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.1 spammy=sometime, 065, clock, rack X-HELO: mx0a-001b2d01.pphosted.com Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable In-Reply-To: References: <20200206144819.19046-1-lamm@linux.ibm.com> Subject: Re: [PATCH v2] Fix time/tst-cpuclock1 intermitent failures From: "Lucas A. M. Magalhaes" To: Adhemerval Zanella , libc-alpha@sourceware.org Date: Wed, 19 Feb 2020 16:42:00 -0000 Message-ID: <158213053262.30076.4354318606424637635@dhcp-9-18-235-152.br.ibm.com> User-Agent: alot/0.9 X-SW-Source: 2020-02/txt/msg00820.txt.bz2 Hi, Thanks for the review :). Quoting Adhemerval Zanella (2020-02-18 09:44:08) >=20 >=20 > On 06/02/2020 11:48, 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. > >=20 > > 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. > >=20 > > The values used by the test boundaries are all empirical. > >=20 > > --- > >=20 > > Hi, > >=20 > > Please Carlos see if this is what you asked. > >=20 > > I spent sometime gathering the spent times to find the values > > for the boundaries. From this I selected values that will fail less > > than 1% of the time, in the tested machines. > >=20 > > Also as I found the spent time deviation completely asymmetrical > > I choose to separate the values in upper and lower bounds. > >=20 > > changes from V2: > > - Add support functions >=20 > None of the files follow the code and style guideline [1]. >=20 > [1] https://sourceware.org/glibc/wiki/Style_and_Conventions >=20 OK, Sorry about that. > >=20 > > support/Makefile | 1 + > > support/cpuclock.c | 51 ++++++++++++++++++++++++++++++++++++++++++++ > > support/cpuclock.h | 30 ++++++++++++++++++++++++++ > > time/tst-cpuclock1.c | 43 ++++++++++--------------------------- > > 4 files changed, 93 insertions(+), 32 deletions(-) > > create mode 100644 support/cpuclock.c > > create mode 100644 support/cpuclock.h > >=20 > > diff --git a/support/Makefile b/support/Makefile > > index 3325feb790..b308fe0856 100644 > > --- a/support/Makefile > > +++ b/support/Makefile > > @@ -31,6 +31,7 @@ libsupport-routines =3D \ > > check_dns_packet \ > > check_hostent \ > > check_netent \ > > + cpuclock \ > > delayed_exit \ > > ignore_stderr \ > > next_to_fault \ > > diff --git a/support/cpuclock.c b/support/cpuclock.c > > new file mode 100644 > > index 0000000000..f40c7e8d4a > > --- /dev/null > > +++ b/support/cpuclock.c > > @@ -0,0 +1,51 @@ > > +/* Support functions for cpuclock tests. > > + Copyright (C) 2018-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 > > + . */ > > + > > +#include "cpuclock.h" > > +#include > > + > > +#define T_1s 1000000000.0 > > + > > +struct timespec time_normalize(struct timespec t) { > > + int diff; > > + diff =3D (t.tv_nsec / T_1s); > > + t.tv_sec +=3D diff; > > + t.tv_nsec +=3D -(diff * T_1s); > > + return t; > > +} > > + > > +struct timespec time_add(struct timespec a, struct timespec b) { > > + struct timespec s =3D {.tv_sec =3D a.tv_sec + b.tv_sec, > > + .tv_nsec =3D a.tv_nsec + b.tv_nsec}; > > + return time_normalize(s); > > +} > > + > > +struct timespec time_sub(struct timespec a, struct timespec b) { > > + struct timespec d =3D {.tv_sec =3D a.tv_sec - b.tv_sec, > > + .tv_nsec =3D a.tv_nsec - b.tv_nsec}; > > + return time_normalize(d); > > +} >=20 > I don't using float operation is the correct solution to handle both > invalid inputs and overflow results. We already have a working solution > (support/timespec-add.c and support/timespec-sub.c), why do you need > to add such functions? >=20 Well I actually didn't notice them, as I was suggested to implement it. Thanks for pointing it out. I will remove them and use the existing solution. > > + > > +int percent_diff_check(struct timespec base, struct timespec dev, > > + float upper_bound, float lower_bound) { > > + double timea =3D base.tv_sec + base.tv_nsec / T_1s; > > + double timeb =3D dev.tv_sec + dev.tv_nsec / T_1s; > > + double diff =3D (timea - timeb) / (0.5 * (timea + timeb)); > > + return (diff >=3D -upper_bound && diff <=3D lower_bound ); > > +} > > + >=20 > Please add some description of what this function do. And I think=20 > it would be better to normalize to nanoseconds instead of seconds > to avoid floating-point cancellation due the range difference. I'm using double here because this "(timea + timeb)" was sometimes overflowing. IMHO this will not be used to check, or compute, really small jitter, so floating-point cancellation should not be a problem. But if you see that it's worth I could to change to long and use: double diff =3D (timea - timeb) / max(timea, timeb); instead of double diff =3D (timea - timeb) / (0.5 * (timea + timeb)); > Something like: >=20 > /* Return true if the DIFF time is within the ratio=20 IMHO it makes more sense to think in it as a relative difference rather than in ration. As "diff time is x% bigger than base" instead of "diff time is x% of the base". So I want to keep this approach. > [upper_bound, lower_bound] of DIFF time, or false otherwise. >=20 > For instance: >=20 > struct timespec diff =3D { 3, 94956 }; > struct timespec base =3D { 4, 0 }; >=20 > The call checks if the ratio of diff/base is within the > bounds of (0.65, 1.0) (i.e, if 'diff' is at least 65% of > the 'base' value). >=20 > support_timespec_check_in_range (base, diff, 1.0, 0.65); */ > bool support_timespec_check_ratio (struct timespec base, > struct timespec diff, > double upper_bound, > double lower_bound) > { > assert (upper_bound >=3D lower_bound); > uint64_t base_norm =3D base.tv_sec * TIMESPEC_HZ + base.tv_nsec; > uint64_t diff_norm =3D diff.tv_sec * TIMESPEC_HZ + diff.tv_nsec; > double ratio =3D (double) base_norm / (double) diff_norm; > return ratio >=3D lower_bound && ratio <=3D upper_bound; > } >=20=20=20=20 >=20 > I think it should follow the libsupport name convention of prepending > 'support_' on file name.=20 >=20 > The same name convention should be used on the exported interfaces > (support_*). >=20 > And I see that using time_* is confusing because the underlying type > is a 'timespec'. On 'include/time.h' the type is used on function > name, I think we should do the same here. >=20 >=20 Ok. > > diff --git a/support/cpuclock.h b/support/cpuclock.h > > new file mode 100644 > > index 0000000000..2505fbdcff > > --- /dev/null > > +++ b/support/cpuclock.h > > @@ -0,0 +1,30 @@ > > +/* Support functions for cpuclock tests. > > + Copyright (C) 2018-2020 Free Software Foundation, Inc. >=20 > This implementation is not based on old tests, I think the copyright > years should be only 2020. >=20 > > + 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 > > + . */ > > + > > +#ifndef SUPPORT_CPUCLOCK_H > > +#define SUPPORT_CPUCLOCK_H > > + > > +#include > > + > > +struct timespec time_normalize(struct timespec t); > > +struct timespec time_add(struct timespec a, struct timespec b); > > +struct timespec time_sub(struct timespec a, struct timespec b); > > +int percent_diff_check(struct timespec base, struct timespec dev, > > + float upper_bound, float lower_bound); > > + > > +#endif /* SUPPORT_CPUCLOCK_H */ >=20 > As before. >=20 > > diff --git a/time/tst-cpuclock1.c b/time/tst-cpuclock1.c > > index 0120906f23..4051de1646 100644 > > --- a/time/tst-cpuclock1.c > > +++ b/time/tst-cpuclock1.c > > @@ -26,6 +26,7 @@ > > #include > > #include > > #include > > +#include > >=20=20 > > /* This function is intended to rack up both user and system time. */ > > static void > > @@ -155,19 +156,11 @@ do_test (void) > > printf ("live PID %d after sleep =3D> %ju.%.9ju\n", > > child, (uintmax_t) after.tv_sec, (uintmax_t) after.tv_nsec); > >=20=20 > > - struct timespec diff =3D { .tv_sec =3D after.tv_sec - before.tv_sec, > > - .tv_nsec =3D after.tv_nsec - before.tv_nsec }; > > - if (diff.tv_nsec < 0) > > - { > > - --diff.tv_sec; > > - diff.tv_nsec +=3D 1000000000; > > - } > > - if (diff.tv_sec !=3D 0 > > - || diff.tv_nsec > 600000000 > > - || diff.tv_nsec < 100000000) > > + struct timespec diff =3D time_sub(after, before); > > + if (!percent_diff_check(sleeptime, diff, .3, 2)) > > { > > 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 =3D 1; > > } > >=20=20 > > @@ -194,19 +187,11 @@ do_test (void) > > } > > else > > { > > - struct timespec d =3D { .tv_sec =3D afterns.tv_sec - after.tv_s= ec, > > - .tv_nsec =3D afterns.tv_nsec - after.tv_n= sec }; > > - if (d.tv_nsec < 0) > > - { > > - --d.tv_sec; > > - d.tv_nsec +=3D 1000000000; > > - } > > - if (d.tv_sec > 0 > > - || d.tv_nsec < sleeptime.tv_nsec > > - || d.tv_nsec > sleeptime.tv_nsec * 2) > > + diff =3D time_sub(afterns, after); > > + if (!percent_diff_check(sleeptime, diff, .6, .34)) > > { > > 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 =3D 1; > > } > > } > > @@ -241,20 +226,14 @@ do_test (void) > > printf ("dead PID %d =3D> %ju.%.9ju\n", > > child, (uintmax_t) dead.tv_sec, (uintmax_t) dead.tv_nsec); > >=20=20 > > - diff.tv_sec =3D dead.tv_sec - after.tv_sec; > > - diff.tv_nsec =3D dead.tv_nsec - after.tv_nsec; > > - if (diff.tv_nsec < 0) > > - { > > - --diff.tv_sec; > > - diff.tv_nsec +=3D 1000000000; > > - } > > - if (diff.tv_sec !=3D 0 || diff.tv_nsec > 200000000) > > + diff =3D time_sub(dead, after); > > + sleeptime.tv_nsec =3D 100000000; > > + if (!percent_diff_check(sleeptime, diff, .6, .36)) > > { > > 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 =3D 1; > > } > > - > > /* Now reap the child and verify that its clock is no longer valid. = */ > > { > > int x; > >