|SUCCESS| [v5,v5,3/3] doc: add new section topology

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/163066

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-04-15

# Review of DPDK Topology Library Patch

commit 20260414193850.1696-2-vipin.varghese at amd.com
Author: Vipin Varghese <vipin.varghese at amd.com>
Subject: [PATCH v5 v5 1/3] eal/topology: add Topology grouping for lcores

This patch introduces NUMA topology awareness in relation to DPDK
logical cores, providing APIs to select optimal logical cores based on
CPU and I/O topology domains.

> diff --git a/config/meson.build b/config/meson.build
> index 9ba7b9a338..db2faccdbc 100644
> --- a/config/meson.build
> +++ b/config/meson.build

[ ... ]

> +has_libhwloc = false
> +find_libhwloc = true
> +
> +if meson.is_cross_build() and not meson.get_external_property('hwloc', true)
> +    # don't look for libhwloc if explicitly disabled in cross build
> +    find_libhwloc = false
> +endif
> +
> +if find_libhwloc
> +    hwloc_dep = cc.find_library('hwloc', required: false)
> +    if hwloc_dep.found() and cc.has_header('hwloc.h')
> +        dpdk_conf.set10('RTE_LIBHWLOC_PROBE', true)
> +        has_libhwloc = true
> +        #add_project_link_arguments('-lhwloc', language: 'c')
> +        #dpdk_extra_ldflags += '-lhwloc'

Are these commented lines intended to be removed or are they placeholders
for future use?

> diff --git a/lib/eal/common/eal_private.h b/lib/eal/common/eal_private.h
> index e032dd10c9..904df0d0b7 100644
> --- a/lib/eal/common/eal_private.h
> +++ b/lib/eal/common/eal_private.h

[ ... ]

> +#define RTE_TOPO_MAX_CPU_CORES 2048

Is there a risk of buffer overflow if the system has more than 2048
CPU cores? This appears to be used to size arrays in the topology
configuration structure.

