dpdk lifecycle - #1820
Draft
daniel-noland wants to merge 27 commits into
Draft
dpdk lifecycle#1820daniel-noland wants to merge 27 commits into
daniel-noland wants to merge 27 commits into
Conversation
`hardware/tests/dpdk_in_vm.rs` was dropped from the n-vm absorption with a note that it was written against the pre-rewrite dpdk API and preserved on `backup/n-vm-again` for a clean revival. This is that revival: twelve tests that bind a NIC to vfio-pci inside a guest, initialise the EAL on guest hugepages, configure a device, and put frames on the wire. The port was mostly mechanical. `StartedDev` became the `Dev<Started>` typestate, `Headers`' fields became accessors, `alloc_bulk`/`transmit` now speak `MbufArray` rather than `Vec<Mbuf>`, and the attribute-based configuration (`#[guest(...)]`, `#[hypervisor(...)]`, `#[network(...)]`) became one `const VmConfig`. Two things needed judgement rather than translation, both recorded at the top of the file. The host page default inverted, so the 1 GiB variants are gated rather than silently put back on a scarce pool. And QEMU is named as a profile, not a backend, because `.backend(Qemu)` skips under the default profile and nextest prints a skip as a pass. The device configuration asks for `RxOffload::NONE` explicitly. `None` would mean the opposite -- every offload the device supports -- and that turns LRO on, which brings DPDK's `max_lro_pkt_size` validation into a test that has no use for it. The first probe burst asserts that the queue accepted every frame: a two-frame burst on a 256-descriptor queue has no legitimate reason to be refused. The retransmit path only reports a refusal, since it exists to paper over a lost frame and dropping the returned mbufs is the correct disposal. Running it found one more defect, in the test's own teardown. The PMD owns every transmitted mbuf until the device is stopped and its tx ring reclaimed, and those mbufs live in the tx pool. Rust drops locals in reverse declaration order, which frees the mempool first and leaves the stop path walking freed memory: a SIGSEGV inside `rte_pktmbuf_free`, reached only after every assertion in the test has already passed. The device is now stopped explicitly, before the pool. That ordering hazard is not really the test's to get right -- a `Pool` handed to a device should not be droppable while the device still holds mbufs from it -- but making it unrepresentable is an API change beyond this revival. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
`PciDriver` is a closed enum of the drivers this crate knows how to unbind a device from, and it had no entry for either Intel emulated NIC. Any attempt to rebind one to vfio-pci failed with "driver e1000 is not supported" before DPDK was reached at all, which is what the four e1000/e1000e road tests were hitting. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
The DPDK build already enables `net/intel/e1000` -- it is in `enabledDrivers` and `librte_net_e1000` is produced -- but `dpdk-sys` never named it among the libraries it links. Under static linkage a PMD that nothing names is never pulled in, so its PCI-driver constructor never runs. The symptom is silent and misleading: the device binds to vfio-pci, gets its noiommu group, EAL reports "VFIO support initialized", and then `rte_eth_dev_count_avail` returns zero with no probe failure logged anywhere, because no driver ever claimed the ID. With the PMD linked, QEMU's 82540EM and 82574L come up as `net_e1000_em` ports. That also puts a genuinely RSS-less NIC under test: `flow_type_rss_offloads` is zero on these, which is exactly the case that made `rte_eth_dev_configure` fail before `mq_mode` was made conditional. `rte_net_ixgbe` and `rte_net_iavf` are built and likewise unnamed here; they are left alone since nothing tests them yet. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Wraps `rte_eth_macaddr_get`. Needed by any test that has to address a frame to a specific port rather than rely on promiscuous mode -- which, on a bifurcated driver like mlx5, is the difference between the frame being delivered and being dropped after the PHY. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Our first-class targets are ConnectX-7/8/9 and BlueField-3, and nothing tested
them. This drives a real card on the host -- no VM, no emulated NIC -- putting
frames on the wire from each port of a cabled pair and asserting the other port
dequeues every one of them.
It needs no root and no vfio-pci. mlx5 is a bifurcated driver: DPDK attaches
through the RDMA verbs/DevX interface while `mlx5_core` keeps the netdev, so
none of the binding and IOMMU work the emulated-NIC suite does applies. What it
does need is three capabilities, each found by bisection against a BlueField-3
rather than guessed:
cap_ipc_lock mlockall and DMA memory registration, against an 8 MiB
default RLIMIT_MEMLOCK
cap_sys_nice set_mempolicy during hugepage allocation; without it EAL
aborts in rte_service_init
cap_net_raw DevX object creation. Its absence is reported as
"DevX create TIS failed errno=121" / "Cannot allocate
memory", which reads like a memory problem and is not one.
cap_net_admin does not substitute.
Two things had to be got right or the test would have been worthless.
Because the kernel keeps the netdev it also keeps the default receive steering,
so a frame matching no filter reaches the PHY and is discarded before any DPDK
queue sees it -- visible only as a gap between `rx_packets_phy` and the port's
`rx_packets`. Probes are therefore addressed to the peer port's actual MAC, read
back from the PMD.
And both ports see background multicast. An early version asserted only that
frames arrived and "passed" on 34 received when the sender had emitted 32. Every
probe now carries a reserved-for-experimental EtherType, a magic, a per-round
nonce and a sequence number, and only frames matching all of them are counted;
the nonce differs per round and direction so a straggler can never be counted
twice.
The PMD installs its receive steering during device start, and frames arriving
before that programming lands are dropped. Measured across repeated runs, the
first burst after start loses between zero and five frames and everything after
is lossless. The test warms up until a round is lossless in both directions
before measuring, so the measured assertion stays exact equality -- tolerating a
few lost frames instead would have blinded it to the loss it exists to catch.
Warming up until the path proves itself clean, rather than for a fixed duration,
matters: an earlier fixed 500 ms warm-up was sometimes too short and simply moved
the loss into the measured round.
Gated behind `--cfg nic_loopback_tests` rather than `#[ignore]`: it needs a
specific rig, but it is a perfectly valid test wherever that rig exists.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Four ownership defects in the DPDK bindings, each of which could only be
avoided by convention, now cannot be expressed.
**A `Pool` could be freed while a device still held its mbufs.** `Pool` owned
its mempool and freed it on drop, so the natural teardown order was the wrong
one -- Rust drops locals in reverse declaration order, and the PMD keeps
transmitted mbufs until the device is stopped *and* closed. The failure was a
SIGSEGV inside `rte_pktmbuf_free` reached only after every assertion in a test
had already passed. `Pool` is now a `Copy` handle over a registry; the mempool
is released during EAL teardown, after devices close, so there is no early free
to write. The registry is drained from `Eal::drop`'s *body*, not from
`mem::Manager`'s `Drop`, because a drop body runs before its fields and the
field version would land after `rte_eal_cleanup`.
**Nothing ever called `rte_eth_dev_close`.** Ports leaked their queue rings and
could not be re-probed. Its contract ("Close a *stopped* device. The device
*cannot be restarted*! The function frees all port resources.") maps exactly
onto a third typestate, so `close` exists only on `Dev<Stopped>`, `Closed` is
terminal, and a new sealed `Open` marker keeps port queries off a closed device.
The stop/close obligation moved into a small `PortLifecycle` field, which lets
`Dev` implement no `Drop` of its own -- so the typestate transition is an
ordinary move and the `ManuallyDrop` plus five `ptr::read`s it needed are gone.
**`Mbuf` was `Send`.** An mbuf is a bare pointer into a pool with no link back
to it, so a `Send` mbuf that escaped to another thread had nothing left keeping
its pool alive. It is now `!Send`, which forces every cross-thread handoff
through a conveyance that can carry a pool guard rather than an ad-hoc
`Vec<Mbuf>` behind a mutex. Nothing outside this crate needed it.
**Two threads could poll one queue.** `rte_eth_rx_burst` and `rte_eth_tx_burst`
must not be called concurrently for the same queue, but `receive` and `transmit`
took `&self` and `Dev::rx_queue` handed out an alias per lookup.
`receive`/`transmit` now take `&mut self`, and `Dev::<Started>::take_queues`
hands the queue set out once with each queue leaving it by value, so one queue
driven by two workers is a borrow error. Handles are branded with the device's
lifetime, so using a queue after the device stops does not compile. They
deliberately have no `Drop` impl: a handle is pure bookkeeping, and NLL already
rejects a *use* after stop while permitting a stop with a dead handle in scope.
Supporting work: `concurrency::local::Local<T>`, a reusable thread-affine
wrapper carrying a `ThreadId` for diagnostics -- documented as pinning
*ownership*, not use, since `AsRef`/`AsMut` hand out references whose
transferability is decided by `T`. `HairpinQueue::start` returns a `Result`
instead of `expect("todo")` twice.
Verified with 76 unit tests and 8 `compile_fail` doctests covering the illegal
transitions; `take_queues`'s once-only behaviour is not unit-testable, since
`--no-pci` means no device exists in the test EAL.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
The headline reason is the new EAL option `-A` / `--no-auto-probing`, which suppresses bus probing during `rte_eal_init` and leaves the application to probe each device itself. That is the missing piece for the note at `dataplane/src/runtime.rs:184`: the runtime has to call `rte_eal_init` early because rte_acl needs the memory subsystem before any configuration is applied, but only one init is permitted per process, so the eventual DPDK datapath driver has to inherit an EAL it did not parameterise. With `-A` the early init need not mention devices at all and the driver can probe them when it starts. Confirmed present in the built library; not adopted here, since using it requires the driver-side probe that does not exist yet. Beyond that: testing against the current release is the point. Refactoring against a release two versions back risks building toward something already superseded. The bump is nearly free. `v26.03-hh` was 49 commits over `v26.03-rc3`, of which 48 were just upstream rc3-to-final; the only Hedgehog change is the four-line meson fix that makes `pmdinfo` resolve `ar` through `find_program` so a cross-compile picks up the toolchain's archiver instead of the host's. That cherry-picks onto v26.07 unchanged, so `v26.07-hh` is upstream v26.07 plus that one commit and nothing else. It is still not upstream, so the fork stays for now. 26.07 is a benign target: no ABI change since 25.11, and every API change is an experimental-to-stable promotion -- including `rte_flow_dynf_metadata_*`, which backs `Mbuf::rx_meta`, and the mlx5 steering controls. The removals are all crypto PMDs this build already disables. DPDK compiled with the existing meson flags untouched. Checked every 26.07 deprecation notice against what this workspace binds. Clean on `rte_vfio_*` (the vfio work goes through sysfs, not the API), `rte_eth_dev_get_name_by_port`, `rte_mempool_cache` internals, `VXLAN_GPE`, the queue-stats mapping functions, and all of the non-compliant `rte_flow_item_*` structs. One genuine hit: the flow actions `SET_IPV4_SRC`, `SET_IPV4_DST`, `SET_TP_SRC` and `SET_TP_DST` -- the NAT rewrite path -- are marked legacy in favour of `MODIFY_FIELD`. There is no removal date, and this crate already has `MODIFY_FIELD` typed setters, so the migration is available whenever it is wanted. Nothing in the resource-lifecycle work touches deprecated surface. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
`Mbuf` became `!Send` in the previous commit, which closed a use-after-free but also forbade something legitimate. Moving mbufs between cores is a real requirement -- trapping control-plane packets (BGP, ARP, ND) to a tap writer, or handing a new connection to the core that owns the state for it -- and the framework should not decide which algorithms are expressible. So restore the capability with the guarantee attached rather than dropped. `Pool::consign` pairs a batch with a clone of the pool handle, and the resulting `Consigned` is `Send` precisely because it carries that guard: the mempool cannot be freed while the batch is in flight. It stays `!Sync`, so two threads working on one batch remains unrepresentable. The cost lands in the right place. A refcount on `Mbuf` itself would mean an atomic per packet on the receive and transmit paths, and at 64-packet bursts across a dozen workers that is millions of read-modify-writes per second on one cache line. One clone per *handoff* costs nothing measurable, because handoffs are the exception path. Ordinary receive-process-transmit still touches no atomic at all. `Pool` accordingly becomes `Clone` over an `Arc<PoolInner>` rather than a `Copy` handle. That also makes the device-to-pool edge structural instead of conventional: a `Dev` configured with a pool holds a strong reference through `RxQueueConfig`, so the pool outlives the device by construction. `PoolInner` may have a `Drop` again without reintroducing the original defect, because the crate's registry keeps one clone until EAL teardown -- a floor on every pool's reference count, so no other holder dropping its clone can free anything. `release_all` now drops handles instead of calling `rte_mempool_free` directly, and warns about any pool still held elsewhere: that pool leaks rather than being freed while referenced, which is the right way round. Two deliberate omissions. There is no way to unwrap a `Consigned` back into an `MbufArray`; that would hand the receiving thread an unguarded batch and reopen the hazard, so the batch is read through `AsRef`, modified through `AsMut`, and sent with `transmit_on`, which keeps the guard throughout. And `consign` checks every mbuf's originating pool rather than trusting the caller -- guarding the wrong pool would keep the wrong memory alive while proving nothing about the memory actually referenced. The check is O(n) on a per-handoff path. The transport is intentionally not part of this: any `Send` channel now carries a batch correctly. A DPDK-native one wants `Ring<T>`, which still has no `Drop` and never calls `rte_ring_free`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
`Consigned` has to return its mbufs to the pool before releasing the handle that keeps that pool alive; the other way round, the guard could be the last handle, the mempool would be freed, and the batch's bulk free would walk memory that no longer exists. That was expressed only as field declaration order plus a comment saying so -- an invariant a reorder breaks silently, with no compile error. Now stated in a `Drop`, following `quiescent::Subscriber`, which solves the same problem the same way for the same reason. Two things worth recording rather than glossing. The hazard is latent, not live. Reversing the fields *and* deleting the `Drop` leaves every test passing, because the pool registry holds a handle to each pool until EAL teardown, so the guard can never be the last one. The ordering becomes real when that floor goes away -- which the planned QSBR ownership work may well do, since its purpose is to decide reclamation by quiescence rather than by a process-lifetime handle. Keeping it explicit means that change need not rediscover this. `Dev` has the same shape and does *not* get the same treatment, so its comment now says why instead of claiming an ordering guarantee it does not enforce. A `Drop` on `Dev` is precisely what would forbid moving its fields out, which is what makes `transition` an ordinary safe move rather than a `ManuallyDrop` plus a `ptr::read` per field. It is safe to leave because no field other than `PortLifecycle` has any teardown effect on DPDK state: the only reachable `Drop` is the `Pool` clone in each queue config, and the registry floor covers it. Also adds `Pool::in_use`, wrapping `rte_mempool_in_use_count`, and a test that uses it as a real oracle for the drop path. "It did not crash" would pass on a leak, and a double free of a bulk batch pushes the same pointers into the ring twice, which shows up as an in-use count *below* the baseline rather than as a fault -- so occupancy accounting is what distinguishes all three outcomes. Break-tested: making the drop leak fails the assertion with `left: 8, right: 0`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
`Eal` carried a bare `unsafe impl Sync` with no safety comment. Worth being precise about what was wrong with it: nothing reachable through `&Eal` was actually unsound. The four manager fields are zero-sized, `mem` and `lcore` have no public methods at all, and `dev` and `socket` offer only read-only DPDK queries -- `rte_eth_dev_info_get`, `rte_eth_dev_count_avail`, `rte_eth_find_next_owned_by`, and NUMA topology lookups. So this is not a live defect being fixed. What was wrong is that the assertion was unconditional, and therefore pre-approved every field the type will ever gain. `Eal` is about to acquire state that genuinely cannot be shared: a QSBR publisher is `!Sync` by design, and the pool registry wants a single owner rather than a static. A blanket `Sync` would have made each of those silently unsound at the moment it was added, with the justification for the whole type having been written before any of them existed. So `Eal` becomes `!Send + !Sync` and `Eal::shared()` returns an `EalShared<'_>` that is `Copy + Send + Sync` by derivation rather than by assertion -- it holds only references to the managers whose operations each have a thread-safety argument, spelled out per operation in its documentation. `mem` and `lcore` are absent from it precisely because they have nothing shared-safe to offer yet and are the two most likely to gain owned state. `!Send` on the owner also encodes something DPDK already requires: `rte_eal_init` "is to be executed on the MAIN lcore only", and the thread that calls it becomes the main lcore, so the handle belongs to that thread and teardown happens there rather than wherever the value was last moved. Not wrapped in `Local<Eal>` despite that being the obvious tool, because the manager fields are public and reached as `eal.dev`; a wrapper turns every call site into `eal.as_ref().dev` for no gain. `Local` stays the right choice for a value whose interior is not a public namespace. The migration cost landed where expected: `test_support::start_eal` held the `Eal` in a `OnceLock`, which requires `Send + Sync`. It now leaks the owning handle -- it is `!Send`, so it has to stay on whichever thread got there first -- and stores the `'static` projection instead. This changes nothing about teardown: a `OnceLock` never dropped its contents either, so unit tests have never exercised `release_all` or `rte_eal_cleanup`. That is now written down rather than implicit. Three tests, all of which use the projection from a second thread, since that is the whole point. The `errno` one provokes a real DPDK failure (a duplicate pool name, `rte_errno_set` not being exported) to confirm the documented thread-local behaviour rather than assert it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
The reclamation protocol the DPDK lifecycle work needs, built and model-checked on its own before anything depends on it. `Reclaimer<T>` tracks a set of live resources and releases a retired one only once every worker has passed a quiescent point since the retirement. It is a thin layer over `quiescent` and inherits that module's central guarantee: the destructor runs on the owner's thread, not on whichever worker happened to hold the last reference. That is the reason for building on quiescence rather than a refcount -- several DPDK resources must be freed from the lcore that allocated them, and a refcount answers the wrong question anyway. It tells you whether anyone still holds a *handle*, when the dangerous references are precisely the ones that are not handles: a raw pointer on a worker's stack, an entry in a hardware ring. Retirement needs no changes to `quiescent`, because the live set *is* the published value. Retiring publishes a set without the entry, which leaves the previous set -- holding the last handle to it -- in the publisher's retired list until every subscriber advances. Entries still live appear in both sets, so only the removed one loses its last holder. Shutdown falls out of the same mechanism. `drain_until` retires everything and waits, and completes as soon as the workers are gone with no handshake: an `Attendant` removes itself from the domain when dropped, so once every worker has exited there is nothing left to wait for. The deadline exists for a worker that is wedged and never drops its attendant, and there the answer is to leak loudly -- releasing memory a live worker may still be reading is the failure this whole module exists to prevent, so it stays the wrong trade even under a stuck shutdown. It lives here rather than in `dpdk` so it can be model-checked at all: the dpdk crate links real DPDK, which no model checker can drive. Placed where it does not lie: `Attendant` is documented as per *logical* worker rather than per thread. An async task parked while holding a reference is its own worker for this purpose, and a per-thread attendant would let the thread report quiescence on behalf of a parked task that is anything but. And the discipline the guarantee rests on -- that a worker holds no reference to a tracked resource at the moment it calls `at_safe_point` -- is stated as a design decision about where that point goes, not buried as an implementation note. Verified under all three backends: the concurrent property passes under loom (exhaustive) and shuttle (random + PCT), and five protocol tests cover the sequential and wall-clock paths under the default backend. Break-tested by replacing deferred retirement with immediate release, which fails all five. Two traps hit while writing the tests, both now documented in place. Shuttle's PCT scheduler panics with "test closure did not exercise any concurrency" on a single-threaded body, so sequential properties cannot live in the model file -- `arc_weak.rs` opts out of shuttle wholesale for the same reason. And `thread::scope` joins on unwind, so a worker parked on a flag that a panicking test body never sets deadlocks the run instead of failing it; every gated worker is now released through a `Drop` guard. The first version of the wedged-worker test hung for ten minutes on exactly that. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
`tracing-test` was removed from `mgmt`'s dev-dependencies as unused, but two lines still referenced it, so `cargo check -p dataplane-mgmt --all-targets` failed to compile the test target. Removing the usage rather than restoring the dependency, because the dependency really was unused in the sense that matters: `#[traced_test]` installs a subscriber so a test can assert over captured logs, and nothing here calls `logs_contain` or `logs_assert`. The attribute was capturing output no assertion ever read. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Wires `mem::Manager` to `concurrency::reclaim`, answering the question the
manager's own source asked ("what if we use quiescent here instead?").
Until now `Eal::drop` freed every pool unconditionally, and its safety comment
had to *assume* that nothing still referenced them. That assumption was sound
only because devices are closed first, and it could say nothing at all about a
worker holding mbufs. The manager now owns a `Reclaimer<Pool>`: a retired pool is
freed only once every worker has passed a safe point since its retirement, and
whatever cannot be released within a five-second grace period is leaked loudly
instead. Leaking beats freeing memory a live worker may still be reading, which
is the whole reason the protocol exists.
It also buys drop affinity. `rte_mempool_free` now provably runs on the thread
that owns the `Eal` rather than on whichever holder happened to drop last -- the
guarantee `quiescent` exists to provide, and the reason this is built on
quiescence instead of a reference count.
The design problem worth recording is where creation lives. The reclaimer must be
`!Sync` for drop affinity, so it can only live in the owning `Eal` -- but
`Pool::new_pkt_pool` is called from arbitrary threads, including test threads
that cannot know which thread initialised the EAL. Requiring the manager for
creation would therefore have made pools unobtainable from most tests, and asked
more than DPDK does (`rte_pktmbuf_pool_create` is internally locked). So creation
stages into a `Sync` inbox and the manager *adopts* from it. The former registry
static becomes that inbox: no longer the authority, just a staging area that
holds a handle so a pool is never unowned between creation and adoption. Twenty-
seven call sites are unchanged as a result.
Two consequences stated rather than left implicit. A pool created with no `Eal`,
or after shutdown, stays in the inbox and is leaked -- correct for memory nothing
is tracking. And `shutdown` forgets the reclaimer whether or not the drain
succeeded, because dropping it during field-drop glue would free mempools after
`rte_eal_cleanup` has already run, which is the ordering bug this file was
cleaned up to remove.
This is also the change that makes `Consigned`'s drop order matter. The registry
was previously a permanent floor on every pool's reference count, which is why
reversing those fields changed no observable behaviour; retirement can now remove
that floor deliberately, so the explicit `Drop` added earlier is doing real work.
Adds a `Debug` for `Reclaimer` that reports only the live count: reading
`pending` borrows the publisher's retired list, and a `Debug` that can panic when
something else holds that borrow is a bad thing to reach for mid-diagnosis.
Four tests cover the manager's side, including one that had to assert a floor on
the reference count rather than an exact value -- the reclaimer keeps a clone in
both its live set and its published snapshot, and how many copies it keeps is its
own business.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
`SocketId::get_by_lcore_id` passed its argument straight to
`rte_lcore_to_socket_id`, which indexes a fixed array and whose contract requires
the id to "be between 0 and RTE_MAX_LCORE-1". The value most likely to arrive
there is `LCoreId::current()`, which is `LCORE_ID_ANY` (`u32::MAX`) on any thread
that is neither an EAL thread nor registered with the EAL. That read wildly out
of bounds and segfaulted.
It was reachable from entirely safe code, and easily. `Preference::CurrentThread`
resolved through that path and is the `#[default]`, so building an
`RxQueueConfig { socket_preference: Preference::CurrentThread, .. }` -- which
every example in this crate does -- was enough, on any thread that did not happen
to be the one that called `rte_eal_init`.
Found by accident: three new `ring` tests crashed the test binary with SIGSEGV,
and only when more than one of them ran. That is the tell. With a single test the
process initialises the EAL on the thread running it, so `current()` is a real
lcore; with two, the second runs on a thread that never registered.
Two fixes. `get_by_lcore_id` now bounds-checks and returns `Option`, which is
what its doc comment already claimed ("Returns `None` if the lcore is not
valid") and its signature did not. And `Preference::CurrentThread` resolves
through `rte_socket_id` instead, a per-thread read that is valid from anywhere
and reports `ANY` for a thread with no NUMA affinity -- which is the right answer
for "this thread's socket" when the thread has none.
Three tests, including a regression test that resolves the default preference
from a plain `std::thread` and asserts it neither faults nor reports a valid
lcore.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Two leaks, each of a resource that is exhaustible. `Ring` never freed anything. `rte_ring_create` reserves a named memzone and nothing in the crate ever called `rte_ring_free`, so a ring's name stayed taken for the life of the process and a second ring with the same name failed with `MemZoneExists`. Currently latent -- `Ring::new` is private and unused -- but the module is the natural transport for the cross-core mbuf handoff added earlier, so it would have been discovered by using it. `ServiceThread` called `rte_thread_unregister` as a bare statement after `run()`, which is skipped when `run()` unwinds. Every skipped unregister strands an lcore id permanently; `RTE_MAX_LCORE` of them and no further thread can register at all. A panicking service thread is exactly the case that matters, being also the one most likely to be retried. Now released by a drop guard, which also documents why it must never move off its thread: `rte_thread_unregister` releases the *calling* thread's id, so running it elsewhere would strand this one and corrupt another. The ring tests use name reuse as the oracle, which is a real one: a name is a process-global memzone name, so it can only be reused if the memzone was actually released. Break-tested by disabling the free, which fails both with `MemZoneExists`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
`Manager::id_for_lcore` rejected an id when `id >= rte_lcore_count()`. That is the count of *enabled* lcores, not the highest valid id, and lcore ids are not required to be dense: `--lcores 0@(0),7@(7)` enables ids 0 and 7 with a count of 2. So the single comparison was wrong in both directions -- it rejected lcore 7, a perfectly valid enabled lcore, while accepting id 1, which is not enabled at all. Both questions are now asked where they belong, in `get_by_lcore_id`, and `id_for_lcore` delegates. The range check is a memory-safety requirement, since `rte_lcore_to_socket_id` indexes a fixed array (see the previous commit). The enabled check is a correctness one: an in-range id the EAL did not enable has whatever socket the array was initialised with, which is a plausible-looking answer to a question that has none. The sparse case that distinguishes old from new is not testable here, and the test says so rather than implying coverage it does not have. The shared test EAL passes `--lcores 0@(0,1,...)`, which enables exactly one lcore and so is dense; the road tests pass no `--lcores` and enable every detected lcore, also dense. DPDK permits one `rte_eal_init` per process, so a test cannot stand up a sparse configuration of its own, and the shared arguments exist for an unrelated reason (thread affinity, see `eal::main_lcore_arg`). The assertion is kept because it is the right one and would hold a regression wherever it does bite. Writing those tests turned up something worth knowing separately: `LCoreId::iter` passes `skip_main = 1` to `rte_get_next_lcore`, so it enumerates *worker* lcores and is **empty** under a configuration whose only lcore is the main one -- which is exactly the test EAL. An early version of these tests used it as "the set of enabled lcores" and failed for that reason. They now scan `rte_lcore_is_enabled` directly, which is the ground truth. Whether `iter`'s name and doc should say "worker" is left alone here; it may well be deliberate, since DPDK's own dispatch convention excludes the main lcore. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
`LCoreId::iter` passed `skip_main = 1` to `rte_get_next_lcore` while its constructor's documentation said it "loops over all available LCoreId". The behaviour won, and the consequence is worse than a doc error: under any configuration whose only enabled lcore *is* the main one, the iterator yielded nothing. That includes the unit-test EAL, whose `--lcores 0@(0,1,...)` maps one lcore across every CPU -- so "iterate the lcores" returned an empty set and looked like a broken EAL. Now `all()` and `workers()`, named for what they do, with the choice a field on the iterator instead of a hard-coded argument. `main()` already existed. The `LCoreIndex` iterator keeps skipping main, which is what its own constructor documentation has always said and presumably meant. Found by writing tests for the `id_for_lcore` fix: an early version used `LCoreId::iter()` as "the set of enabled lcores" and failed, because it is empty here. The test that would have caught the original is now present -- under the test EAL `workers()` is legitimately empty while `all()` must not be, which is precisely the case the old single iterator got wrong. A second test cross-checks that everything `all()` yields is in range and enabled per DPDK's own predicate, so the two notions of "enabled" cannot drift apart silently. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
`chore(build): remove unused deps` dropped three dependencies that were still referenced, and the references sit in places a plain `cargo check --all-targets` does not build, which is why they went unnoticed. `mgmt/tests/reconcile.rs` is declared with `required-features = ["bolero"]`, so it is skipped unless that feature is on -- and it accounted for two of the three. The two deps want opposite treatments, so they get them. `caps` is restored to `mgmt`'s dev-dependencies. It is genuinely used: `#[wrap(with_caps([Capability::CAP_NET_ADMIN]))]` gates those tests on real Linux capabilities, so dropping the usage would quietly change what they exercise. `tracing-test` stays removed and the last two usages go instead, matching the two already cleaned up. Every remaining reference was a bare `#[cfg_attr(not(emulated), traced_test)]` with no `logs_contain` or `logs_assert` anywhere in the file, so the attribute was installing a subscriber to capture output that no assertion ever read. Seven other crates still declare `tracing-test` and still use it; only `mgmt` and `k8s-less` had it removed. Verified with `cargo check --workspace --all-targets`, plus a sweep of every crate that declares a `required-features` test target built with those features on -- which is the check that would have caught this in the first place. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
…l frees `hardware/tests/dpdk_in_vm.rs` called `std::mem::forget(eal)` in every test, so the teardown path this branch spent its time building was never once executed. The stated reason was that `Eal::drop` "calls `rte_eal_mp_wait_lcore()` which blocks indefinitely waiting for worker lcores to finish". That attribution is measurably wrong. `dpdk/examples/eal_teardown_probe.rs`, added here, times each teardown phase from arguments given on the command line (`rte_eal_init` may run once per process, so comparing configurations needs separate processes). `rte_eal_mp_wait_lcore` returns in **731ns** with 64 lcores enabled and none launched. Telemetry -- the leading suspect, since these EAL arguments omit `--no-telemetry` while the unit-test ones pass it -- makes no difference, and neither does the road test's four `--log-level=...:debug` flags. `rte_eal_cleanup` itself takes under 300µs in every host configuration tried. In the VM the whole teardown now runs in about 120µs and all 8 unskipped variants pass. What is *not* established is why it ever hung. The candidates are the DPDK 26.03 -> 26.07 bump and the device now being closed explicitly, since nothing ever called `rte_eth_dev_close` before; distinguishing them would mean running the suite against the old pin. Rather than invent a cause, the tests now assert the property that matters -- teardown completes inside a five-second budget -- so a recurrence fails on a timer instead of wedging the VM, which is what made the original so unpleasant to diagnose. Removing the leak immediately exposed a defect the leak had been hiding, and it is one this branch introduced. Making `Pool` reference-counted fixed pools being freed too *early*; it created the mirror image. A handle that outlives the `Eal` becomes the last one, and its `Drop` then calls `rte_mempool_free` against an EAL that `rte_eal_cleanup` has already dismantled -- a segfault, reached in `run_rx_test` simply because `tx_pool` is still in scope at function exit. The counterfactual is worth recording too: dropping the `Eal` while the device is still open does not hang either, it completes in 867µs and then segfaults when the device is closed against a torn-down EAL. A `Pool` is an ordinary value with no lifetime tying it to the EAL, and giving it one would mean `Pool<'eal>` spreading a parameter through every holder -- the cost this design exists to avoid. So the guard is a runtime flag set between the pool drain and `rte_eal_cleanup`, after which `PoolInner::drop` logs and leaks instead of freeing. That is the same trade `Manager::shutdown` already makes for a pool a worker still holds: a deliberate leak beats a free that cannot be safe. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
A spike, and it answers the question that prompted it: the viral generic is real
inside this crate and **invisible at its boundary**.
`Pool<'eal>` is a `Copy` handle whose mempool belongs to `mem::Manager` and is
freed once, during `Eal` teardown, after every device is closed. The brand is
what makes it safe, in the type rather than in a runtime check. This crate has
now tried all three options and only the third works:
- an owning `Pool` freed its mempool when the handle went out of scope, which
made the natural teardown order the wrong one -- a SIGSEGV inside
`rte_pktmbuf_free` after every assertion had already passed;
- a reference-counted `Pool` fixed that and introduced the mirror image, a
handle outliving the `Eal` and freeing against a dismantled EAL, which
needed a runtime flag to stop being a segfault;
- the brand makes both unrepresentable and needs neither.
So this reverts the `Arc<PoolInner>`, the `EAL_TORN_DOWN` guard, and the wiring
of `concurrency::reclaim` into `mem::Manager`. Pools that live for the whole EAL
scope never retire mid-run, so quiescence buys them nothing; `reclaim` remains a
tested generic module for resources that do.
**Measured ripple.** Branding `Pool` propagated to `DevInfo`, `RxQueueConfig`,
`RxQueue`, `TxQueue`, `HairpinQueue`, `QueueStore`, `Dev`, `Consigned`, and the
`flow` builder and rule -- ten types, all inside this crate. It produced **68
errors across 27 files, and not one of them was a lifetime error in caller code**.
Twenty-nine were the mempool constructor moving from `Pool::new_pkt_pool` onto
`Manager::new_pkt_pool`, which is mechanical and orthogonal. The rest were dead
code from the reverted design. Elision covers every example, because an `Eal` is
a local that outlives what it lends.
Two places the brand had to be *derived* rather than chosen, or it would have
proved nothing. `DevConfig::apply` taking an unconstrained `<'eal>` would let a
caller name `'static` and manufacture a `Dev<'static, _>` out of nothing, so it
takes a branded `DevInfo` instead; and `DevIndex::info` has no input to derive a
brand from at all, so it is now crate-private and the public routes
(`Manager::iter`, `Manager::info`) take `&'eal self` and shorten the result.
The cost lands where you predicted -- one API opinion, and a mild one. A function
cannot create the `Eal` *and* return resources borrowed from it, so
`setup_dpdk_device` in the in-VM road test splits into `init_dpdk_eal` plus a
borrowing `setup_dpdk_device(&eal, ..)`. Which is where EAL creation belonged
anyway: once, at the top of the scope that uses it.
And it immediately earned its keep. The road test's teardown helper took the
`Eal` and the `Dev` by value together, and that no longer compiles, because the
device borrows the EAL. The ordering that file previously got right by convention
-- with a comment explaining why the natural order was unsafe -- is now a compile
error to get wrong.
`mem::Manager` rejoins `EalShared`: it is `Sync` again with the reclaimer gone,
and pool creation has to be reachable from a thread that does not own the `Eal`,
since a test cannot know which thread initialised it.
Verified: 93 unit tests, 12 doctests including two `compile_fail` cases pinning
the brand, clippy and rustfmt clean, `cargo check --workspace --all-targets`
clean, and 8/8 in-VM road-test variants passing with teardown running for real.
`Mbuf` is deliberately not branded yet; that is the one expensive case, and it is
separable.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Two live use-after-frees, and a correction to what I estimated the fix would cost.
`MbufArray` and `Mbuf` were unbranded, so both of these compiled -- each a
`rte_pktmbuf_free` into a mempool the EAL had already released:
let pool = eal.mem.new_pkt_pool(cfg)?;
let mbufs = pool.alloc_bulk(4)?;
drop(eal);
drop(mbufs); // and the same with a single Mbuf via into_iter()
`Pool<'eal>` alone could not stop it: `alloc_bulk` took `&self` and returned an
unbranded value, so the batch simply outlived the borrow.
**The cost estimate was wrong, and badly.** I had put this at ~40 files across 8
crates and recommended deferring it on that basis. That figure was `Packet<Buf>`
*usage*, which is generic: `Buf` is a type parameter, so substituting
`Mbuf<'eal>` needs no change to `Packet` or to anything generic over it. Outside
this crate, `Mbuf` is named only in doc comments in `net`. The real ripple is 0
errors in the dpdk lib after the signatures were updated, **0 across all 25
examples**, and 4 in the in-VM road test.
Three of those four were the borrow checker enforcing what the file previously
enforced by comment -- queue handles borrow the device, the device borrows the
EAL -- so the teardown chain is now a compile error to get wrong rather than a
convention with an explanation attached.
The fourth was a real bug nobody had noticed. `run_rx_test` left the `unsent`
batch returned by `transmit` alive until the end of the function, so on any run
where the tx queue *did* refuse a frame, that batch's `Drop` freed mbufs after
the pool was released. The assertion above it says the batch is empty, which is
why this never showed: the bug was reachable only on the failure path, where the
test was already going to fail for a different reason and would have segfaulted
instead of reporting it.
One thing worth recording for the next branding: a closure cannot express "the
output borrows as long as the input does" -- it infers a fresh lifetime per
parameter and cannot tie them -- so `make_batch` had to become a `fn` with a
named lifetime.
Verified: 93 unit tests, 14 doctests including four `compile_fail` cases pinning
both holes shut, clippy and rustfmt clean, `cargo check --workspace
--all-targets` clean, and 8/8 in-VM road-test variants passing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
`Packet<Mbuf<'eal>>` did not compile, and so the pipeline this dataplane exists to run had never been run over the buffer it exists to run on. `TestBuffer` was not merely the convenient inhabitant of `PacketBufferMut` -- it was the only possible one. Two bounds stood in the way, and neither was a property of packet buffers. **`PacketBuffer: 'static`** was there to satisfy `DynNetworkFunction: Any`, which backed `DynPipeline::get_stage_by_id` and `get_stage_dyn_by_id`. Those have no production caller; the only code that ever downcast a pipeline stage was the two unit tests written for the feature. `TypeId` requires `'static` for soundness, so a runtime stage downcast and a buffer that borrows are mutually exclusive -- one of them had to go, and it is not the one the datapath needs. An `Mbuf` carries the lifetime of the `Eal` that owns the mempool its bytes live in, precisely so that an mbuf whose `Drop` would free into a dismantled pool cannot be written. **`PacketBufferMut: Send`** asserted something that is false. `Mbuf` is deliberately `!Send`: it is a bare pointer into a mempool with no refcount tying the two together, so an mbuf that escapes to another thread has nothing left proving its pool outlives it. Crossing a thread goes through `Pool::consign`, which guards the whole batch and earns its `Send`. Requiring it here would have forced either an unsound `unsafe impl` or a per-mbuf atomic on the datapath. Nothing wanted it: removing it produces no error anywhere in the workspace. `DynPipeline` and `StageId` gain an `'nf` lifetime. This cannot be elided away. A `dyn Trait<Buf> + 'a` is well formed only when `Buf: 'a`, and a bare `Box<dyn DynNetworkFunction<Buf>>` in a struct field takes the default object lifetime bound of `'static`, which reintroduces exactly the requirement being removed. Rust has no way to say "this trait object lives as long as that type parameter". Every existing user becomes `DynPipeline<'static, Buf>` and is otherwise untouched. One sharp edge worth recording. `NetworkFunction::process` returns `impl Iterator<..> + 'a`, so an impl whose concrete return type *names* `Buf` -- `FlowFilter` and `PacketStatsNF`, which both materialise the burst into a `Vec` -- needs `where Buf: 'a`. Declaring it on the trait method is not enough; the impl must repeat it, because the impl signature's well-formedness is checked on its own. Every other network function returns a `Map<Input, _>`, which never names `Buf`, and needed nothing. `get_stage_by_id` and `get_stage_dyn_by_id` are removed rather than deprecated, along with `DynNetworkFunctionImpl::get_nf`, which existed only to serve them. Their two tests went with them; since they were also the only exercise of `add_stage_with_id`, that path keeps a test -- a better one, which asserts the duplicate-id rejection the old pair never reached. Verified: 1938 workspace tests, doctests in `net`, `pipeline` and `dpdk` including all 14 `compile_fail` cases pinning the EAL brand, clippy clean per crate as well as across the workspace, rustfmt clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
`rte_eth_stats_get` was bound in `dpdk-sys` and wrapped nowhere, so nothing in this workspace could read a port's counters. That is a gap for a production dataplane on its own, but the reason it surfaced now is that it makes a whole class of receive-side fault undiagnosable. On a bifurcated driver such as mlx5 the kernel keeps the netdev, so `ethtool -S` reports the *kernel's* queues. It will happily show `rx_vport_unicast_packets` counting every frame the device accepted while saying nothing at all about whether any DPDK queue received them. Chasing a 45% loss through those counters gets as far as "the hardware definitely got them" and then stops. `PortStats::imissed` is where the missing frames are: the port had them and had no free descriptor to put them in. `rx_nombuf` separates that from the mempool having run dry, which presents identically from the outside and has a different fix. Neither has any other observable. `PortStats` is an owned snapshot rather than a borrow of `rte_eth_stats`: the counters are read by value at a point in time, and copying eight `u64`s is cheaper than reasoning about when the driver may rewrite the struct underneath a reference. `reset_stats` is included because a caller measuring one interval should not have to subtract by hand, though it takes `&mut self` and so is unavailable while queue handles borrow the device -- a delta across two `stats()` calls is the usable form there. Both live on `Dev<S: Open>`, alongside `mac_address`: a closed port has no counters to report. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
`dpdk_on_nic.rs` has not compiled since the `'eal` branding landed. Five errors:
`Dev` and `Pool` grew a lifetime parameter, and the mempool constructor moved
from `Pool::new_pkt_pool` to `Manager::new_pkt_pool`. The in-VM road test was
updated in the same change; this one was not.
The reason it went unnoticed is the interesting part. This test is gated behind
`--cfg nic_loopback_tests` rather than `#[ignore]`, which is the right call --
it needs a specific rig and should not pretend to be skippable -- but it means no
ordinary build compiles it, including `cargo check --workspace --all-targets`.
A cfg-gated test is invisible to every gate the repository has. It rotted the
moment the crate it drives changed shape, and nothing said so.
`bring_up` now takes `&'eal Eal` so the pools it creates are branded, `Port`
carries `'eal`, and `Direction::tx_pool` borrows a branded `Pool<'dev>`.
`Port::shutdown` needed more than a signature. It was written to defeat Rust's
drop order by hand:
let Port { dev, tx_pool, .. } = self;
drop(dev);
drop(tx_pool);
with a comment explaining that freeing the pool before stopping the device would
leave the stop path walking freed memory. That is now a no-op the compiler warns
about -- `Pool` is a `Copy` handle, and the mempool belongs to `mem::Manager`,
which frees it during EAL teardown after every device is closed. The ordering the
comment worked to preserve is now structural and cannot be got wrong, so the
`drop` dance is replaced by what the file actually wants: an explicit
`stop()?.close()?`, which reports the driver's error instead of leaving
`PortLifecycle`'s backstop to log it.
Verified on a BlueField-3 with both ports cabled: passes, 64/64 in each
direction.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
`offloads: dev.info.rx_queue_offload_caps()` reads like "accept the device's
defaults". It is the opposite: it turns on every receive offload the hardware is
capable of, whether or not the application can cope with the consequences. On a
BlueField-3 that mask is 0x18601f -- VLAN_STRIP, the L3/L4 checksums, TIMESTAMP,
RSS_HASH, SCATTER, BUFFER_SPLIT, and TCP_LRO.
TCP_LRO costs a factor of 32 in receive buffering.
LRO has to be able to hand back a coalesced segment of up to 64 KiB. These pools
have a 2048-byte data room, so the PMD reserves ceil(65536 / 2048) = 32
descriptors for every packet it might have to coalesce into. A ring asked for
1024 descriptors then buffers 32 frames. `rte_eth_dev_adjust_nb_rx_tx_desc`
reports 1024 in and 1024 out, so the descriptor count is not where the lie is.
Measured on the rig, one variable at a time, with a 64-frame burst and 5 ms
before the first poll:
full capability mask 32 received, 32 imissed
mask minus TCP_LRO 64 received, 0 imissed
mask minus BUFFER_SPLIT 32 received, 32 imissed
RxOffload::NONE 64 received, 0 imissed
and the shortfall scales: 128 descriptors buffer 4 frames, 1024 buffer 32,
always nb_rx_desc / 32.
The irony is that this file already knew. Its port-level config sets
`rx_offloads: Some(RxOffload::NONE)` with a comment saying that requesting
everything would include "LRO, which would coalesce the very frames we are
counting" -- and then the queue config turned LRO back on one field later. The
port mask and the per-queue mask are separate in DPDK, and only one of them was
being chosen deliberately.
It survived because this test polls in a tight loop. Frames are consumed as fast
as they land, so a 32-deep ring never fills and the test passes at 64/64. The
defect appears the moment anything real happens between polls, which is the
situation every actual dataplane is in.
`tests/dpdk_in_vm.rs` carries the same pattern and is deliberately left alone: it
drives an emulated e1000 whose capability mask is nothing like this one, and it
was not re-verified here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Nothing had ever run a network function over a DPDK mbuf. `dpdk_on_nic` proves
the transport -- a frame put on the wire by one port is dequeued by the other --
but it never builds a `Packet`, never parses a header and never runs a stage.
Everything above the queue had only ever seen `TestBuffer`, and until the buffer
bounds were relaxed it could not have seen anything else.
The two are not interchangeable. A `TestBuffer` is a `Vec` with generous
headroom and no ownership, alignment or aliasing constraints. An mbuf's bytes
live in a mempool, its headroom is whatever the PMD left, and the
parse/mutate/serialize round trip writes back into memory the NIC DMA'd into.
Port A generates and verifies; port B is the dataplane. A frame crosses the fibre
twice: A sends an IPv4/UDP probe with ttl 64 addressed to B, B receives it as an
mbuf, wraps it in a `Packet`, runs `DynPipeline`, serializes back into the same
mbuf and transmits, and A checks what comes back. No copy and no second pool --
the batch that leaves is the batch that arrived.
The oracle is arithmetic, not "a frame came back":
- ttl is exactly 63. Not `< 64`. `DecrementTtl` is production code; if the
pipeline ran twice, or not at all, or the write landed at the wrong offset,
the answer is not 63.
- the IPv4 header checksum still verifies. Decrementing the ttl invalidates it,
so a correct checksum on arrival means the serializer recomputed it *and*
wrote it into mbuf memory.
- the destination MAC is A's -- written by a network function at offset 0, the
byte most likely to be wrong if headroom accounting is off.
- payload magic, nonce and sequence are unchanged, so the header rewrite did
not walk into the payload.
It also asserts `imissed == 0` across the measured round, which is what makes
this an oracle for receive configuration rather than a race it happens to win.
The `TCP_LRO` misconfiguration fixed in the previous commit cost a factor of 32
in receive buffering and was caught here only because a debug build is slow
enough between polls for the shortfall to matter. That is not something to rely
on. Break-tested in both directions: the assertion fires on the misconfiguration
and fails when given a value the port does not report.
Two stages, not the router's fourteen. The production stages want a configured
router, VPC tables, NAT allocators and interface metadata; standing that up here
would test configuration plumbing rather than the buffer substitution this
exists for. `DecrementTtl` is taken unmodified from `pipeline::sample_nfs`.
`RewriteDstMac` is local, because `BroadcastMacs` would address the forwarded
frame to the broadcast MAC, which the kernel's default steering on the receiving
netdev consumes -- and a next-hop MAC rewrite is what a forwarding dataplane does
anyway.
The steering gotcha applies in both directions here, which is why that stage is
not optional: the kernel keeps each netdev and its default steering, so a frame
matching no filter is discarded at the PHY before any DPDK queue sees it.
Gated behind `--cfg nic_loopback_tests`, like its neighbour. Verified on a
BlueField-3 with both ports cabled: 64/64 verified, zero failures, lossless on
the first round.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueComment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
scratch: don't yet review please
🤖 Generated with Claude Code