Pull request #4318: appid: re-enabling appid cpu profiler and crash fix

Merge in SNORT/snort3 from ~UMASHARM/snort3:appid_profiler_fix to master

Squashed commit of the following:

commit 61e74d2982ec6495087652300c2afc33ff1a3945
Author: Umang Sharma <umasharm@cisco.com>
Date:   Thu May 9 08:39:00 2024 -0400

    appid : re-enabling appid cpu profiler making it thread safe

diff --git a/src/network_inspectors/appid/app_cpu_profile_table.cc b/src/network_inspectors/appid/app_cpu_profile_table.cc
index c4bd1cfa40311c59c80f72edefae90791fb46a7f..a8e433f31d40f555bf732b6d12593d07116b01bd 100644 (file)
--- a/src/network_inspectors/appid/app_cpu_profile_table.cc
+++ b/src/network_inspectors/appid/app_cpu_profile_table.cc
@@ -33,11 +33,11 @@
 
 using namespace snort;
 
-const char* table_header = "AppId Performance Statistics (all)\n========================================================================================================================\n";
-const char* columns = " AppId   App Name                             Microsecs        Packets        Avg/Packet       Sessions     Avg/Session \n";
-const char* partition = "------------------------------------------------------------------------------------------------------------------------\n";
+static const char* table_header = "AppId Performance Statistics (all)\n========================================================================================================================\n";
+static const char* columns = " AppId   App Name                             Microsecs        Packets        Avg/Packet       Sessions     Avg/Session \n";
+static const char* partition = "------------------------------------------------------------------------------------------------------------------------\n";
 
