From 4de3a4e025b44445eff373e5233d18e4c5c4e9ba Mon Sep 17 00:00:00 2001 From: ashoksingh Date: Sat, 16 Apr 2016 09:23:18 +0530 Subject: [PATCH] Fix Agent crash because of parallel access between kTaskFlowStatsCollector and kTaskDBExclude When kTaskFlowStatsCollector was updating stats in VnUveEntry, kTaskDBExclude went ahead and deleted the VnUveEntry. There are other such structures for which parallel access between kTaskFlowStatsCollector and kTaskDBExclude can cause issues Fixed all parallel access between kTaskFlowStatsCollector and kTaskDBExclude by acquiring required locks. Also added task exclusion between kTaskDBExclude and Agent::Uve to prevent simultaneous access of UVE data-structures between kTaskDBExclude and Agent::Uve. The code executed under kTaskDBExclude was earlier executed in db::DBTable which had exclusion with Agent::Uve. Also added locks to prevent simultaneous access between kTaskFlowStatsCollector and Agent::Uve. Closes-Bug: #1569645 (cherry picked from commit 7fbb3cee261bea0c11f31bbedff9c1497dc1a6f9) Change-Id: I744691ffe29c8121289bae91f53009f3acc36786 --- src/vnsw/agent/cmn/agent.cc | 9 ++++++++ .../agent/uve/interface_uve_stats_table.cc | 4 +++- src/vnsw/agent/uve/interface_uve_table.cc | 21 ++++++++++++++++--- src/vnsw/agent/uve/interface_uve_table.h | 5 ++++- src/vnsw/agent/uve/vm_uve_entry.cc | 6 ++++++ src/vnsw/agent/uve/vm_uve_entry_base.cc | 2 +- src/vnsw/agent/uve/vm_uve_entry_base.h | 3 +++ src/vnsw/agent/uve/vm_uve_table.cc | 1 + src/vnsw/agent/uve/vm_uve_table_base.cc | 3 ++- src/vnsw/agent/uve/vm_uve_table_base.h | 2 ++ src/vnsw/agent/uve/vn_uve_entry.cc | 10 +++++++++ src/vnsw/agent/uve/vn_uve_entry.h | 1 + src/vnsw/agent/uve/vn_uve_table.cc | 2 ++ src/vnsw/agent/uve/vn_uve_table_base.cc | 3 ++- src/vnsw/agent/uve/vn_uve_table_base.h | 3 +++ .../vrouter/flow_stats/flow_stats_collector.h | 2 +- 16 files changed, 68 insertions(+), 9 deletions(-) diff --git a/src/vnsw/agent/cmn/agent.cc b/src/vnsw/agent/cmn/agent.cc index 00cae2b1422..5578af54b92 100644 --- a/src/vnsw/agent/cmn/agent.cc +++ b/src/vnsw/agent/cmn/agent.cc @@ -271,6 +271,15 @@ void Agent::SetAgentTaskPolicy() { TaskScheduler *scheduler = TaskScheduler::GetInstance(); scheduler->RegisterLog(boost::bind(&Agent::TaskTrace, this, _1, _2, _3, _4, _5)); + + const char *db_exclude_task_exclude_list[] = { + "Agent::Uve", + AGENT_SHUTDOWN_TASKNAME, + AGENT_INIT_TASKNAME + }; + SetTaskPolicyOne(kTaskDBExclude, db_exclude_task_exclude_list, + sizeof(db_exclude_task_exclude_list) / sizeof(char *)); + } void Agent::CreateLifetimeManager() { diff --git a/src/vnsw/agent/uve/interface_uve_stats_table.cc b/src/vnsw/agent/uve/interface_uve_stats_table.cc index 2e0fbaa6cad..e548b89554e 100644 --- a/src/vnsw/agent/uve/interface_uve_stats_table.cc +++ b/src/vnsw/agent/uve/interface_uve_stats_table.cc @@ -146,6 +146,7 @@ void InterfaceUveStatsTable::UpdateFloatingIpStats(const FipInfo &fip_info) { if (intf == NULL) { return; } + tbb::mutex::scoped_lock lock(interface_tree_mutex_); VmInterface *vmi = static_cast(intf); InterfaceMap::iterator intf_it = interface_tree_.find(vmi->cfg_name()); @@ -185,11 +186,12 @@ bool InterfaceUveStatsTable::FrameFipStatsMsg(const VmInterface *itf, void InterfaceUveStatsTable::UpdatePortBitmap (const string &name, uint8_t proto, uint16_t sport, uint16_t dport) { + tbb::mutex::scoped_lock lock(interface_tree_mutex_); InterfaceMap::const_iterator it = interface_tree_.find(name); if (it != interface_tree_.end()) { UveInterfaceEntry *entry = it->second.get(); - entry->port_bitmap_.AddPort(proto, sport, dport); + entry->UpdatePortBitmap(proto, sport, dport); } } diff --git a/src/vnsw/agent/uve/interface_uve_table.cc b/src/vnsw/agent/uve/interface_uve_table.cc index ce17da7d024..4fd8772ed13 100644 --- a/src/vnsw/agent/uve/interface_uve_table.cc +++ b/src/vnsw/agent/uve/interface_uve_table.cc @@ -8,7 +8,7 @@ #include InterfaceUveTable::InterfaceUveTable(Agent *agent, uint32_t default_intvl) - : agent_(agent), interface_tree_(), + : agent_(agent), interface_tree_(), interface_tree_mutex_(), intf_listener_id_(DBTableBase::kInvalidId), timer_last_visited_(""), timer_(TimerManager::CreateTimer @@ -42,6 +42,7 @@ bool InterfaceUveTable::TimerExpiry() { if (entry->deleted_) { SendInterfaceDeleteMsg(cfg_name); if (!entry->renewed_) { + tbb::mutex::scoped_lock lock(interface_tree_mutex_); interface_tree_.erase(prev); } else { entry->deleted_ = false; @@ -184,6 +185,17 @@ void InterfaceUveTable::UveInterfaceEntry::Reset() { renewed_ = false; } +void InterfaceUveTable::UveInterfaceEntry::UpdatePortBitmap + (uint8_t proto, uint16_t sport, uint16_t dport) { + tbb::mutex::scoped_lock lock(mutex_); + /* No need to update stats if the entry is marked for delete and not + * renewed */ + if (deleted_ && !renewed_) { + return; + } + port_bitmap_.AddPort(proto, sport, dport); +} + bool InterfaceUveTable::UveInterfaceEntry::FipAggStatsChanged (const vector &list) const { if (list != uve_info_.get_fip_agg_stats()) { @@ -393,6 +405,11 @@ bool InterfaceUveTable::UveInterfaceEntry::OutBandChanged(uint64_t out_band) void InterfaceUveTable::UveInterfaceEntry::UpdateFloatingIpStats (const FipInfo &fip_info) { tbb::mutex::scoped_lock lock(mutex_); + /* No need to update stats if the entry is marked for delete and not + * renewed */ + if (deleted_ && !renewed_) { + return; + } FloatingIp *entry = FipEntry(fip_info.fip_, fip_info.vn_); /* Ignore stats update request if it comes after entry is removed */ if (entry == NULL) { @@ -546,7 +563,6 @@ void InterfaceUveTable::UveInterfaceEntry::SetStats void InterfaceUveTable::UveInterfaceEntry::AddFloatingIp (const VmInterface::FloatingIp &fip) { - tbb::mutex::scoped_lock lock(mutex_); FloatingIpPtr key(new FloatingIp(fip.floating_ip_, fip.vn_.get()->GetName())); FloatingIpSet::iterator it = fip_tree_.find(key); @@ -558,7 +574,6 @@ void InterfaceUveTable::UveInterfaceEntry::AddFloatingIp void InterfaceUveTable::UveInterfaceEntry::RemoveFloatingIp (const VmInterface::FloatingIp &fip) { - tbb::mutex::scoped_lock lock(mutex_); FloatingIpPtr key(new FloatingIp(fip.floating_ip_, fip.vn_.get()->GetName())); FloatingIpSet::iterator it = fip_tree_.find(key); if (it != fip_tree_.end()) { diff --git a/src/vnsw/agent/uve/interface_uve_table.h b/src/vnsw/agent/uve/interface_uve_table.h index 1a8c55d9c48..e7ba18b646c 100644 --- a/src/vnsw/agent/uve/interface_uve_table.h +++ b/src/vnsw/agent/uve/interface_uve_table.h @@ -115,7 +115,7 @@ class InterfaceUveTable { bool ace_stats_changed_; UveVMInterfaceAgent uve_info_; AceStatsSet ace_set_; - /* For exclusion between Agent::StatsCollector and Agent::Uve tasks */ + /* For exclusion between kTaskFlowStatsCollector and Agent::Uve */ tbb::mutex mutex_; UveInterfaceEntry(const VmInterface *i) : intf_(i), @@ -149,6 +149,7 @@ class InterfaceUveTable { void SetVnVmInfo(UveVMInterfaceAgent *uve) const; void UpdateInterfaceAceStats(const std::string &ace_uuid); void Reset(); + void UpdatePortBitmap(uint8_t proto, uint16_t sport, uint16_t dport); }; typedef boost::shared_ptr UveInterfaceEntryPtr; @@ -170,6 +171,8 @@ class InterfaceUveTable { Agent *agent_; InterfaceMap interface_tree_; + /* For exclusion between kTaskFlowStatsCollector and kTaskDBExclude */ + tbb::mutex interface_tree_mutex_; private: virtual UveInterfaceEntryPtr Allocate(const VmInterface *vm); void InterfaceNotify(DBTablePartBase *partition, DBEntryBase *e); diff --git a/src/vnsw/agent/uve/vm_uve_entry.cc b/src/vnsw/agent/uve/vm_uve_entry.cc index f4da05b88a8..d04c180a85f 100644 --- a/src/vnsw/agent/uve/vm_uve_entry.cc +++ b/src/vnsw/agent/uve/vm_uve_entry.cc @@ -17,6 +17,11 @@ VmUveEntry::~VmUveEntry() { void VmUveEntry::UpdatePortBitmap(uint8_t proto, uint16_t sport, uint16_t dport) { + tbb::mutex::scoped_lock lock(mutex_); + if (deleted_ && !renewed_) { + /* Skip updates on VmUveEntry if it is marked for delete */ + return; + } //Update VM bitmap port_bitmap_.AddPort(proto, sport, dport); @@ -32,6 +37,7 @@ void VmUveEntry::UpdatePortBitmap(uint8_t proto, uint16_t sport, bool VmUveEntry::SetVmPortBitmap(UveVirtualMachineAgent *uve) { bool changed = false; + tbb::mutex::scoped_lock lock(mutex_); vector tcp_sport; if (port_bitmap_.tcp_sport_.Sync(tcp_sport)) { diff --git a/src/vnsw/agent/uve/vm_uve_entry_base.cc b/src/vnsw/agent/uve/vm_uve_entry_base.cc index 09d81d33cff..9befaece3d2 100644 --- a/src/vnsw/agent/uve/vm_uve_entry_base.cc +++ b/src/vnsw/agent/uve/vm_uve_entry_base.cc @@ -9,7 +9,7 @@ using namespace std; VmUveEntryBase::VmUveEntryBase(Agent *agent, const string &vm_name) : agent_(agent), interface_tree_(), uve_info_(), changed_(true), - deleted_(false), renewed_(false), add_by_vm_notify_(false), + deleted_(false), renewed_(false), mutex_(), add_by_vm_notify_(false), vm_config_name_(vm_name), vm_name_() { } diff --git a/src/vnsw/agent/uve/vm_uve_entry_base.h b/src/vnsw/agent/uve/vm_uve_entry_base.h index e2d4ba7ccca..3f1b7c910ea 100644 --- a/src/vnsw/agent/uve/vm_uve_entry_base.h +++ b/src/vnsw/agent/uve/vm_uve_entry_base.h @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include @@ -54,6 +55,8 @@ class VmUveEntryBase { bool changed_; bool deleted_; bool renewed_; + /* For exclusion between kTaskFlowStatsCollector and Agent::Uve */ + tbb::mutex mutex_; private: bool UveVmInterfaceListChanged (const std::vector &new_l) const; diff --git a/src/vnsw/agent/uve/vm_uve_table.cc b/src/vnsw/agent/uve/vm_uve_table.cc index 2ffb867035d..653696de176 100644 --- a/src/vnsw/agent/uve/vm_uve_table.cc +++ b/src/vnsw/agent/uve/vm_uve_table.cc @@ -22,6 +22,7 @@ VmUveTable::~VmUveTable() { void VmUveTable::UpdateBitmap(const VmEntry* vm, uint8_t proto, uint16_t sport, uint16_t dport) { + tbb::mutex::scoped_lock lock(uve_vm_map_mutex_); UveVmMap::iterator it = uve_vm_map_.find(vm->GetUuid()); if (it != uve_vm_map_.end()) { VmUveEntry *entry = static_cast(it->second.get()); diff --git a/src/vnsw/agent/uve/vm_uve_table_base.cc b/src/vnsw/agent/uve/vm_uve_table_base.cc index cdc3c0c46f3..9fc20f610ad 100644 --- a/src/vnsw/agent/uve/vm_uve_table_base.cc +++ b/src/vnsw/agent/uve/vm_uve_table_base.cc @@ -7,7 +7,7 @@ #include VmUveTableBase::VmUveTableBase(Agent *agent, uint32_t default_intvl) - : uve_vm_map_(), agent_(agent), + : uve_vm_map_(), agent_(agent), uve_vm_map_mutex_(), intf_listener_id_(DBTableBase::kInvalidId), vm_listener_id_(DBTableBase::kInvalidId), timer_last_visited_(nil_uuid()), timer_(TimerManager::CreateTimer @@ -40,6 +40,7 @@ bool VmUveTableBase::TimerExpiry() { if (entry->deleted()) { SendVmDeleteMsg(entry->vm_config_name()); if (!entry->renewed()) { + tbb::mutex::scoped_lock lock(uve_vm_map_mutex_); uve_vm_map_.erase(prev); } else { entry->set_deleted(false); diff --git a/src/vnsw/agent/uve/vm_uve_table_base.h b/src/vnsw/agent/uve/vm_uve_table_base.h index 3274d6c8857..a5a977983f2 100644 --- a/src/vnsw/agent/uve/vm_uve_table_base.h +++ b/src/vnsw/agent/uve/vm_uve_table_base.h @@ -61,6 +61,8 @@ class VmUveTableBase { UveVmMap uve_vm_map_; Agent *agent_; + /* For exclusion between kTaskFlowStatsCollector and kTaskDBExclude */ + tbb::mutex uve_vm_map_mutex_; private: virtual VmUveEntryPtr Allocate(const VmEntry *vm); void InterfaceNotify(DBTablePartBase *partition, DBEntryBase *e); diff --git a/src/vnsw/agent/uve/vn_uve_entry.cc b/src/vnsw/agent/uve/vn_uve_entry.cc index 03846d70e51..2656afcfb8d 100644 --- a/src/vnsw/agent/uve/vn_uve_entry.cc +++ b/src/vnsw/agent/uve/vn_uve_entry.cc @@ -22,12 +22,21 @@ VnUveEntry::~VnUveEntry() { void VnUveEntry::UpdatePortBitmap(uint8_t proto, uint16_t sport, uint16_t dport) { + tbb::mutex::scoped_lock lock(mutex_); + if (deleted_ && !renewed_) { + /* Skip updates on VnUveEntry if it is marked for delete */ + return; + } port_bitmap_.AddPort(proto, sport, dport); } void VnUveEntry::UpdateInterVnStats(const string &dst_vn, uint64_t bytes, uint64_t pkts, bool outgoing) { tbb::mutex::scoped_lock lock(mutex_); + if (deleted_ && !renewed_) { + /* Skip updates on VnUveEntry if it is marked for delete */ + return; + } VnStatsPtr key(new VnStats(dst_vn, 0, 0, false)); VnStatsSet::iterator stats_it = inter_vn_stats_.find(key); if (stats_it == inter_vn_stats_.end()) { @@ -64,6 +73,7 @@ void VnUveEntry::ClearInterVnStats() { bool VnUveEntry::SetVnPortBitmap(UveVirtualNetworkAgent &uve) { bool changed = false; + tbb::mutex::scoped_lock lock(mutex_); vector tcp_sport; if (port_bitmap_.tcp_sport_.Sync(tcp_sport)) { diff --git a/src/vnsw/agent/uve/vn_uve_entry.h b/src/vnsw/agent/uve/vn_uve_entry.h index 7878d4d959a..3a2b2472b63 100644 --- a/src/vnsw/agent/uve/vn_uve_entry.h +++ b/src/vnsw/agent/uve/vn_uve_entry.h @@ -113,6 +113,7 @@ class VnUveEntry : public VnUveEntryBase { void BuildArpStats(const StatsManager::VrfStats *s, UveVrfStats &vrf_stats) const; + /* For exclusion between kTaskFlowStatsCollector and Agent::Uve */ tbb::mutex mutex_; uint64_t in_bytes_; uint64_t out_bytes_; diff --git a/src/vnsw/agent/uve/vn_uve_table.cc b/src/vnsw/agent/uve/vn_uve_table.cc index 45f4fd94ef2..f34f3811e23 100644 --- a/src/vnsw/agent/uve/vn_uve_table.cc +++ b/src/vnsw/agent/uve/vn_uve_table.cc @@ -86,6 +86,7 @@ void VnUveTable::SendVnStatsMsg(const VnEntry *vn, bool only_vrf_stats) { void VnUveTable::UpdateBitmap(const string &vn, uint8_t proto, uint16_t sport, uint16_t dport) { + tbb::mutex::scoped_lock lock(uve_vn_map_mutex_); UveVnMap::iterator it = uve_vn_map_.find(vn); if (it == uve_vn_map_.end()) { return; @@ -98,6 +99,7 @@ void VnUveTable::UpdateBitmap(const string &vn, uint8_t proto, uint16_t sport, void VnUveTable::UpdateInterVnStats(const string &src, const string &dst, uint64_t bytes, uint64_t pkts, bool outgoing) { + tbb::mutex::scoped_lock lock(uve_vn_map_mutex_); UveVnMap::iterator it = uve_vn_map_.find(src); if (it == uve_vn_map_.end()) { return; diff --git a/src/vnsw/agent/uve/vn_uve_table_base.cc b/src/vnsw/agent/uve/vn_uve_table_base.cc index d99da000b50..de5235b4a40 100644 --- a/src/vnsw/agent/uve/vn_uve_table_base.cc +++ b/src/vnsw/agent/uve/vn_uve_table_base.cc @@ -6,7 +6,7 @@ #include VnUveTableBase::VnUveTableBase(Agent *agent, uint32_t default_intvl) - : uve_vn_map_(), agent_(agent), + : uve_vn_map_(), agent_(agent), uve_vn_map_mutex_(), vn_listener_id_(DBTableBase::kInvalidId), intf_listener_id_(DBTableBase::kInvalidId), timer_last_visited_(""), @@ -148,6 +148,7 @@ void VnUveTableBase::SendDeleteVnMsg(const string &vn) { void VnUveTableBase::Delete(const std::string &name) { UveVnMap::iterator it = uve_vn_map_.find(name); if (it != uve_vn_map_.end()) { + tbb::mutex::scoped_lock lock(uve_vn_map_mutex_); uve_vn_map_.erase(it); } } diff --git a/src/vnsw/agent/uve/vn_uve_table_base.h b/src/vnsw/agent/uve/vn_uve_table_base.h index 1f508f33e00..4802de3570e 100644 --- a/src/vnsw/agent/uve/vn_uve_table_base.h +++ b/src/vnsw/agent/uve/vn_uve_table_base.h @@ -39,6 +39,7 @@ class VnUveTableBase { void Shutdown(void); void SendVnAclRuleCount(); bool TimerExpiry(); + void DeleteVnEntry(const UveVnMap::iterator &it); protected: void Delete(const std::string &name); @@ -51,6 +52,8 @@ class VnUveTableBase { UveVnMap uve_vn_map_; Agent *agent_; + /* For exclusion between kTaskFlowStatsCollector and kTaskDBExclude */ + tbb::mutex uve_vn_map_mutex_; private: VnUveEntryBase* Add(const VnEntry *vn); void Add(const std::string &vn); diff --git a/src/vnsw/agent/vrouter/flow_stats/flow_stats_collector.h b/src/vnsw/agent/vrouter/flow_stats/flow_stats_collector.h index 2fd1623805c..d2974e87c3a 100644 --- a/src/vnsw/agent/vrouter/flow_stats/flow_stats_collector.h +++ b/src/vnsw/agent/vrouter/flow_stats/flow_stats_collector.h @@ -27,7 +27,7 @@ class FlowStatsManager; //Defines the functionality to periodically read flow stats from //shared memory (between agent and Kernel) and export this stats info to //collector. Also responsible for aging of flow entries. Runs in the context -//of "Agent::StatsCollector" which has exclusion with "db::DBTable", +//of kTaskFlowStatsCollector which has exclusion with "db::DBTable", class FlowStatsCollector : public StatsCollector { public: static const uint64_t FlowAgeTime = 1000000 * 180;