From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 118219 invoked by alias); 18 Feb 2020 12:44:18 -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 118207 invoked by uid 89); 18 Feb 2020 12:44:17 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-22.0 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy=065, 1000000000 X-HELO: mail-qt1-f193.google.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=to:references:from:autocrypt:subject:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=6W0p8Dg/rX92fPULB1pt/4UncMUvGyW6vedsb/rpqU8=; b=pxuDW7GeY+v73R4b5d1UpCAfOAwJRul/vHKWDiZGVLvaNZppI4nh2B5f3eJiYUpGs+ mNSLI137/69LUr2ILjSh56Hg8fBahT0rQv/fF7eeOllu1TVVPwLAyNa5J3eU/y78v9WN F+Y9C2BA9WdHQZ3Prkx8MFWCM/hcQmIfEw0z/cz8KQwr4mBgaIaIkCDS28JIMOJbLoS0 Tl5cuz5/EyjG+he8nEFqJJvPxmV0jNUC1+zhBZd6Im58nFRBBmm2OImpz7n6iuI/mhYe JGwfhfIvGXMopJzCsMmX+/rUbG+72lHYXBg/HqT7HqPcb/wYMrkg0JXVWR3Az6tIQcBg wouQ== Return-Path: To: libc-alpha@sourceware.org References: <20200206144819.19046-1-lamm@linux.ibm.com> From: Adhemerval Zanella Subject: Re: [PATCH v2] Fix time/tst-cpuclock1 intermitent failures Message-ID: Date: Tue, 18 Feb 2020 12:44:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1 MIME-Version: 1.0 In-Reply-To: <20200206144819.19046-1-lamm@linux.ibm.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-SW-Source: 2020-02/txt/msg00780.txt.bz2 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. > > 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, > > Please Carlos see if this is what you asked. > > 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. > > Also as I found the spent time deviation completely asymmetrical > I choose to separate the values in upper and lower bounds. > > changes from V2: > - Add support functions None of the files follow the code and style guideline [1]. [1] https://sourceware.org/glibc/wiki/Style_and_Conventions > > 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 > > diff --git a/support/Makefile b/support/Makefile > index 3325feb790..b308fe0856 100644 > --- a/support/Makefile > +++ b/support/Makefile > @@ -31,6 +31,7 @@ libsupport-routines = \ > 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 = (t.tv_nsec / T_1s); > + t.tv_sec += diff; > + t.tv_nsec += -(diff * T_1s); > + return t; > +} > + > +struct timespec time_add(struct timespec a, struct timespec b) { > + struct timespec s = {.tv_sec = a.tv_sec + b.tv_sec, > + .tv_nsec = a.tv_nsec + b.tv_nsec}; > + return time_normalize(s); > +} > + > +struct timespec time_sub(struct timespec a, struct timespec b) { > + struct timespec d = {.tv_sec = a.tv_sec - b.tv_sec, > + .tv_nsec = a.tv_nsec - b.tv_nsec}; > + return time_normalize(d); > +} 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? > + > +int percent_diff_check(struct timespec base, struct timespec dev, > + float upper_bound, float lower_bound) { > + double timea = base.tv_sec + base.tv_nsec / T_1s; > + double timeb = dev.tv_sec + dev.tv_nsec / T_1s; > + double diff = (timea - timeb) / (0.5 * (timea + timeb)); > + return (diff >= -upper_bound && diff <= lower_bound ); > +} > + Please add some description of what this function do. And I think it would be better to normalize to nanoseconds instead of seconds to avoid floating-point cancellation due the range difference. Something like: /* Return true if the DIFF time is within the ratio [upper_bound, lower_bound] of DIFF time, or false otherwise. For instance: struct timespec diff = { 3, 94956 }; struct timespec base = { 4, 0 }; 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). 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 >= lower_bound); uint64_t base_norm = base.tv_sec * TIMESPEC_HZ + base.tv_nsec; uint64_t diff_norm = diff.tv_sec * TIMESPEC_HZ + diff.tv_nsec; double ratio = (double) base_norm / (double) diff_norm; return ratio >= lower_bound && ratio <= upper_bound; } I think it should follow the libsupport name convention of prepending 'support_' on file name. The same name convention should be used on the exported interfaces (support_*). 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. > 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. This implementation is not based on old tests, I think the copyright years should be only 2020. > + 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 */ As before. > 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 > > /* 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 => %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) > + struct timespec diff = 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 = 1; > } > > @@ -194,19 +187,11 @@ 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) > + diff = 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 = 1; > } > } > @@ -241,20 +226,14 @@ 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) > + diff = time_sub(dead, after); > + sleeptime.tv_nsec = 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 = 1; > } > - > /* Now reap the child and verify that its clock is no longer valid. */ > { > int x; >