Re: [PATCH 00/39] Rework EAL configuration

[Bruce Richardson <bruce.richardson@intel.com>]

On Mon, Jul 27, 2026 at 04:30:07PM -0700, Stephen Hemminger wrote:
> On Tue, 21 Jul 2026 10:45:08 +0100
> Bruce Richardson <bruce.richardson@intel.com> wrote:
> 
> > This patchset reworks how configuration is stored and managed in EAL.
> > The existing "internal_config", "rte_config", "lcore_config" structures,
> > which sometimes have arbitrary separation between them (especially the
> > first two) are replaced by three new structures with clearly defined
> > roles:
> > 
> > - eal_platform_info - contains the raw HW info for the system, details
> >   of CPUs and hugepage mounts. This is initialized on first use - even
> >   before EAL init is called - and is then immutable, since our HW should
> >   not change much underneath us. Its early availability means that it
> >   can be used to sanity check the contents of the other structs as they
> >   are being built up.
> > 
> > - eal_user_cfg - contains the config settings passed in by the user. For
> >   existing rte_eal_init, this is built up in the arg parse stage, and
> >   it's contents verified against the platform info, e.g. to check core
> >   masks are valid etc. Once argument parsing is completed, is also
> >   immutable.
> > 
> > - runtime_cfg - basically all the runtime settings that need to be there
> >   for DPDK to run, or which change over time. Largely combined content
> >   of the old rte_config, internal_config and lcore_config structs. This
> >   is initialized from the other two structs by eal initialization and
> >   can be modified by EAL at any time.
> > 
> > Once that is done, we have a clean separation between user provided
> > configuration and the rest of EAL, we can split EAL init into two parts,
> > the first of which parses cmdline arguments and then calls the second
> > which takes the eal_user_cfg struct result of that parse and does the
> > actual initialization. The longer-term objective is to have other
> > first-stage functions that prepare the user_cfg struct for
> > initialization, so that we can move away from argc/argv as the only
> > method of configuring DPDK initialization.

<snip>

> > --
> > 2.53.0
> > 
> 
> Ran deeper AI review on this and it found a couple small things:
> 
> Review of "eal: rework EAL initialization" (39 patches)
> 
> Series applies cleanly to main (9231dc7). All 39 commits build individually
> with -Dwerror=true, so bisect is safe. Findings verified against the merged
> tree rather than the diffs alone.
> 
> 
> Patch 02/39 - argparse: check for range overflow in CPU lists
> 
> Warning: missing Fixes: and Cc: stable.
> 
> This is an out-of-bounds write, not a cleanup - CPU_SET(min, cpuset) with
> min >= CPU_SETSIZE writes past the end of rte_cpuset_t. The introducing
> commit is in v25.11, so it needs backporting:
> 
>   Fixes: d78103fb9488 ("argparse: support core lists")
>   Cc: stable@dpdk.org
> 
> The fix itself is correct and complete: all three branches assign max before
> the new check, min >= 0 is guaranteed by the isdigit() gate, and min <= max
> holds in every branch, so the CPU_SET loop is fully bounded.
> 

Added in v2

