Skip to content

Commit

Permalink
Fix data race with the read/write of lastDownTime_
Browse files Browse the repository at this point in the history
Summary:
Fixes the following TSAN data race -

```
Write of size 8 at 0x7b8400009888 by thread T40 (mutexes: write M507916304074681368, read M61215687833552496):
  #0 facebook::fboss::QsfpModule::markLastDownTime() fboss/qsfp_service/module/QsfpModule.cpp:951 (qsfp_service+0x206ea68)
  #1 facebook::fboss::TransceiverManager::markLastDownTime(facebook::fboss::TransceiverID) fboss/qsfp_service/TransceiverManager.cpp:1279 (qsfp_service+0x2143c4a)
  #2 void facebook::fboss::markLastDownTime_impl::operator()<facebook::fboss::ALL_PORTS_DOWN_helper

Previous read of size 8 at 0x7b8400009888 by thread T75 (mutexes: read M61215687833552496, write M401237648678623312):
  #0 facebook::fboss::QsfpModule::shouldRemediateLocked() fboss/qsfp_service/module/QsfpModule.cpp:649 (qsfp_service+0x206dbeb)
  #1 facebook::fboss::QsfpModule::shouldRemediate()::$_36::operator()() const fboss/qsfp_service/module/QsfpModule.cpp:590 (qsfp_service+0x20722b0)
  #2 facebook::fboss::QsfpModule::shouldRemediate() fboss/qsfp_service/module/QsfpModule.cpp:596 (qsfp_service+0x20722b0)
  #3 facebook::fboss::TransceiverManager::triggerRemediateEvents(std::vector<facebook::fboss::TransceiverID, std::allocator<facebook::fboss::TransceiverID> > const&) fboss/qsfp_service/TransceiverManager.cpp:1257 (qsfp_service+0x212d79a)
  #4 facebook::fboss::TransceiverManager::refreshStateMachines() fboss/qsfp_service/TransceiverManager.cpp:978 (qsfp_service+0x2142a52)
```

Reviewed By: rajank7

Differential Revision:
D43183960

Privacy Context Container: L1125642

fbshipit-source-id: 9169d1ea2f03db86cbfa20b9f3e615b0871ea2ec
  • Loading branch information
Harshit Gulati authored and facebook-github-bot committed Feb 10, 2023
1 parent 18e1295 commit 0d1a215
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 7 deletions.
10 changes: 5 additions & 5 deletions fboss/qsfp_service/module/QsfpModule.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -646,10 +646,10 @@ bool QsfpModule::shouldRemediateLocked() {
// `FLAGS_remediate_interval`, instead we just need to wait for
// `FLAGS_initial_remediate_interval`. (D26014510)
bool remediationCooled = false;
if (lastDownTime_ > lastRemediateTime_) {
// New lastDownTime_ means the port just recently went down
remediationCooled =
(now - lastDownTime_) > FLAGS_initial_remediate_interval;
auto lastDownTime = lastDownTime_.load();
if (lastDownTime > lastRemediateTime_) {
// New lastDownTime means the port just recently went down
remediationCooled = (now - lastDownTime) > FLAGS_initial_remediate_interval;
} else {
remediationCooled = (now - lastRemediateTime_) > FLAGS_remediate_interval;
}
Expand Down Expand Up @@ -948,7 +948,7 @@ bool QsfpModule::tryRemediateLocked() {
}

void QsfpModule::markLastDownTime() {
lastDownTime_ = std::time(nullptr);
lastDownTime_.store(std::time(nullptr));
}

/*
Expand Down
4 changes: 2 additions & 2 deletions fboss/qsfp_service/module/QsfpModule.h
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ class QsfpModule : public Transceiver {
void markLastDownTime() override;

time_t getLastDownTime() const override {
return lastDownTime_;
return lastDownTime_.load();
}

phy::PrbsStats getPortPrbsStats(phy::Side side) override {
Expand Down Expand Up @@ -268,7 +268,7 @@ class QsfpModule : public Transceiver {
time_t lastRemediateTime_{0};

// last time we know that no port was up on this transceiver.
time_t lastDownTime_{0};
std::atomic<time_t> lastDownTime_{0};

// Diagnostic capabilities of the module
folly::Synchronized<std::optional<DiagsCapability>> diagsCapability_;
Expand Down

0 comments on commit 0d1a215

Please sign in to comment.