[dhcpmon] Protect counters from concurrent packet and health updates

[Xichen96]

Description of PR

Serialize packet-counter writes with health sampling, counter-clear cache
updates, and DB synchronization.

This is extracted from the oversized packet-quiescing draft. It does not add
CONFIG_DB listeners, runtime topology reconciliation, event suspension, or
command-line changes.

Dependency:

• [dhcpmon] Apply small packet and C++ hardening fixes #96 bounds each packet
  callback, ensuring an in-flight reader releases the shared lock promptly.

This is a stacked draft. The displayed packet-callback change belongs to #96
and disappears from this diff after that prerequisite merges.

Work item tracking

• Microsoft ADO (number only): 38615655

Type of change

• Bug fix
• New feature
• Refactor / cleanup
• Documentation update
• Test improvement

Approach

What is the motivation for this PR?

Packet callbacks update plain counter maps on socket event-loop threads while
the main loop reads or rewrites those maps for health checks, snapshots,
counter clear, and COUNTERS_DB updates. Those unsynchronized accesses are C++
data races and can seed unreliable disparity state.

How did you do it?

• Packet callbacks take a shared counter-state guard.
• Main/cache-update counter operations take an exclusive guard.
• Pending writers stop admitting new packet readers and use a condition
  variable to guarantee writer progress without busy loops.
• Lock acquisition failures are logged and the affected operation is skipped
  rather than unwinding through libevent C callbacks.
• Preserve the existing counter, command-line, and CONFIG_DB interfaces.

How did you verify/test it?

Azure PR CI passed on amd64, arm64, and armhf. No local compilation was used.
Hardware validation of the combined stack passed regular DHCPv4/DHCPv6
regression, counter clear during traffic, and the F2 true/false alert
scenarios.

Any platform specific information?

No.

Back port request

None. This targets master only.

Tested branch

master

Test result

All architecture builds passed. Packet handling and health sampling remained
stable through regular traffic and F2 counter-clear churn.

Documentation

Not applicable.

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[Copilot]

Pull request overview

This PR introduces cross-thread synchronization for DHCP packet counters to eliminate C++ data races between per-socket libevent packet threads and main-thread operations (health sampling, snapshotting, cache-clear reconciliation, and COUNTERS_DB synchronization).

Changes:

• Adds a global counter-state std::shared_mutex with RAII read/write lock helpers and a “writers pending” gate/condition-variable to ensure writer progress.
• Wraps main-thread counter consumers/rewriters (health check, snapshot update, cache sync, DB update, signal status dumps) with an exclusive counter-state lock.
• Wraps packet callback processing with a shared counter-state lock and bounds per-callback receive processing (max packets per callback), resetting socklen_t per recvfrom().

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
src/sock_mgr.h	Declares global counter-state synchronization primitives and RAII lock helpers.
src/sock_mgr.cpp	Defines the counter-state lock implementation (shared mutex + pending-writer gating).
src/packet_handler.cpp	Guards packet callback work with the shared counter-state lock and bounds per-callback packet processing.
src/dhcp_mon.cpp	Serializes main-thread health sampling, cache sync, and DB sync operations with an exclusive counter-state lock.

Comments suppressed due to low confidence (1)

src/dhcp_mon.cpp:442

• db_update_callback now holds the exclusive counter_state_write_lock across COUNTERS_DB synchronization and stale-key cleanup (keys()/hset()/del()). Those DB operations can block and may significantly delay packet processing while the global lock is held. Consider snapshotting the cache counters under the lock, then releasing it before performing DB I/O (write the snapshot to DB without holding counter_state_write_lock).

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<std::mutex> lock(db_sync_mutex);

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (3)

src/dhcp_mon.cpp:276

• update_cache_counter_callback takes counter_state_write_lock before db_sync_mutex, so packet callbacks can be blocked while this thread waits on db_sync_mutex. Swapping the acquisition order reduces the time packet processing is stalled and keeps a consistent lock ordering at the call sites that need both locks.

    counter_state_write_lock counter_lock;
    if (!counter_lock.owns_lock()) {
        return;
    }
    std::lock_guard<std::mutex> lock(db_sync_mutex);

src/dhcp_mon.cpp:442

• db_update_callback acquires counter_state_write_lock before db_sync_mutex. If db_sync_mutex is contended, this holds the global counter write lock longer than necessary and stalls packet callbacks. Prefer acquiring db_sync_mutex first (and keep the same order in all call sites that take both locks) to minimize packet-path disruption.

    counter_state_write_lock counter_lock;
    if (!counter_lock.owns_lock()) {
        return;
    }
    std::lock_guard<std::mutex> lock(db_sync_mutex);

src/sock_mgr.h:13

• sock_mgr.h now includes , but the header doesn’t use std::atomic (the atomic is only used in sock_mgr.cpp). Dropping the unused include reduces header coupling and rebuild surface area.

#include <atomic>
#include <mutex>

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (4)

src/dhcp_mon.cpp:235

• Lock acquisition order holds the global exclusive counter lock while waiting on db_sync_mutex. If db_sync_mutex is contended, this blocks packet callbacks longer than needed; acquire db_sync_mutex first, then take counter_state_write_lock to minimize time blocking packet threads.

        counter_state_write_lock counter_lock;
        if (!counter_lock.owns_lock()) {
            return;
        }
        std::lock_guard<std::mutex> lock(db_sync_mutex);

src/dhcp_mon.cpp:276

• Lock acquisition order holds counter_state_write_lock while waiting on db_sync_mutex, which can unnecessarily stall packet handling when DB sync is busy. Acquire db_sync_mutex first, then take the counter lock.

    counter_state_write_lock counter_lock;
    if (!counter_lock.owns_lock()) {
        return;
    }
    std::lock_guard<std::mutex> lock(db_sync_mutex);

src/dhcp_mon.cpp:442

• Lock acquisition order holds the exclusive counter lock while waiting on db_sync_mutex. If DB updates are slow or contended, this extends packet-handler blocking unnecessarily; acquire db_sync_mutex first, then counter_state_write_lock.

    counter_state_write_lock counter_lock;
    if (!counter_lock.owns_lock()) {
        return;
    }
    std::lock_guard<std::mutex> lock(db_sync_mutex);

src/sock_mgr.cpp:100

• If std::unique_lock::unlock() throws here, the destructor returns without decrementing counter_state_writers_pending, leaving readers blocked on counter_state_wait_cv indefinitely. Avoid calling unlock() explicitly in the destructor; let unique_lock release via RAII and always decrement/notify when registered_writer is true.

    try {
        lock.unlock();
    } catch (const std::system_error &e) {
        syslog(LOG_ALERT, "Failed to unlock DHCP counter state: %s", e.what());
        return;

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

src/sock_mgr.cpp:104

• counter_state_write_lock notifies the condition variable while the exclusive counter_state_mutex is still held (the unique_lock member isn’t released until after the destructor body). This can wake packet-handler threads only to have them immediately block on counter_state_mutex, adding avoidable context switching/latency. Unlock the counter_state_mutex before calling notify_all().

        notify = counter_state_writers_pending.fetch_sub(1, std::memory_order_acq_rel) == 1;
    }
    if (notify) {
        counter_state_wait_cv.notify_all();
    }

[Xichen96]

Removed from the F2 repair stack after review. The concurrent counter access is not demonstrated as an F2 failure mechanism, and the custom writer-priority synchronization adds disproportionate complexity. Any synchronization hardening should return as a separately justified, simpler change.