From c23acb9b5636e7b908fba03d6b3584d8f80dba6d Mon Sep 17 00:00:00 2001 From: Derek Mauro Date: Wed, 12 Apr 2023 13:26:48 -0700 Subject: Synchronization: Consolidate the logic for whether steady clocks are supported for relative timeouts PiperOrigin-RevId: 523789416 Change-Id: Ide4cfdcae9ea7bffca3355c80ea9c8833a9536e6 --- absl/synchronization/BUILD.bazel | 4 ++++ absl/synchronization/CMakeLists.txt | 4 ++++ absl/synchronization/internal/futex.h | 25 ---------------------- absl/synchronization/internal/futex_waiter.cc | 24 ++++++++++++++++++++- absl/synchronization/internal/futex_waiter.h | 5 +++++ absl/synchronization/internal/kernel_timeout.cc | 13 +++++------ absl/synchronization/internal/kernel_timeout.h | 5 +++++ .../internal/kernel_timeout_test.cc | 20 +++++++++++++++++ absl/synchronization/internal/pthread_waiter.cc | 8 +------ absl/synchronization/internal/sem_waiter.cc | 8 +------ absl/synchronization/internal/stdcpp_waiter.cc | 2 +- absl/synchronization/internal/waiter_test.cc | 20 +++++++++++++++++ 12 files changed, 91 insertions(+), 47 deletions(-) (limited to 'absl/synchronization') diff --git a/absl/synchronization/BUILD.bazel b/absl/synchronization/BUILD.bazel index b1c4e9a3..b6721e14 100644 --- a/absl/synchronization/BUILD.bazel +++ b/absl/synchronization/BUILD.bazel @@ -61,7 +61,9 @@ cc_library( "//absl/synchronization:__pkg__", ], deps = [ + "//absl/base", "//absl/base:config", + "//absl/base:core_headers", "//absl/base:raw_logging_internal", "//absl/time", ], @@ -75,6 +77,7 @@ cc_test( deps = [ ":kernel_timeout_internal", "//absl/base:config", + "//absl/random", "//absl/time", "@com_google_googletest//:gtest_main", ], @@ -348,6 +351,7 @@ cc_test( ":kernel_timeout_internal", ":synchronization", ":thread_pool", + "//absl/random", "//absl/time", "@com_google_googletest//:gtest_main", ], diff --git a/absl/synchronization/CMakeLists.txt b/absl/synchronization/CMakeLists.txt index 9926fb72..b6b3b27c 100644 --- a/absl/synchronization/CMakeLists.txt +++ b/absl/synchronization/CMakeLists.txt @@ -44,7 +44,9 @@ absl_cc_library( COPTS ${ABSL_DEFAULT_COPTS} DEPS + absl::base absl::config + absl::core_headers absl::raw_logging_internal absl::time ) @@ -59,6 +61,7 @@ absl_cc_test( DEPS absl::kernel_timeout_internal absl::config + absl::random_random absl::time GTest::gmock_main ) @@ -257,6 +260,7 @@ absl_cc_test( ${ABSL_TEST_COPTS} DEPS absl::kernel_timeout_internal + absl::random_random absl::synchronization absl::thread_pool absl::time diff --git a/absl/synchronization/internal/futex.h b/absl/synchronization/internal/futex.h index 8d973263..55078f14 100644 --- a/absl/synchronization/internal/futex.h +++ b/absl/synchronization/internal/futex.h @@ -83,31 +83,6 @@ namespace synchronization_internal { class FutexImpl { public: - // Atomically check that `*v == val`, and if it is, then sleep until the - // timeout `t` has been reached, or until woken by `Wake()`. - static int WaitUntil(std::atomic* v, int32_t val, - KernelTimeout t) { - // Monotonic waits are disabled for production builds because go/btm - // requires synchronized clocks. - // TODO(b/160682823): Find a way to enable this when BTM is not enabled - // since production builds don't always run on Borg. -#if defined(CLOCK_MONOTONIC) && !defined(__GOOGLE_GRTE_VERSION__) - constexpr bool kRelativeTimeoutSupported = true; -#else - constexpr bool kRelativeTimeoutSupported = false; -#endif - - if (!t.has_timeout()) { - return Wait(v, val); - } else if (kRelativeTimeoutSupported && t.is_relative_timeout()) { - auto rel_timespec = t.MakeRelativeTimespec(); - return WaitRelativeTimeout(v, val, &rel_timespec); - } else { - auto abs_timespec = t.MakeAbsTimespec(); - return WaitAbsoluteTimeout(v, val, &abs_timespec); - } - } - // Atomically check that `*v == val`, and if it is, then sleep until the until // woken by `Wake()`. static int Wait(std::atomic* v, int32_t val) { diff --git a/absl/synchronization/internal/futex_waiter.cc b/absl/synchronization/internal/futex_waiter.cc index 0a76bc9a..7c07fbe2 100644 --- a/absl/synchronization/internal/futex_waiter.cc +++ b/absl/synchronization/internal/futex_waiter.cc @@ -35,6 +35,28 @@ namespace synchronization_internal { constexpr char FutexWaiter::kName[]; #endif +int FutexWaiter::WaitUntil(std::atomic* v, int32_t val, + KernelTimeout t) { +#ifdef CLOCK_MONOTONIC + constexpr bool kHasClockMonotonic = true; +#else + constexpr bool kHasClockMonotonic = false; +#endif + + // We can't call Futex::WaitUntil() here because the prodkernel implementation + // does not know about KernelTimeout::SupportsSteadyClock(). + if (!t.has_timeout()) { + return Futex::Wait(v, val); + } else if (kHasClockMonotonic && KernelTimeout::SupportsSteadyClock() && + t.is_relative_timeout()) { + auto rel_timespec = t.MakeRelativeTimespec(); + return Futex::WaitRelativeTimeout(v, val, &rel_timespec); + } else { + auto abs_timespec = t.MakeAbsTimespec(); + return Futex::WaitAbsoluteTimeout(v, val, &abs_timespec); + } +} + bool FutexWaiter::Wait(KernelTimeout t) { // Loop until we can atomically decrement futex from a positive // value, waiting on a futex while we believe it is zero. @@ -54,7 +76,7 @@ bool FutexWaiter::Wait(KernelTimeout t) { } if (!first_pass) MaybeBecomeIdle(); - const int err = Futex::WaitUntil(&futex_, 0, t); + const int err = WaitUntil(&futex_, 0, t); if (err != 0) { if (err == -EINTR || err == -EWOULDBLOCK) { // Do nothing, the loop will retry. diff --git a/absl/synchronization/internal/futex_waiter.h b/absl/synchronization/internal/futex_waiter.h index a6a72095..11dfa93b 100644 --- a/absl/synchronization/internal/futex_waiter.h +++ b/absl/synchronization/internal/futex_waiter.h @@ -43,6 +43,11 @@ class FutexWaiter : public WaiterCrtp { static constexpr char kName[] = "FutexWaiter"; private: + // Atomically check that `*v == val`, and if it is, then sleep until the + // timeout `t` has been reached, or until woken by `Wake()`. + static int WaitUntil(std::atomic* v, int32_t val, + KernelTimeout t); + // Futexes are defined by specification to be 32-bits. // Thus std::atomic must be just an int32_t with lockfree methods. std::atomic futex_; diff --git a/absl/synchronization/internal/kernel_timeout.cc b/absl/synchronization/internal/kernel_timeout.cc index bfbf282f..48ea6287 100644 --- a/absl/synchronization/internal/kernel_timeout.cc +++ b/absl/synchronization/internal/kernel_timeout.cc @@ -21,9 +21,13 @@ #include #include // NOLINT(build/c++11) #include +#include +#include #include #include +#include "absl/base/attributes.h" +#include "absl/base/call_once.h" #include "absl/base/config.h" #include "absl/time/time.h" @@ -37,15 +41,12 @@ constexpr int64_t KernelTimeout::kMaxNanos; #endif int64_t KernelTimeout::SteadyClockNow() { -#ifdef __GOOGLE_GRTE_VERSION__ - // go/btm requires synchronized clocks, so we have to use the system - // clock. - return absl::GetCurrentTimeNanos(); -#else + if (!SupportsSteadyClock()) { + return absl::GetCurrentTimeNanos(); + } return std::chrono::duration_cast( std::chrono::steady_clock::now().time_since_epoch()) .count(); -#endif } KernelTimeout::KernelTimeout(absl::Time t) { diff --git a/absl/synchronization/internal/kernel_timeout.h b/absl/synchronization/internal/kernel_timeout.h index 4e361a6a..06404a75 100644 --- a/absl/synchronization/internal/kernel_timeout.h +++ b/absl/synchronization/internal/kernel_timeout.h @@ -128,6 +128,11 @@ class KernelTimeout { // case of a spurious wakeup. std::chrono::nanoseconds ToChronoDuration() const; + // Returns true if steady (aka monotonic) clocks are supported by the system. + // This method exists because go/btm requires synchronized clocks, and + // thus requires we use the system (aka walltime) clock. + static constexpr bool SupportsSteadyClock() { return true; } + private: // Returns the current time, expressed as a count of nanoseconds since the // epoch used by an arbitrary clock. The implementation tries to use a steady diff --git a/absl/synchronization/internal/kernel_timeout_test.cc b/absl/synchronization/internal/kernel_timeout_test.cc index c853c4bc..e3286f7f 100644 --- a/absl/synchronization/internal/kernel_timeout_test.cc +++ b/absl/synchronization/internal/kernel_timeout_test.cc @@ -14,14 +14,34 @@ #include "absl/synchronization/internal/kernel_timeout.h" +#include #include // NOLINT(build/c++11) #include +#include "absl/random/random.h" #include "gtest/gtest.h" #include "absl/base/config.h" #include "absl/time/clock.h" #include "absl/time/time.h" +// Test go/btm support by randomizing the value clock_gettime() for +// CLOCK_MONOTONIC. This works by overriding a weak symbol in glibc. +// We should be resistant to this randomization when !SupportsSteadyClock(). +#ifdef __GOOGLE_GRTE_VERSION__ +extern "C" int __clock_gettime(clockid_t c, struct timespec* ts); + +extern "C" int clock_gettime(clockid_t c, struct timespec* ts) { + if (c == CLOCK_MONOTONIC && + !absl::synchronization_internal::KernelTimeout::SupportsSteadyClock()) { + absl::SharedBitGen gen; + ts->tv_sec = absl::Uniform(gen, 0, 1'000'000'000); + ts->tv_nsec = absl::Uniform(gen, 0, 1'000'000'000); + return 0; + } + return __clock_gettime(c, ts); +} +#endif // __GOOGLE_GRTE_VERSION__ + namespace { #if defined(ABSL_HAVE_ADDRESS_SANITIZER) || \ diff --git a/absl/synchronization/internal/pthread_waiter.cc b/absl/synchronization/internal/pthread_waiter.cc index 8d90cc45..366aaea2 100644 --- a/absl/synchronization/internal/pthread_waiter.cc +++ b/absl/synchronization/internal/pthread_waiter.cc @@ -88,14 +88,8 @@ PthreadWaiter::PthreadWaiter() : waiter_count_(0), wakeup_count_(0) { // KernelTimeout requested. The return value is the same as the return // value of pthread_cond_timedwait(). int PthreadWaiter::TimedWait(KernelTimeout t) { -#ifndef __GOOGLE_GRTE_VERSION__ - constexpr bool kRelativeTimeoutSupported = true; -#else - constexpr bool kRelativeTimeoutSupported = false; -#endif - assert(t.has_timeout()); - if (kRelativeTimeoutSupported && t.is_relative_timeout()) { + if (KernelTimeout::SupportsSteadyClock() && t.is_relative_timeout()) { #ifdef ABSL_INTERNAL_HAS_PTHREAD_COND_TIMEDWAIT_RELATIVE_NP const auto rel_timeout = t.MakeRelativeTimespec(); return pthread_cond_timedwait_relative_np(&cv_, &mu_, &rel_timeout); diff --git a/absl/synchronization/internal/sem_waiter.cc b/absl/synchronization/internal/sem_waiter.cc index 7dd27fb7..3b3849bf 100644 --- a/absl/synchronization/internal/sem_waiter.cc +++ b/absl/synchronization/internal/sem_waiter.cc @@ -53,13 +53,7 @@ SemWaiter::SemWaiter() : wakeups_(0) { // KernelTimeout requested. The return value is the same as a call to the return // value to a call to sem_timedwait(). int SemWaiter::TimedWait(KernelTimeout t) { -#ifndef __GOOGLE_GRTE_VERSION__ - constexpr bool kRelativeTimeoutSupported = true; -#else - constexpr bool kRelativeTimeoutSupported = false; -#endif - - if (kRelativeTimeoutSupported && t.is_relative_timeout()) { + if (KernelTimeout::SupportsSteadyClock() && t.is_relative_timeout()) { #if defined(ABSL_INTERNAL_HAVE_SEM_CLOCKWAIT) && defined(CLOCK_MONOTONIC) const auto abs_clock_timeout = t.MakeClockAbsoluteTimespec(CLOCK_MONOTONIC); return sem_clockwait(&sem_, CLOCK_MONOTONIC, &abs_clock_timeout); diff --git a/absl/synchronization/internal/stdcpp_waiter.cc b/absl/synchronization/internal/stdcpp_waiter.cc index 8b5d1df4..355718a7 100644 --- a/absl/synchronization/internal/stdcpp_waiter.cc +++ b/absl/synchronization/internal/stdcpp_waiter.cc @@ -50,7 +50,7 @@ bool StdcppWaiter::Wait(KernelTimeout t) { if (!t.has_timeout()) { cv_.wait(lock); } else { - auto wait_result = t.is_relative_timeout() + auto wait_result = t.SupportsSteadyClock() && t.is_relative_timeout() ? cv_.wait_for(lock, t.ToChronoDuration()) : cv_.wait_until(lock, t.ToChronoTimePoint()); if (wait_result == std::cv_status::timeout) { diff --git a/absl/synchronization/internal/waiter_test.cc b/absl/synchronization/internal/waiter_test.cc index 66b255de..57a1a3da 100644 --- a/absl/synchronization/internal/waiter_test.cc +++ b/absl/synchronization/internal/waiter_test.cc @@ -14,9 +14,11 @@ #include "absl/synchronization/internal/waiter.h" +#include #include #include +#include "absl/random/random.h" #include "absl/synchronization/internal/create_thread_identity.h" #include "absl/synchronization/internal/futex_waiter.h" #include "absl/synchronization/internal/kernel_timeout.h" @@ -29,6 +31,24 @@ #include "absl/time/time.h" #include "gtest/gtest.h" +// Test go/btm support by randomizing the value clock_gettime() for +// CLOCK_MONOTONIC. This works by overriding a weak symbol in glibc. +// We should be resistant to this randomization when !SupportsSteadyClock(). +#ifdef __GOOGLE_GRTE_VERSION__ +extern "C" int __clock_gettime(clockid_t c, struct timespec* ts); + +extern "C" int clock_gettime(clockid_t c, struct timespec* ts) { + if (c == CLOCK_MONOTONIC && + !absl::synchronization_internal::KernelTimeout::SupportsSteadyClock()) { + absl::SharedBitGen gen; + ts->tv_sec = absl::Uniform(gen, 0, 1'000'000'000); + ts->tv_nsec = absl::Uniform(gen, 0, 1'000'000'000); + return 0; + } + return __clock_gettime(c, ts); +} +#endif // __GOOGLE_GRTE_VERSION__ + namespace { TEST(Waiter, PrintPlatformImplementation) { -- cgit v1.2.3