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.
