Repository navigation
(3a) init: honor the hugepage allotment, size receive rings to it - #2038
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
|
|
750d436 to
7e0155e
Compare
5b4ae94 to
c93ff50
Compare
c93ff50 to
7b6a57a
Compare
mvachhar
left a comment
There was a problem hiding this comment.
Note
🤖 AI-drafted (Claude) on behalf of @mvachhar, reviewed by him before posting.
Approving: no blockers, and deeper rings sized to the hugepages are a good change. Verified:
- Without
--hugepages-gib, the old path is unchanged, and the deeper-ring path can't introduce a failure. - The cgroup allowance is honored on every path.
- One shared formula: pool sizing lives in
args::dpdk_memory, shared by init and the dataplane, so they can't drift. - Ports that can't take 4096 descriptors are clamped by the dataplane; init only over-reserves.
- Heap metrics are cheap (once a second), and the routing test's wait is bounded.
Should fix (not blocking):
-
--hugepages-gibmeans different things to fabric and the dataplane. Fabric's gateway controller (pkg/ctrl/gateway_ctrl.go:557-581) passesgatewayHugePages1G = Nboth as the pod'shugepages-1Gilimit (N GiB total) and as--hugepages-gib N. The dataplane treats N as a budget per NIC NUMA node. On a gateway with NICs on two nodes, init plans 2N GiB against a cgroup limit of N and fails. The mismatch predates this PR, but now that the allotment is a strict budget with no 2 MiB fallback, it's worth settling: either a total here, or fabric multiplying by NIC nodes. @Frostman, which do you prefer? Please also update the stale help and field docs (args/src/lib.rs:1810-1813,~:643): they still say it "overrides the amount init computes", and mention neither "budget" nor "1 GiB pages only". -
The doc comment on the ignored power test is wrong (
dpdk/src/power/tests.rs, and the PR description too). It says the EAL hands MWAITX the low 32 bits of the absolute deadline and that the relative-time fix isn't in. It is in:amd-mwaitx-relative-timeout.patchis applied innix/pkgs/dpdk/default.nix:49-50, on main since 16a6967. With it applied, the remaining issue is waits returning in about 450 ns on the Ryzen 7000 runner. Please fix the comment so it doesn't send the next reader after an already-fixed bug. -
Tests:
reserve_foritself isn't tested. That means:- the precedence (allotment → deeper rings in free pages → the old path);
- only ports without
/rxdbeing changed; - multi-node allotments with a different depth per node;
- the assumption in
deepest_fittingthatRX_DESCRIPTOR_CHOICESis sorted withRX_DESCRIPTORSlast.
A property test (development/code/property-testing.md) would cover it well: for any need and budget, the chosen ring plus the reserve fits, and nothing deeper does.
-
SocketId::heap(dpdk/src/socket.rs:91-105) is a safepub(crate)fn that callsrte_malloc_get_socket_stats, which reads EAL's memory config. That's undefined behavior before EAL init. The SAFETY comment relies on a manager existing, butDev::heapscalls it without one. Please take&Manager/&Eal, or tie it to the'ealborrow. -
The chosen depth goes back to the dataplane by re-parsing PCI addresses (
name_rx_descriptors,init/src/main.rs:1110), and a parse failure is skipped silently. Carrying the depth on the resolved device would keep the invariant in a type. A miss is safe today, since the dataplane falls back to 1024, which init over-reserved for.
Notes:
- Misleading log:
within_allotmentlogs a chosen depth for a node even when every port there set/rxd. - "Per node" only applies with an allotment. Without one, a single depth is chosen for all nodes; the PR text says per node.
- Page premapping: the deeper-ring path premaps pools + 512 MiB in free 1 GiB pages, more than before. Fine on gateway nodes, but worth a release note.
- Doc drift: the
RESERVE_MIBdoc says "~95 MiB … ~310 MiB"; the PR body says "~80 MiB … ~295 MiB". - CI: the
docsstep ofcheck/debugis failing. - Slop (~30 lines):
- the test-only
placeshim ininit/src/hugepages.rs(~14 lines); - the trivial
Manager::heapwrapper (~8 lines); - measurement prose duplicated between
RESERVE_MIBand the PR body.
- the test-only
What of the hugepages EAL was given the pools, rings, ACL tables and
drivers have allocated was not visible anywhere: EAL maps the whole
--numa-mem up front, and runs without telemetry.
- dpdk: SocketId::heap / socket::Manager::heap read
rte_malloc_get_socket_stats; Dev::heap gives the heap of a device's
node.
- The driver logs each NIC node's heap once the ports are up, and
publishes dpdk_heap_bytes{node,state=total|free|allocated} and
dpdk_heap_largest_free_bytes{node} every second.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Production gives the gateway far more 1 GiB hugepages than the default rings need. Init now uses them for deeper receive rings, and treats an allotment as a budget: - With --hugepages-gib N, N 1 GiB pages per NIC NUMA node are the budget. They must be available: there is no falling back to 2 MiB pages. The ports and workers must fit in them, or init fails with what they need. Workers are taken as given. - Interfaces without /rxd=N get the deepest of 4096, 2048 and 1024 receive descriptors per queue whose pools leave RESERVE_MIB (512 MiB) of the budget free, chosen per node and logged; init passes the choice to the dataplane in the launch configuration. Deeper than 4096 is never chosen. - Without --hugepages-gib, an interface without /rxd=N gets 4096 or 2048 only if 1 GiB pages already free hold that with the reserve; init does not grow the pools for a deeper ring. Otherwise 1024, with the requirement computed (and the pools grown) as before. - An explicit /rxd=N is never changed. The reserve covers what the heap holds beyond the pools: measured on a BF-3 with two ports and NIC offload, about 80 MiB plus 3.6 MiB per worker, and at most 99 MiB more for 16,384 ACL denies and 2,048 rate limits through a change of every rule. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
test_cli_handling took the reply to a ctl request as proof that the router had done everything asked of it. It had processed every ctl message, but it records the FRR config as applied only when the (fake) frr agent answers, which can come later: the check of the last applied config then failed now and then (2 of 15 runs locally, and in CI). The sync now polls until the config shows as applied, for up to 10 s, as frr/test.rs does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
…understood an_idle_wait_sleeps_until_its_deadline fails now and then on AMD, in CI and locally: the EAL hands MWAITX the low 32 bits of the absolute TSC deadline (DPDK 1213a8895b, still so upstream), where its timer takes the time to wait, so an idle wait ends at the next interrupt instead (2 us to 1 ms for a 20 us wait on a Zen 4 workstation). Passing MWAITX the time to wait made every wait 10 us on a Threadripper 7000 (family 0x19 model 0x18), but on the Ryzen 7000 CI runner (model 0x61) every such wait returned in about 450 ns, so that change is not made yet. The test is ignored with a note until this is understood, so CI does not fail at random, and a failure now reports the CPU's vendor, family and model, the sleeper, and every wait's duration. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
7e0155e to
c63c947
Compare
7b6a57a to
5324052
Compare
Stack (NIC offload; review and merge in order): #2030 (1) → #2031 (2) → #2032 (3) → this PR (3a); #2033 (4) is parked.
Production gives the gateway far more 1 GiB hugepages than the default rings need (vlab does not). Init now treats an allotment as a budget and sizes the receive rings to it.
--hugepages-gib Nis a budget of N 1 GiB pages per NIC NUMA node./rxd=Nget the deepest of 4096, 2048 and 1024 receive descriptors per queue whose pools leaveRESERVE_MIB(512 MiB) of the budget free. The choice is made per node, logged, and passed to the dataplane in the launch configuration. Nothing deeper than 4096 is chosen.--hugepages-gib, 4096 or 2048 only if 1 GiB pages that are already free hold that with the reserve; init does not grow the pools for a deeper ring. Otherwise 1024, with the requirement computed (and the pools grown) as before./rxd=Nis never changed.dpdk_heap_bytes{node,state},dpdk_heap_largest_free_bytes{node}, and a startup log line). What the pools, ACL tables and drivers hold of the hugepages was not visible before: EAL maps all of--numa-memup front and runs without telemetry.Reserve, measured on a BlueField-3 with two ports and NIC offload on:
Beyond the pools that is about 80 MiB plus 3.6 MiB per worker, and at most about 99 MiB for the largest policy tried. The worst case, about 295 MiB, fits within the existing 256 MiB overhead plus the 512 MiB reserve.
Tested on the BF-3:
Unit tests cover choosing within an allotment, a too-small or unavailable allotment, and deeper rings from free pages only.
Two flaky tests on main, fixed here so CI is quiet:
power::tests::an_idle_wait_sleeps_until_its_deadlineis ignored for now, with a note in its doc comment. On AMD, DPDK'srte_power_monitorhands MWAITX the low 32 bits of the absolute TSC deadline, but MWAITX's timer takes the time to wait (DPDK 1213a8895b, still so upstream), so an idle wait ends at the next interrupt: 2 µs to 1 ms for a 20 µs wait on a Zen 4. Passing MWAITX the time to wait fixed it on a Threadripper 7000 (family 0x19 model 0x18), but on the Ryzen 7000 CI runner (model 0x61) every wait then returned in about 450 ns, so no fix is in this PR. When run with--ignored, a failure now reports the CPU, the sleeper and every wait's duration.routingtest_cli_handlingread the last-applied FRR config before the fake FRR agent had answered. It now waits for it (40 of 40 runs pass).🤖 Generated with Claude Code