Pull request #4179: host_cache: fixed update_stats to remove race_condition causing...

Merge in SNORT/snort3 from ~RSHAFIQ/snort3:lru_race to master

Squashed commit of the following:

commit 75cf5786a801c3858cb8ac3c48c718b7420163b3
Author: rshafiq <rshafiq@cisco.com>
Date:   Wed Jan 24 08:51:45 2024 -0500

    host_cache: fixed update_stats to remove race_condition

diff --git a/src/host_tracker/host_cache.cc b/src/host_tracker/host_cache.cc
index f23de1697e7ffa0dfdd5d133179e358e2a1bf07b..9fa4da7e93a48a5be21c4c2d20d1c9f272c870f2 100644 (file)
--- a/src/host_tracker/host_cache.cc
+++ b/src/host_tracker/host_cache.cc
@@ -27,5 +27,6 @@
 
 using namespace snort;
 
+THREAD_LOCAL struct LruCacheSharedStats host_cache_counts;
 HostCacheIp default_host_cache(LRU_CACHE_INITIAL_SIZE);
 HostCacheSegmentedIp host_cache;

diff --git a/src/host_tracker/host_cache_module.cc b/src/host_tracker/host_cache_module.cc
index f9f197f2cbfff449a1acb35fce0df67b2cffe806..8ed5adb82662a2b2e693319a19fc1653c94b4bd5 100644 (file)
--- a/src/host_tracker/host_cache_module.cc
+++ b/src/host_tracker/host_cache_module.cc
@@ -496,14 +496,6 @@ string HostCacheModule::get_host_cache_segment_stats(int seg_idx)
             + to_string(lru_data.size()) + " trackers, memcap: " + to_string(host_cache.get_max_size())
             + " bytes\n";
 
-        for(auto cache : host_cache.seg_list)
-        {
-            cache->lock();
-            cache->stats.bytes_in_use = cache->current_size;
-            cache->stats.items_in_use = cache->list.size();
-            cache->unlock();
-        }
-
         PegCount* counts = (PegCount*) host_cache.get_counts();
         for ( int i = 0; pegs[i].type != CountType::END; i++ )
         {
@@ -559,14 +551,6 @@ string HostCacheModule::get_host_cache_stats()
         + to_string(lru_data.size()) + " trackers, memcap: " + to_string(host_cache.get_max_size())
         + " bytes\n";
 
-    for(auto cache : host_cache.seg_list)
-    {
-        cache->lock();
-        cache->stats.bytes_in_use = cache->current_size;
-        cache->stats.items_in_use = cache->list.size();
-        cache->unlock();
-    }
-
     PegCount* counts = (PegCount*) host_cache.get_counts();
     const PegInfo* pegs = host_cache.get_pegs();
 
@@ -590,19 +574,19 @@ const PegInfo* HostCacheModule::get_pegs() const
 PegCount* HostCacheModule::get_counts() const
 { return (PegCount*)host_cache.get_counts(); }
 
+void HostCacheModule::prep_counts(bool)
+{ host_cache.update_counts(); }
+
 void HostCacheModule::sum_stats(bool dump_stats)
 {
-    // These could be set in prep_counts but we set them here
-    // to save an extra cache lock.
-    for(auto cache : host_cache.seg_list)
-    {
-        cache->lock();
-        cache->stats.bytes_in_use = cache->current_size;
-        cache->stats.items_in_use = cache->list.size();
-        cache->unlock();
-    }
-
     Module::sum_stats(dump_stats);
+    host_cache.sum_stats(dump_stats);
+}
+
+void HostCacheModule::reset_stats()
+{
+    host_cache.reset_counts();
+    Module::reset_stats();
 }
 
 void HostCacheModule::set_trace(const Trace* trace) const

diff --git a/src/host_tracker/host_cache_module.h b/src/host_tracker/host_cache_module.h
index 539d931ec53bfce1e89ffa289a1adcd2960f1b78..d004650e839fee2974b1bcfacf23edd3b2bad943 100644 (file)
--- a/src/host_tracker/host_cache_module.h
+++ b/src/host_tracker/host_cache_module.h
@@ -65,7 +65,9 @@ public:
     const snort::Command* get_commands() const override;
     const PegInfo* get_pegs() const override;
     PegCount* get_counts() const override;
+    void prep_counts(bool) override;
     void sum_stats(bool) override;
+    void reset_stats() override;
 
     // in sum_stats, just populate the counts vector with whatever we have now
     bool global_stats() const override

diff --git a/src/host_tracker/host_cache_segmented.h b/src/host_tracker/host_cache_segmented.h
index 85f21a850cc1854822a0d833843a4211c23f81a1..3f0d9fdb54fd41574c37870405901857550ac34e 100644 (file)
--- a/src/host_tracker/host_cache_segmented.h
+++ b/src/host_tracker/host_cache_segmented.h
@@ -34,6 +34,7 @@
 #define DEFAULT_HOST_CACHE_SEGMENTS 4
 
 extern SO_PUBLIC HostCacheIp default_host_cache;
+extern THREAD_LOCAL struct LruCacheSharedStats host_cache_counts;
 
 template<typename Key, typename Value>
 class HostCacheSegmented
@@ -62,6 +63,9 @@ public:
     bool reload_resize(size_t memcap_per_segment);
     bool reload_prune(size_t new_size, unsigned max_prune);
     void invalidate();
+    void update_counts();
+    void reset_counts();
+    void sum_stats(bool dump_stats);
 
     std::shared_ptr<Value> operator[](const Key& key);
 
@@ -79,12 +83,11 @@ public:
     HostCacheIp* default_cache = &default_host_cache; // Default cache used for host tracker
 
 private:
-    void update_counts();
-
     uint8_t segment_count;
     std::atomic<size_t> memcap_per_segment;
     struct LruCacheSharedStats counts;
     bool init_done = false;
+    std::mutex stats_lock;
 };
 
 
