Fix Agent crash because of parallel access between kTaskFlowStatsColl…

…ector 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 7fbb3ce)

Change-Id: I744691ffe29c8121289bae91f53009f3acc36786

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() {

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<VmInterface *>(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);
     }
 }
 

src/vnsw/agent/uve/interface_uve_table.cc

@@ -8,7 +8,7 @@
 #include <uve/agent_uve_base.h>
 
 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<VmFloatingIPStats>  &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()) {

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<UveInterfaceEntry> 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);

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<uint32_t> tcp_sport;
     if (port_bitmap_.tcp_sport_.Sync(tcp_sport)) {

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_() {
 }
 

src/vnsw/agent/uve/vm_uve_entry_base.h

@@ -9,6 +9,7 @@
 #include <vector>
 #include <set>
 #include <map>
+#include <tbb/mutex.h>
 #include <sandesh/sandesh_types.h>
 #include <sandesh/sandesh_constants.h>
 #include <sandesh/sandesh.h>
@@ -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<std::string> &new_l) const;

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<VmUveEntry *>(it->second.get());

src/vnsw/agent/uve/vm_uve_table_base.cc

@@ -7,7 +7,7 @@
 #include <uve/agent_uve_base.h>
 
 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);

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);

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<uint32_t> tcp_sport;
     if (port_bitmap_.tcp_sport_.Sync(tcp_sport)) {

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_;

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;

src/vnsw/agent/uve/vn_uve_table_base.cc

@@ -6,7 +6,7 @@
 #include <uve/agent_uve_base.h>
 
 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);
     }
 }

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);

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;