-std::string FormatWithCommas(uint64_t value) 
+static std::string FormatWithCommas(uint64_t value)
 {
     std::string numStr = std::to_string(value);
     int insertPosition = numStr.length() - 3;
@@ -52,20 +52,20 @@ std::string FormatWithCommas(uint64_t value)
 // Comparator for priority queue based on avg_processing_time/session
 struct CompareByAvgProcessingTime {
     bool operator()(const std::pair<AppId, AppidCPUProfilerStats>& a, const std::pair<AppId, AppidCPUProfilerStats>& b) const {
-        if (a.second.processed_packets == 0 or b.second.processed_packets == 0) {
+        if (a.second.per_appid_sessions == 0 or b.second.per_appid_sessions == 0)
             return false;
-        }
-        return a.second.processing_time/a.second.per_appid_sessions < b.second.processing_time/b.second.per_appid_sessions ; 
+
+        return a.second.processing_time/a.second.per_appid_sessions < b.second.processing_time/b.second.per_appid_sessions;
     }
 };
 
-void AppidCPUProfilingManager::display_appid_cpu_profiler_table(AppId appid)
+AppidCpuTableDisplayStatus AppidCPUProfilingManager::display_appid_cpu_profiler_table(AppId appid, OdpContext& odp_ctxt)
 {
-    if (appid_cpu_profiling_table.empty())
-    {
-        appid_log(nullptr, TRACE_INFO_LEVEL,"Appid CPU Profiler Table is empty\n", appid);
-        return;
-    }
+    if (odp_ctxt.is_appid_cpu_profiler_running())
+        return DISPLAY_ERROR_APPID_PROFILER_RUNNING;
+    else if (appid_cpu_profiling_table.empty())
+        return DISPLAY_ERROR_TABLE_EMPTY;
+
     auto bucket = appid_cpu_profiling_table.find(appid);
 
     if (bucket != appid_cpu_profiling_table.end())
@@ -82,22 +82,20 @@ void AppidCPUProfilingManager::display_appid_cpu_profiler_table(AppId appid)
     {
         appid_log(nullptr, TRACE_INFO_LEVEL,"Appid %d not found in the table\n", appid);
     }
+    return DISPLAY_SUCCESS;
 }
 
-void AppidCPUProfilingManager::display_appid_cpu_profiler_table()
+AppidCpuTableDisplayStatus AppidCPUProfilingManager::display_appid_cpu_profiler_table(OdpContext& odp_ctxt, bool override_running_flag)
 {
-    if (appid_cpu_profiling_table.empty())
-    {
-        appid_log(nullptr, TRACE_INFO_LEVEL,"Appid CPU Profiler Table is empty\n");
-        return;
-    }
+    if (odp_ctxt.is_appid_cpu_profiler_running() and !override_running_flag)
+        return DISPLAY_ERROR_APPID_PROFILER_RUNNING;
+    else if (appid_cpu_profiling_table.empty())
+        return DISPLAY_ERROR_TABLE_EMPTY;
 
     std::priority_queue<std::pair<AppId, AppidCPUProfilerStats>, std::vector<std::pair<AppId, AppidCPUProfilerStats>>, CompareByAvgProcessingTime> sorted_appid_cpu_profiler_table;
 
     for (const auto& entry : appid_cpu_profiling_table) 
-    {
-        sorted_appid_cpu_profiler_table.push(entry); // Push the pair (AppId, AppidCPUProfilerStats)
-    }
+        sorted_appid_cpu_profiler_table.push(entry);
 
     appid_log(nullptr, TRACE_INFO_LEVEL, table_header);
     appid_log(nullptr, TRACE_INFO_LEVEL, columns);
@@ -107,27 +105,26 @@ void AppidCPUProfilingManager::display_appid_cpu_profiler_table()
     {
         auto entry = sorted_appid_cpu_profiler_table.top();
         sorted_appid_cpu_profiler_table.pop();
-        if (!entry.second.processed_packets) 
+        if (!entry.second.processed_packets or !entry.second.per_appid_sessions)
             continue;
             
         appid_log(nullptr, TRACE_INFO_LEVEL, " %5d   %-25.25s   %18s   %12s   %15s   %12s    %12s\n",
                     entry.first, entry.second.app_name.c_str(), FormatWithCommas(entry.second.processing_time).c_str(), FormatWithCommas(entry.second.processed_packets).c_str(), FormatWithCommas(entry.second.processing_time/entry.second.processed_packets).c_str(),
                                                                                FormatWithCommas(entry.second.per_appid_sessions).c_str(), FormatWithCommas(entry.second.processing_time/entry.second.per_appid_sessions).c_str());
     } 
-}
-
-AppidCPUProfilingManager::AppidCPUProfilingManager()
-{
-    appid_cpu_profiling_table.clear();
+    return DISPLAY_SUCCESS;
 }
 
 void AppidCPUProfilingManager::cleanup_appid_cpu_profiler_table()
 { 
+    std::lock_guard<std::mutex> lock(appid_cpu_profiler_mutex);
     appid_cpu_profiling_table.clear();
 }
 
 void AppidCPUProfilingManager::insert_appid_cpu_profiler_record(AppId appId, const AppidCPUProfilerStats& stats)
 {
+    std::lock_guard<std::mutex> lock(appid_cpu_profiler_mutex);
+
     auto it = appid_cpu_profiling_table.find(appId);
     if (it == appid_cpu_profiling_table.end()) 
     {

diff --git a/src/network_inspectors/appid/app_cpu_profile_table.h b/src/network_inspectors/appid/app_cpu_profile_table.h
index 8278608199cd436b1a18620e3107af062d8c8858..49ef0e2b550260c2bce794572202f298fe454a08 100644 (file)
--- a/src/network_inspectors/appid/app_cpu_profile_table.h
+++ b/src/network_inspectors/appid/app_cpu_profile_table.h
@@ -23,6 +23,7 @@
 
 #include <unordered_map>
 #include <vector>
+#include <mutex>
 
 #include "main/thread.h"
 #include "utils/util.h"
@@ -33,6 +34,12 @@
 class AppIdSession;
 class OdpContext;
 
+enum AppidCpuTableDisplayStatus {
+    DISPLAY_SUCCESS = 0,
+    DISPLAY_ERROR_TABLE_EMPTY,
+    DISPLAY_ERROR_APPID_PROFILER_RUNNING
+};
+
 struct AppidCPUProfilerStats {
     std::string app_name;
     uint64_t processing_time    = 0;
@@ -46,19 +53,20 @@ struct AppidCPUProfilerStats {
 
 class AppidCPUProfilingManager {
 private:
-    typedef std::unordered_map<AppId, AppidCPUProfilerStats> AppidCPUProfilingTable;
+    using AppidCPUProfilingTable = std::unordered_map<AppId, AppidCPUProfilerStats>;
     AppidCPUProfilingTable appid_cpu_profiling_table;
+    std::mutex appid_cpu_profiler_mutex;
         
 public:
-    AppidCPUProfilingManager();
+    AppidCPUProfilingManager() = default;
     
     void stats_bucket_insert(AppId appid, const char* app_name, uint64_t processing_time, uint64_t processed_packets);
     void insert_appid_cpu_profiler_record(AppId appId, const AppidCPUProfilerStats& stats);
     void check_appid_cpu_profiler_table_entry(const AppIdSession* asd, AppId service_id, AppId client_id, AppId payload_id, AppId misc_id);
     void check_appid_cpu_profiler_table_entry(const AppIdSession* asd, AppId payload_id);
 
-    void display_appid_cpu_profiler_table();
-    void display_appid_cpu_profiler_table(AppId appid);
+    AppidCpuTableDisplayStatus display_appid_cpu_profiler_table(OdpContext&, bool override_running_flag = false);
+    AppidCpuTableDisplayStatus display_appid_cpu_profiler_table(AppId, OdpContext&);
     
     void cleanup_appid_cpu_profiler_table();
 };

diff --git a/src/network_inspectors/appid/app_info_table.cc b/src/network_inspectors/appid/app_info_table.cc
index 1fd2c0e3a86606f4e0abb1d49b862a37ff5ed990..c87c70fcad0b5d0fe8cd8772fbbc8dc1ce3be981 100644 (file)
--- a/src/network_inspectors/appid/app_info_table.cc
+++ b/src/network_inspectors/appid/app_info_table.cc
@@ -622,10 +622,6 @@ void AppInfoManager::load_odp_config(OdpContext& odp_ctxt, const char* path)
                 {
                     odp_ctxt.appid_cpu_profiler = false;
                 }
-                else if (!(strcasecmp(conf_val, "enabled")))
-                {
-                    odp_ctxt.appid_cpu_profiler = true;
-                }
             }
             else
                 ParseWarning(WARN_CONF, "appid: unsupported configuration: %s\n", conf_key);

diff --git a/src/network_inspectors/appid/appid_config.cc b/src/network_inspectors/appid/appid_config.cc
index 9ef62ccc8d952005875d94fe38cfb02aa240b68d..ebcb1fd3bc1abb769f9013df61bde5681d5e3ba9 100644 (file)
--- a/src/network_inspectors/appid/appid_config.cc
+++ b/src/network_inspectors/appid/appid_config.cc
@@ -111,8 +111,8 @@ void AppIdContext::pterm()
     if (odp_ctxt)
     {
         odp_ctxt->get_app_info_mgr().cleanup_appid_info_table();
-        if (odp_ctxt->is_appid_cpu_profiler_running())
-            odp_ctxt->get_appid_cpu_profiler_mgr().display_appid_cpu_profiler_table();
+        if (odp_ctxt->is_appid_cpu_profiler_enabled())
+            odp_ctxt->get_appid_cpu_profiler_mgr().display_appid_cpu_profiler_table(*odp_ctxt, true);
 
         odp_ctxt->get_appid_cpu_profiler_mgr().cleanup_appid_cpu_profiler_table();
         delete odp_ctxt;

diff --git a/src/network_inspectors/appid/appid_config.h b/src/network_inspectors/appid/appid_config.h
index 8818e4cf6c564dedc3238c5eb13e37782628323b..516e2ac77af5d52b224efa5205d3f98e3d8c1b4a 100644 (file)
--- a/src/network_inspectors/appid/appid_config.h
+++ b/src/network_inspectors/appid/appid_config.h
@@ -144,7 +144,7 @@ public:
     uint16_t max_packet_service_fail_ignore_bytes = DEFAULT_MAX_PKT_BEFORE_SERVICE_FAIL_IGNORE_BYTES;
     FirstPktAppIdDiscovered first_pkt_appid_prefix = NO_APPID_FOUND;
     bool eve_http_client = true;
-    bool appid_cpu_profiler = false;
+    bool appid_cpu_profiler = true;
 
     OdpContext(const AppIdConfig&, snort::SnortConfig*);
     void initialize(AppIdInspector& inspector);

diff --git a/src/network_inspectors/appid/appid_module.cc b/src/network_inspectors/appid/appid_module.cc
index c6c4c85f06f5aa329f81164c51b683e71438eff8..ef5b88fffb3a703eb24972c1fcfee31ea3914330 100644 (file)
--- a/src/network_inspectors/appid/appid_module.cc
+++ b/src/network_inspectors/appid/appid_module.cc
@@ -50,6 +50,7 @@
 #include "appid_inspector.h"
 #include "appid_peg_counts.h"
 #include "service_state.h"
+#include "app_cpu_profile_table.h"
 
 using namespace snort;
 using namespace std;
@@ -392,7 +393,6 @@ static void clear_dynamic_host_cache_services()
     }
 }
 
-
 static int show_cpu_profiler_stats(lua_State* L)
 {
     int appid = luaL_optint(L, 1, 0);
@@ -404,14 +404,27 @@ static int show_cpu_profiler_stats(lua_State* L)
         return 0;
     }
     const AppIdContext& ctxt = inspector->get_ctxt();
-    OdpContext& odp_ctxt = ctxt.get_odp_ctxt();
+    OdpContext& odp_ctxt = ctxt.get_odp_ctxt(); 
+
     if (odp_ctxt.is_appid_cpu_profiler_enabled())
     {
+        AppidCpuTableDisplayStatus displayed = DISPLAY_SUCCESS;
         ctrlcon->respond("== showing appid cpu profiler table\n");
         if (!appid)
-            odp_ctxt.get_appid_cpu_profiler_mgr().display_appid_cpu_profiler_table();
+            displayed = odp_ctxt.get_appid_cpu_profiler_mgr().display_appid_cpu_profiler_table(odp_ctxt);
         else
-            odp_ctxt.get_appid_cpu_profiler_mgr().display_appid_cpu_profiler_table(appid);
+            displayed = odp_ctxt.get_appid_cpu_profiler_mgr().display_appid_cpu_profiler_table(appid, odp_ctxt);
+
+        switch (displayed){
+            case DISPLAY_ERROR_TABLE_EMPTY:
+                ctrlcon->respond("== appid cpu profiler table is empty\n");
+                break;
+            case DISPLAY_ERROR_APPID_PROFILER_RUNNING:
+                ctrlcon->respond("== appid cpu profiler is still running\n");
+                break;
+            case DISPLAY_SUCCESS:
+                break;
+        }
     }
     else
         ctrlcon->respond("appid cpu profiler is disabled\n");

diff --git a/src/network_inspectors/appid/detector_plugins/test/detector_plugins_mock.h b/src/network_inspectors/appid/detector_plugins/test/detector_plugins_mock.h
index c9a397094225ee74cc1ed38515b6f52039aded29..da39e9882ea5dc976f2029033ccfaa4ac32623c0 100644 (file)
--- a/src/network_inspectors/appid/detector_plugins/test/detector_plugins_mock.h
+++ b/src/network_inspectors/appid/detector_plugins/test/detector_plugins_mock.h
@@ -206,9 +206,6 @@ AppInfoTableEntry* AppInfoManager::get_app_info_entry(AppId, const AppInfoTable&
     return nullptr;
 }
 
-
-AppidCPUProfilingManager::AppidCPUProfilingManager() { }
-
 bool AppIdReloadTuner::tinit() { return false; }
 
 bool AppIdReloadTuner::tune_resources(unsigned int)

diff --git a/src/network_inspectors/appid/service_plugins/test/service_plugin_mock.h b/src/network_inspectors/appid/service_plugins/test/service_plugin_mock.h
index 0e255657db93c8b5d2014aa6154560d3c3bd9702..2e1dc719791b01c53d326d7a4ca9a36cf59cad8d 100644 (file)
--- a/src/network_inspectors/appid/service_plugins/test/service_plugin_mock.h
+++ b/src/network_inspectors/appid/service_plugins/test/service_plugin_mock.h
@@ -196,7 +196,6 @@ ServiceDiscoveryState* AppIdServiceState::add(SfIp const*, IpProtocol,
 {
   return nullptr;
 }
-AppidCPUProfilingManager::AppidCPUProfilingManager() { }
 
 void ServiceDiscoveryState::set_service_id_valid(ServiceDetector*) { }
 

diff --git a/src/network_inspectors/appid/test/appid_debug_test.cc b/src/network_inspectors/appid/test/appid_debug_test.cc
index e1abc0a14417706fafddfcdcf224eb12b6b450b4..3275eb06d220dc09fc7affb7d14367361eda5baa 100644 (file)
--- a/src/network_inspectors/appid/test/appid_debug_test.cc
+++ b/src/network_inspectors/appid/test/appid_debug_test.cc
@@ -70,7 +70,6 @@ public:
 
 AppIdConfig::~AppIdConfig() = default;
 OdpContext::OdpContext(const AppIdConfig&, snort::SnortConfig*) { }
-AppidCPUProfilingManager::AppidCPUProfilingManager() {}
 
 AppIdConfig stub_config;
 AppIdContext stub_ctxt(stub_config);

diff --git a/src/network_inspectors/appid/test/appid_http_session_test.cc b/src/network_inspectors/appid/test/appid_http_session_test.cc
index 9c9eb7c1c4994432d7be3484711e3b4e23364378..ff64eb9ceb083ed1a1ce2ff69910ea128755d0ed 100644 (file)
--- a/src/network_inspectors/appid/test/appid_http_session_test.cc
+++ b/src/network_inspectors/appid/test/appid_http_session_test.cc
@@ -176,7 +176,6 @@ void Profiler::reset_stats(snort::ProfilerType) { }
 void Profiler::show_stats() { }
 
 OdpContext::OdpContext(const AppIdConfig&, snort::SnortConfig*) { }
-AppidCPUProfilingManager::AppidCPUProfilingManager() { }
 
 AppIdConfig::~AppIdConfig() = default;
 

diff --git a/src/network_inspectors/appid/test/appid_mock_session.h b/src/network_inspectors/appid/test/appid_mock_session.h
index d89e099b44dcdf4cb0e62b2c693eeb1925a9d656..61d6ec10ba45ca2841abd3bc552b54a33896e2a0 100644 (file)
--- a/src/network_inspectors/appid/test/appid_mock_session.h
+++ b/src/network_inspectors/appid/test/appid_mock_session.h
@@ -210,6 +210,4 @@ bool AppIdSession::is_tp_appid_available() const
     return true;
 }
 
-AppidCPUProfilingManager::AppidCPUProfilingManager() { }
-
 #endif

diff --git a/src/network_inspectors/appid/test/service_state_test.cc b/src/network_inspectors/appid/test/service_state_test.cc
index 7ff85e657c545471fa7b056a2f9c98300d969087..658f03f6f5d79c9263fc2e2dcb66786e5641465b 100644 (file)
--- a/src/network_inspectors/appid/test/service_state_test.cc
+++ b/src/network_inspectors/appid/test/service_state_test.cc
@@ -104,7 +104,6 @@ void ServiceAppDescriptor::set_port_service_id(AppId){}
 void ClientAppDescriptor::update_user(AppId, const char*, AppidChangeBits&){}
 AppIdConfig::~AppIdConfig() = default;
 OdpContext::OdpContext(const AppIdConfig&, snort::SnortConfig*) { }
-AppidCPUProfilingManager::AppidCPUProfilingManager() {}
 AppIdConfig stub_config;
 AppIdContext stub_ctxt(stub_config);
 OdpContext stub_odp_ctxt(stub_config, nullptr);

diff --git a/src/network_inspectors/appid/test/tp_lib_handler_test.cc b/src/network_inspectors/appid/test/tp_lib_handler_test.cc
index 421e6f7a6cb9bcb82931d99cf54cbd04716f2a2a..7cf38c627ae20205c9365488c70d0e9a286f08f1 100644 (file)
--- a/src/network_inspectors/appid/test/tp_lib_handler_test.cc
+++ b/src/network_inspectors/appid/test/tp_lib_handler_test.cc
@@ -68,7 +68,6 @@ SslPatternMatchers::~SslPatternMatchers() = default;
 AlpnPatternMatchers::~AlpnPatternMatchers() = default;
 CipPatternMatchers::~CipPatternMatchers() = default;
 AppIdConfig::~AppIdConfig() = default;
-AppidCPUProfilingManager::AppidCPUProfilingManager() = default;
 OdpContext::OdpContext(const AppIdConfig&, snort::SnortConfig*) { }
 void ServiceDiscovery::initialize(AppIdInspector&) { }
 void ServiceDiscovery::reload() { }