@@ -239,6 +242,7 @@ std::shared_ptr<Value> HostCacheSegmented<Key, Value>::find(const Key& key)
 template<typename Key, typename Value>
 void HostCacheSegmented<Key, Value>::update_counts()
 {
+    std::lock_guard<std::mutex> guard(stats_lock);
     PegCount* pcs = (PegCount*)&counts;
     const PegInfo* pegs = get_pegs();
 
@@ -249,10 +253,51 @@ void HostCacheSegmented<Key, Value>::update_counts()
     {
         const PegCount* cache_counts = cache->get_counts();
         cache->lock();
-        for (int i = 0; pegs[i].type != CountType::END; i++) 
+        cache->stats.bytes_in_use = cache->current_size;
+        cache->stats.items_in_use = cache->list.size();
+        for (int i = 0; pegs[i].type != CountType::END; i++)
             pcs[i] += cache_counts[i];
         cache->unlock();
     }
+    host_cache_counts = counts;
+}
+
+template<typename Key, typename Value>
+void HostCacheSegmented<Key, Value>::reset_counts()
+{
+    std::lock_guard<std::mutex> guard(stats_lock);
+    const PegInfo* pegs = get_pegs();
+
+    for (auto cache : seg_list)
+    {
+        PegCount* cache_counts = reinterpret_cast<PegCount*> (&cache->stats);
+        cache->lock();
+        for (int i = 0; pegs[i].type != CountType::END; i++)
+            cache_counts[i] = 0;
+        cache->unlock();
+    }
+}
+
+template<typename Key, typename Value>
+void HostCacheSegmented<Key, Value>::sum_stats(bool dump_stats)
+{
+    std::lock_guard<std::mutex> guard(stats_lock);
+    if ( !dump_stats )
+    {
+        const PegInfo* pegs = get_pegs();
+
+        for (auto cache : seg_list)
+        {
+            PegCount* cache_counts = reinterpret_cast<PegCount*> (&cache->stats);
+            cache->lock();
+            for (int i = 0; pegs[i].type != CountType::END; i++)
+            {
+                if ( pegs[i].type == CountType::SUM )
+                    cache_counts[i] = 0;
+            }
+            cache->unlock();
+        }
+    }
 }
 
 template<typename Key, typename Value>
@@ -285,9 +330,10 @@ bool HostCacheSegmented<Key, Value>::find_else_insert(const Key& key, std::share
 template<typename Key, typename Value>
 PegCount* HostCacheSegmented<Key, Value>::get_counts()
 {
-    if(init_done)
+    if( init_done )
         update_counts();
-    return (PegCount*)&counts;
+
+    return (PegCount*)&host_cache_counts;
 }
 
 template<typename Key, typename Value>

diff --git a/src/host_tracker/test/host_cache_module_test.cc b/src/host_tracker/test/host_cache_module_test.cc
index 49cddafeb94b15901609a9af14107d7cb0f05cd1..8b8f629441b3f40a8b55c3d71d20b9dcf690db28 100644 (file)
--- a/src/host_tracker/test/host_cache_module_test.cc
+++ b/src/host_tracker/test/host_cache_module_test.cc
@@ -42,6 +42,7 @@ using namespace snort;
 
 HostCacheIp default_host_cache(LRU_CACHE_INITIAL_SIZE);
 HostCacheSegmentedIp host_cache(4,LRU_CACHE_INITIAL_SIZE);
+THREAD_LOCAL struct LruCacheSharedStats host_cache_counts;
 
 // All tests here use the same module since host_cache is global. Creating a local module for each
 // test will cause host_cache PegCount testing to be dependent on the order of running these tests.