Hi Stephen, Thanks for the detailed review. I agree v2 needs a design rework rather than just patching the reported failures.
For the octeontx issue, I’ll handle this in v3 by disabling mempool/octeontx at Meson configure time when mbuf_dynfield3_size != 0, with a clear reason that it requires sizeof(struct rte_mbuf) <= 128. I don’t plan to change OCTEONTX_FPAVF_BUF_OFFSET in this patch since that looks like hardware- programmed layout behavior and should be owned/validated by the Marvell maintainers. For CN20K, that specific size-truncation problem has already been fixed upstream by 570c0b273cde (“drivers: fix CN20K mbuf size truncation"), so I’m using that as confirmation that the current tree no longer has the same CN20K blocker. For the copy semantics, agreed. The build-wide mbuf_dynfield3_copy option is the wrong abstraction. In v3 I’m dropping it and preserving the existing default dynamic-field behavior: fields registered with flags = 0 are copied by generic mbuf copy/clone paths. For deployments that need non-copy metadata, I’m adding an explicit per-field flag, RTE_MBUF_DYNFIELD_F_NO_COPY. Fields using that flag are restricted to the optional dynfield3 area, so the no-copy behavior is explicit and cannot accidentally affect existing dynamic-field users. That also fixes the dynfield1/dynfield3 straddling problem. Normal fields keep copy semantics, and no-copy fields are constrained to dynfield3. The copy path tracks the registered copied portions of dynfield3 and only copies those bytes, instead of relying on a global copy switch. For the allocator score overflow, I’m widening free_space[] from uint8_t to uint16_t. For the mbuf autotest issue, I’ll change the “too big” negative case to use sizeof(struct rte_mbuf) rather than a fixed 256-byte field, and add dynfield3- specific coverage for placement and copy/no-copy behavior. I’ll also add a devtools/test-meson-builds.sh build with -Dmbuf_dynfield3_size=256, update the programmer guide and release notes, drop the unused dynfield3 count/offset macros, and remove the redundant sizeof(uint64_t) Meson check. Thanks again — the v3 will be a more general-purpose API with default copy semantics preserved, and Cisco’s no-copy use case handled explicitly through the new field flag. -rt From: Stephen Hemminger <[email protected]> Date: Saturday, September 26, 2026 at 12:51 PM To: Randy Tice (rtice) <[email protected]> Cc: [email protected] <[email protected]>; Morten Brørup <[email protected]>; Bruce Richardson <[email protected]> Subject: Re: [PATCH v2 1/1] mbuf: add optional dynfield3 storage On Fri, 25 Sep 2026 15:31:08 -0400 Randy L Tice <[email protected]> wrote: > From: Randy L Tice <[email protected]> > Date: Thu, 03 Sep 2026 09:13:28 -0400 > > Add build-time support for optional cache-line-aligned dynamic-field > storage at the end of struct rte_mbuf. > > The mbuf_dynfield3_size Meson option sets RTE_MBUF_DYNFIELD3_SIZE > in rte_build_config.h. A non-zero value enables the extra area. The > storage is represented as uint64_t elements for 32-bit and 64-bit > build consistency. > > When enabled, dynfield3 is made available to the mbuf dynamic field > allocator. The mbuf_dynfield3_copy option controls whether the area is > copied by the generic mbuf dynamic-field copy helper, and defaults to > false. > > Validate that the configured size is non-negative, is a multiple of > sizeof(uint64_t), and reserves a multiple of the cache line size. > > Signed-off-by: Randy L Tice <[email protected]> > --- Ran this through AI review with full model Subject: Re: [PATCH v2 1/1] mbuf: add optional dynfield3 storage Applied to main (4f795dd), built and tested on x86_64 with -Dmbuf_dynfield3_size=256 and 512. Errors ------ 1. Enabling the option breaks the build. drivers/mempool/octeontx is built on all 64-bit Linux targets, including x86_64, and has: RTE_BUILD_BUG_ON(sizeof(struct rte_mbuf) > OCTEONTX_FPAVF_BUF_OFFSET); with OCTEONTX_FPAVF_BUF_OFFSET fixed at 128. Any non-zero mbuf_dynfield3_size fails to compile with the default driver set. The Known Issues entry is not a substitute. Drivers that depend on a 128 byte mbuf must be disabled at configure time when the option is set, the same way require_iova_in_mbuf handles enable_iova_as_pa=false. Please audit drivers that program sizeof(struct rte_mbuf) or a fixed offset into hardware (octeontx FPA buf_offset, cnxk first_skip) and state the result in the commit message. 2. Fields can straddle dynfield1 and dynfield3; clone copies half. On 64-bit targets dynfield1 ends at offset 128 and dynfield3 starts at 128 (for both 64 and 128 byte cache lines), so init_shared_mem() creates one contiguous free run. The best-fit allocator places a field across the boundary, and with mbuf_dynfield3_copy=false (the default) rte_mbuf_dynfield_copy() copies only the dynfield1 part. Reproduced with size=512: register three 8 byte fields (96, 104, 112), then a 16 byte align 8 field. It lands at 120..135. Set it to 0xab and rte_pktmbuf_clone(): abababababababab0000000000000000 The second half is whatever the clone mbuf held before, not zero. Even without straddling, whether a field is copied now depends on registration order and on what other libraries and PMDs registered first. The field owner cannot know or control this, and every existing user of rte_mbuf_dynfield_register() assumes copy on clone. 3. free_space[] score overflows for sizes >= 384. struct mbuf_dyn_shm keeps the score in uint8_t free_space[]. process_score() computes align = 256 for a free run of 256 bytes or more at a 256 byte aligned offset; the store truncates it to 0, which means occupied, permanently. With size=512 (dynfield3 at 128..639), bytes 256..511 are never allocatable: a 256 byte field fails with ENOENT, and only 288 bytes of 8 byte fields can be registered in total. Widen free_space[] or cap the option. Meson integer options take min/max, which also replaces the explicit < 0 check: option('mbuf_dynfield3_size', type: 'integer', min: 0, max: 256, value: 0, description: ...) Warnings -------- 4. mbuf_autotest fails with size >= 256. test_mbuf_dyn() expects dynfield_fail_big (size 256, align 1) to be rejected. With size=256 it registers at offset 92, spanning 92..347 across both areas (see 2). The "too big" case should use sizeof(struct rte_mbuf). 5. Copy policy is at the wrong level. One build-time switch for the whole area cannot be right for every field placed there. struct rte_mbuf_dynfield has a flags member, reserved and required to be 0 today. Keep dynfield3 out of the default allocator and hand it out only to callers that ask for it with a new flag. That makes the no-copy semantics explicit and per field, fixes 2, and removes mbuf_dynfield3_copy. 6. No test or CI coverage. The option defaults to 0, so CI compiles none of the new code. Add a devtools/test-meson-builds.sh build with the option set (it would have caught 1), and a test_mbuf.c case that checks placement and clone behaviour of a field in dynfield3. 7. Missing documentation and rationale. doc/guides/prog_guide/mbuf_lib.rst describes dynamic fields and needs to cover the new area, its copy semantics, and that the mbuf layout now depends on a build option: applications and secondary processes must be built with the same value. The commit message does not say why the existing per-mbuf private area (priv_size in rte_pktmbuf_pool_create()) is not sufficient. The cover letter does not go into git; the rationale belongs in the commit message, along with the cost: every mbuf grows by at least one cache line. Info ---- 8. RTE_MBUF_DYNFIELD3_CNT and RTE_MBUF_DYNFIELD3_OFFSET are not needed, and defining the offset as 0 when disabled is misleading (offset 0 is buf_addr). Use the member directly, like dynfield1: memcpy(mdst->dynfield3, msrc->dynfield3, sizeof(mdst->dynfield3)); and drop the <stddef.h> include. 9. The sizeof(uint64_t) check in config/meson.build is redundant; a multiple of RTE_CACHE_LINE_SIZE is always a multiple of 8.
