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