| Message ID | or4k0db5iw.fsf@lxoliva.fsfla.org |
|---|---|
| State | New |
| Headers | show |
| Series | libstdc++: 60241.cc: tolerate slightly shorter aggregate sleep | expand |
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)