From f0a713adc308dc2246ff75c42cea8c3f2ba8415c Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Sun, 26 Jul 2026 16:29:49 +1000 Subject: [PATCH 01/16] [dhcpmon]: Bound packet callback work Limit each raw-socket callback to 64 packets and reset the recvfrom address length before every receive so the event loop remains responsive. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83 Signed-off-by: Xichen96 --- src/packet_handler.cpp | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/packet_handler.cpp b/src/packet_handler.cpp index 7ec5d07a3..9f38ed5b2 100644 --- a/src/packet_handler.cpp +++ b/src/packet_handler.cpp @@ -16,6 +16,8 @@ #include "dhcp_check_profile.h" /** to get dhcp/v6 check profile */ #include "util.h" +static constexpr int MAX_PACKETS_PER_CALLBACK = 64; + /** * @code _increase_cache_counter(ifname, sock, type); * @brief helper function to increase cache counter. Simple increase of counter, no complications. In the event of @@ -864,8 +866,13 @@ void callback_common(int fd, short event, void *arg) socklen_t slen = sizeof(sll); sock_info_t &sock_info = sock_mgr_get_sock_info(fd); - while ((buffer_sz = recvfrom(fd, sock_info.buffer, sock_info.snaplen, MSG_DONTWAIT, (struct sockaddr *)&sll, &slen)) > 0) - { + for (int packet_count = 0; packet_count < MAX_PACKETS_PER_CALLBACK; packet_count++) { + slen = sizeof(sll); + buffer_sz = recvfrom(fd, sock_info.buffer, sock_info.snaplen, MSG_DONTWAIT, + (struct sockaddr *)&sll, &slen); + if (buffer_sz <= 0) { + break; + } char ifname_buf[IF_NAMESIZE]; if (if_indextoname(sll.sll_ifindex, ifname_buf) == NULL) { syslog_debug(LOG_WARNING, "if_indextoname: invalid input interface index %d %s", sll.sll_ifindex, strerror(errno)); From b03bcec028c8c900e202b831a98dc383af36f2b6 Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Sun, 26 Jul 2026 16:33:48 +1000 Subject: [PATCH 02/16] [dhcpmon]: Synchronize counter state access Use writer-priority shared/exclusive guards so packet callbacks cannot race health sampling, snapshot, counter-clear, or COUNTERS_DB synchronization. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83 Signed-off-by: Xichen96 --- src/dhcp_mon.cpp | 27 ++++++++++-- src/packet_handler.cpp | 4 ++ src/sock_mgr.cpp | 93 ++++++++++++++++++++++++++++++++++++++++++ src/sock_mgr.h | 29 +++++++++++++ 4 files changed, 150 insertions(+), 3 deletions(-) diff --git a/src/dhcp_mon.cpp b/src/dhcp_mon.cpp index ef4f6623d..3504737a8 100644 --- a/src/dhcp_mon.cpp +++ b/src/dhcp_mon.cpp @@ -212,9 +212,14 @@ static void cleanup_stale_db_counters() static void signal_callback(evutil_socket_t fd, short event, void *arg) { syslog(LOG_INFO, "Received signal: %s", strsignal(fd)); - - dhcp_devman_print_all_status(DHCP_COUNTERS_CURRENT); - dhcp_devman_print_all_status(DHCP_COUNTERS_CURRENT_V6); + + { + counter_state_write_lock counter_lock; + if (counter_lock.owns_lock()) { + dhcp_devman_print_all_status(DHCP_COUNTERS_CURRENT); + dhcp_devman_print_all_status(DHCP_COUNTERS_CURRENT_V6); + } + } if ((fd == SIGTERM) || (fd == SIGINT)) { syslog(LOG_INFO, "Received signal to stop dhcpmon"); @@ -223,6 +228,10 @@ static void signal_callback(evutil_socket_t fd, short event, void *arg) if (fd == SIGUSR1) { // we need to sync cache counter from COUNTERS_DB syslog(LOG_INFO, "Received signal to stop writing to DB counter"); + counter_state_write_lock counter_lock; + if (!counter_lock.owns_lock()) { + return; + } std::lock_guard lock(db_sync_mutex); sock_mgr_pause_write_cache_to_db(); syslog(LOG_INFO, "Stopped writing to DB counter"); @@ -260,6 +269,10 @@ static void update_cache_counter_callback(evutil_socket_t fd, short event, void syslog(LOG_INFO, "Start updating %s cache counter from DB counter", sock_info.name); + counter_state_write_lock counter_lock; + if (!counter_lock.owns_lock()) { + return; + } std::lock_guard lock(db_sync_mutex); // can only sync db to cache counter and db updater is paused, otherwise its unexpected @@ -392,6 +405,10 @@ static void update_cache_counter_callback(evutil_socket_t fd, short event, void static void timeout_callback(evutil_socket_t fd, short event, void *arg) { syslog_debug(LOG_INFO, "Received timeout signal for DHCP relay health check"); + counter_state_write_lock counter_lock; + if (!counter_lock.owns_lock()) { + return; + } dhcp_devman_print_all_status_debug(DHCP_COUNTERS_CURRENT); dhcp_devman_print_all_status_debug(DHCP_COUNTERS_SNAPSHOT); @@ -418,6 +435,10 @@ static void db_update_callback(evutil_socket_t fd, short event, void *arg) { syslog_debug(LOG_INFO, "Received db update signal"); syslog_debug(LOG_INFO, "Sync cache counter to DB counter"); + counter_state_write_lock counter_lock; + if (!counter_lock.owns_lock()) { + return; + } std::lock_guard lock(db_sync_mutex); // If there is clear counter going on and its been longer than expected // consider the clear counter operation failed so we don't block db update forever diff --git a/src/packet_handler.cpp b/src/packet_handler.cpp index 9f38ed5b2..aa564c1a0 100644 --- a/src/packet_handler.cpp +++ b/src/packet_handler.cpp @@ -861,6 +861,10 @@ void packet_handler_v6(int sock, const std::string &ifname, const dhcp_device_co void callback_common(int fd, short event, void *arg) { + counter_state_read_lock counter_lock; + if (!counter_lock.owns_lock()) { + return; + } ssize_t buffer_sz; struct sockaddr_ll sll; socklen_t slen = sizeof(sll); diff --git a/src/sock_mgr.cpp b/src/sock_mgr.cpp index 8d3e48d81..114d2cfa3 100644 --- a/src/sock_mgr.cpp +++ b/src/sock_mgr.cpp @@ -11,6 +11,9 @@ #include #include #include +#include +#include +#include #include #include "sock_mgr.h" @@ -40,10 +43,100 @@ static const char cache_counter_updater_tag[] = "CacheCounterUpdater"; /* sock fd to sock_info mapping */ std::unordered_map sock_map; +std::shared_mutex counter_state_mutex; +std::atomic counter_state_writers_pending{0}; +static std::mutex counter_state_wait_mutex; +static std::condition_variable counter_state_wait_cv; + extern std::shared_ptr mCountersDbPtr; extern std::string downstream_ifname; +counter_state_write_lock::counter_state_write_lock() +{ + { + std::lock_guard wait_lock(counter_state_wait_mutex); + counter_state_writers_pending.fetch_add(1, std::memory_order_acq_rel); + } + try { + lock = std::unique_lock(counter_state_mutex); + } catch (const std::system_error &e) { + bool notify = false; + { + std::lock_guard wait_lock(counter_state_wait_mutex); + notify = counter_state_writers_pending.fetch_sub(1, std::memory_order_acq_rel) == 1; + } + if (notify) { + counter_state_wait_cv.notify_all(); + } + syslog(LOG_ALERT, "Failed to lock DHCP counter state: %s", e.what()); + } +} + +counter_state_write_lock::~counter_state_write_lock() +{ + if (!lock.owns_lock()) { + return; + } + lock.unlock(); + bool notify = false; + { + std::lock_guard wait_lock(counter_state_wait_mutex); + notify = counter_state_writers_pending.fetch_sub(1, std::memory_order_acq_rel) == 1; + } + if (notify) { + counter_state_wait_cv.notify_all(); + } +} + +bool counter_state_write_lock::owns_lock() const +{ + return lock.owns_lock(); +} + +counter_state_read_lock::counter_state_read_lock() +{ + if (counter_state_writers_pending.load(std::memory_order_acquire) == 0) { + try { + lock = std::shared_lock(counter_state_mutex, std::try_to_lock); + } catch (const std::system_error &e) { + syslog(LOG_ALERT, "Failed to lock DHCP counter state for packet handling: %s", e.what()); + return; + } + if (lock.owns_lock() && + counter_state_writers_pending.load(std::memory_order_acquire) == 0) { + return; + } + if (lock.owns_lock()) { + lock.unlock(); + } + } + + while (true) { + { + std::unique_lock wait_lock(counter_state_wait_mutex); + counter_state_wait_cv.wait(wait_lock, [] { + return counter_state_writers_pending.load(std::memory_order_acquire) == 0; + }); + } + try { + lock = std::shared_lock(counter_state_mutex); + } catch (const std::system_error &e) { + syslog(LOG_ALERT, "Failed to lock DHCP counter state for packet handling: %s", e.what()); + return; + } + if (counter_state_writers_pending.load(std::memory_order_acquire) == 0) { + return; + } + lock.unlock(); + } +} + +bool counter_state_read_lock::owns_lock() const +{ + return lock.owns_lock(); +} + /** * @code opensocket(); * diff --git a/src/sock_mgr.h b/src/sock_mgr.h index 9619a9526..8d17b56c2 100644 --- a/src/sock_mgr.h +++ b/src/sock_mgr.h @@ -9,7 +9,10 @@ #ifndef SOCKET_MANAGER_H_ #define SOCKET_MANAGER_H_ +#include +#include #include +#include #include #include #include @@ -41,6 +44,32 @@ typedef struct { /** sock file descriptors, serve as the identifier of all related information described in sock_info_t */ extern int rx_sock, tx_sock, rx_sock_v6, tx_sock_v6; +extern std::shared_mutex counter_state_mutex; +extern std::atomic counter_state_writers_pending; + +class counter_state_write_lock +{ + public: + counter_state_write_lock(); + ~counter_state_write_lock(); + bool owns_lock() const; + counter_state_write_lock(const counter_state_write_lock &) = delete; + counter_state_write_lock &operator=(const counter_state_write_lock &) = delete; + + private: + std::unique_lock lock; +}; + +class counter_state_read_lock +{ + public: + counter_state_read_lock(); + bool owns_lock() const; + + private: + std::shared_lock lock; +}; + /** Initialize socket manager with given snaplen */ int sock_mgr_init(uint32_t snaplen); From b89f78559f890e0b5eb6aa788d7d245df27fc13c Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Sun, 26 Jul 2026 13:52:11 +1000 Subject: [PATCH 03/16] [dhcpmon]: Write DB counters from a stable snapshot Copy counter maps while holding counter-state and DB synchronization locks, then release packet callbacks before Redis I/O while retaining DB serialization through writeback. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83 Signed-off-by: Xichen96 --- src/dhcp_mon.cpp | 37 +++++++++++++++++++++---------------- src/sock_mgr.cpp | 21 ++++++++++++++++++--- src/sock_mgr.h | 5 +++++ 3 files changed, 44 insertions(+), 19 deletions(-) diff --git a/src/dhcp_mon.cpp b/src/dhcp_mon.cpp index 3504737a8..10bf9c561 100644 --- a/src/dhcp_mon.cpp +++ b/src/dhcp_mon.cpp @@ -435,26 +435,31 @@ static void db_update_callback(evutil_socket_t fd, short event, void *arg) { syslog_debug(LOG_INFO, "Received db update signal"); syslog_debug(LOG_INFO, "Sync cache counter to DB counter"); - counter_state_write_lock counter_lock; - if (!counter_lock.owns_lock()) { - return; - } - std::lock_guard lock(db_sync_mutex); - // If there is clear counter going on and its been longer than expected - // consider the clear counter operation failed so we don't block db update forever - if (!sock_mgr_pause_write_cache_to_db_all_cleared() && last_update_time != default_time_point) { - auto now = std::chrono::steady_clock::now(); - auto elapsed = std::chrono::duration_cast(now - last_update_time); - if (elapsed.count() >= clear_counter_timeout) { - syslog(LOG_WARNING, "Clear counter going on for too long, abort clear counter"); - sock_mgr_clear_pause_write_cache_to_db(); - } else { - syslog(LOG_INFO, "Clear counter is ongoing, skip syncing write cache counter to DB counter"); + socket_counters_t counters_by_socket; + std::unique_lock lock; + { + counter_state_write_lock counter_lock; + if (!counter_lock.owns_lock()) { return; } + lock = std::unique_lock(db_sync_mutex); + // If there is clear counter going on and its been longer than expected + // consider the clear counter operation failed so we don't block db update forever + if (!sock_mgr_pause_write_cache_to_db_all_cleared() && last_update_time != default_time_point) { + auto now = std::chrono::steady_clock::now(); + auto elapsed = std::chrono::duration_cast(now - last_update_time); + if (elapsed.count() >= clear_counter_timeout) { + syslog(LOG_WARNING, "Clear counter going on for too long, abort clear counter"); + sock_mgr_clear_pause_write_cache_to_db(); + } else { + syslog(LOG_INFO, "Clear counter is ongoing, skip syncing write cache counter to DB counter"); + return; + } + } + counters_by_socket = sock_mgr_copy_cache_counters(); } last_update_time = std::chrono::steady_clock::now(); - sock_mgr_update_db_counters(); + sock_mgr_update_db_counters(counters_by_socket); cleanup_stale_db_counters(); syslog_debug(LOG_INFO, "Successfully synced cache counter to DB counter"); } diff --git a/src/sock_mgr.cpp b/src/sock_mgr.cpp index 114d2cfa3..210597504 100644 --- a/src/sock_mgr.cpp +++ b/src/sock_mgr.cpp @@ -707,17 +707,27 @@ bool sock_mgr_all_cache_counters_initialized(const std::string &ifname) return true; } -void sock_mgr_update_db_counters() +socket_counters_t sock_mgr_copy_cache_counters() +{ + socket_counters_t counters_by_socket; + for (const auto &[sock, info] : sock_map) { + counters_by_socket.emplace(sock, info.all_counters); + } + return counters_by_socket; +} + +void sock_mgr_update_db_counters(const socket_counters_t &counters_by_socket) { syslog_debug(LOG_INFO, "Updating all cache counters to DB counters"); - for (const auto &[sock, info] : sock_map) { + for (const auto &[sock, all_counters] : counters_by_socket) { + const sock_info_t &info = sock_mgr_get_sock_info(sock); syslog_debug(LOG_INFO, "Start updating socket %d %s DB counter from cache counter", sock, info.name); int msg_type_count = info.is_v6 ? DHCPV6_MESSAGE_TYPE_COUNT : DHCP_MESSAGE_TYPE_COUNT; const std::string *msg_type_name = info.is_v6 ? db_counter_name_v6 : db_counter_name; std::string all_ifname; std::string all_skipped_ifname; - for (const auto &[ifname, counter] : info.all_counters) { + for (const auto &[ifname, counter] : all_counters) { if (is_agg_counter(ifname) == true) { all_skipped_ifname += ifname + ", "; continue; @@ -732,4 +742,9 @@ void sock_mgr_update_db_counters() syslog_debug(LOG_INFO, "Skipped aggregated device counter entry of %sfor downstream vlan %s", all_skipped_ifname.c_str(), downstream_ifname.c_str()); } +} + +void sock_mgr_update_db_counters() +{ + sock_mgr_update_db_counters(sock_mgr_copy_cache_counters()); } \ No newline at end of file diff --git a/src/sock_mgr.h b/src/sock_mgr.h index 8d17b56c2..aade0624a 100644 --- a/src/sock_mgr.h +++ b/src/sock_mgr.h @@ -22,6 +22,7 @@ typedef std::unordered_map counter_t; typedef std::unordered_map all_counters_t; +typedef std::unordered_map socket_counters_t; /** struct for socket information */ typedef struct { @@ -135,5 +136,9 @@ bool sock_mgr_all_cache_counters_initialized(const std::string &ifname); /** Update database counters from cache counters for all sockets */ void sock_mgr_update_db_counters(); +void sock_mgr_update_db_counters(const socket_counters_t &counters_by_socket); + +/** Copy cache counters for all sockets */ +socket_counters_t sock_mgr_copy_cache_counters(); #endif /* SOCKET_MANAGER_H_ */ From ee3f4d960f4adec608f19fd61fed610623287cce Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Sun, 26 Jul 2026 14:14:15 +1000 Subject: [PATCH 04/16] [dhcpmon]: Skip unknown counter snapshot sockets Validate snapshot socket keys before looking up metadata so malformed or stale caller data cannot throw from sock_map.at(). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83 Signed-off-by: Xichen96 --- src/sock_mgr.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/sock_mgr.cpp b/src/sock_mgr.cpp index 210597504..599c906ec 100644 --- a/src/sock_mgr.cpp +++ b/src/sock_mgr.cpp @@ -721,7 +721,12 @@ void sock_mgr_update_db_counters(const socket_counters_t &counters_by_socket) syslog_debug(LOG_INFO, "Updating all cache counters to DB counters"); for (const auto &[sock, all_counters] : counters_by_socket) { - const sock_info_t &info = sock_mgr_get_sock_info(sock); + const auto sock_info = sock_map.find(sock); + if (sock_info == sock_map.end()) { + syslog(LOG_WARNING, "Skip DB counter snapshot for unknown socket %d", sock); + continue; + } + const sock_info_t &info = sock_info->second; syslog_debug(LOG_INFO, "Start updating socket %d %s DB counter from cache counter", sock, info.name); int msg_type_count = info.is_v6 ? DHCPV6_MESSAGE_TYPE_COUNT : DHCP_MESSAGE_TYPE_COUNT; const std::string *msg_type_name = info.is_v6 ? db_counter_name_v6 : db_counter_name; From c84af913b112f8fb4480dcd1048cd20d8a7e48f3 Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Sun, 26 Jul 2026 17:37:04 +1000 Subject: [PATCH 05/16] [dhcpmon]: Avoid blocking counters while waiting for DB Acquire DB serialization before the counter writer guard and document snapshot preconditions, preserving one lock order while packet callbacks continue during Redis waits and writes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83 Signed-off-by: Xichen96 --- src/dhcp_mon.cpp | 7 +++---- src/sock_mgr.h | 3 ++- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/dhcp_mon.cpp b/src/dhcp_mon.cpp index 10bf9c561..b9022f69e 100644 --- a/src/dhcp_mon.cpp +++ b/src/dhcp_mon.cpp @@ -228,11 +228,11 @@ static void signal_callback(evutil_socket_t fd, short event, void *arg) if (fd == SIGUSR1) { // we need to sync cache counter from COUNTERS_DB syslog(LOG_INFO, "Received signal to stop writing to DB counter"); + std::lock_guard lock(db_sync_mutex); counter_state_write_lock counter_lock; if (!counter_lock.owns_lock()) { return; } - std::lock_guard lock(db_sync_mutex); sock_mgr_pause_write_cache_to_db(); syslog(LOG_INFO, "Stopped writing to DB counter"); mStateDbPtr->hset(STATE_DB_COUNTER_UPDATE_PREFIX + downstream_ifname, "pause_write_to_db", "done"); @@ -269,11 +269,11 @@ static void update_cache_counter_callback(evutil_socket_t fd, short event, void syslog(LOG_INFO, "Start updating %s cache counter from DB counter", sock_info.name); + std::lock_guard lock(db_sync_mutex); counter_state_write_lock counter_lock; if (!counter_lock.owns_lock()) { return; } - std::lock_guard lock(db_sync_mutex); // can only sync db to cache counter and db updater is paused, otherwise its unexpected if (!sock_info.pause_write_cache_to_db) { @@ -436,13 +436,12 @@ static void db_update_callback(evutil_socket_t fd, short event, void *arg) syslog_debug(LOG_INFO, "Received db update signal"); syslog_debug(LOG_INFO, "Sync cache counter to DB counter"); socket_counters_t counters_by_socket; - std::unique_lock lock; + std::unique_lock lock(db_sync_mutex); { counter_state_write_lock counter_lock; if (!counter_lock.owns_lock()) { return; } - lock = std::unique_lock(db_sync_mutex); // If there is clear counter going on and its been longer than expected // consider the clear counter operation failed so we don't block db update forever if (!sock_mgr_pause_write_cache_to_db_all_cleared() && last_update_time != default_time_point) { diff --git a/src/sock_mgr.h b/src/sock_mgr.h index aade0624a..04dab883e 100644 --- a/src/sock_mgr.h +++ b/src/sock_mgr.h @@ -136,9 +136,10 @@ bool sock_mgr_all_cache_counters_initialized(const std::string &ifname); /** Update database counters from cache counters for all sockets */ void sock_mgr_update_db_counters(); +/** Write a previously locked, immutable cache-counter snapshot */ void sock_mgr_update_db_counters(const socket_counters_t &counters_by_socket); -/** Copy cache counters for all sockets */ +/** Copy cache counters for all sockets; caller must hold counter_state_write_lock */ socket_counters_t sock_mgr_copy_cache_counters(); #endif /* SOCKET_MANAGER_H_ */ From abf43be9061d1fb43d1d4eed3b96d684aa029c42 Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Sun, 26 Jul 2026 17:51:23 +1000 Subject: [PATCH 06/16] [dhcpmon]: Remove unlocked DB update wrapper Expose only the immutable-snapshot writeback API so callers cannot copy live counter maps without the required counter-state guard. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83 Signed-off-by: Xichen96 --- src/sock_mgr.cpp | 5 ----- src/sock_mgr.h | 2 -- 2 files changed, 7 deletions(-) diff --git a/src/sock_mgr.cpp b/src/sock_mgr.cpp index 599c906ec..087377e16 100644 --- a/src/sock_mgr.cpp +++ b/src/sock_mgr.cpp @@ -748,8 +748,3 @@ void sock_mgr_update_db_counters(const socket_counters_t &counters_by_socket) all_skipped_ifname.c_str(), downstream_ifname.c_str()); } } - -void sock_mgr_update_db_counters() -{ - sock_mgr_update_db_counters(sock_mgr_copy_cache_counters()); -} \ No newline at end of file diff --git a/src/sock_mgr.h b/src/sock_mgr.h index 04dab883e..915218f78 100644 --- a/src/sock_mgr.h +++ b/src/sock_mgr.h @@ -134,8 +134,6 @@ void sock_mgr_init_cache_counters(const std::string &ifname, uint8_t dhcp_messag /** Check if cache counters are initialized for given ifname for all sockets */ bool sock_mgr_all_cache_counters_initialized(const std::string &ifname); -/** Update database counters from cache counters for all sockets */ -void sock_mgr_update_db_counters(); /** Write a previously locked, immutable cache-counter snapshot */ void sock_mgr_update_db_counters(const socket_counters_t &counters_by_socket); From 168d2d5b3cd594926aca695452f99028a96cc52f Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Sun, 26 Jul 2026 18:13:22 +1000 Subject: [PATCH 07/16] [dhcpmon]: Explain packet callback batch bound Document that the 64-packet batch keeps periodic health and DB events responsive during bursts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83 Signed-off-by: Xichen96 --- src/packet_handler.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/packet_handler.cpp b/src/packet_handler.cpp index aa564c1a0..10409ca0b 100644 --- a/src/packet_handler.cpp +++ b/src/packet_handler.cpp @@ -16,6 +16,7 @@ #include "dhcp_check_profile.h" /** to get dhcp/v6 check profile */ #include "util.h" +// Bound one callback to keep health/DB events responsive during packet bursts. static constexpr int MAX_PACKETS_PER_CALLBACK = 64; /** From 33d646fc6fa9b1bb71cf5ea62702acc516ece5b4 Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Sun, 26 Jul 2026 18:42:10 +1000 Subject: [PATCH 08/16] [dhcpmon]: Use C++14 shared timed mutex Use std::shared_timed_mutex for counter-state guards so the synchronization primitive does not depend on an implicit C++17 compiler default. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83 Signed-off-by: Xichen96 --- src/sock_mgr.cpp | 8 ++++---- src/sock_mgr.h | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/sock_mgr.cpp b/src/sock_mgr.cpp index 087377e16..f91777e6c 100644 --- a/src/sock_mgr.cpp +++ b/src/sock_mgr.cpp @@ -43,7 +43,7 @@ static const char cache_counter_updater_tag[] = "CacheCounterUpdater"; /* sock fd to sock_info mapping */ std::unordered_map sock_map; -std::shared_mutex counter_state_mutex; +std::shared_timed_mutex counter_state_mutex; std::atomic counter_state_writers_pending{0}; static std::mutex counter_state_wait_mutex; static std::condition_variable counter_state_wait_cv; @@ -59,7 +59,7 @@ counter_state_write_lock::counter_state_write_lock() counter_state_writers_pending.fetch_add(1, std::memory_order_acq_rel); } try { - lock = std::unique_lock(counter_state_mutex); + lock = std::unique_lock(counter_state_mutex); } catch (const std::system_error &e) { bool notify = false; { @@ -98,7 +98,7 @@ counter_state_read_lock::counter_state_read_lock() { if (counter_state_writers_pending.load(std::memory_order_acquire) == 0) { try { - lock = std::shared_lock(counter_state_mutex, std::try_to_lock); + lock = std::shared_lock(counter_state_mutex, std::try_to_lock); } catch (const std::system_error &e) { syslog(LOG_ALERT, "Failed to lock DHCP counter state for packet handling: %s", e.what()); return; @@ -120,7 +120,7 @@ counter_state_read_lock::counter_state_read_lock() }); } try { - lock = std::shared_lock(counter_state_mutex); + lock = std::shared_lock(counter_state_mutex); } catch (const std::system_error &e) { syslog(LOG_ALERT, "Failed to lock DHCP counter state for packet handling: %s", e.what()); return; diff --git a/src/sock_mgr.h b/src/sock_mgr.h index 915218f78..21ec272f6 100644 --- a/src/sock_mgr.h +++ b/src/sock_mgr.h @@ -45,7 +45,7 @@ typedef struct { /** sock file descriptors, serve as the identifier of all related information described in sock_info_t */ extern int rx_sock, tx_sock, rx_sock_v6, tx_sock_v6; -extern std::shared_mutex counter_state_mutex; +extern std::shared_timed_mutex counter_state_mutex; extern std::atomic counter_state_writers_pending; class counter_state_write_lock @@ -58,7 +58,7 @@ class counter_state_write_lock counter_state_write_lock &operator=(const counter_state_write_lock &) = delete; private: - std::unique_lock lock; + std::unique_lock lock; }; class counter_state_read_lock @@ -68,7 +68,7 @@ class counter_state_read_lock bool owns_lock() const; private: - std::shared_lock lock; + std::shared_lock lock; }; /** Initialize socket manager with given snaplen */ From d3bc0a589de9f724ec717b01c141ef86a248a81f Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Sun, 26 Jul 2026 18:44:41 +1000 Subject: [PATCH 09/16] [dhcpmon]: Require a writer token for counter snapshots Accept the active counter-state writer guard as an explicit snapshot API token so future callers cannot copy live unordered_maps without synchronization. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83 Signed-off-by: Xichen96 --- src/dhcp_mon.cpp | 2 +- src/sock_mgr.cpp | 6 +++++- src/sock_mgr.h | 4 ++-- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/dhcp_mon.cpp b/src/dhcp_mon.cpp index b9022f69e..624437819 100644 --- a/src/dhcp_mon.cpp +++ b/src/dhcp_mon.cpp @@ -455,7 +455,7 @@ static void db_update_callback(evutil_socket_t fd, short event, void *arg) return; } } - counters_by_socket = sock_mgr_copy_cache_counters(); + counters_by_socket = sock_mgr_copy_cache_counters(counter_lock); } last_update_time = std::chrono::steady_clock::now(); sock_mgr_update_db_counters(counters_by_socket); diff --git a/src/sock_mgr.cpp b/src/sock_mgr.cpp index f91777e6c..b231996bf 100644 --- a/src/sock_mgr.cpp +++ b/src/sock_mgr.cpp @@ -707,9 +707,13 @@ bool sock_mgr_all_cache_counters_initialized(const std::string &ifname) return true; } -socket_counters_t sock_mgr_copy_cache_counters() +socket_counters_t sock_mgr_copy_cache_counters(const counter_state_write_lock &counter_lock) { socket_counters_t counters_by_socket; + if (!counter_lock.owns_lock()) { + syslog(LOG_ALERT, "Cannot copy DHCP counters without the counter-state writer lock"); + return counters_by_socket; + } for (const auto &[sock, info] : sock_map) { counters_by_socket.emplace(sock, info.all_counters); } diff --git a/src/sock_mgr.h b/src/sock_mgr.h index 21ec272f6..57b77684a 100644 --- a/src/sock_mgr.h +++ b/src/sock_mgr.h @@ -137,7 +137,7 @@ bool sock_mgr_all_cache_counters_initialized(const std::string &ifname); /** Write a previously locked, immutable cache-counter snapshot */ void sock_mgr_update_db_counters(const socket_counters_t &counters_by_socket); -/** Copy cache counters for all sockets; caller must hold counter_state_write_lock */ -socket_counters_t sock_mgr_copy_cache_counters(); +/** Copy cache counters for all sockets while holding the supplied writer guard */ +socket_counters_t sock_mgr_copy_cache_counters(const counter_state_write_lock &counter_lock); #endif /* SOCKET_MANAGER_H_ */ From 525ead31ec9071bed9b025ee3251029c6c5b7033 Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Sun, 26 Jul 2026 18:59:18 +1000 Subject: [PATCH 10/16] [dhcpmon]: Keep signal status printing best effort Hide raw counter synchronization state behind RAII guards and use a nonblocking writer guard for signal-triggered status output so shutdown signals are never delayed by counter work. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83 Signed-off-by: Xichen96 --- src/dhcp_mon.cpp | 2 +- src/sock_mgr.cpp | 18 ++++++++++++++++-- src/sock_mgr.h | 5 ++--- 3 files changed, 19 insertions(+), 6 deletions(-) diff --git a/src/dhcp_mon.cpp b/src/dhcp_mon.cpp index 624437819..f91daaaa9 100644 --- a/src/dhcp_mon.cpp +++ b/src/dhcp_mon.cpp @@ -214,7 +214,7 @@ static void signal_callback(evutil_socket_t fd, short event, void *arg) syslog(LOG_INFO, "Received signal: %s", strsignal(fd)); { - counter_state_write_lock counter_lock; + counter_state_write_lock counter_lock(std::try_to_lock); if (counter_lock.owns_lock()) { dhcp_devman_print_all_status(DHCP_COUNTERS_CURRENT); dhcp_devman_print_all_status(DHCP_COUNTERS_CURRENT_V6); diff --git a/src/sock_mgr.cpp b/src/sock_mgr.cpp index b231996bf..1d815294b 100644 --- a/src/sock_mgr.cpp +++ b/src/sock_mgr.cpp @@ -43,8 +43,8 @@ static const char cache_counter_updater_tag[] = "CacheCounterUpdater"; /* sock fd to sock_info mapping */ std::unordered_map sock_map; -std::shared_timed_mutex counter_state_mutex; -std::atomic counter_state_writers_pending{0}; +static std::shared_timed_mutex counter_state_mutex; +static std::atomic counter_state_writers_pending{0}; static std::mutex counter_state_wait_mutex; static std::condition_variable counter_state_wait_cv; @@ -57,6 +57,7 @@ counter_state_write_lock::counter_state_write_lock() { std::lock_guard wait_lock(counter_state_wait_mutex); counter_state_writers_pending.fetch_add(1, std::memory_order_acq_rel); + registered_writer = true; } try { lock = std::unique_lock(counter_state_mutex); @@ -69,16 +70,29 @@ counter_state_write_lock::counter_state_write_lock() if (notify) { counter_state_wait_cv.notify_all(); } + registered_writer = false; syslog(LOG_ALERT, "Failed to lock DHCP counter state: %s", e.what()); } } +counter_state_write_lock::counter_state_write_lock(std::try_to_lock_t) +{ + try { + lock = std::unique_lock(counter_state_mutex, std::try_to_lock); + } catch (const std::system_error &e) { + syslog(LOG_ALERT, "Failed to try-lock DHCP counter state: %s", e.what()); + } +} + counter_state_write_lock::~counter_state_write_lock() { if (!lock.owns_lock()) { return; } lock.unlock(); + if (!registered_writer) { + return; + } bool notify = false; { std::lock_guard wait_lock(counter_state_wait_mutex); diff --git a/src/sock_mgr.h b/src/sock_mgr.h index 57b77684a..85838acaa 100644 --- a/src/sock_mgr.h +++ b/src/sock_mgr.h @@ -45,13 +45,11 @@ typedef struct { /** sock file descriptors, serve as the identifier of all related information described in sock_info_t */ extern int rx_sock, tx_sock, rx_sock_v6, tx_sock_v6; -extern std::shared_timed_mutex counter_state_mutex; -extern std::atomic counter_state_writers_pending; - class counter_state_write_lock { public: counter_state_write_lock(); + explicit counter_state_write_lock(std::try_to_lock_t); ~counter_state_write_lock(); bool owns_lock() const; counter_state_write_lock(const counter_state_write_lock &) = delete; @@ -59,6 +57,7 @@ class counter_state_write_lock private: std::unique_lock lock; + bool registered_writer = false; }; class counter_state_read_lock From 905cff20102a1c846024a7d50f661ca23c027335 Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Sun, 26 Jul 2026 19:02:51 +1000 Subject: [PATCH 11/16] [dhcpmon]: Fix counter synchronization log spacing Separate formatted interface lists from following words in cache and DB synchronization logs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83 Signed-off-by: Xichen96 --- src/dhcp_mon.cpp | 4 ++-- src/sock_mgr.cpp | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/dhcp_mon.cpp b/src/dhcp_mon.cpp index f91daaaa9..43ae01519 100644 --- a/src/dhcp_mon.cpp +++ b/src/dhcp_mon.cpp @@ -357,9 +357,9 @@ static void update_cache_counter_callback(evutil_socket_t fd, short event, void updated_intfs.insert(ifname); } - syslog(LOG_INFO, "Processing DB entry of %sfor downstream vlan %s", + syslog(LOG_INFO, "Processing DB entry of %s for downstream vlan %s", all_ifname.c_str(), downstream_ifname.c_str()); - syslog(LOG_INFO, "Skipped DB entry of %sbecause we are only interested in %s", + syslog(LOG_INFO, "Skipped DB entry of %s because we are only interested in %s", all_skipped_ifname.c_str(), downstream_ifname.c_str()); // log any cache counter entry not appearing in db counter. This is highly unexpected. diff --git a/src/sock_mgr.cpp b/src/sock_mgr.cpp index 1d815294b..cbac792a9 100644 --- a/src/sock_mgr.cpp +++ b/src/sock_mgr.cpp @@ -760,9 +760,9 @@ void sock_mgr_update_db_counters(const socket_counters_t &counters_by_socket) std::string table_name = construct_counter_db_table_key(ifname, info.is_v6); mCountersDbPtr->hset(table_name, info.is_rx ? "RX" : "TX", value); } - syslog_debug(LOG_INFO, "Processing cache counter entry of %sfor downstream vlan %s", + syslog_debug(LOG_INFO, "Processing cache counter entry of %s for downstream vlan %s", all_ifname.c_str(), downstream_ifname.c_str()); - syslog_debug(LOG_INFO, "Skipped aggregated device counter entry of %sfor downstream vlan %s", + syslog_debug(LOG_INFO, "Skipped aggregated device counter entry of %s for downstream vlan %s", all_skipped_ifname.c_str(), downstream_ifname.c_str()); } } From cda0c3e92e3c5014a5654f0438e52ee88085760d Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Sun, 26 Jul 2026 19:17:58 +1000 Subject: [PATCH 12/16] [dhcpmon]: Make counter guard teardown fail-safe Require C++14 explicitly and catch shared-mutex unlock failures so RAII destruction cannot terminate the daemon or corrupt the pending-writer count. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83 Signed-off-by: Xichen96 --- src/sock_mgr.cpp | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/sock_mgr.cpp b/src/sock_mgr.cpp index cbac792a9..7a9d2d60a 100644 --- a/src/sock_mgr.cpp +++ b/src/sock_mgr.cpp @@ -16,6 +16,10 @@ #include #include +#if __cplusplus < 201402L +#error "dhcpmon counter synchronization requires C++14 or newer" +#endif + #include "sock_mgr.h" #include "packet_handler.h" /** for attaching packet handler */ @@ -89,7 +93,12 @@ counter_state_write_lock::~counter_state_write_lock() if (!lock.owns_lock()) { return; } - lock.unlock(); + try { + lock.unlock(); + } catch (const std::system_error &e) { + syslog(LOG_ALERT, "Failed to unlock DHCP counter state: %s", e.what()); + return; + } if (!registered_writer) { return; } From 0df5a2ed30f52ea79b71ef0443f5bc5507336267 Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Sun, 26 Jul 2026 19:20:49 +1000 Subject: [PATCH 13/16] [dhcpmon]: Release counter lock before STATE_DB writes Keep DB serialization while limiting the counter writer guard to the in-memory pause-flag update in SIGUSR1 handling. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83 Signed-off-by: Xichen96 --- src/dhcp_mon.cpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/dhcp_mon.cpp b/src/dhcp_mon.cpp index 43ae01519..c3aa1a82f 100644 --- a/src/dhcp_mon.cpp +++ b/src/dhcp_mon.cpp @@ -229,11 +229,13 @@ static void signal_callback(evutil_socket_t fd, short event, void *arg) // we need to sync cache counter from COUNTERS_DB syslog(LOG_INFO, "Received signal to stop writing to DB counter"); std::lock_guard lock(db_sync_mutex); - counter_state_write_lock counter_lock; - if (!counter_lock.owns_lock()) { - return; + { + counter_state_write_lock counter_lock; + if (!counter_lock.owns_lock()) { + return; + } + sock_mgr_pause_write_cache_to_db(); } - sock_mgr_pause_write_cache_to_db(); syslog(LOG_INFO, "Stopped writing to DB counter"); mStateDbPtr->hset(STATE_DB_COUNTER_UPDATE_PREFIX + downstream_ifname, "pause_write_to_db", "done"); mStateDbPtr->hset(STATE_DB_COUNTER_UPDATE_V6_PREFIX + downstream_ifname, "pause_write_to_db", "done"); From 005b7d44b9d0dca4050b72f53249980329c69e45 Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Sun, 26 Jul 2026 19:49:22 +1000 Subject: [PATCH 14/16] [dhcpmon]: Keep counter unlocks RAII-owned Let unique/shared lock destructors release mutex ownership and use local candidate read locks, avoiding manual unlock exception paths while preserving writer priority. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83 Signed-off-by: Xichen96 --- src/sock_mgr.cpp | 40 +++++++++++++++------------------------- 1 file changed, 15 insertions(+), 25 deletions(-) diff --git a/src/sock_mgr.cpp b/src/sock_mgr.cpp index 7a9d2d60a..1554c5ca7 100644 --- a/src/sock_mgr.cpp +++ b/src/sock_mgr.cpp @@ -16,8 +16,8 @@ #include #include -#if __cplusplus < 201402L -#error "dhcpmon counter synchronization requires C++14 or newer" +#if __cplusplus < 201703L +#error "dhcpmon counter synchronization requires C++17 or newer" #endif #include "sock_mgr.h" @@ -93,17 +93,8 @@ counter_state_write_lock::~counter_state_write_lock() if (!lock.owns_lock()) { return; } - try { - lock.unlock(); - } catch (const std::system_error &e) { - syslog(LOG_ALERT, "Failed to unlock DHCP counter state: %s", e.what()); - return; - } - if (!registered_writer) { - return; - } bool notify = false; - { + if (registered_writer) { std::lock_guard wait_lock(counter_state_wait_mutex); notify = counter_state_writers_pending.fetch_sub(1, std::memory_order_acq_rel) == 1; } @@ -121,18 +112,17 @@ counter_state_read_lock::counter_state_read_lock() { if (counter_state_writers_pending.load(std::memory_order_acquire) == 0) { try { - lock = std::shared_lock(counter_state_mutex, std::try_to_lock); + std::shared_lock candidate(counter_state_mutex, + std::try_to_lock); + if (candidate.owns_lock() && + counter_state_writers_pending.load(std::memory_order_acquire) == 0) { + lock = std::move(candidate); + return; + } } catch (const std::system_error &e) { syslog(LOG_ALERT, "Failed to lock DHCP counter state for packet handling: %s", e.what()); return; } - if (lock.owns_lock() && - counter_state_writers_pending.load(std::memory_order_acquire) == 0) { - return; - } - if (lock.owns_lock()) { - lock.unlock(); - } } while (true) { @@ -143,15 +133,15 @@ counter_state_read_lock::counter_state_read_lock() }); } try { - lock = std::shared_lock(counter_state_mutex); + std::shared_lock candidate(counter_state_mutex); + if (counter_state_writers_pending.load(std::memory_order_acquire) == 0) { + lock = std::move(candidate); + return; + } } catch (const std::system_error &e) { syslog(LOG_ALERT, "Failed to lock DHCP counter state for packet handling: %s", e.what()); return; } - if (counter_state_writers_pending.load(std::memory_order_acquire) == 0) { - return; - } - lock.unlock(); } } From 825a1093f177f5f0ab36386e311222d05e76b11f Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Sun, 26 Jul 2026 20:02:34 +1000 Subject: [PATCH 15/16] [dhcpmon]: Reserve counter snapshot socket map Reserve the known socket count before copying counter maps to avoid rehashing while the counter writer guard is held. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83 Signed-off-by: Xichen96 --- src/sock_mgr.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/sock_mgr.cpp b/src/sock_mgr.cpp index 1554c5ca7..85878d7d2 100644 --- a/src/sock_mgr.cpp +++ b/src/sock_mgr.cpp @@ -727,6 +727,7 @@ socket_counters_t sock_mgr_copy_cache_counters(const counter_state_write_lock &c syslog(LOG_ALERT, "Cannot copy DHCP counters without the counter-state writer lock"); return counters_by_socket; } + counters_by_socket.reserve(sock_map.size()); for (const auto &[sock, info] : sock_map) { counters_by_socket.emplace(sock, info.all_counters); } From 063973364af8fda676f1908ab752bbbce8a62d06 Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Sun, 26 Jul 2026 20:12:20 +1000 Subject: [PATCH 16/16] [dhcpmon]: Pin the C++17 build standard Compile C++ sources explicitly as gnu++17 and include utility directly for std::move, making the counter guard build requirements deterministic. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83 Signed-off-by: Xichen96 --- src/sock_mgr.cpp | 1 + src/subdir.mk | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/sock_mgr.cpp b/src/sock_mgr.cpp index 85878d7d2..845e0ade2 100644 --- a/src/sock_mgr.cpp +++ b/src/sock_mgr.cpp @@ -15,6 +15,7 @@ #include #include #include +#include #if __cplusplus < 201703L #error "dhcpmon counter synchronization requires C++17 or newer" diff --git a/src/subdir.mk b/src/subdir.mk index 4708a6fa5..e44d3f471 100644 --- a/src/subdir.mk +++ b/src/subdir.mk @@ -42,6 +42,6 @@ C_DEPS += \ src/%.o: src/%.cpp @echo 'Building file: $<' @echo 'Invoking: GCC C Compiler' - $(CC) -O3 -g3 -Wall -I/usr/include/swss -c -fmessage-length=0 -MMD -MP -MF"$(@:%.o=%.d)" -MT"$(@)" -o "$@" "$<" + $(CC) -std=gnu++17 -O3 -g3 -Wall -I/usr/include/swss -c -fmessage-length=0 -MMD -MP -MF"$(@:%.o=%.d)" -MT"$(@)" -o "$@" "$<" @echo 'Finished building: $<' @echo ' '