[dhcpmon] Add packet event quiescing by Xichen96 · Pull Request #91 · sonic-net/sonic-dhcpmon

Xichen96 · 2026-07-25T19:43:23Z

Description of PR

Summary:

dhcpmon needs a safe way to pause packet processing briefly while replacing
runtime interface membership state. Removing the last packet event from a
socket event base can terminate its event-loop thread, and the existing packet
callback drains the socket without a batch bound, so a pause can block
indefinitely under sustained traffic.

This focused prerequisite adds reusable packet-event suspend/resume operations,
keeps every socket event base alive while packet events are suspended, and
bounds each callback batch.

Work item tracking

Microsoft ADO (number only): 38615412

Type of change

Approach

What is the motivation for this PR?

Runtime VLAN_MEMBER and PORTCHANNEL_MEMBER refresh must not restart
dhcpmon, terminate packet event-loop threads, or hang while DHCP traffic is
continuous.

How did you do it?

Add tagged event suspend/resume operations without freeing event objects.
Add socket-manager wrappers for packet-handler suspension and resumption.
Register a long-period persistent no-op event on each socket event base so
temporarily removing packet events cannot make the event loop exit.
Process at most 64 packets per callback invocation so event suspension has a
bounded wait under sustained traffic.
Read the event file descriptor before freeing it in the existing deletion
path.

How did you verify/test it?

Pending this PR's Azure CI and combined master hardware validation with the
follow-up runtime-membership PR.

Any platform specific information?

No.

Back port request

None. This prerequisite targets master only.

Tested branch

Pending.

Test result

Pending this PR's Azure CI.

Documentation

Not applicable; no user-facing command or counter schema changes.

Allow packet handlers to be suspended and resumed without terminating socket event loops, and bound each callback batch so quiescing completes under sustained traffic. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83 Signed-off-by: Xichen96 <lukelin0907@gmail.com>

mssonicbld · 2026-07-25T19:43:30Z

/azp run

azure-pipelines · 2026-07-25T19:43:31Z

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

azure-pipelines · 2026-07-25T19:43:40Z

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

Copilot

Pull request overview

Adds infrastructure to safely “quiesce” DHCP packet processing in dhcpmon while runtime socket/interface membership state is refreshed, without tearing down event-loop threads or getting stuck draining sockets under sustained traffic.

Changes:

Added packet-handler suspend/resume APIs in sock_mgr and tagged suspend/resume support in event_mgr.
Added a long-period persistent “keepalive” timer event per socket event base to prevent event-loop exit when packet events are temporarily removed.
Bounded packet processing per callback invocation (max 64 packets) to ensure suspension has a bounded wait time.

Reviewed changes

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

Show a summary per file

File	Description
src/sock_mgr.h	Exposes new packet-handler suspend/resume APIs.
src/sock_mgr.cpp	Adds keepalive event per socket base and implements suspend/resume wrappers for packet handler events.
src/packet_handler.cpp	Limits per-callback packet processing to a fixed batch size.
src/event_mgr.h	Declares new suspend/resume-by-tag operations.
src/event_mgr.cpp	Implements suspend/resume, and fixes logging by reading event fd before freeing the event.

Copilot

Pull request overview

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

Comments suppressed due to low confidence (1)

src/packet_handler.cpp:873

