mrregister: fix race condition in displacement update#3277
mrregister: fix race condition in displacement update#3277
Conversation
Replace the shared max norm lambda in scaling-and-squaring with a thread local functor reduction and mutex-protected merge.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| MaxNormSquaredFunctor &operator=(MaxNormSquaredFunctor &&) = delete; | ||
|
|
||
| ~MaxNormSquaredFunctor() { | ||
| std::lock_guard<std::mutex> lock(mutex); |
There was a problem hiding this comment.
warning: variable 'lock' of type 'std::lock_guardstd::mutex' can be declared 'const' [misc-const-correctness]
| std::lock_guard<std::mutex> lock(mutex); | |
| std::lock_guard<std::mutex> const lock(mutex); |
| } | ||
|
|
||
| private: | ||
| default_type &overall_max_norm_squared; |
There was a problem hiding this comment.
warning: member 'overall_max_norm_squared' of type 'default_type &' (aka 'double &') is a reference [cppcoreguidelines-avoid-const-or-ref-data-members]
default_type &overall_max_norm_squared;
^
There was a problem hiding this comment.
I know we've used this approach throughout the code base, but I think it may have been criticised at some point. Is there something better, even if only for my own knowledge when I need to do similar in the future? My naive instinct would be std::shared_ptr<default_type>, but there's lots in the language I don't know about.
| private: | ||
| default_type &overall_max_norm_squared; | ||
| default_type local_max_norm_squared; | ||
| inline static std::mutex mutex; |
There was a problem hiding this comment.
warning: variable 'mutex' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
inline static std::mutex mutex;
^
|
Even if the fault was discovered on |
2 participants
The following race condition was identified with TSAN:
The cause was this piece of code:
Here,
max_normis a local stack variable. The lambda captures it by reference thus the variable is subject to reads/writes from different threads without any synchronisation. This PR fixes the problem by replacing the shared max norm lambda with a thread local functor reduction and mutex-protected merge. Also rather than computing as square root inside the functor, we compute the squared norm and only at the end evaluate the square root.