Add ZmqRouteServer and ZmqRouteConsumerStateTable for burst-coalesced route ingress#1187
Add ZmqRouteServer and ZmqRouteConsumerStateTable for burst-coalesced route ingress#1187venkit-nexthop wants to merge 4 commits into
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
… route ingress
Under sustained route ingress, ZmqConsumerStateTable fires its
SelectableEvent once per ZMQ message. Each notification wakes the
consumer's main loop, which runs execute() -> drain() -> bulker.flush()
even though the bulker exists specifically to amortize the flush across
many tuples. The main loop should wake once per burst, not once per
message.
This change adds two new classes alongside the existing ZmqServer /
ZmqConsumerStateTable, and makes a few base-class adjustments to support
subclassing:
- ZmqRouteServer (common/zmqrouteserver.{h,cpp}) derives from ZmqServer
and overrides mqPollThread(). It replaces the base's
"1 zmq_poll -> 1 zmq_recv -> handle" loop with:
1. zmq_poll with an adaptive timeout (1s when idle,
BURST_QUIESCE_MS=5ms while a burst is in progress).
2. An inner drain loop calling zmq_recv(ZMQ_DONTWAIT) until EAGAIN,
accumulating touched handlers in a set.
3. After the burst quiesces (a BURST_QUIESCE_MS poll timeout with a
non-empty dirty-handlers set), call notifyPending() on each
touched handler exactly once. eventfd writes coalesce, so a
mid-burst notify from the consumer side is harmless.
The oneToOneSync request/response mode supported by ZmqServer is
intentionally not available here -- burst coalescing assumes
streaming ingress.
- ZmqRouteConsumerStateTable
(common/zmqrouteconsumerstatetable.{h,cpp}) derives from
ZmqConsumerStateTable. It exposes setIngressCallback(IngressCallback)
so a consumer can merge tuples directly from mqPollThread into its
own sync map, skipping the internal std::queue and per-message
eventfd notify. notifyPending() overrides the base no-op and signals
the SelectableEvent. hasData() always returns true and
hasCachedData() returns false: the Selectable's readiness is
event-driven via notifyPending.
Base-class adjustments:
- ZmqMessageHandler gains a virtual notifyPending() default no-op so
ZmqConsumerStateTable (which doesn't need it) inherits silently.
- ZmqServer: destructor is now virtual; mqPollThread() and
handleReceivedData() are protected so subclasses can override.
handleReceivedData(buf, size) now returns the matched
ZmqMessageHandler* so the route server can record dirty handlers
without re-doing the lookup.
- ZmqConsumerStateTable::handleReceivedData(kcos) is now virtual, and
m_selectableEvent / m_asyncDBUpdater are protected so the override
can fire the eventfd and forward to the async updater.
How to verify it
----------------
6 new unit tests in tests/zmq_route_ut.cpp:
- NotifyPendingDefaultIsNoOp: base default doesn't crash.
- ConstSelectableProperties: hasData=true, hasCachedData=false,
initializedWithData=false.
- NotifyPendingFiresSelectableEvent: direct notifyPending() wakes
Select.
- IngressCallbackReceivesData: end-to-end producer -> ZmqRouteServer ->
ingress callback gets the deserialized tuple; Select wakes on the
post-burst notify.
- BurstCoalescingFiresFewerWakeups: the core new behavior -- send 200
messages back-to-back, all reach the callback but Select wakes far
fewer times (asserts wakeups < N/4; observed 1-3 in practice).
- NoIngressCallbackIsSafe: callback unset case doesn't crash;
notifyPending still wakes Select.
Signed-off-by: Venkit Kasiviswanathan <venkit@nexthop.ai>
cdd850e to
e36380b
Compare
securely1g
left a comment
There was a problem hiding this comment.
Thanks for the PR — the core optimization (drain-then-notify via adaptive zmq_poll timeout) is solid and well-motivated. A few concerns:
1. zmqserver.h access control layout
There are now 4 alternating protected:/private: sections. Should consolidate into one protected: block and one private: block for readability.
2. hasData() always returns true
If orch code ever checks hasData() to decide whether to call pops(), it'll always enter that path even when there's nothing. Could this cause spinning or unexpected behavior in any existing consumer patterns?
3. No pops() override — consumption pattern unclear
ZmqRouteConsumerStateTable inherits ZmqConsumerStateTable::pops() which drains m_receivedOperationQueue. But with the ingress callback path, tuples never enter that queue — they go straight to the callback. So pops() would always return empty. If the orch main loop calls pops() after Select wakes it (the normal pattern), it gets nothing.
How does the consumer actually retrieve data after Select wakes? The ingress callback presumably merges into m_toSync directly, but that contract isn't enforced or documented in the class interface. This is the main thing that needs clarification.
4. Thread safety of m_ingressCallback
The callback is set from the main thread via setIngressCallback() but invoked from mqPollThread. There's no synchronization on the std::function itself. If setIngressCallback is called after server.bind() (after the poll thread starts), there's a data race. Should either:
- Document that
setIngressCallbackmust be called beforebind() - Use
std::atomicor a mutex around the callback check
5. oneToOneSync not explicitly blocked
The comment says "intentionally not available here" but nothing prevents it structurally. Fine now since the 3-arg constructor doesn't expose it, but fragile if someone adds constructors later. A runtime check or static_assert would harden this.
6. Test coverage
- Hardcoded ports 1238-1242 could collide in parallel test runs
- The
dbPersistence=truepath inhandleReceivedDataforwards tom_asyncDBUpdaterbut has no test. Since this is a new class, at least one test withdbPersistence=truewould be good.
Minor
zmqrouteserver.h uses 2-space indentation for public/private but the rest of swss-common uses 4. Should match codebase style.
Overall the burst coalescing approach is sound — the adaptive timeout with dirty handler tracking is clean. Main blocker is #3 (clarifying the data consumption contract).
…esolve # Conflicts: # common/zmqserver.cpp # common/zmqserver.h
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Adds ZmqRouteConsumerStateTable.MultipleBurstsEachWakeSeparately: two bursts separated by an idle gap must each produce their own wakeup, verifying ZmqRouteServer re-arms notifyPending() across burst boundaries rather than coalescing both bursts into a single wakeup. The first burst's wakeup is drained before the second so the two phases are provably independent (the SelectableEvent eventfd would otherwise collapse them). Addresses PR review comment requesting a multiple-burst-transition test. Signed-off-by: Venkit Kasiviswanathan <venkit@nexthop.ai>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…ility The BuildSwss CI stage builds sonic-swss against this change, and swss's test double (orchagent/p4orch/tests/fake_zmqserver.cpp) defines 'void ZmqServer::handleReceivedData(const char*, size_t)'. Returning ZmqMessageHandler* from the header broke that build with a signature mismatch. Revert handleReceivedData() to void so it no longer breaks swss. The touched handler is still available to burst-coalescing callers because ZmqHandlerRegistry::dispatch() returns it; ZmqRouteServer::mqPollThread now deserializes and calls dispatch() directly (via getHandlerRegistry()) instead of relying on handleReceivedData()'s return value. Behavior is unchanged. Signed-off-by: Venkit Kasiviswanathan <venkit@nexthop.ai>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Hi, there are workflow run(s) waiting for approval, you may be first-time contributor. I will notify maintainers to help approve once PR is approved. Thanks! ---Powered by SONiC BuildBot
|
deepak-singhal0408
left a comment
There was a problem hiding this comment.
the burst drain+once-per-burst-wakeup model is sound and cleanly scoped and implementation looks good to me.
Why I did it
Under sustained route ingress,
ZmqConsumerStateTablefires itsSelectableEventonce per ZMQ message. Each notification wakes the consumer's main loop, which runsexecute() -> drain() -> bulker.flush()even though the bulker exists specifically to amortize that flush across many tuples. We want the consumer main loop to wake once per burst, not once per message.How I did it
Two new classes plus small refactors of the existing ones:
ZmqRouteServer(new,common/zmqrouteserver.{h,cpp}) — derives fromZmqServer, overridesmqPollThread(). Replaces the base's "1 zmq_poll -> 1 zmq_recv -> handle" loop with:zmq_pollwith adaptive timeout (1 s when idle, 5 msBURST_QUIESCE_MSwhile a burst is in progress).zmq_recv(ZMQ_DONTWAIT)untilEAGAIN, accumulating touched handlers in a set.BURST_QUIESCE_MSpoll timeout with non-empty dirty-handlers set), callnotifyPending()on each touched handler exactly once. eventfd writes coalesce, so a mid-burst notify from the consumer side is harmless.oneToOneSync(request/response) mode supported byZmqServeris intentionally not available here — burst coalescing assumes streaming ingress.ZmqRouteConsumerStateTable(new,common/zmqrouteconsumerstatetable.{h,cpp}) — derives fromZmqConsumerStateTable. Adds a publicsetIngressCallback(IngressCallback)so a consumer can merge tuples directly frommqPollThreadinto its own sync map (skipping the internalstd::queue+ per-message eventfd notify).notifyPending()overrides the base no-op and signals theSelectableEvent.hasData()always returns true andhasCachedData()returns false: the Selectable's readiness is event-driven vianotifyPending.ZmqMessageHandlergains avirtual void notifyPending()default no-op soZmqConsumerStateTable(which doesn't need it) inherits silently.ZmqServer: destructor is nowvirtual(the class has a virtualmqPollThread, so a non-virtual dtor would UB-delete a derivedZmqRouteServerthrough a base pointer). A few private members (m_socket,m_endpoint,m_runThread,m_buffer,m_oneToOneSync,m_allowZmqPoll) andhandleReceivedData(buf, size)/mqPollThread()are exposed asprotectedso the route subclass can access them. Other internals (m_mqPollThread,m_HandlerMap,m_vrf,m_context,findMessageHandler,startMqPollThread) stay private.handleReceivedData(buf, size)now returns the matchedZmqMessageHandler*(ornullptr) so the route server can record dirty handlers without re-doing the lookup.ZmqConsumerStateTable::handleReceivedData(kcos)is nowvirtual(overridden byZmqRouteConsumerStateTable), andm_selectableEvent/m_asyncDBUpdaterareprotectedso the override can fire the eventfd and forward to the async updater.How to verify it
6 new unit tests in
tests/zmq_route_ut.cpp:ZmqMessageHandler.NotifyPendingDefaultIsNoOp— base default doesn't crash.ZmqRouteConsumerStateTable.ConstSelectableProperties—hasData()=true,hasCachedData()=false,initializedWithData()=false.ZmqRouteConsumerStateTable.NotifyPendingFiresSelectableEvent— directnotifyPending()wakesSelect.ZmqRouteConsumerStateTable.IngressCallbackReceivesData— end-to-end:ZmqProducerStateTable->ZmqRouteServer-> ingress callback gets the deserialized tuple;Selectwakes on the post-burst notify.ZmqRouteConsumerStateTable.BurstCoalescingFiresFewerWakeups— the core new behavior: send 200 messages back-to-back, all reach the callback butSelectwakes far fewer times (assertswakeups < N/4; observed 1–3 in practice).ZmqRouteConsumerStateTable.NoIngressCallbackIsSafe— callback unset case doesn't crash;notifyPendingstill wakesSelect.```
$ ./tests/tests --gtest_filter="ZmqMessageHandler.:ZmqRouteConsumerStateTable."
[==========] 6 tests from 2 test suites ran. (3134 ms total)
[ PASSED ] 6 tests.
```
Coverage on the new files:
zmqrouteserver.cpp90.0% lines,zmqrouteconsumerstatetable.cpp71.4% (gap:dbPersistence=trueasync-updater path),zmqrouteconsumerstatetable.h100%. Remaining uncovered lines are defensive throws (SWSS_LOG_THROWon non-EAGAIN/EINTRzmq_recverrors and on message truncation past 16 MiB) that aren't reachable from a unit test.Which release branch to backport (provide reason below if selected)