[dhcpmon] Write COUNTERS_DB without blocking packet handling

[Xichen96]

Description of PR

Write COUNTERS_DB from an immutable in-memory snapshot so packet callbacks are
not blocked during Redis I/O.

Dependency:

• Counter-state synchronization PR [dhcpmon] Protect counters from concurrent packet and health updates #97.

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?

Holding the exclusive counter lock through every Redis hset stalls packet
monitoring and can overflow raw-socket buffers under load. Releasing the lock
before taking a coherent snapshot would instead let counter clear or topology
changes race the DB writeback.

How did you do it?

• Hold counter-state and DB synchronization while validating clear state and
  copying all per-socket counter maps.
• Release only the counter-state lock before Redis I/O.
• Retain DB synchronization until snapshot writeback and cleanup finish.
• Validate snapshot socket keys before reading live socket metadata.

How did you verify/test it?

Azure PR CI passed on amd64, arm64, and armhf. No local compilation was used.
The combined hardware stack passed F2 traffic, counter-clear churn, and
regular master regression.

Any platform specific information?

No.

Back port request

None. This targets master only.

Tested branch

master

Test result

All architecture builds passed. COUNTERS_DB writeback remained stable while
traffic and counter clear overlapped on F2.

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 improves dhcpmon’s runtime behavior under load by writing COUNTERS_DB from an immutable in-memory snapshot, reducing contention between packet-processing threads and Redis I/O.

Changes:

• Introduces counter-state reader/writer locking primitives (counter_state_read_lock / counter_state_write_lock) to serialize counter access across packet callbacks, health sampling, and DB sync.
• Adds snapshot-based DB updates via sock_mgr_copy_cache_counters() and sock_mgr_update_db_counters(const socket_counters_t&) to decouple Redis writes from live counter mutation.
• Limits per-callback packet draining (MAX_PACKETS_PER_CALLBACK) to bound time spent in the packet callback.

Reviewed changes

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

File	Description
src/sock_mgr.h	Adds counter-state lock APIs and introduces a per-socket counters snapshot type.
src/sock_mgr.cpp	Implements counter-state lock behavior and snapshot-based COUNTERS_DB writeback.
src/packet_handler.cpp	Acquires counter-state read lock during packet callback and bounds packets processed per callback.
src/dhcp_mon.cpp	Uses counter-state write locking around status/health/DB-sync paths and writes DB counters from a stable snapshot.

Comments suppressed due to low confidence (2)

src/dhcp_mon.cpp:277

• Same lock-order issue as SIGUSR1: update_cache_counter_callback acquires counter_state_write_lock and then takes db_sync_mutex. If db_update_callback is holding db_sync_mutex during Redis I/O, this callback can block on db_sync_mutex while still holding the counter-state lock, which stalls packet callbacks. Take db_sync_mutex first, then take counter_state_write_lock before touching cache counters / pause flags.

    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:462

• db_update_callback currently acquires counter_state_write_lock and then db_sync_mutex. To avoid deadlock after changing other call sites to db_sync_mutex -> counter_state_write_lock, and to ensure writers never hold counter-state while waiting on a long-held db_sync_mutex, this callback should also lock db_sync_mutex first and only then take counter_state_write_lock briefly to validate/clone the snapshot. This keeps packet callbacks running while db_sync_mutex is contended and maintains a consistent global lock order.

    socket_counters_t counters_by_socket;
    std::unique_lock<std::mutex> lock;
    {
        counter_state_write_lock counter_lock;
        if (!counter_lock.owns_lock()) {
            return;
        }
        lock = std::unique_lock<std::mutex>(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<std::chrono::seconds>(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(counters_by_socket);

[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.

Comments suppressed due to low confidence (1)

src/dhcp_mon.cpp:291

• update_cache_counter_callback() now holds counter_state_write_lock across Redis I/O (e.g., mCountersDbPtr->keys(...)), which can still stall packet callbacks during Redis access. To better match the goal of avoiding packet stalls on Redis operations, consider acquiring counter_state_write_lock only after the Redis reads, and keeping it only for applying updates to the in-memory counters.

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

    // can only sync db to cache counter and db updater is paused, otherwise its unexpected
    if (!sock_info.pause_write_cache_to_db) {
        syslog(LOG_WARNING, "Failed to update cache counter from DB counter on %s because pause_write_cache_to_db is not set", sock_info.name);
        return;
    }

    std::string match_pattern = (sock_info.is_v6 ? COUNTERS_DB_COUNTER_TABLE_V6_PREFIX : COUNTERS_DB_COUNTER_TABLE_PREFIX)
                                + downstream_ifname + "*";
    auto keys = mCountersDbPtr->keys(match_pattern);

    // Store interfaces in DB counter table
    std::unordered_set<std::string> updated_intfs;
    all_counters_t &all_counters = sock_info.all_counters;

[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.

[mssonicbld]

/azp run

[azure-pipelines]

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

[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.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (2)

src/sock_mgr.cpp:21

• The new language-version guard claims C++14 is sufficient, but this translation unit already uses C++17-only features (e.g., structured bindings like for (auto &[sock, info] : sock_map)), so compiling with C++14 would still fail later with a less clear error. Make the guard match the actual minimum standard to fail fast with an accurate message.

#if __cplusplus < 201402L
#error "dhcpmon counter synchronization requires C++14 or newer"
#endif

src/sock_mgr.cpp:154

• counter_state_read_lock::counter_state_read_lock() calls shared_lock::unlock() in the retry loop without catching std::system_error. If an unlock failure occurs, an exception would escape into the packet callback path.

        lock.unlock();

[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 no new comments.

[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 1 comment.

Comments suppressed due to low confidence (1)

src/sock_mgr.cpp:749

• This per-socket log message says "from cache counter", but the writeback source is now the passed-in snapshot map (all_counters from counters_by_socket), not the live cache map. Tweaking the wording avoids confusion when correlating logs with lock scopes.

        syslog_debug(LOG_INFO, "Start updating socket %d %s DB counter from cache counter", sock, info.name);

[Xichen96]

Removed with #97. This snapshot-writeback change exists to reduce packet blocking introduced by that synchronization layer; without #97 it is not an independent F2 repair.