Message ID | or4k0db5iw.fsf@lxoliva.fsfla.org |
---|---|
State | Dropped |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 7BF1C38356A0 for <patchwork@sourceware.org>; Wed, 22 Jun 2022 06:02:22 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 7BF1C38356A0 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1655877742; bh=795z4Xl5tKqDeosvyUw6XCjCtqYy515TqfkFlFLRAIs=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=IuF5BKrDw2hyiqOuDQCJg4XMTxX/OpNsJifA2Voc2l77ewniU7BJytr4eAVOqA+ck RXNvH8GYwVR0XMoyCoTUPc66aKCWro8Q3E0pyxNNGTy07Hak/ymf6nxzqKvZZ55K1a Sb2chCdfVwvhQxqbcEoBbvs5ZZbEESyeWNpfHcPg= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from rock.gnat.com (rock.gnat.com [IPv6:2620:20:4000:0:a9e:1ff:fe9b:1d1]) by sourceware.org (Postfix) with ESMTPS id 6BE65386C585; Wed, 22 Jun 2022 06:01:53 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 6BE65386C585 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 3528F1160FA; Wed, 22 Jun 2022 02:01:53 -0400 (EDT) X-Virus-Scanned: Debian amavisd-new at gnat.com Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id lqHpn2I7AAYa; Wed, 22 Jun 2022 02:01:53 -0400 (EDT) Received: from free.home (tron.gnat.com [IPv6:2620:20:4000:0:46a8:42ff:fe0e:e294]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by rock.gnat.com (Postfix) with ESMTPS id F37901160F1; Wed, 22 Jun 2022 02:01:52 -0400 (EDT) Received: from livre (livre.home [172.31.160.2]) by free.home (8.15.2/8.15.2) with ESMTPS id 25M61hfR723397 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 22 Jun 2022 03:01:44 -0300 To: gcc-patches@gcc.gnu.org, libstdc++@gcc.gnu.org Subject: [PATCH] libstdc++: 60241.cc: tolerate slightly shorter aggregate sleep Organization: Free thinker, does not speak for AdaCore Date: Wed, 22 Jun 2022 03:01:43 -0300 Message-ID: <or4k0db5iw.fsf@lxoliva.fsfla.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Scanned-By: MIMEDefang 2.84 X-Spam-Status: No, score=-12.3 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> From: Alexandre Oliva via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: Alexandre Oliva <oliva@adacore.com> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
libstdc++: 60241.cc: tolerate slightly shorter aggregate sleep
|
|
Commit Message
Alexandre Oliva
June 22, 2022, 6:01 a.m. UTC
On rtems under qemu, the frequently-interrupted nanosleep ends up sleeping shorter than expected, by a margin of less than 0,3%. I figured failing the library test over a system (emulator?) bug is undesirable, so I put in some tolerance for the drift. Regstrapped on x86_64-linux-gnu, also tested with a cross to aarch64-rtems6. Ok to install? PS: I see nothing wrong with the implementation of clock_nanosleep (used by nanosleep) on rtems6 that could cause it to wake up too early. I suspect some artifact of the emulation environment. for libstdc++-v3/ChangeLog * testsuite/30_threads/this_thread/60421.cc: Tolerate a slightly early wakeup. --- .../testsuite/30_threads/this_thread/60421.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
Comments
On 22/06/2022 08:01, Alexandre Oliva via Gcc-patches wrote: > > On rtems under qemu, the frequently-interrupted nanosleep ends up > sleeping shorter than expected, by a margin of less than 0,3%. > > I figured failing the library test over a system (emulator?) bug is > undesirable, so I put in some tolerance for the drift. > > Regstrapped on x86_64-linux-gnu, also tested with a cross to > aarch64-rtems6. Ok to install? > > PS: I see nothing wrong with the implementation of clock_nanosleep (used > by nanosleep) on rtems6 that could cause it to wake up too early. I > suspect some artifact of the emulation environment. > > > for libstdc++-v3/ChangeLog > > * testsuite/30_threads/this_thread/60421.cc: Tolerate a > slightly early wakeup. > --- > .../testsuite/30_threads/this_thread/60421.cc | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/libstdc++-v3/testsuite/30_threads/this_thread/60421.cc b/libstdc++-v3/testsuite/30_threads/this_thread/60421.cc > index 12dbeba1cc492..f3a5af453c4ad 100644 > --- a/libstdc++-v3/testsuite/30_threads/this_thread/60421.cc > +++ b/libstdc++-v3/testsuite/30_threads/this_thread/60421.cc > @@ -51,9 +51,10 @@ test02() > std::thread t([&result, &sleeping] { > auto start = std::chrono::system_clock::now(); > auto time = std::chrono::seconds(3); > + auto tolerance = std::chrono::milliseconds(10); > sleeping = true; > std::this_thread::sleep_for(time); > - result = std::chrono::system_clock::now() >= (start + time); > + result = std::chrono::system_clock::now() + tolerance >= (start + time); > sleeping = false; > }); > while (!sleeping) This looks like a bug in RTEMS or the BSP for the test platform. I would first investigate this and then change the test which looks all right to me.
On 22/06/2022 08:22, Sebastian Huber wrote: > On 22/06/2022 08:01, Alexandre Oliva via Gcc-patches wrote: >> >> On rtems under qemu, the frequently-interrupted nanosleep ends up >> sleeping shorter than expected, by a margin of less than 0,3%. >> >> I figured failing the library test over a system (emulator?) bug is >> undesirable, so I put in some tolerance for the drift. >> >> Regstrapped on x86_64-linux-gnu, also tested with a cross to >> aarch64-rtems6. Ok to install? >> >> PS: I see nothing wrong with the implementation of clock_nanosleep (used >> by nanosleep) on rtems6 that could cause it to wake up too early. I >> suspect some artifact of the emulation environment. >> >> >> for libstdc++-v3/ChangeLog >> >> * testsuite/30_threads/this_thread/60421.cc: Tolerate a >> slightly early wakeup. >> --- >> .../testsuite/30_threads/this_thread/60421.cc | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/libstdc++-v3/testsuite/30_threads/this_thread/60421.cc >> b/libstdc++-v3/testsuite/30_threads/this_thread/60421.cc >> index 12dbeba1cc492..f3a5af453c4ad 100644 >> --- a/libstdc++-v3/testsuite/30_threads/this_thread/60421.cc >> +++ b/libstdc++-v3/testsuite/30_threads/this_thread/60421.cc >> @@ -51,9 +51,10 @@ test02() >> std::thread t([&result, &sleeping] { >> auto start = std::chrono::system_clock::now(); >> auto time = std::chrono::seconds(3); >> + auto tolerance = std::chrono::milliseconds(10); >> sleeping = true; >> std::this_thread::sleep_for(time); >> - result = std::chrono::system_clock::now() >= (start + time); >> + result = std::chrono::system_clock::now() + tolerance >= (start + >> time); >> sleeping = false; >> }); >> while (!sleeping) > > This looks like a bug in RTEMS or the BSP for the test platform. I would > first investigate this and then change the test which looks all right to > me. This is a problem in RTEMS. RTEMS uses the FreeBSD timecounters to maintain CLOCK_REALTIME and provides two methods to get the time in a coarse and fine resolution. The std::chrono::system_clock::now() uses the fine resolution (higher overhead). The clock_nanosleep() uses the coarse resolution which may give a time before now().
On Jun 22, 2022, Sebastian Huber <sebastian.huber@embedded-brains.de> wrote:
> The clock_nanosleep() uses the coarse resolution
Thanks for looking into this. So, is it missing a rounding-up to ensure
the sleep time is >= the requested time, or is it even more elaborate
than that?
On Jun 22, 2022, Sebastian Huber <sebastian.huber@embedded-brains.de> wrote: > The clock_nanosleep() uses the coarse resolution which may give a time > before now(). Uhh, sorry, hit send too early. I also meant to ask whether you'd like me to file an RTEMS ticket about this issue.
On 23/06/2022 02:19, Alexandre Oliva wrote: > On Jun 22, 2022, Sebastian Huber<sebastian.huber@embedded-brains.de> wrote: > >> The clock_nanosleep() uses the coarse resolution which may give a time >> before now(). > Uhh, sorry, hit send too early. > > I also meant to ask whether you'd like me to file an RTEMS ticket about > this issue. I already created a ticket for this and work on it: http://devel.rtems.org/ticket/4669
On 23/06/2022 08:44, Sebastian Huber wrote: > On 23/06/2022 02:19, Alexandre Oliva wrote: >> On Jun 22, 2022, Sebastian Huber<sebastian.huber@embedded-brains.de> >> wrote: >> >>> The clock_nanosleep() uses the coarse resolution which may give a time >>> before now(). >> Uhh, sorry, hit send too early. >> >> I also meant to ask whether you'd like me to file an RTEMS ticket about >> this issue. > > I already created a ticket for this and work on it: > > http://devel.rtems.org/ticket/4669 This problem should be fixed now in the RTEMS master branch. I had to adjust the test case so that it works in a system with only one processor: while (!sleeping) { // Wait for the thread to start sleeping. std::this_thread::yield(); }
On Jun 23, 2022, Sebastian Huber <sebastian.huber@embedded-brains.de> wrote: > On 23/06/2022 08:44, Sebastian Huber wrote: >> http://devel.rtems.org/ticket/4669 Thanks! > This problem should be fixed now in the RTEMS master branch. Double thanks! I've applied the patch, and I haven't seen the fails any more. It's a little too soon to confirm it fixed, but the patch makes plenty of sense. > I had to adjust the test case so that it works in a system with only > one processor: *nod*, I ran into that myself, and IIRC I've pushed an equivalent fix earlier this week. Anyway... I was considering this xfail patch before, and I wonder if it would still be appropriate to install something like it, narrowed down to rtems < 6.1, or if it would be better to let it fail noisily so that people look it up, find the fix proper and merge it. libstdc++: xfail nanosleep tests on rtems Since it has been determined that nanosleep may return slightly too early on RTEMS, due to clock resolution differences, expect 30_thread/this_thread tests that have detected too-early wakeups to fail on RTEMS targets. for libstdc++-v3/ChangeLog * testsuite/30_threads/this_thread/60421.cc: xfail on RTEMS. --- .../testsuite/30_threads/this_thread/60421.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/libstdc++-v3/testsuite/30_threads/this_thread/60421.cc b/libstdc++-v3/testsuite/30_threads/this_thread/60421.cc index 12dbeba1cc492..4d86e0df20de4 100644 --- a/libstdc++-v3/testsuite/30_threads/this_thread/60421.cc +++ b/libstdc++-v3/testsuite/30_threads/this_thread/60421.cc @@ -23,6 +23,7 @@ // { dg-require-gthreads "" } // { dg-require-time "" } // { dg-require-sleep "" } +// { dg-xfail-if "nanosleep may wake up too early" { *-*-rtems* } } #include <thread> #include <chrono>
On 23/06/2022 13:33, Alexandre Oliva wrote: > Anyway... I was considering this xfail patch before, and I wonder if it > would still be appropriate to install something like it, narrowed down > to rtems < 6.1, or if it would be better to let it fail noisily so that > people look it up, find the fix proper and merge it. I would not make it an xfail. It is now likely fixed and if someone uses a broken RTEMS version getting an error message would be nice.
diff --git a/libstdc++-v3/testsuite/30_threads/this_thread/60421.cc b/libstdc++-v3/testsuite/30_threads/this_thread/60421.cc index 12dbeba1cc492..f3a5af453c4ad 100644 --- a/libstdc++-v3/testsuite/30_threads/this_thread/60421.cc +++ b/libstdc++-v3/testsuite/30_threads/this_thread/60421.cc @@ -51,9 +51,10 @@ test02() std::thread t([&result, &sleeping] { auto start = std::chrono::system_clock::now(); auto time = std::chrono::seconds(3); + auto tolerance = std::chrono::milliseconds(10); sleeping = true; std::this_thread::sleep_for(time); - result = std::chrono::system_clock::now() >= (start + time); + result = std::chrono::system_clock::now() + tolerance >= (start + time); sleeping = false; }); while (!sleeping)