feat[needs minor version update]: replace POSIX MQ publish notification with eventfd#1272
feat[needs minor version update]: replace POSIX MQ publish notification with eventfd#1272k1832 wants to merge 5 commits intoautowarefoundation:mainfrom
Conversation
Replace the POSIX MQ-based publish notification mechanism with kernel-managed eventfd. The kernel module now signals subscriber eventfds directly within ioctl(PUBLISH), eliminating the publisher's userspace mq_send loop entirely. Key changes: - Kernel: subscriber_info stores eventfd_ctx, publish_msg calls eventfd_signal() for each subscriber instead of returning subscriber IDs to userspace - Userspace: subscriber creates eventfd and passes to kernel via ioctl(ADD_SUBSCRIBER). Publisher's publish_core is simplified from ~80 lines to ~15 lines (MQ send loop removed) - Syscalls per publish reduced from N+1 to 1 (where N = subscriber count) Signed-off-by: Keita Morisaki <keita.morisaki@tier4.jp>
Signed-off-by: Keita Morisaki <keita.morisaki@tier4.jp>
Move eventfd_signal() calls outside the topic_rwsem write lock to avoid lock contention when subscriber count is large (e.g., /tf with 100+ subscribers). Notify_ctx pointers are copied under topic_rwsem, then signaled after releasing it. This is safe because global_htables_rwsem read lock (still held) prevents subscriber removal (which requires write lock). Uses a 64-entry stack buffer for the common case (N <= 64). Falls back to kcalloc for larger subscriber counts. Signed-off-by: Keita Morisaki <keita.morisaki@tier4.jp>
Replace nested if blocks with early-continue pattern using an explicit notify_within_lock fallback flag. Behavior unchanged. Signed-off-by: Keita Morisaki <keita.morisaki@tier4.jp>
Explain the value choice: covers common ROS 2 fan-out (typical N <= 10, outliers like /tf reach 100+) while keeping stack usage bounded to 512 B (64 * sizeof(void *)) within the 16 KB default kernel stack. Signed-off-by: Keita Morisaki <keita.morisaki@tier4.jp>
dc0adbc to
cdf494f
Compare
There was a problem hiding this comment.
Pull request overview
This PR replaces Agnocast’s POSIX message-queue–based publish notification path with per-subscriber eventfd, moving fan-out notification into the kernel ioctl(PUBLISH) path to avoid N-per-subscriber syscalls in userspace.
Changes:
- Kernel module: store
eventfd_ctxper subscriber and signal subscriber eventfds duringAGNOCAST_PUBLISH_MSG_CMD(with stack buffer + heap fallback for large subscriber counts). - Userspace library: create/pass eventfd via
AGNOCAST_ADD_SUBSCRIBER_CMD, update epoll handling to read/clear eventfd notifications, and simplify publisher publish path (remove MQ send loop). - Tests/KUnit: update ioctl call signatures and remove publish subscriber-id buffer expectations tied to the old MQ design.
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/agnocastlib/test/integration/src/node_for_executor_test.cpp | Replace MQ-based executor-test notification with eventfd creation and write()-based triggering. |
| src/agnocastlib/test/integration/include/node_for_executor_test.hpp | Swap MQ receiver/sender members for an eventfds_ list. |
| src/agnocastlib/src/agnocast_subscription.cpp | Create eventfd for non-take subscriptions, pass it to kernel via ioctl, and track it for cleanup. |
| src/agnocastlib/src/agnocast_publisher.cpp | Remove userspace MQ notification loop; publishing becomes a single ioctl call. |
| src/agnocastlib/src/agnocast_epoll.cpp | Switch wake-up handling from mq_receive to read(eventfd) to clear notifications. |
| src/agnocastlib/include/agnocast/agnocast_subscription.hpp | Remove MQ subscription state; register callbacks with notify_eventfd_ and close it on destruction. |
| src/agnocastlib/include/agnocast/agnocast_publisher.hpp | Update publish_core signature and remove publisher-side MQ state/cleanup. |
| src/agnocastlib/include/agnocast/agnocast_mq.hpp | Remove MqMsgAgnocast (no longer needed for publish notification). |
| src/agnocastlib/include/agnocast/agnocast_ioctl.hpp | Add eventfd to add-subscriber args; remove publish subscriber-id buffer fields. |
| src/agnocastlib/include/agnocast/agnocast_epoll.hpp | Use notify_eventfd in epoll_ctl(ADD) for subscription readiness. |
| src/agnocastlib/include/agnocast/agnocast_callback_info.hpp | Replace mqdes with notify_eventfd in callback registration/info. |
| agnocast_kmod/agnocast_kunit/agnocast_kunit_take_msg.c | Update ioctl helper calls for new publish/add-subscriber signatures. |
| agnocast_kmod/agnocast_kunit/agnocast_kunit_set_ros2_subscriber_num.c | Update add-subscriber calls to pass eventfd argument. |
| agnocast_kmod/agnocast_kunit/agnocast_kunit_remove_subscriber.c | Update publish/add-subscriber calls for new ioctl signatures. |
| agnocast_kmod/agnocast_kunit/agnocast_kunit_remove_publisher.c | Update publish/add-subscriber calls for new ioctl signatures. |
| agnocast_kmod/agnocast_kunit/agnocast_kunit_release_sub_ref.c | Update publish/add-subscriber calls for new ioctl signatures. |
| agnocast_kmod/agnocast_kunit/agnocast_kunit_receive_msg.c | Update publish/add-subscriber calls for new ioctl signatures. |
| agnocast_kmod/agnocast_kunit/agnocast_kunit_publish_msg.h | Remove test case tied to the removed subscriber-id buffer API. |
| agnocast_kmod/agnocast_kunit/agnocast_kunit_publish_msg.c | Update publish/add-subscriber calls and remove subscriber-id buffer assertions/tests. |
| agnocast_kmod/agnocast_kunit/agnocast_kunit_get_topic_subscriber_info.c | Update add-subscriber call signature to include eventfd argument. |
| agnocast_kmod/agnocast_kunit/agnocast_kunit_get_topic_publisher_info.c | Update add-subscriber call signature to include eventfd argument. |
| agnocast_kmod/agnocast_kunit/agnocast_kunit_get_subscriber_qos.c | Update add-subscriber call signature to include eventfd argument. |
| agnocast_kmod/agnocast_kunit/agnocast_kunit_get_subscriber_num.c | Update add-subscriber calls to pass eventfd argument (including bridge variants). |
| agnocast_kmod/agnocast_kunit/agnocast_kunit_get_publisher_num.c | Update add-subscriber calls to pass eventfd argument (including bridge variants). |
| agnocast_kmod/agnocast_kunit/agnocast_kunit_get_node_subscriber_topics.c | Update add-subscriber calls to pass eventfd argument. |
| agnocast_kmod/agnocast_kunit/agnocast_kunit_do_exit.c | Update publish/add-subscriber calls for new ioctl signatures. |
| agnocast_kmod/agnocast_kunit/agnocast_kunit_add_subscriber.c | Update add-subscriber calls to pass eventfd argument. |
| agnocast_kmod/agnocast_ioctl.c | Kernel: accept eventfd, store eventfd_ctx, and signal eventfds during publish (outside topic_rwsem when possible). |
| agnocast_kmod/agnocast_internal.h | Add eventfd support (eventfd_ctx in subscriber_info) and compatibility wrapper for eventfd_signal API change. |
| agnocast_kmod/agnocast_internal.c | Ensure eventfd_ctx_put() is called during subscriber exit cleanup. |
| agnocast_kmod/agnocast_exit.c | Ensure eventfd_ctx_put() is called when removing all topics/subscribers on shutdown. |
| agnocast_kmod/agnocast.h | Kernel ABI: add eventfd to add-subscriber args; remove publish subscriber-id buffer fields; update function prototypes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| union ioctl_publish_msg_args publish_core( | ||
| [[maybe_unused]] const void * publisher_handle, /* for CARET */ const std::string & topic_name, | ||
| const topic_local_id_t publisher_id, const uint64_t msg_virtual_address, | ||
| std::unordered_map<topic_local_id_t, std::tuple<mqd_t, bool>> & opened_mqs); | ||
| const topic_local_id_t publisher_id, const uint64_t msg_virtual_address); |
There was a problem hiding this comment.
The signature of publish_core has changed (removed the opened_mqs parameter), but there is still a test-side mock implementation with the old signature (src/agnocastlib/test/unit/test_mocked_agnocast.cpp). This will cause a compile/link failure due to the mismatched declaration/definition. Update the mock to match the new publish_core signature (and remove MQ-related types/includes there).
Description
Replace the POSIX MQ-based publish notification mechanism with kernel-managed eventfd.
The previous MQ path required the publisher to call
mq_send()once per subscriberin userspace (N+1 syscalls per publish including the PUBLISH ioctl). With eventfd,
the kernel module signals subscriber eventfds directly within
ioctl(PUBLISH),reducing the cost to a single syscall regardless of subscriber count.
Key changes:
subscriber_infostoreseventfd_ctx.publish_msgcollects notify_ctxpointers under
topic_rwsemwrite lock, then signals outside the lock to avoidcontention. Stack buffer (64 entries) with heap fallback for larger subscriber counts.
ioctl(ADD_SUBSCRIBER).Publisher's
publish_coreis simplified from ~80 to ~15 lines (MQ send loop removed).agnocast_eventfd_signalwrapper for kernel < 6.8 API difference.Detailed design, benchmark analysis, and alternative considered: see design doc (linked below).
Benchmark Results
Measured with isorc26_tmp,
10 topics, 100Hz, 3-iter mean of per-iter p50/p99,
--no-rt. Test machine: 32 logical CPUs.Publish latency (publisher wall-clock, from
loan()topublish()completion)E2E latency (from publisher timestamp just before
loan()to subscriber callback entry)Observations
scheduler's wake_up O(N) cost dominates and the mechanism difference gets buried.
publish=594 us). This is because there is 1 publish-latency sample but N E2E samples
per publish, so the E2E median points to the "middle-woken" subscriber. In the p99
column the last-woken subscriber's scheduling latency dominates, so the ordering
reverses (E2E p99 > publish p99 at high N).
Alternative explored: per-topic shared eventfd
We also prototyped a per-topic shared eventfd (one eventfd shared by all subscribers
of a topic, signaled once per publish) to test whether consolidating N
eventfd_signalcalls into one would improve high-N latency. Contrary to the hypothesis, the single
wake_up_poll()iterating Nep_poll_callbacks under one spin_lock showed similaror slightly worse latency due to concentrated scheduler contention.
experiment/shared-eventfdRelated links
How was this PR tested?
bash scripts/test/e2e_test_1to1.bash(required)bash scripts/test/e2e_test_2to2.bash(required)bash scripts/test/run_requires_kernel_module_tests.bash(required)Notes for reviewers
MQ usage in other paths (bridge request and bridge delegation) remains unchanged.
Dead code from the old subscription MQ cleanup path (daemon mq_unlink loop) will be
removed in a follow-up PR.
Version Update Label (Required)
Please add exactly one of the following labels to this PR:
need-major-update: User API breaking changesneed-minor-update: Internal API breaking changes (heaphook/kmod/agnocastlib compatibility)need-patch-update: Bug fixes and other changesImportant notes:
need-major-updateorneed-minor-update, please include this in the PR title as well.fix(foo)[needs major version update]: barorfeat(baz)[needs minor version update]: quxrun-build-testlabel. The PR can only be merged after the build tests pass.See CONTRIBUTING.md for detailed versioning rules.