recvfrom can modify the socklen_t length passed via slen. Since slen is initialized once before the loop, subsequent iterations may pass an incorrect length, which can cause truncated address data or errors. Reset slen to sizeof(sll) before each recvfrom call.

    struct sockaddr_ll sll;
    socklen_t slen = sizeof(sll);
    sock_info_t &sock_info = sock_mgr_get_sock_info(fd);

    for (int packet_count = 0; packet_count < MAX_PACKETS_PER_CALLBACK; packet_count++) {
        buffer_sz = recvfrom(fd, sock_info.buffer, sock_info.snaplen, MSG_DONTWAIT,
                             (struct sockaddr *)&sll, &slen);
        if (buffer_sz <= 0) {
            break;

Reset sockaddr length for each batch receive and reject timeout events from the fd-only resume path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83 Signed-off-by: Xichen96 <lukelin0907@gmail.com>

mssonicbld · 2026-07-25T23:06:55Z

/azp run

azure-pipelines · 2026-07-25T23:07:04Z

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 (2)

src/event_mgr.cpp:110

event_mgr::resume_all_events() returns -1 when event_add() fails but does not log any details about the failing event/tag/name. This makes operational debugging difficult when resume fails at runtime.

        if (event_add(event, NULL) < 0) {
            this->suspend_all_events(tag);
            return -1;
        }

src/sock_mgr.cpp:465

When resuming packet handlers fails for a particular socket, the function rolls back by suspending all handlers but provides no log indicating which socket failed. Adding a syslog entry here will make failures diagnosable in production.

        if (info.event_mgr_ptr->resume_all_events(packet_handler_tag) < 0) {
            sock_mgr_suspend_packet_handler();
            return -1;
        }

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/event_mgr.cpp:103

resume_all_events() should defensively reject an empty tag (otherwise it will iterate the internal tagless set of all events), and it should log when event_add() fails to aid operational debugging.

int event_mgr::resume_all_events(const std::string &tag)
{
    for (const auto &event : this->event_map[tag]) {
        if (event_get_fd(event) < 0) {
            syslog(LOG_ALERT, "event_mgr: Cannot resume non-fd event with tag %s", tag.c_str());

Reject untagged suspend/resume requests and log the event manager and fd when a packet event cannot be restored. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83 Signed-off-by: Xichen96 <lukelin0907@gmail.com>

mssonicbld · 2026-07-26T01:03:29Z

/azp run

azure-pipelines · 2026-07-26T01:03:39Z

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.

Look up tagged event sets without default insertion, logging unknown suspend tags and returning an error for unknown resume tags. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83 Signed-off-by: Xichen96 <lukelin0907@gmail.com>

mssonicbld · 2026-07-26T01:33:48Z

/azp run

Copilot

Pull request overview

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

Try the shared counter lock directly when no writer is pending, using the condition-variable wait path only when quiescing or writer contention requires it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83 Signed-off-by: Xichen96 <lukelin0907@gmail.com>

mssonicbld · 2026-07-26T03:21:33Z

/azp run

azure-pipelines · 2026-07-26T03:21:42Z

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

Copilot

Pull request overview

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

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 <lukelin0907@gmail.com>

mssonicbld · 2026-07-26T03:52:22Z

/azp run

azure-pipelines · 2026-07-26T03:52:31Z

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

Copilot

Pull request overview

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

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 <lukelin0907@gmail.com>

mssonicbld · 2026-07-26T04:14:25Z

/azp run

azure-pipelines · 2026-07-26T04:14:35Z

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

Copilot

Pull request overview

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

Comments suppressed due to low confidence (1)

src/sock_mgr.cpp:577

sock_mgr_suspend_packet_handler() returns 0 even if suspend_all_events(packet_handler_tag) fails for one or more sockets (today that failure is only logged inside event_mgr). This can leave packet-handler events active while packet_handlers_enabled is set to false, reintroducing the readable-fd busy-loop risk the quiescing design is trying to avoid. Consider collecting per-socket suspend results and returning an error (and/or avoiding flipping the enabled gate) if any suspend fails.

int sock_mgr_suspend_packet_handler()
{
    if (packet_handler_quiesce_lock.owns_lock()) {
        syslog(LOG_ALERT, "Packet handlers are already suspended");
        return -1;
    }
    for (const auto &entry : sock_map) {
        entry.second.event_mgr_ptr->suspend_all_events(packet_handler_tag);
    }
    set_packet_handlers_enabled(false);
    try {

+void event_mgr::suspend_all_events(const std::string &tag)
+{
+    if (tag.empty()) {
+        syslog(LOG_ALERT, "event_mgr: Refusing to suspend untagged events for %s",
+               this->name.c_str());
+        return;
+    }
+    const auto tagged_events = this->event_map.find(tag);
+    if (tagged_events == this->event_map.end()) {
+        syslog(LOG_ALERT, "event_mgr: Cannot suspend unknown tag %s for %s",
+               tag.c_str(), this->name.c_str());
+        return;
+    }


@@ -639,4 +845,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());


Reject non-resumable event tags before deletion, roll back partial event_del failures, and surface suspend errors through the socket manager for coherent fail-stop recovery. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83 Signed-off-by: Xichen96 <lukelin0907@gmail.com>

mssonicbld · 2026-07-26T05:51:34Z

/azp run

azure-pipelines · 2026-07-26T05:51:44Z

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

Copilot

Pull request overview

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

+    for (const auto &event : tagged_events->second) {
+        if (event_get_fd(event) < 0) {
+            syslog(LOG_ALERT, "event_mgr: Cannot resume non-fd event with tag %s for %s",
+                   tag.c_str(), this->name.c_str());
+            this->suspend_all_events(tag);
+            return -1;
+        }
+        if (event_add(event, NULL) < 0) {
+            syslog(LOG_ALERT, "event_mgr: Failed to resume event (fd=%d) with tag %s for %s",
+                   event_get_fd(event), tag.c_str(), this->name.c_str());
+            this->suspend_all_events(tag);
+            return -1;
+        }
+    }
+    return 0;
+}


+void sock_mgr_update_db_counters()
+{
+    sock_mgr_update_db_counters(sock_mgr_copy_cache_counters());


Xichen96 · 2026-07-26T06:36:34Z

Closing after informal scope review. This draft combined unrelated callback batching, counter synchronization, DB snapshotting, and future event suspension. Replacements: #96 bounded callbacks, #97 counter-state synchronization, and #98 stable DB snapshot writeback. CONFIG_DB event suspension remains deferred.

Copilot AI review requested due to automatic review settings July 25, 2026 19:43

Copilot started reviewing on behalf of Xichen96 July 25, 2026 19:43 View session

Copilot AI reviewed Jul 25, 2026

View reviewed changes

Comment thread src/packet_handler.cpp

Comment thread src/event_mgr.cpp

This was referenced Jul 25, 2026

[dhcpmon] Listen for membership updates #93

Closed

[TEMP][dhcpmon] Validate Fungible F2 repair stack #94

Closed

Xichen96 requested a review from Copilot July 25, 2026 22:57

Copilot started reviewing on behalf of Xichen96 July 25, 2026 22:57 View session

Copilot AI reviewed Jul 25, 2026

View reviewed changes

Comment thread src/sock_mgr.cpp Outdated

Comment thread src/event_mgr.cpp Outdated

Copilot AI review requested due to automatic review settings July 25, 2026 23:06

Copilot started reviewing on behalf of Xichen96 July 25, 2026 23:07 View session

Copilot AI reviewed Jul 25, 2026

View reviewed changes

Xichen96 requested a review from Copilot July 25, 2026 23:43

Copilot started reviewing on behalf of Xichen96 July 25, 2026 23:43 View session

Copilot AI reviewed Jul 25, 2026

View reviewed changes

Comment thread src/event_mgr.cpp Outdated

Copilot AI review requested due to automatic review settings July 26, 2026 01:03

Copilot started reviewing on behalf of Xichen96 July 26, 2026 01:03 View session

Copilot AI reviewed Jul 26, 2026

View reviewed changes

Comment thread src/sock_mgr.cpp Outdated

Copilot AI review requested due to automatic review settings July 26, 2026 01:33

Copilot started reviewing on behalf of Xichen96 July 26, 2026 02:56 View session

Copilot AI reviewed Jul 26, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 26, 2026 03:21

Copilot started reviewing on behalf of Xichen96 July 26, 2026 03:21 View session

Copilot AI reviewed Jul 26, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 26, 2026 03:52

Copilot started reviewing on behalf of Xichen96 July 26, 2026 03:52 View session

Copilot AI reviewed Jul 26, 2026

View reviewed changes

Comment thread src/sock_mgr.cpp

Copilot AI review requested due to automatic review settings July 26, 2026 04:14

Copilot started reviewing on behalf of Xichen96 July 26, 2026 04:14 View session

Xichen96 mentioned this pull request Jul 26, 2026

[dhcpmon] Make relay disparity detection resilient #90

Closed

5 tasks

Copilot AI reviewed Jul 26, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 26, 2026 05:51

Copilot started reviewing on behalf of Xichen96 July 26, 2026 05:51 View session

Copilot AI reviewed Jul 26, 2026

View reviewed changes

Xichen96 closed this Jul 26, 2026

Xichen96 deleted the dev/xichenlin/quiesce-packet-events branch July 26, 2026 06:36

		@@ -639,4 +845,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());

Uh oh!

Conversation

Xichen96 commented Jul 25, 2026

Description of PR

Work item tracking

Type of change

Approach

What is the motivation for this PR?

How did you do it?

How did you verify/test it?

Any platform specific information?

Back port request

Tested branch

Test result

Documentation

Uh oh!

mssonicbld commented Jul 25, 2026

Uh oh!

azure-pipelines Bot commented Jul 25, 2026

Uh oh!

azure-pipelines Bot commented Jul 25, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

mssonicbld commented Jul 25, 2026

Uh oh!

azure-pipelines Bot commented Jul 25, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

mssonicbld commented Jul 26, 2026

Uh oh!

azure-pipelines Bot commented Jul 26, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

mssonicbld commented Jul 26, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

mssonicbld commented Jul 26, 2026

Uh oh!

azure-pipelines Bot commented Jul 26, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

mssonicbld commented Jul 26, 2026

Uh oh!

azure-pipelines Bot commented Jul 26, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!