Stephen, Please see inline, look for #RT: Thanks, -rt From: Stephen Hemminger <[email protected]> Date: Tuesday, October 6, 2026 at 12:51 PM To: Randy Tice (rtice) <[email protected]> Cc: [email protected] <[email protected]>; Morten Brørup <[email protected]>; Nithin Dabilpuram <[email protected]>; Harman Kalra <[email protected]> Subject: Re: [PATCH v4 0/1] mbuf: add runtime metadata dynamic-field storage
On Tue, 06 Oct 2026 11:54:37 -0400 Randy L Tice <[email protected]> wrote: > This revision changes direction from the v3 build-time dynfield3 > layout. It adds a runtime EAL-configured per-mbuf metadata area that is > placed after the fixed struct rte_mbuf header and before per-pool private > data. This keeps sizeof(struct rte_mbuf) fixed while allowing deployments > that need globally consistent per-mbuf metadata to reserve that storage. > > The metadata area is still managed by the mbuf dynamic-field registry. > Fields registered with RTE_MBUF_DYNFIELD_F_METADATA are allocated from the > metadata area and are not copied by generic mbuf copy, clone, or attach > operations. Fields registered without the flag continue to use the existing > copied mbuf dynamic-field storage and cannot overlap the metadata area. > > The existing per-pool private data area does not provide a central layout > registry and is configured independently for each mbuf pool. That makes it > hard for multiple libraries, drivers, or application modules to safely share > metadata without out-of-band coordination, especially when pools are created > by different components. > > Mbuf object layout calculations that need to include the optional metadata > area now use rte_mbuf_size(). The primary process validates and publishes > the metadata size through the shared mem config; secondary processes may > omit the option, but if provided it must match the primary value. > > The octeontx mempool driver rejects allocation when mbuf metadata is > configured because the hardware mbuf header offset must remain 128 bytes. I did a brief look at this and still not convinced about the exact use case. What is the problem this is trying to solve and why can't it be done by using existing API’s. #RT: Our implementation currently requires an additional 256 bytes of metadata per mbuf. That does not fit in the existing dynamic-field storage, so today we carry a private patch to extend struct rte_mbuf. The goal of this work is to replace that private struct change with a supported upstream mechanism. We did look at using mbuf private data first. It can work when the application owns all mbuf pool creation, but it is not sufficient for our case without another global/base reservation mechanism. Some mbuf pools are created by drivers or libraries rather than directly by the application; the CNXK inline IPsec/OOP meta pool is one example (NIX_INL_META_POOL, created through cnxk_nix_inl_meta_pool_cb()). To make private data work there, we had to add an EAL argument that reserved a base private size for all pktmbufs, including PMD-created pools. Private data also lacks a central layout registry. If multiple modules use private data, they must coordinate offsets out of band to avoid overlaying each other. The dynamic-field registry solves that coordination problem, but the existing copied dynamic-field area is too small and has copy/clone semantics that are wrong for this metadata. That is why this version uses a globally configured per-mbuf metadata area managed by the dynamic-field registry, with explicit metadata fields that are not copied by generic copy/clone/attach paths. This direction came out of the prior discussion with you, Morten, and me: avoid a Cisco-private mbuf struct patch, avoid per-pool private-data layout coordination, and keep sizeof(struct rte_mbuf) fixed. On a reviewer level, you need to break this up into: - core mbuf changes - per-driver patch to use those core mbuf changes - example of usage - documentation - any review safe guards that future changes don't break this. #RT: I will split the next revision into a series instead of carrying this as one patch. The exact split may depend on the helper model we settle on, but the intent will be the same: introduce any mbuf layout helper first with no behavior change, then convert core/libs/apps/examples, then convert drivers by family so maintainers can review/ack their areas, and finally add the EAL metadata option, registry flag, validation/rejection logic, tests, and documentation. CNXK will be its own driver patch because it needs range checks and fast-path caching rather than a purely mechanical conversion. The more detailed AI review focused on problems as well... [PATCH v4 1/1] mbuf: add runtime metadata dynamic-field storage Does not apply to current main: rte_mbuf_from_indirect() and rte_mbuf_to_priv() now have RTE_ASSERT lines. Needs rebase. Fixed up by hand for testing; builds with -Dwerror=true for the cnxk, bnxt, nfp, softnic and mempool drivers. mbuf_autotest passes with metadata size 0, 64, 256 and 65472. #RT: Agreed. I will rebase v5 on current main and preserve the new RTE_ASSERT checks in rte_mbuf_from_indirect() and rte_mbuf_to_priv(). Thanks for fixing it up locally and for the additional build/test coverage. I will also include comparable validation results in the next revision, including normal metadata sizes and rejected/limit cases rather than relying on very large values such as 65472 if we cap the option more tightly. Error lib/mbuf/rte_mbuf_core.h: RTE_MARKER8 is not defined when building with MSVC (rte_common.h wraps the marker typedefs in #ifndef RTE_TOOLCHAIN_MSVC). The markers were removed from struct rte_mbuf for exactly this reason; this re-adds one and breaks the Windows MSVC build. The marker is also unnecessary: the metadata area starts at sizeof(struct rte_mbuf), so use that instead of offsetof(struct rte_mbuf, metadata) and drop the member. #RT: I will fix this! lib/eal/common/eal_common_config.c: rte_mbuf_metadata_size is exported with RTE_EXPORT_SYMBOL and lands in DPDK_27 (stable). New API must be experimental. Same for rte_mbuf_metadata_size_get(), rte_mbuf_size() and RTE_MBUF_DYNFIELD_F_METADATA. Exporting a bare variable as stable ABI is worse than a function; it freezes the type and the symbol location. #RT: I agree with the concern about exporting rte_mbuf_metadata_size as a writable global. That is the wrong ABI shape and I will remove it. I want to pause before spinning v5, though, because this exposes a design choice around the existing mbuf helpers. The current patch makes helpers such as rte_mbuf_to_priv(), rte_mbuf_buf_addr(), rte_mbuf_from_indirect(), and rte_pktmbuf_detach() metadata-aware. That gives the least ambiguous semantics — private data and buffer data remain after the full mbuf object — but those helpers are public inline functions, so they need some runtime-visible mbuf object size. If that value is hidden entirely inside EAL/mbuf C files, the inline helpers cannot access it without becoming out-of-line calls. The alternative is an explicit opt-in model: leave existing stable helpers with their current fixed sizeof(struct rte_mbuf) behavior, use internal cached layout values in PMDs/libraries that support metadata, and expose separate metadata-aware helpers for applications that do raw mbuf layout math. For example, rte_mbuf_to_priv() would continue to return (char *)m + sizeof(struct rte_mbuf), while a new metadata-aware helper would return (char *)m + sizeof(struct rte_mbuf) + metadata_size. PMDs that support metadata would use cached internal equivalents of the latter during setup/fast path preparation, without requiring the application to compile with experimental APIs. That avoids silently changing existing helper performance/behavior, and the new application-facing helpers could be experimental. The downside is parallel helper semantics and a requirement that metadata-enabled code use the new helpers correctly. For PMDs, I think the right implementation either way is to avoid repeated runtime helper loads in fast paths. The metadata size is system-wide after EAL init, so DPDK can compute an internal mbuf object size once and PMDs can cache any derived queue/device values during setup. CNXK also needs explicit guards: the hardware skip fields cap the supported metadata size and related private- data/headroom combinations. In particular, with the current 128-byte mbuf header, 256 bytes of metadata already reaches the wqe_skip limit, so CNXK support must reject configurations that exceed the hardware-encodable offsets. Before I rework the series, I’d like your guidance on which ABI/API model you think is acceptable upstream: 1. make the existing helpers metadata-aware, accepting a small stable read path for the configured mbuf object size; or 2. keep existing helpers fixed and add explicit metadata-aware APIs for opt-in users, while PMDs/libraries use internal cached layout values. My preference is to remove the writable global, keep PMD fast paths cached, add the necessary driver guards, and avoid requiring applications to enable experimental API merely because a PMD supports metadata. I’m open to either helper model, but I’d rather align on that before producing another revision. drivers/net/cnxk, drivers/event/cnxk: the hardware skip fields are narrow and roc_nix_rq_init() only checks 8-byte alignment, not range: wqe_skip 2 bits, in 128B lines -> rte_mbuf_size() <= 384 later_skip 6 bits, in 8B units -> mbuf + meta + priv <= 504 first_skip 7 bits, in 8B units -> mbuf + meta + priv + headroom <= 1016 With --mbuf-metadata-size=384, wqe_skip becomes 4 and is truncated to 0. With metadata 256 and priv_size 128, later_skip is 512 and is truncated to 0. Hardware then writes the WQE/packet over the mbuf. The driver must reject configurations that do not fit, as was done for octeontx. #RT: Agreed. This is a real correctness issue, not just a local type-width issue. The local roc_nix_rq fields are wider, but the programmed CNXK hardware context fields are limited to wqe_skip:2, later_skip:6, and first_skip:7, so values beyond those limits can truncate. I will add explicit CNXK validation before programming the RQ context and reject unsupported combinations. The checks need to cover both the global metadata size and the per-pool layout: wqe_skip limits sizeof(struct rte_mbuf) + metadata_size to 384 bytes, later_skip limits sizeof(struct rte_mbuf) + metadata_size + priv_size to 504 bytes, and first_skip limits sizeof(struct rte_mbuf) + metadata_size + priv_size + headroom to 1016 bytes. With the current 128-byte mbuf header, --mbuf-metadata-size=256 is already the CNXK metadata ceiling from wqe_skip, while remaining private-data headroom is checked per pool. Warning EAL defines and exports a symbol named rte_mbuf_*, and it is declared extern in both eal_private.h and rte_mbuf_core.h. Follow the existing --mbuf-pool-ops-name pattern: EAL stores the value and provides a getter (rte_eal_mbuf_user_pool_ops() equivalent); mbuf owns any mbuf-named symbols. #RT: Agreed. I will remove the rte_mbuf_* storage/export from EAL. The configured value should be stored and shared by EAL, following the existing --mbuf- pool-ops-name style, while mbuf-owned APIs/helpers remain in the mbuf library. The next revision will separate EAL configuration/storage from mbuf naming/ownership instead of declaring the same external value through both EAL-private and mbuf public headers. Fast path cost. rte_mbuf_size() turns a compile-time constant into a load of a global (through the GOT in shared builds) in rte_mbuf_to_priv(), rte_mbuf_from_indirect(), rte_mbuf_buf_addr() and rte_pktmbuf_detach(), all inline and used per packet. cnxk Rx vector paths now do vdupq_n_u64(rte_mbuf_size()) inside the burst loop, and the cn9k get_work asm needs an extra register load per event. The cnxk paths should cache the value in rxq/ws at setup like data_off. Need l3fwd or testpmd numbers with the option unset, on cnxk at least, before this goes in. lib/eal/common/eal_common_options.c: upper bound of UINT16_MAX is not justified. Beyond the cnxk limits above, examples/fips_validation DEF_MBUF_SEG_SIZE is UINT16_MAX - rte_mbuf_size() - headroom and wraps for large values. Cap the option at a small number of cache lines and document the limit. #RT: Agreed. UINT16_MAX is not a justified upper bound. I will cap the option to A small documented number of cache lines and add boundary tests. CNXK also needs separate queue/pool validation because its later_skip and first_skip limits include priv_size and headroom; with the current 128-byte mbuf header, its global metadata ceiling is 256 bytes due to wqe_skip. drivers/net/cnxk/cn10k_rx.h nix_cqe_xtract_mseg(): the rx_inj block was reindented one tab too deep; it is still inside the same if. Only the two wqe assignment lines need to change. Too much in one patch. Put the mbuf helpers first, then the conversions, then the feature: 1. mbuf: add rte_mbuf_size() returning sizeof(struct rte_mbuf), no behaviour change 2. convert libs, apps and examples 3. convert drivers (one per driver family, so maintainers can ack) 4. add the EAL option, metadata dynfield flag, driver rejections and docs Patches 1-3 are a no-op and can be reviewed and merged on their own. Patch 4 is then small enough to review for what it actually changes. It also makes it bisectable when a driver conversion is wrong. Many of the conversions open-code rte_mbuf_size() where an existing helper fits: RTE_PTR_ADD(m, rte_mbuf_size()) is rte_mbuf_to_priv(m), and mbuf + rte_mbuf_size() + priv_size is rte_mbuf_buf_addr(). Use the helpers so drivers stop depending on the layout directly. Info Unrelated whitespace churn: blank line removed in drivers/mempool/octeontx/meson.build, in the release notes Known Issues section, and in test_mbuf(). Drop these. lib/mbuf/rte_mbuf_dyn.c: the "check if this offset can be used" comment now sits above dynfield_in_metadata() instead of check_offset(). drivers/mempool/octeontx: the check rejects every fpavf pool, not just pktmbuf pools. mp->private_data_size is known at alloc time, so the check could be limited to pools carrying rte_pktmbuf_pool_private. Not required since net/octeontx is the only user that cares. app/test/test_mbuf.c: with metadata configured, dynfield_fail_big (size 128) passes the size check and fails only because no free space exists, so the size limit path is no longer tested in the metadata run. Any out-of-tree code using (m + 1) or sizeof(struct rte_mbuf) to find private data silently breaks when a user adds this EAL option. That belongs in the API changes section of the release notes, not only under New Features. #RT: I will also clean up the lower-level review items in the next revision: remove unrelated whitespace churn, restore the misplaced check_offset() comment, tighten or document the octeontx rejection as appropriate, keep the mbuf tests covering the intended size-limit path, and move the out-of-tree layout impact note into the API changes release-note section.
