From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id 84E773844047 for ; Fri, 11 Sep 2020 14:41:08 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 84E773844047 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-363-iIFpgYQKMIaPfPGA04o6Gg-1; Fri, 11 Sep 2020 10:41:02 -0400 X-MC-Unique: iIFpgYQKMIaPfPGA04o6Gg-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 47BA48030BA; Fri, 11 Sep 2020 14:41:00 +0000 (UTC) Received: from localhost (unknown [10.33.37.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id E8F8260C04; Fri, 11 Sep 2020 14:40:59 +0000 (UTC) Date: Fri, 11 Sep 2020 15:40:59 +0100 From: Jonathan Wakely To: Mike Crowe Cc: libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org Subject: Re: [PATCH v5 6/8] libstdc++ atomic_futex: Avoid rounding errors in std::future::wait_* [PR91486] Message-ID: <20200911144059.GU6061@redhat.com> References: MIME-Version: 1.0 In-Reply-To: X-Clacks-Overhead: GNU Terry Pratchett X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Mimecast-Spam-Score: 0.002 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline X-Spam-Status: No, score=-13.7 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H5, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libstdc++@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libstdc++ mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 11 Sep 2020 14:41:10 -0000 On 29/05/20 07:17 +0100, Mike Crowe via Libstdc++ wrote: >Convert the specified duration to the target clock's duration type >before adding it to the current time in >__atomic_futex_unsigned::_M_load_when_equal_for and >_M_load_when_equal_until. This removes the risk of the timeout being >rounded down to the current time resulting in there being no wait at >all when the duration type lacks sufficient precision to hold the >steady_clock current time. > >Rather than using the style of fix from PR68519, let's expose the C++17 >std::chrono::ceil function as std::chrono::__detail::ceil so that it can >be used in code compiled with earlier standards versions and simplify >the fix. This was suggested by John Salmon in >https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91486#c5 . > >This problem has become considerably less likely to trigger since I >switched the __atomic__futex_unsigned::__clock_t reference clock from >system_clock to steady_clock and added the loop, but the consequences of >triggering it have changed too. > >By my calculations it takes just over 194 days from the epoch for the >current time not to be representable in a float. This means that >system_clock is always subject to the problem (with the standard 1970 >epoch) whereas steady_clock with float duration only runs out of >resolution machine has been running for that long (assuming the Linux >implementation of CLOCK_MONOTONIC.) > >The recently-added loop in >__atomic_futex_unsigned::_M_load_when_equal_until turns this scenario >into a busy wait. > >Unfortunately the combination of both of these things means that it's >not possible to write a test case for this occurring in >_M_load_when_equal_until as it stands. > > * libstdc++-v3/include/std/chrono: (__detail::ceil) Move > implementation of std::chrono::ceil into private namespace so > that it's available to pre-C++17 code. > > * libstdc++-v3/include/bits/atomic_futex.h: > (__atomic_futex_unsigned::_M_load_when_equal_for, > __atomic_futex_unsigned::_M_load_when_equal_until): Use > __detail::ceil to convert delta to the reference clock > duration type to avoid resolution problems > > * libstdc++-v3/testsuite/30_threads/async/async.cc: (test_pr91486): > New test for __atomic_futex_unsigned::_M_load_when_equal_for. >--- > libstdc++-v3/include/bits/atomic_futex.h | 6 +++-- > libstdc++-v3/include/std/chrono | 19 +++++++++++++---- > libstdc++-v3/testsuite/30_threads/async/async.cc | 15 +++++++++++++- > 3 files changed, 34 insertions(+), 6 deletions(-) > >diff --git a/libstdc++-v3/include/bits/atomic_futex.h b/libstdc++-v3/include/bits/atomic_futex.h >index 5f95ade..aa137a7 100644 >--- a/libstdc++-v3/include/bits/atomic_futex.h >+++ b/libstdc++-v3/include/bits/atomic_futex.h >@@ -219,8 +219,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > _M_load_when_equal_for(unsigned __val, memory_order __mo, > const chrono::duration<_Rep, _Period>& __rtime) > { >+ using __dur = typename __clock_t::duration; > return _M_load_when_equal_until(__val, __mo, >- __clock_t::now() + __rtime); >+ __clock_t::now() + chrono::__detail::ceil<__dur>(__rtime)); > } > > // Returns false iff a timeout occurred. >@@ -233,7 +234,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > do { > const __clock_t::time_point __s_entry = __clock_t::now(); > const auto __delta = __atime - __c_entry; >- const auto __s_atime = __s_entry + __delta; >+ const auto __s_atime = __s_entry + >+ chrono::__detail::ceil<_Duration>(__delta); > if (_M_load_when_equal_until(__val, __mo, __s_atime)) > return true; > __c_entry = _Clock::now(); >diff --git a/libstdc++-v3/include/std/chrono b/libstdc++-v3/include/std/chrono >index 6d78f32..4257c7c 100644 >--- a/libstdc++-v3/include/std/chrono >+++ b/libstdc++-v3/include/std/chrono >@@ -299,6 +299,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > #endif > #endif // C++20 > >+ // We want to use ceil even when compiling for earlier standards versions >+ namespace __detail >+ { >+ template >+ constexpr __enable_if_is_duration<_ToDur> >+ ceil(const duration<_Rep, _Period>& __d) >+ { >+ auto __to = chrono::duration_cast<_ToDur>(__d); >+ if (__to < __d) >+ return __to + _ToDur{1}; >+ return __to; >+ } >+ } >+ > #if __cplusplus >= 201703L > # define __cpp_lib_chrono 201611 > >@@ -316,10 +330,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > constexpr __enable_if_is_duration<_ToDur> > ceil(const duration<_Rep, _Period>& __d) > { >- auto __to = chrono::duration_cast<_ToDur>(__d); >- if (__to < __d) >- return __to + _ToDur{1}; >- return __to; >+ return __detail::ceil<_ToDur>(__d); This isn't valid in C++11, a constexpr function needs to be just a return statement. Fix incoming ...