cmssw: MeasurementTrackerEvent is not thread safe
While looking through a helgrind log I found the following problem
TkStripMeasurementDet::empty(MeasurementTrackerEvent const&) const calls MeasurementTrackerEvent::stripData which returns a StMeasurementDetSet. The function empty then calls StMeasurementDetSet::detSet. The code for that call attempts to do lazy caching
const StripDetset & detSet(int i) const { if (ready_[i]) const_cast<StMeasurementDetSet*>(this)->getDetSet(i); return detSet_[i]; }
The getDetSet call is
void getDetSet(int i) {
if(detIndex_[i]>=0) {
detSet_[i].set(*handle_,handle_->item(detIndex_[i]));
empty_[i]=false; // better be false already
incAct();
} else { // we should not be here
detSet_[i] = StripDetset();
empty_[i]=true;
}
ready_[i]=false;
incSet();
}
The call to set is
template<typename T>
inline void DetSet<T>::set(DetSetVector<T> const & icont,
typename Container::Item const & item, bool update) {
// if an item is being updated we wait
if (update) icont.update(item);
while(item.initializing()) nanosleep(0,0);
m_data=&icont.data();
m_id=item.id;
m_offset = item.offset;
m_size=item.size;
}
The call to icont.update(item) is done in a correct way such that the modification to item is handled in a thread safe manner so that reading the member data of item after the update call will be correct. Unfortunately, the same can not be said for the data members of DetSet<T> nor StMeasurementDetSet::ready_ where these are pure race conditions and are undefined behavior. The compiler is allowed to set ready_ to false before setting the value of any of DetSet<T> member data (since they are all inlined together).
A remedy would be to change StMeasurementDetSet::ready_ to be of type std::vector<std::atomic<bool>> ready_. Then ready_ would properly protect the call into StMeasurementDetSet::detSet and synchronize the memory at the end of the call to StMeasurementDetSet::getDetSet. It would not, however, stop multiple calls into StMeasurementDetSet::getDetSet from all calling DetSet<T>::set.
About this issue
- Original URL
- State: open
- Created 7 years ago
- Comments: 17 (17 by maintainers)
type tracking
is it still a “no” ?
@mmusich @vmariani @mtosi