This is an automated email from the ASF dual-hosted git repository. joemcdonnell pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/impala.git
commit 4114fe8db6ec80b2e1679e946555f91ab7043f2e Author: Andrew Sherman <[email protected]> AuthorDate: Thu Dec 14 10:26:57 2023 -0800 IMPALA-12632: Use Atomics for CpuUsageRatio counters Now that IMPALA-12614 is fixed we see another data race in TSAN builds. Fix this by using the same strategy as IMPALA-12614, and use AtomicInt32 for the CpuUsageRatios values. Testing: TSAN tests cover this case. Change-Id: I373cb8ae1a45e5ec07318ccb5870e65efc906cca Reviewed-on: http://gerrit.cloudera.org:8080/20798 Reviewed-by: Impala Public Jenkins <[email protected]> Tested-by: Impala Public Jenkins <[email protected]> --- be/src/runtime/query-state.cc | 6 +++--- be/src/util/system-state-info-test.cc | 16 ++++++++-------- be/src/util/system-state-info.cc | 8 +++++--- be/src/util/system-state-info.h | 6 +++--- 4 files changed, 19 insertions(+), 17 deletions(-) diff --git a/be/src/runtime/query-state.cc b/be/src/runtime/query-state.cc index dafc90b3d..4417aa589 100644 --- a/be/src/runtime/query-state.cc +++ b/be/src/runtime/query-state.cc @@ -220,15 +220,15 @@ Status QueryState::Init(const ExecQueryFInstancesRequestPB* exec_rpc_params, SystemStateInfo* system_state_info = exec_env->system_state_info(); host_profile_->AddSamplingTimeSeriesCounter( "HostCpuUserPercentage", TUnit::BASIS_POINTS, [system_state_info] () { - return system_state_info->GetCpuUsageRatios().user; + return system_state_info->GetCpuUsageRatios().user.Load(); }); host_profile_->AddSamplingTimeSeriesCounter( "HostCpuSysPercentage", TUnit::BASIS_POINTS, [system_state_info] () { - return system_state_info->GetCpuUsageRatios().system; + return system_state_info->GetCpuUsageRatios().system.Load(); }); host_profile_->AddSamplingTimeSeriesCounter( "HostCpuIoWaitPercentage", TUnit::BASIS_POINTS, [system_state_info] () { - return system_state_info->GetCpuUsageRatios().iowait; + return system_state_info->GetCpuUsageRatios().iowait.Load(); }); // Add network usage host_profile_->AddSamplingTimeSeriesCounter( diff --git a/be/src/util/system-state-info-test.cc b/be/src/util/system-state-info-test.cc index 78735831f..f639ea36e 100644 --- a/be/src/util/system-state-info-test.cc +++ b/be/src/util/system-state-info-test.cc @@ -32,7 +32,7 @@ class SystemStateInfoTest : public testing::Test { TEST_F(SystemStateInfoTest, FirstCallReturnsZero) { const SystemStateInfo::CpuUsageRatios& r = info.GetCpuUsageRatios(); - EXPECT_EQ(0, r.user + r.system + r.iowait); + EXPECT_EQ(0, r.user.Load() + r.system.Load() + r.iowait.Load()); const SystemStateInfo::NetworkUsage& n = info.GetNetworkUsage(); EXPECT_EQ(0, n.rx_rate.Load() + n.tx_rate.Load()); @@ -144,9 +144,9 @@ TEST_F(SystemStateInfoTest, ComputeCpuRatiosIntOverflow) { info.ReadProcStatString("cpu 100953598 534 18552882 6318826150 4119619 0 0 0 0 0"); info.ComputeCpuRatios(); const SystemStateInfo::CpuUsageRatios& r = info.GetCpuUsageRatios(); - EXPECT_EQ(r.user, 1649); - EXPECT_EQ(r.system, 304); - EXPECT_EQ(r.iowait, 0); + EXPECT_EQ(r.user.Load(), 1649); + EXPECT_EQ(r.system.Load(), 304); + EXPECT_EQ(r.iowait.Load(), 0); } // Smoke test for the public interface. @@ -158,7 +158,7 @@ TEST_F(SystemStateInfoTest, GetCpuUsageRatios) { SleepForMs(200); info.CaptureSystemStateSnapshot(); const SystemStateInfo::CpuUsageRatios& r = info.GetCpuUsageRatios(); - EXPECT_GT(r.user + r.system + r.iowait, 0); + EXPECT_GT(r.user.Load() + r.system.Load() + r.iowait.Load(), 0); } running.Store(false); t.join(); @@ -170,9 +170,9 @@ TEST_F(SystemStateInfoTest, ComputeCpuRatios) { info.ReadProcStatString("cpu 30 30 20 70 100 0 0 0 0 0"); info.ComputeCpuRatios(); const SystemStateInfo::CpuUsageRatios& r = info.GetCpuUsageRatios(); - EXPECT_EQ(r.user, 5000); - EXPECT_EQ(r.system, 5000); - EXPECT_EQ(r.iowait, 0); + EXPECT_EQ(r.user.Load(), 5000); + EXPECT_EQ(r.system.Load(), 5000); + EXPECT_EQ(r.iowait.Load(), 0); } // Tests the computation logic for network usage. diff --git a/be/src/util/system-state-info.cc b/be/src/util/system-state-info.cc index 931717d28..387812d36 100644 --- a/be/src/util/system-state-info.cc +++ b/be/src/util/system-state-info.cc @@ -146,9 +146,11 @@ void SystemStateInfo::ComputeCpuRatios() { DCHECK_GT(total_tics, 0); // Convert each ratio to basis points. const int BASIS_MAX = 10000; - cpu_ratios_.user = ((cur[CPU_USER] - old[CPU_USER]) * BASIS_MAX) / total_tics; - cpu_ratios_.system = ((cur[CPU_SYSTEM] - old[CPU_SYSTEM]) * BASIS_MAX) / total_tics; - cpu_ratios_.iowait = ((cur[CPU_IOWAIT] - old[CPU_IOWAIT]) * BASIS_MAX) / total_tics; + cpu_ratios_.user.Store(((cur[CPU_USER] - old[CPU_USER]) * BASIS_MAX) / total_tics); + cpu_ratios_.system.Store( + ((cur[CPU_SYSTEM] - old[CPU_SYSTEM]) * BASIS_MAX) / total_tics); + cpu_ratios_.iowait.Store( + ((cur[CPU_IOWAIT] - old[CPU_IOWAIT]) * BASIS_MAX) / total_tics); } void SystemStateInfo::ReadCurrentProcNetDev() { diff --git a/be/src/util/system-state-info.h b/be/src/util/system-state-info.h index fbdddef1c..ad03bc7b6 100644 --- a/be/src/util/system-state-info.h +++ b/be/src/util/system-state-info.h @@ -47,9 +47,9 @@ class SystemStateInfo { /// Ratios use basis points as their unit (1/100th of a percent, i.e. 0.0001). struct CpuUsageRatios { - int32_t user; - int32_t system; - int32_t iowait; + AtomicInt32 user; + AtomicInt32 system; + AtomicInt32 iowait; }; /// Returns a struct containing the CPU usage ratios for the interval between the last
