This is an automated email from the ASF dual-hosted git repository. michaelsmith pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/impala.git
commit 0c9bfcaa4cc1c74a406265e1d9a83721af19bd57 Author: Kurt Deschler <[email protected]> AuthorDate: Tue Dec 12 11:52:10 2023 -0500 IMPALA-12614: Use atomics for 64-bit host metrics This patch changes 64-bit host metrics to use atomics to avoid potential partial load/store data races. There are no functional changes. This issue was exposed by TSAN tests when IMPALA-12385 enabled periodic metrics by default. Testing: TSAN tests cover this case. Change-Id: I88a15d3ccfeab29506427b3bfcaeebf3a2831176 Reviewed-on: http://gerrit.cloudera.org:8080/20776 Reviewed-by: Impala Public Jenkins <[email protected]> Tested-by: Impala Public Jenkins <[email protected]> --- be/src/runtime/query-state.cc | 8 ++++---- be/src/util/system-state-info-test.cc | 10 +++++----- be/src/util/system-state-info.cc | 8 ++++---- be/src/util/system-state-info.h | 9 +++++---- 4 files changed, 18 insertions(+), 17 deletions(-) diff --git a/be/src/runtime/query-state.cc b/be/src/runtime/query-state.cc index 5738448e8..dafc90b3d 100644 --- a/be/src/runtime/query-state.cc +++ b/be/src/runtime/query-state.cc @@ -233,20 +233,20 @@ Status QueryState::Init(const ExecQueryFInstancesRequestPB* exec_rpc_params, // Add network usage host_profile_->AddSamplingTimeSeriesCounter( "HostNetworkRx", TUnit::BYTES_PER_SECOND, [system_state_info] () { - return system_state_info->GetNetworkUsage().rx_rate; + return system_state_info->GetNetworkUsage().rx_rate.Load(); }); host_profile_->AddSamplingTimeSeriesCounter( "HostNetworkTx", TUnit::BYTES_PER_SECOND, [system_state_info] () { - return system_state_info->GetNetworkUsage().tx_rate; + return system_state_info->GetNetworkUsage().tx_rate.Load(); }); // Add disk stats host_profile_->AddSamplingTimeSeriesCounter( "HostDiskReadThroughput", TUnit::BYTES_PER_SECOND, [system_state_info] () { - return system_state_info->GetDiskStats().read_rate; + return system_state_info->GetDiskStats().read_rate.Load(); }); host_profile_->AddSamplingTimeSeriesCounter( "HostDiskWriteThroughput", TUnit::BYTES_PER_SECOND, [system_state_info] () { - return system_state_info->GetDiskStats().write_rate; + return system_state_info->GetDiskStats().write_rate.Load(); }); } diff --git a/be/src/util/system-state-info-test.cc b/be/src/util/system-state-info-test.cc index 50b0c9715..78735831f 100644 --- a/be/src/util/system-state-info-test.cc +++ b/be/src/util/system-state-info-test.cc @@ -35,7 +35,7 @@ TEST_F(SystemStateInfoTest, FirstCallReturnsZero) { EXPECT_EQ(0, r.user + r.system + r.iowait); const SystemStateInfo::NetworkUsage& n = info.GetNetworkUsage(); - EXPECT_EQ(0, n.rx_rate + n.tx_rate); + EXPECT_EQ(0, n.rx_rate.Load() + n.tx_rate.Load()); } // Smoke test to make sure that we read non-zero values from /proc/stat. @@ -194,8 +194,8 @@ TEST_F(SystemStateInfoTest, ComputeNetworkUsage) { int period_ms = 500; info.ComputeNetworkUsage(period_ms); const SystemStateInfo::NetworkUsage& n = info.GetNetworkUsage(); - EXPECT_EQ(n.rx_rate, 8000); - EXPECT_EQ(n.tx_rate, 12000); + EXPECT_EQ(n.rx_rate.Load(), 8000); + EXPECT_EQ(n.tx_rate.Load(), 12000); } // Tests the computation logic for disk statistics. @@ -216,8 +216,8 @@ TEST_F(SystemStateInfoTest, ComputeDiskStats) { int period_ms = 500; info.ComputeDiskStats(period_ms); const SystemStateInfo::DiskStats& ds = info.GetDiskStats(); - EXPECT_EQ(ds.read_rate, 2 * 1000 * SystemStateInfo::BYTES_PER_SECTOR); - EXPECT_EQ(ds.write_rate, 2 * 2000 * SystemStateInfo::BYTES_PER_SECTOR); + EXPECT_EQ(ds.read_rate.Load(), 2 * 1000 * SystemStateInfo::BYTES_PER_SECTOR); + EXPECT_EQ(ds.write_rate.Load(), 2 * 2000 * SystemStateInfo::BYTES_PER_SECTOR); } } // namespace impala diff --git a/be/src/util/system-state-info.cc b/be/src/util/system-state-info.cc index b0344cc97..931717d28 100644 --- a/be/src/util/system-state-info.cc +++ b/be/src/util/system-state-info.cc @@ -208,10 +208,10 @@ void SystemStateInfo::ComputeNetworkUsage(int64_t period_ms) { const NetworkValues& cur = network_values_[net_val_idx_]; const NetworkValues& old = network_values_[1 - net_val_idx_]; int64_t rx_bytes = cur[NET_RX_BYTES] - old[NET_RX_BYTES]; - network_usage_.rx_rate = rx_bytes * 1000L / period_ms; + network_usage_.rx_rate.Store(rx_bytes * 1000L / period_ms); int64_t tx_bytes = cur[NET_TX_BYTES] - old[NET_TX_BYTES]; - network_usage_.tx_rate = tx_bytes * 1000L / period_ms; + network_usage_.tx_rate.Store(tx_bytes * 1000L / period_ms); } void SystemStateInfo::ReadCurrentProcDiskStats() { @@ -263,10 +263,10 @@ void SystemStateInfo::ComputeDiskStats(int64_t period_ms) { const DiskValues& cur = disk_values_[disk_val_idx_]; const DiskValues& old = disk_values_[1 - disk_val_idx_]; int64_t read_sectors = cur[DISK_SECTORS_READ] - old[DISK_SECTORS_READ]; - disk_stats_.read_rate = read_sectors * BYTES_PER_SECTOR * 1000 / period_ms; + disk_stats_.read_rate.Store(read_sectors * BYTES_PER_SECTOR * 1000 / period_ms); int64_t write_sectors = cur[DISK_SECTORS_WRITTEN] - old[DISK_SECTORS_WRITTEN]; - disk_stats_.write_rate = write_sectors * BYTES_PER_SECTOR * 1000 / period_ms; + disk_stats_.write_rate.Store(write_sectors * BYTES_PER_SECTOR * 1000 / period_ms); } } // namespace impala diff --git a/be/src/util/system-state-info.h b/be/src/util/system-state-info.h index a4c9b882a..fbdddef1c 100644 --- a/be/src/util/system-state-info.h +++ b/be/src/util/system-state-info.h @@ -23,6 +23,7 @@ #include <gtest/gtest_prod.h> // for FRIEND_TEST +#include "common/atomic.h" #include "common/names.h" #include "common/logging.h" @@ -57,8 +58,8 @@ class SystemStateInfo { /// Network usage rates in bytes per second. struct NetworkUsage { - int64_t rx_rate; - int64_t tx_rate; + AtomicInt64 rx_rate; + AtomicInt64 tx_rate; }; /// Returns a struct containing the network usage for the interval between the last two @@ -67,8 +68,8 @@ class SystemStateInfo { /// Disk statistics rates in bytes per second struct DiskStats { - int64_t read_rate; - int64_t write_rate; + AtomicInt64 read_rate; + AtomicInt64 write_rate; }; /// Returns a struct containing the disk throughput for the interval between the last