> 
> Patch 33/39 - eal: remove internal config reset function
> 
> Error: removing eal_reset_internal_config() drops the lock_descriptor = -1
> preset, which can lead to close(0) on stdin. Linux only.
> 
> The deleted function did:
> 
>     for (i = 0; i < MAX_HUGEPAGE_SIZES; i++) {
>         memset(&internal_cfg->hugepage_info[i], 0, ...);
>         internal_cfg->hugepage_info[i].lock_descriptor = -1;
>     }
> 
> That -1 was the invariant eal_hugedirs_unlock() relied on. hugepage_info[]
> now lives in eal_runtime_state, which is a static initialised only with
> .mem_config, so every lock_descriptor starts at 0 - a valid fd.
> 
> This patch compensates by widening the guard:
> 
>     if (hugepage_info[i].hugepage_sz == 0 ||
>             hugepage_info[i].lock_descriptor < 0)
>         continue;
> 
> but hugepage_sz == 0 is not a sufficient proxy. In hugepage_info_init()
> (lib/eal/linux/eal_hugepage_info.c) hpi->hugepage_sz is assigned *before*
> the mountpoint check, and the no-mountpoint path continues without ever
> assigning lock_descriptor:
> 
>     hpi = &rs->hugepage_info[num_sizes];
>     hpi->hugepage_sz = hps->size;          /* set first */
> 
>     if (get_hugepage_dir(...) < 0) {
>         if (user_cfg->in_memory) {
>             calc_num_pages(hpi, hps, 0);
>             num_sizes++;                   /* entry accepted */
>         }
>         continue;                          /* lock_descriptor never set */
>     }
> 
>     hpi->lock_descriptor = open(hpi->hugedir, O_RDONLY);
> 
> Two reachable cases:
> 
>   (a) --in-memory with a reserved-but-unmounted size (e.g. 1G reserved,
>       only 2M mounted). The entry is accepted with hugepage_sz != 0 and
>       lock_descriptor == 0.
> 
>   (b) Default mode where the *last* size has no mountpoint. num_sizes is
>       not incremented, so slot [num_sizes] keeps a nonzero hugepage_sz with
>       lock_descriptor == 0. eal_hugedirs_unlock() iterates to
>       MAX_HUGEPAGE_SIZES, not num_hugepage_sizes, so it still visits it.
> 
> In both, the guard passes and the code runs flock(0, LOCK_UN) followed by
> close(0) on the normal init path (lib/eal/linux/eal.c:831, unconditional
> after rte_eal_memory_init()). Confirmed with a standalone harness
> reproducing the two functions' control flow.
> 
> FreeBSD is unaffected (single entry, fd assigned unconditionally, no
> unlock loop); Windows sets -1 explicitly in eal_hugepages.c.
> 
> Simplest fix is to restore the invariant rather than widen the guard - set
> lock_descriptor = -1 for all MAX_HUGEPAGE_SIZES entries when runtime state
> is set up, or initialise the entry immediately after hugepage_sz is
> assigned in hugepage_info_init(). Bounding the unlock loop by
> num_hugepage_sizes would fix (b) but not (a).
> 
> The other non-zero defaults from the deleted function are all preserved
> correctly: hugepage_file.unlink_existing, no_hpet, and
> max_simd_bitwidth.bitwidth are in EAL_USER_CFG_INITIALIZER, and
> RTE_IOVA_DC / RTE_INTR_MODE_NONE / RTE_PROC_PRIMARY are all genuinely 0.
> lock_descriptor is the only one lost.
> 

Yes, this is a valid issue. Reworked a couple of patches to fix it for v2.

> 
> Patch 29/39 - eal: move trace config into user config struct
> 
> Warning: --trace-dir accumulate semantics changed, plus a leak on repeat.
> 
> The old path went through trace_dir_update(), which concatenated onto any
> existing value:
> 
>     asprintf(&dir, "%s%s", trace->dir != NULL ? trace->dir : "", str);
> 
> The new code does a plain asprintf into user_cfg->trace_dir. Passing
> --trace-dir more than once now replaces rather than appends, and the
> earlier allocation leaks since trace_dir is overwritten without a free.
> If the replace behaviour is intended, worth saying so in the commit
> message; otherwise free the previous value first.
> 

This I believe to be a false positive. The trace-dir EAL flag can only be
specified once on the command line, enforced by the argparse library, so
the fact that the later functions don't handle multiple values is not a
problem. The concatenation here is actually for appending a filename to an
existing trace dir.

> 
> Patch 39/39 - eal: provide hooks for init with externally supplied config
> 
> Error: rte_eal_runtime_init() returns -1 without setting rte_errno on the
> platform-info path. Identical in all three platform copies:
> 
>     if (rte_eal_get_platform_info() == NULL) {
>         rte_eal_init_alert("Platform information is not available.");
>         return -1;              /* rte_errno not set */
>     }
> 
> The other two error paths in the same function set EINVAL and EALREADY,
> and the equivalent path in rte_eal_init() sets ENOTSUP. A caller checking
> rte_errno gets a stale value. Suggest rte_errno = ENOTSUP to match.
> 

Fixed in v2. Explicitly set rte_errno = 0 at the start of function and set
it explicitly only when it's not already set by a subfunction of
get_platform_info.

> Warning: the stated purpose is not reachable as posted.
> 
> The commit message says the hooks let "other libraries init EAL by passing
> in that structure pre-configured", but struct eal_user_cfg and both new
> prototypes live in lib/eal/common/eal_internal_cfg.h. lib/meson.build:143
> only adds eal/common to the include path when RTE_LIB_EAL is not yet set,
> i.e. for EAL's own sub-build; afterwards dependent libraries get
> deps += ['eal'], which exposes only EAL's public include dirs. No in-tree
> library can declare the type or call the function without the explicit
> include_directories() hack used by drivers/common/mlx5/linux/meson.build.
> Either make the header reachable or note that a follow-up is required.
> 

Expected. In RFC I included an example of use, but dropped from this v1
series as it's already long enough.

> Warning: both new __rte_internal symbols have no in-tree consumer and no
> test, so the deep-copy path in eal_user_cfg_copy() is never exercised by
> anything. A test driving rte_eal_runtime_init() with a hand-populated
> config would be worth adding alongside.
> 

As above, will hopefully be added later if this makes it in.

> Info: eal_internal_cfg.h uses #include "rte_compat.h" while every other
> public RTE header in the same file uses angle brackets.
> 
> 
> Checked and found correct
>
<snip>