From: Umang Sharma (umasharm) Date: Fri, 24 May 2024 18:50:44 +0000 (+0000) Subject: Pull request #4318: appid: re-enabling appid cpu profiler and crash fix X-Git-Tag: 3.2.2.0~9 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=9a1ea4f2744793fc3bd14dfbe65e245ff7a6ece2;p=thirdparty%2Fsnort3.git 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 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 c4bd1cfa4..a8e433f31 100644 --- 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& a, const std::pair& 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::vector>, 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 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 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 827860819..49ef0e2b5 100644 --- 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 #include +#include #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 AppidCPUProfilingTable; + using AppidCPUProfilingTable = std::unordered_map; 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 1fd2c0e3a..c87c70fca 100644 --- 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 9ef62ccc8..ebcb1fd3b 100644 --- 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 8818e4cf6..516e2ac77 100644 --- 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 c6c4c85f0..ef5b88fff 100644 --- 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 c9a397094..da39e9882 100644 --- 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 0e255657d..2e1dc7197 100644 --- 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 e1abc0a14..3275eb06d 100644 --- 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 9c9eb7c1c..ff64eb9ce 100644 --- 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 d89e099b4..61d6ec10b 100644 --- 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 7ff85e657..658f03f6f 100644 --- 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 421e6f7a6..7cf38c627 100644 --- 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() { }