> +struct topology_config {
> +#ifdef RTE_LIBHWLOC_PROBE
> +	hwloc_topology_t topology;
> +#endif
> +
> +	/* domain count */
> +	uint16_t l1_count;

[ ... ]

> +	/* dpdk lcore to cpu core map */
> +	uint16_t lcore_to_cpu_map[RTE_TOPO_MAX_CPU_CORES];
> +
> +	/* two dimensional array for each domain */
> +	struct core_domain_mapping **l1;
> +	struct core_domain_mapping **l2;
> +	struct core_domain_mapping **l3;
> +	struct core_domain_mapping **l4;
> +	struct core_domain_mapping **numa;

Does this code properly handle cleanup of all these dynamically allocated
structures (l1, l2, l3, l4, numa) if initialization fails partway through?

> diff --git a/lib/eal/common/eal_topology.c b/lib/eal/common/eal_topology.c
> new file mode 100644
> index 0000000000..7362d8e723
> --- /dev/null
> +++ b/lib/eal/common/eal_topology.c

[ ... ]

> +static unsigned int
> +get_lcore_count_from_domain_index(unsigned int domain_sel, unsigned int domain_indx)
> +{
> +	if ((is_valid_single_domain(domain_sel) == false) ||

The coding style guide prefers direct boolean comparisons. Should this be
`if (!is_valid_single_domain(domain_sel)` instead?

> +		(domain_indx >= get_domain_count(domain_sel)))
> +		return 0;
> +
> +	struct core_domain_mapping *ptr = get_domain_lcore_mapping(domain_sel, domain_indx);
> +	if (ptr == NULL)

The coding style guide prefers `if (!ptr)` for NULL checks.

> +RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_topo_get_lcore_cpuset_in_domain, 26.07)
> +rte_cpuset_t
> +rte_topo_get_lcore_cpuset_in_domain(unsigned int domain_sel __rte_unused,
> +unsigned int domain_indx __rte_unused)
> +{
> +	rte_cpuset_t ret_cpu_set;
> +	CPU_ZERO(&ret_cpu_set);

Does this function leak the cpuset contents if called without
RTE_LIBHWLOC_PROBE defined? The function always returns an empty cpuset
in that case, which may be correct behavior but should be documented.

> +#ifdef RTE_LIBHWLOC_PROBE
> +	const struct core_domain_mapping *ptr = get_domain_lcore_mapping(domain_sel, domain_indx);
> +
> +	if ((ptr == NULL) || (ptr->core_count == 0))

Prefer `if (!ptr || !ptr->core_count)` per coding style.

> +		return ret_cpu_set;
> +
> +	CPU_OR(&ret_cpu_set, &ret_cpu_set, &ptr->core_set);

Does this code properly validate that ptr->core_set is initialized before
OR-ing it into ret_cpu_set?

> +RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_topo_get_next_lcore, 26.07)
> +unsigned int
> +rte_topo_get_next_lcore(uint16_t lcore __rte_unused,
> +bool skip_main __rte_unused, bool wrap __rte_unused, uint32_t flag __rte_unused)
> +{
> +#ifdef RTE_LIBHWLOC_PROBE
> +	const uint16_t main_lcore = rte_get_main_lcore();
> +	const unsigned int lcore_in_domain = get_domain_lcore_count(flag);
> +	const unsigned int domain_count = get_domain_count(flag);
> +
> +	if ((domain_count == 0) || (lcore_in_domain <= 1))

Style: prefer `if (!domain_count || lcore_in_domain <= 1)`.

> +		return RTE_MAX_LCORE;
> +
> +	if (wrap)
> +		lcore = lcore % RTE_MAX_LCORE;

Can this modulo operation produce incorrect results if lcore is already
less than RTE_MAX_LCORE?

> +	bool wrap_once = false;
> +	unsigned int new_lcore_pos = lcore_pos + 1;
> +
> +	while (1) {

Infinite loop with internal breaks. Does this function guarantee termination
in all cases? Could it spin indefinitely if the data structures are in an
inconsistent state?

> +static int
> +lcore_to_core(unsigned int lcore)
> +{
> +	rte_cpuset_t cpu;
> +	CPU_ZERO(&cpu);
> +
> +	cpu = rte_lcore_cpuset(lcore);
> +
> +	for (int i = 0; i < RTE_TOPO_MAX_CPU_CORES; i++) {
> +		if (CPU_ISSET(i, &cpu))
> +			return i;
> +	}
> +
> +	return -1;
> +}

Does this function handle the case where rte_lcore_cpuset returns an
invalid or empty cpuset?

> +static int
> +eal_topology_map_layer(hwloc_topology_t topology, int depth,
> +uint16_t *layer_cnt, struct core_domain_mapping ***layer_ptr,
> +uint16_t *total_core_cnt, const char *layer_name)
> +{
> +	if (depth == HWLOC_TYPE_DEPTH_UNKNOWN || *layer_cnt == 0)

Style: prefer `if (depth == HWLOC_TYPE_DEPTH_UNKNOWN || !*layer_cnt)`.

> +		return 0;
> +
> +	*layer_ptr = rte_malloc(NULL, sizeof(struct core_domain_mapping *) * (*layer_cnt), 0);
> +	if (*layer_ptr == NULL)

Does this function properly clean up previously allocated resources if this
allocation fails?

> +		return -1;
> +
> +	/* create lcore-domain-mapping */
> +	for (uint16_t j = 0; j < *layer_cnt; j++) {
> +		hwloc_obj_t obj = hwloc_get_obj_by_depth(topology, depth, j);
> +		int cpu_count = hwloc_bitmap_weight(obj->cpuset);
> +		if (cpu_count == -1)

Does this code validate that obj is non-NULL before dereferencing obj->cpuset?

> +			continue;
> +
> +		struct core_domain_mapping *dm =
> +			rte_zmalloc(NULL, sizeof(struct core_domain_mapping), 0);
> +		if (!dm)
> +			return -1;

Does this path leak the previously allocated layer_ptr?

> +		(*layer_ptr)[j] = dm;
> +		CPU_ZERO(&dm->core_set);
> +		dm->core_count = 0;
> +
> +		dm->cores = rte_malloc(NULL, sizeof(uint16_t) * cpu_count, 0);
> +		if (!dm->cores)
> +			return -1;

Does this path leak both layer_ptr and dm?

> +RTE_EXPORT_INTERNAL_SYMBOL(rte_eal_topology_init)
> +int rte_eal_topology_init(void)
> +{
> +#ifdef RTE_LIBHWLOC_PROBE
> +	memset(&topo_cnfg, 0, sizeof(struct topology_config));

Is it necessary to memset the entire structure, or could specific fields
be initialized instead for better performance?

> +	if (hwloc_topology_init(&topo_cnfg.topology) < 0)
> +		return -1;
> +
> +	if (hwloc_topology_load(topo_cnfg.topology) < 0) {
> +		hwloc_topology_destroy(topo_cnfg.topology);
> +		return -2;
> +	}
> +
> +	struct {
> +		int depth;
> +		uint16_t *count;
> +		struct core_domain_mapping ***ptr;
> +		uint16_t *total_cores;
> +		const char *name;
> +	} layers[] = {

This structure is quite large. Would it be more efficient to use a smaller
representation?

> +	for (int i = 0; i < 5; i++) {

Should this magic number 5 be replaced with RTE_DIM(layers)?

> +		*layers[i].count = hwloc_get_nbobjs_by_depth(topo_cnfg.topology, layers[i].depth);
> +		if (eal_topology_map_layer(topo_cnfg.topology, layers[i].depth, layers[i].count,
> +			layers[i].ptr, layers[i].total_cores, layers[i].name) < 0) {
> +			rte_eal_topology_release();
> +			return -1;

Does rte_eal_topology_release properly clean up the hwloc topology handle
that was created earlier?

> diff --git a/lib/eal/freebsd/eal.c b/lib/eal/freebsd/eal.c
> index 60f5e676a8..0d016a379f 100644
> --- a/lib/eal/freebsd/eal.c
> +++ b/lib/eal/freebsd/eal.c

[ ... ]

> +	ret = rte_eal_topology_init();
> +	if (ret) {
> +		rte_eal_init_alert("Cannot invoke topology, skipping topology!!!");

Should this error message use more professional wording without multiple
exclamation marks?

> diff --git a/lib/eal/include/rte_topology.h b/lib/eal/include/rte_topology.h
> new file mode 100644
> index 0000000000..1ecee6b031
> --- /dev/null
> +++ b/lib/eal/include/rte_topology.h

[ ... ]

> +#define RTE_TOPO_FOREACH_LCORE_IN_DOMAIN(lcore, domain_indx, lcore_pos, flag)	\
> +	for (lcore = rte_topo_get_nth_lcore_from_domain(domain_indx, lcore_pos, 0, flag);	\
> +		lcore < RTE_MAX_LCORE;	\
> +		lcore = rte_topo_get_nth_lcore_from_domain(domain_indx, ++lcore_pos, 0, flag))

Does this macro properly handle the case where lcore_pos overflows? The
pre-increment ++lcore_pos could cause issues if not properly bounds-checked
in the called function.

> +#define RTE_TOPO_FOREACH_WORKER_LCORE_IN_DOMAIN(lcore, domain_indx, flag)	\
> +	lcore = rte_topo_get_nth_lcore_from_domain(domain, 0, 0, flag);	\

Does this macro use the correct variable name? It references `domain` but
the parameter is `domain_indx`.

> diff --git a/lib/eal/linux/eal.c b/lib/eal/linux/eal.c
> index d848de03d8..f6a49badf2 100644
> --- a/lib/eal/linux/eal.c
> +++ b/lib/eal/linux/eal.c

[ ... ]

> +	if (rte_eal_topology_init()) {
> +		rte_eal_init_alert("Cannot invoke topology, skipping topologly!!!");

Typo: "topologly" should be "topology". Also, excessive exclamation marks.