feat: prototype recvmmsg/sendmmsg batched datagram I/O#41
Conversation
Add batched UDP send/receive behind default-off Linux build flags (MAFIANET_USE_RECVMMSG / MAFIANET_USE_SENDMMSG), plus a portable, unit-tested core. Portable core (MmsgBatch.h/.cpp), compiled and tested on every platform: - DriveBatchedSend: sendmmsg partial-send resume state machine. - SockaddrToSystemAddress: byte-order-correct sockaddr -> SystemAddress. - DispatchRecvBatch: fan a received batch to the event handler, freeing zero-length reads and the unused tail. Linux syscall wiring (guarded, inert by default): - RNS2_Berkley::RecvFromBatchedLoop drains the socket with one recvmmsg(MSG_WAITFORONE) per burst instead of one recvfrom per packet. - RNS2_Linux::SendBatch coalesces datagrams via sendmmsg; a loop-local RNS2SendBatch in ReliabilityLayer's resend loop defers only the transmit (encryption/simulation/metrics still run per datagram) and flushes at the loop's single exit. Tests: 11 hermetic unit tests for the core (Tests/Unit/MmsgBatchTests.cpp). The Linux path is compile-verified (gcc 13, both flags on); runtime validation on Linux via the integration suite is still pending.
|
Warning Review limit reached
Next review available in: 23 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
WalkthroughAdds optional Linux ChangesUDP batching
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)Batched receive flowsequenceDiagram
participant RNS2_Berkley
participant recvmmsg
participant DispatchRecvBatch
participant RNS2EventHandler
RNS2_Berkley->>recvmmsg: receive datagram batch
recvmmsg-->>RNS2_Berkley: lengths and source addresses
RNS2_Berkley->>DispatchRecvBatch: dispatch received slots
DispatchRecvBatch->>RNS2EventHandler: dispatch valid slots and deallocate others
Batched send flowsequenceDiagram
participant ReliabilityLayer
participant RNS2SendBatch
participant RNS2_Linux
participant sendmmsg
ReliabilityLayer->>RNS2SendBatch: add prepared datagram
ReliabilityLayer->>RNS2SendBatch: flush update batch
RNS2SendBatch->>RNS2_Linux: send datagram batch
RNS2_Linux->>sendmmsg: transmit batch
sendmmsg-->>RNS2_Linux: return sent count or error
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Source/CMakeLists.txt (1)
344-350: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep the batched-UDP macros off the installed library consumers.
MAFIANET_USE_RECVMMSGandMAFIANET_USE_SENDMMSGare installed public options, butMAFIANET_USE_SENDMMSGcontrols theRNS2_Linux::SendBatchoverride and theRNS2SendBatchclass body. Consumers linking against the installed target only get the macro definitions the library was initialized with when building withMAFIANET_USE_*OFF, while installed consumers can include and useRNS2_Linux::SendBatchorRNS2SendBatchonly when the option was ON. Keep these flags internal, or expose them as target options and ensure exported consumers receive the matching config.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/CMakeLists.txt` around lines 344 - 350, Keep MAFIANET_USE_RECVMMSG and MAFIANET_USE_SENDMMSG private to the library build in target_compile_definitions(${target_name}), and ensure the installed/exported target does not propagate them to consumers. Update the public RNS2_Linux::SendBatch and RNS2SendBatch configuration to remain consistent without requiring installed consumers to define these macros, or explicitly export matching option values if that API must remain conditional.
🧹 Nitpick comments (1)
Source/src/MmsgBatch.cpp (1)
13-32: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueUnhandled address family leaves
outwith stale/uninitialized data.When
RAKNET_SUPPORT_IPV6 != 1andfrom.ss_family != AF_INET, neither branch executes, so*out(theRNS2RecvStruct::systemAddressof a possibly-reused struct) is left with whatever it held before.DispatchRecvBatchdoesn't gate dispatch on family validity, only onlens[i] > 0, so such a slot would still be handed toOnRNS2Recvwith a stale source address. This mirrors the existingRecvFromBlockingIPV4And6pattern by design, and a bound IPv4-only socket should never hand back a non-AF_INET address, so the practical exposure is low — but worth a defensive zero-init or assert given it silently misattributes a datagram's origin if it ever does trigger.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/src/MmsgBatch.cpp` around lines 13 - 32, Update SockaddrToSystemAddress to defensively initialize or otherwise invalidate *out before handling the address family, and explicitly handle unsupported families so no stale source address or port remains when neither AF_INET nor IPv6 applies. Preserve the existing IPv4 and IPv6 byte-order handling for supported families.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Source/include/mafianet/MmsgBatch.h`:
- Around line 116-125: Update RNS2SendBatch::Add to reject oversized datagrams
rather than clamping length to MAXIMUM_MTU_SIZE. Preserve the full payload
behavior of the non-batched send path by signaling failure before memcpy when
length exceeds the supported limit, and adjust the caller as needed to handle
that failure without incrementing count.
In `@Source/include/mafianet/socket2.h`:
- Around line 267-269: Update the conditional declaration of
RNS2Socket2::SendBatch in the socket2 header so it is available to consumers
whenever the implementation supports MAFIANET_USE_SENDMMSG, rather than relying
on a library-private compile definition. Align this visibility with the
corresponding CMakeLists.txt and MmsgBatch.h changes.
In `@Source/src/ReliabilityLayer.cpp`:
- Around line 2206-2210: Remove the per-call allocation in the UpdateInternal
send path by making the RNS2SendBatch storage reusable across ticks, such as
persistent member- or thread-owned storage initialized once and reused by
Add/Flush. Update the RNS2SendBatch construction near the send loop to use that
storage, while preserving the existing batch capacity and behavior and only
growing the buffer when necessary.
In `@Tests/Unit/MmsgBatchTests.cpp`:
- Line 21: Replace the direct arpa/inet.h include in MmsgBatchTests.cpp with
platform-conditional socket headers matching the library’s Windows handling: use
winsock2.h and ws2tcpip.h on MSVC/mingw, while retaining the appropriate POSIX
header elsewhere so htons and inet_pton remain available.
---
Outside diff comments:
In `@Source/CMakeLists.txt`:
- Around line 344-350: Keep MAFIANET_USE_RECVMMSG and MAFIANET_USE_SENDMMSG
private to the library build in target_compile_definitions(${target_name}), and
ensure the installed/exported target does not propagate them to consumers.
Update the public RNS2_Linux::SendBatch and RNS2SendBatch configuration to
remain consistent without requiring installed consumers to define these macros,
or explicitly export matching option values if that API must remain conditional.
---
Nitpick comments:
In `@Source/src/MmsgBatch.cpp`:
- Around line 13-32: Update SockaddrToSystemAddress to defensively initialize or
otherwise invalidate *out before handling the address family, and explicitly
handle unsupported families so no stale source address or port remains when
neither AF_INET nor IPv6 applies. Preserve the existing IPv4 and IPv6 byte-order
handling for supported families.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a84bf948-9853-4f37-8fef-812670c91381
📒 Files selected for processing (9)
Source/CMakeLists.txtSource/include/mafianet/MmsgBatch.hSource/include/mafianet/ReliabilityLayer.hSource/include/mafianet/socket2.hSource/src/MmsgBatch.cppSource/src/RakNetSocket2.cppSource/src/RakNetSocket2_Berkley.cppSource/src/ReliabilityLayer.cppTests/Unit/MmsgBatchTests.cpp
Address CI + review feedback on the batched I/O prototype: - Windows build: MmsgBatchTests.cpp used <arpa/inet.h>; guard the include (<ws2tcpip.h> on Windows) so the hermetic tests build cross-platform. - RNS2SendBatch::Add rejected oversized datagrams instead of silently clamping to the MTU, which diverged from the scalar send path and could ship truncated payloads in release builds. - RNS2SendBatch no longer heap-allocates ~95 KB per UpdateInternal call; it reuses a thread_local buffer (single update thread per RakPeer, flushed synchronously) so the send path stays allocation-free. - MAFIANET_USE_RECVMMSG/SENDMMSG are now PUBLIC compile definitions so consumers see the same conditional declarations the library was built with (no header/ABI mismatch).
… in CI Pre-landing review follow-ups on the batched datagram I/O prototype: - SendBatch returned a byte total from the portable base implementation but a datagram count from the sendmmsg override. Both now return a datagram count (and the error only when nothing was sent) by routing the scalar loop through the same DriveBatchedSend state machine. - SockaddrToSystemAddress silently left *out untouched for an address family the build cannot decode, so a recycled recv struct would attribute the datagram to its previous sender. It now returns bool, sets UNASSIGNED_SYSTEM_ADDRESS, and DispatchRecvBatch frees instead of dispatching. - RNS2SendBatch is non-copyable and asserts against a second live batch on the same thread; its shared buffer block moved from TLS to a lazily allocated per-thread block, so threads that never batch pay nothing. - The batched recv loop kept reallocating the whole slot batch on every recvmmsg failure. Slots are now held across failures with progressive back-off, so a persistent async error (ECONNREFUSED on a closed peer port) cannot spin the polling thread. - RNS2SendBatch is compiled unconditionally (it only needs the portable SendBatch), with unit tests for payload copying, batch-boundary flush, destructor backstop, and the oversized-datagram drop. - New linux-mmsg CI job builds and tests with both flags ON; the syscall paths and their reliability-layer wiring were previously never compiled anywhere. Verified: 98/98 (unit + integration) on Linux with MAFIANET_USE_RECVMMSG and MAFIANET_USE_SENDMMSG ON; 69/69 unit on macOS with both OFF.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Tests/Unit/MmsgBatchTests.cpp (1)
57-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse MafiaNet containers in test scaffolding.
These
std::vectorusages conflict with the repository container convention. Replace them with an appropriateDataStructurescontainer.As per coding guidelines, use “custom MafiaNet data structure containers (DS_List, DS_Queue, DS_Map, DS_OrderedList, DS_MemoryPool) for consistency.”
Also applies to: 97-98, 309-312, 451-451
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/Unit/MmsgBatchTests.cpp` around lines 57 - 58, Replace the std::vector declarations in the MmsgBatchTests test scaffolding, including the referenced locations, with the appropriate DataStructures container types from the repository convention. Preserve each container’s required ordering and access behavior, and update any dependent operations or iteration as needed to compile with the selected MafiaNet containers.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Tests/Unit/MmsgBatchTests.cpp`:
- Around line 57-58: Replace the std::vector declarations in the MmsgBatchTests
test scaffolding, including the referenced locations, with the appropriate
DataStructures container types from the repository convention. Preserve each
container’s required ordering and access behavior, and update any dependent
operations or iteration as needed to compile with the selected MafiaNet
containers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c6c3ed44-6c07-4210-b8b1-ca5eae4d3eab
📒 Files selected for processing (9)
.github/workflows/build.ymlSource/CMakeLists.txtSource/include/mafianet/MmsgBatch.hSource/include/mafianet/ReliabilityLayer.hSource/include/mafianet/socket2.hSource/src/MmsgBatch.cppSource/src/RakNetSocket2.cppSource/src/RakNetSocket2_Berkley.cppTests/Unit/MmsgBatchTests.cpp
🚧 Files skipped from review as they are similar to previous changes (4)
- Source/include/mafianet/socket2.h
- Source/CMakeLists.txt
- Source/include/mafianet/ReliabilityLayer.h
- Source/src/RakNetSocket2.cpp
Pre-landing review follow-ups on the batched datagram I/O work. - RecvFromBatchedLoop refilled all MMSG_BATCH_MAX slots every pass and handed the whole array to DispatchRecvBatch, so a pass that received a single datagram still cost 64 allocs plus 63 frees -- each taking the event handler's pool mutex. Only the slots that actually received a datagram now change hands; the untouched tail carries over, putting the steady state at one alloc/free round trip per datagram, the same as the scalar path. - RNS2_Linux::SendBatch silently ignored RNS2_SendParameters::ttl, which the scalar Send() honours by bracketing the sendto with setsockopt(IP_TTL). sendmmsg has no per-message TTL, so a batch carrying one is now deferred to the portable base loop. Nothing sets ttl today (RNS2SendBatch always flushes with zero), so this closes a trap rather than a live bug. - RNS2SendBatch::Flush discarded SendBatch's result: a failed or partial sendmmsg dropped datagrams with no trace, where the scalar path at least reports its sendto failures. Both that shortfall and a dropped oversized datagram now emit RAKNET_DEBUG_PRINTF. Also drops the dead lens[] writes past the received count, corrects the comment claiming shutdown arrives on the recvmmsg error branch (the poke is a real datagram, so it surfaces as a normal pass), and switches to _FILE_AND_LINE_ to match the rest of the codebase. Tests: cover the DispatchRecvBatch clamp, the IPv6 decode branch of SockaddrToSystemAddress (and its IPv4-only-build rejection), TTL pass-through in the base SendBatch, and that RNS2SendBatch flushes with a zero TTL so the sendmmsg path is never deferred. Verified with the full suite on Linux with both flags on (102/102).
DriveBatchedSend treated any negative transmit() return as "stop", so a single undeliverable datagram silently discarded every datagram queued behind it -- the scalar Send() loop it replaced would have delivered those. Split the contract: 0 now means a transient socket-wide condition (stop, don't spin), negative means the message at that offset is itself undeliverable (drop it and continue). RNS2_Linux::SendBatch classifies errno accordingly, since sendmmsg funnels both through the same -1. Also: - Assert in RNS2SendBatch::Flush that SendBatch returned a datagram count rather than a byte total. That is the one contract the sendmmsg override cannot be unit-tested against, so check it at runtime. - Build the linux-mmsg CI job in Debug. It is the only job that compiles these paths, and RakAssert -- which guards the return contract, the RNS2SendBatch reentrancy invariant and the IPv4-only address branch -- is compiled out unless _DEBUG. - Make the IPv4-only branch of the sendmmsg address setup explicit instead of silently leaving msg_name null. - Cover the previously untested failure paths: a partially failing flush, a fully failing flush not resending its batch, per-message skip, and first-error propagation. - Ignore build-*/ and docs/superpowers/.
1 participant
Summary
Prototype of batched UDP datagram I/O (
recvmmsg/sendmmsg) for MafiaNet, replacing the current one-syscall-per-datagram model on the hot paths. All new behaviour is behind default-off Linux build flags — default builds are completely unchanged.Motivation: MafiaNet's workload is many small UDP datagrams at high packet rates, where the per-packet syscall tax dominates.
recvmmsg/sendmmsgmove a whole array of datagrams per syscall, cutting that overhead — the low-effort first step toward io_uring/RIO, slotting into the existing RNS2 seams with no threading-model change.What's here
Portable, unit-tested core (
MmsgBatch.h/MmsgBatch.cpp) — compiled and tested on every platform, including ones without the mmsg syscalls:DriveBatchedSend— thesendmmsgpartial-send resume state machine (short-count retry; error only propagated when nothing was sent, mirroringsendmmsg(2)).SockaddrToSystemAddress— byte-order-correctsockaddr→SystemAddress(network-order port +ntohs).DispatchRecvBatch— fans a received batch out to the event handler, freeing zero-length reads and the unused tail so no buffer leaks.Linux syscall wiring, behind
MAFIANET_USE_RECVMMSG/MAFIANET_USE_SENDMMSG(both OFF by default):RNS2_Berkley::RecvFromBatchedLoop— onerecvmmsg(..., MSG_WAITFORONE)per burst instead of onerecvfromper packet.MSG_WAITFORONEis deliberate: it returns as soon as one datagram is ready, so low packet rates aren't delayed waiting to fill the array.RNS2_Linux::SendBatch— coalesces datagrams viasendmmsg+DriveBatchedSend. A loop-localRNS2SendBatchinReliabilityLayer's resend loop defers only the transmit — encryption, packet-loss simulation and metrics still run per datagram — and flushes at the loop's single exit, so no datagram is stranded (the receive-pathSendACKsroute stays scalar on purpose).Testing
Tests/Unit/MmsgBatchTests.cpp), written test-first. These run in CI.-fsyntax-onlyclean on gcc 13 — therecvmmsg/sendmmsgcode actually compiles.Known limitations / follow-ups
Summary by CodeRabbit
recvmmsg, with configurable build options.sendmmsg, integrated into the reliability/datagram sending path.