[PATCH] libstdc++: 60241.cc: tolerate slightly shorter aggregate sleep

[PATCH] libstdc++: 60241.cc: tolerate slightly shorter aggregate sleep

[Alexandre Oliva]

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)


-- 
Alexandre Oliva, happy hacker                https://FSFLA.org/blogs/lxo/
   Free Software Activist                       GNU Toolchain Engineer
Disinformation flourishes because many people care deeply about injustice
but very few check the facts.  Ask me about <https://stallmansupport.org>

Re: [PATCH] libstdc++: 60241.cc: tolerate slightly shorter aggregate sleep

[Sebastian Huber]

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.

-- 
embedded brains GmbH
Herr Sebastian HUBER
Dornierstr. 4
82178 Puchheim
Germany
email: sebastian.huber@embedded-brains.de
phone: +49-89-18 94 741 - 16
fax:   +49-89-18 94 741 - 08

Registergericht: Amtsgericht München
Registernummer: HRB 157899
Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
Unsere Datenschutzerklärung finden Sie hier:
https://embedded-brains.de/datenschutzerklaerung/

Re: [PATCH] libstdc++: 60241.cc: tolerate slightly shorter aggregate sleep

[Sebastian Huber]

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().

-- 
embedded brains GmbH
Herr Sebastian HUBER
Dornierstr. 4
82178 Puchheim
Germany
email: sebastian.huber@embedded-brains.de
phone: +49-89-18 94 741 - 16
fax:   +49-89-18 94 741 - 08

Registergericht: Amtsgericht München
Registernummer: HRB 157899
Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
Unsere Datenschutzerklärung finden Sie hier:
https://embedded-brains.de/datenschutzerklaerung/

Re: [PATCH] libstdc++: 60241.cc: tolerate slightly shorter aggregate sleep

[Alexandre Oliva]

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?

-- 
Alexandre Oliva, happy hacker                https://FSFLA.org/blogs/lxo/
   Free Software Activist                       GNU Toolchain Engineer
Disinformation flourishes because many people care deeply about injustice
but very few check the facts.  Ask me about <https://stallmansupport.org>

Re: [PATCH] libstdc++: 60241.cc: tolerate slightly shorter aggregate sleep

[Alexandre Oliva]

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.

-- 
Alexandre Oliva, happy hacker                https://FSFLA.org/blogs/lxo/
   Free Software Activist                       GNU Toolchain Engineer
Disinformation flourishes because many people care deeply about injustice
but very few check the facts.  Ask me about <https://stallmansupport.org>

Re: [PATCH] libstdc++: 60241.cc: tolerate slightly shorter aggregate sleep

[Sebastian Huber]

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

-- 
embedded brains GmbH
Herr Sebastian HUBER
Dornierstr. 4
82178 Puchheim
Germany
email: sebastian.huber@embedded-brains.de
phone: +49-89-18 94 741 - 16
fax:   +49-89-18 94 741 - 08

Registergericht: Amtsgericht München
Registernummer: HRB 157899
Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
Unsere Datenschutzerklärung finden Sie hier:
https://embedded-brains.de/datenschutzerklaerung/

Re: [PATCH] libstdc++: 60241.cc: tolerate slightly shorter aggregate sleep

[Sebastian Huber]

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();
   }

-- 
embedded brains GmbH
Herr Sebastian HUBER
Dornierstr. 4
82178 Puchheim
Germany
email: sebastian.huber@embedded-brains.de
phone: +49-89-18 94 741 - 16
fax:   +49-89-18 94 741 - 08

Registergericht: Amtsgericht München
Registernummer: HRB 157899
Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
Unsere Datenschutzerklärung finden Sie hier:
https://embedded-brains.de/datenschutzerklaerung/

Re: [PATCH] libstdc++: 60241.cc: tolerate slightly shorter aggregate sleep

[Alexandre Oliva]

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>


-- 
Alexandre Oliva, happy hacker                https://FSFLA.org/blogs/lxo/
   Free Software Activist                       GNU Toolchain Engineer
Disinformation flourishes because many people care deeply about injustice
but very few check the facts.  Ask me about <https://stallmansupport.org>

Re: [PATCH] libstdc++: 60241.cc: tolerate slightly shorter aggregate sleep

[Sebastian Huber]

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.

-- 
embedded brains GmbH
Herr Sebastian HUBER
Dornierstr. 4
82178 Puchheim
Germany
email: sebastian.huber@embedded-brains.de
phone: +49-89-18 94 741 - 16
fax:   +49-89-18 94 741 - 08

Registergericht: Amtsgericht München
Registernummer: HRB 157899
Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
Unsere Datenschutzerklärung finden Sie hier:
https://embedded-brains.de/datenschutzerklaerung/