Guard max_latency_ with latencies_mutex_ #878
Conversation
There was a race in the computation of atomic max and std::memory_order_release is not a valid order for atomic loads. The code isn't performance critical, so it is easier and safer to use mutexes instead.
|
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
loadgen/logging.h
Outdated
| @@ -372,7 +372,7 @@ class AsyncLog { | |||
| std::condition_variable all_latencies_recorded_; | |||
| uint64_t latencies_first_sample_sequence_id_ = 0; | |||
| std::vector<QuerySampleLatency> latencies_; | |||
| std::atomic<QuerySampleLatency> max_latency_{0}; | |||
| QuerySampleLatency max_latency_; | |||
There was a problem hiding this comment.
Should we initialize this to 0?
| @@ -421,7 +417,8 @@ PerfClock::time_point AsyncLog::GetMaxCompletionTime() { | |||
| } | |||
|
|
|||
| QuerySampleLatency AsyncLog::GetMaxLatencySoFar() { | |||
| return max_latency_.load(std::memory_order_release); | |||
| std::unique_lock<std::mutex> lock(latencies_mutex_); | |||
There was a problem hiding this comment.
Just curious: why does this need to hold mutex?
There was a problem hiding this comment.
If GetMaxLatencySoFar were called in one thread and RecordSampleCompletion were called concurrently in another thread, there would be a race. Looking at the surrounding code, I don't think that is possible, but if a variable is guarded by a mutex in one context, I think it ought to have an equivalent guard in all contexts to be safe. This function isn't performance critical, so I'm not worried about grabbing a lock here, particularly since I am pretty certain there will be no contention.
There was a race in the computation of atomic max and std::memory_order_release
is not a valid order for atomic loads. The code isn't performance critical, so
it is easier and safer to use mutexes instead.