|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

# DPDK Patch Review

commit SHA: 20260414193850.1696-2-vipin.varghese at amd.com (patch 1/3)
commit SHA: 20260414193850.1696-3-vipin.varghese at amd.com (patch 2/3)
commit SHA: 20260414193850.1696-4-vipin.varghese at amd.com (patch 3/3)

Author: Vipin Varghese <vipin.varghese at amd.com>

Subject: [PATCH v5 v5 1/3] eal/topology: add Topology grouping for lcores

This series introduces NUMA topology awareness for DPDK logical cores
with support for domain partitioning based on CPU and I/O topology.

---

## Patch 1/3: eal/topology

> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2026 AMD Corporation
> + */

Does this file need the Intel copyright from the header template comment
indicating it was based on rte_lcore.h?

> +#include <inttypes.h>
> +#include <stdlib.h>
> +#include <string.h>
> +
> +#include <rte_topology.h>
> +#include <rte_malloc.h>

Whitespace: Are these includes meant to have a blank line between system
and DPDK headers?

> +static inline bool is_valid_single_domain(unsigned int domainbits)
> +{
> +	if ((domainbits == 0) || (domainbits & ~RTE_TOPO_DOMAIN_ALL))
> +		return false;
> +
> +	return (__builtin_popcount(domainbits) == 1);
> +}

Coding style: Are the extra parentheses around the conditions needed?
The coding standard discourages unnecessary parentheses in conditionals.

> +static unsigned int
> +get_domain_count(unsigned int domain_sel)
> +{
> +	if (is_valid_single_domain(domain_sel) == false)
> +		return 0;

Coding style: Should this use "!is_valid_single_domain()" rather than
comparing to false explicitly?

> +	unsigned int domain_cnt =
> +		(domain_sel & RTE_TOPO_DOMAIN_NUMA) ? topo_cnfg.numa_count :
> +		(domain_sel & RTE_TOPO_DOMAIN_L4) ? topo_cnfg.l4_count :
> +		(domain_sel & RTE_TOPO_DOMAIN_L3) ? topo_cnfg.l3_count :
> +		(domain_sel & RTE_TOPO_DOMAIN_L2) ? topo_cnfg.l2_count :
> +		(domain_sel & RTE_TOPO_DOMAIN_L1) ? topo_cnfg.l1_count : 0;

Does this chained ternary need more clarity? Would a switch statement or
helper function improve maintainability?

> +	return ptr->cores[pos];
> +}
> +#endif

Can the #ifdef blocks cause issues if RTE_LIBHWLOC_PROBE is undefined?
Are all the functions properly stubbed?

> +	RTE_SET_USED(domain_sel);
> +	RTE_SET_USED(lcore);
> +	return -3;

Does returning -3 here have a specific meaning that should be documented?

> +	return ((domain_sel & RTE_TOPO_DOMAIN_NUMA) ? topo_cnfg.lcore_map[lcore].numa_domain :
> +		(domain_sel & RTE_TOPO_DOMAIN_L4) ? topo_cnfg.lcore_map[lcore].l4_domain :
> +		(domain_sel & RTE_TOPO_DOMAIN_L3) ? topo_cnfg.lcore_map[lcore].l3_domain :
> +		(domain_sel & RTE_TOPO_DOMAIN_L2) ? topo_cnfg.lcore_map[lcore].l2_domain :
> +		(domain_sel & RTE_TOPO_DOMAIN_L1) ? topo_cnfg.lcore_map[lcore].l1_domain : -3);

Long line: Does this line exceed 79 characters? Should it be wrapped?

> +RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_topo_get_lcore_count_from_domain, 26.07)
> +unsigned int
> +rte_topo_get_lcore_count_from_domain(unsigned int domain_sel __rte_unused,
> +unsigned int domain_indx __rte_unused)

Whitespace: Is the indentation of the second parameter line correct?

> +			for (unsigned int i = 0; i < ptr->core_count; i++) {
> +				uint16_t lcore = ptr->cores[i];
> +
> +				EAL_LOG(DEBUG, "Found lcore (%u) in domain (%d) at pos %u",
> +					lcore, domain_index, i);

Format string: Should domain_index be %u since it's unsigned int, not %d?

> +				return lcore;
> +			}
> +		}
> +
> +		return RTE_MAX_LCORE;

Does this code path always ensure a valid return or could undefined
behavior occur?

> +	while (1) {

Coding style: Should this be "for (;;)" per DPDK conventions?

> +		/* check if the domain has cores_to_skip */
> +		uint16_t new_lcore = ptr->cores[new_lcore_pos];

Comment mismatch: The comment mentions cores_to_skip but the code
doesn't check for that. Is this comment accurate?

> +	fprintf(f, "| %15s | %15s | %15s | %15s |\n",
> +		"Domain-Name", "Domains", "Domains-with-lcore", "Domain-total-lcore");

Long line: Does this fprintf statement exceed the line length limit?

> +			(domain_types[d] == RTE_TOPO_DOMAIN_NUMA) ? "NUMA" :
> +			(domain_types[d] == RTE_TOPO_DOMAIN_L4) ? "L4" :
> +			(domain_types[d] == RTE_TOPO_DOMAIN_L3) ? "L3" :
> +			(domain_types[d] == RTE_TOPO_DOMAIN_L2) ? "L2" :
> +			(domain_types[d] == RTE_TOPO_DOMAIN_L1) ? "L1" : NULL,

Does this pattern repeat in multiple places? Would a helper function
reduce duplication?

> +	for (uint16_t j = 0; j < *layer_cnt; j++) {

Variable naming: Is the use of single-letter variables like 'j' clear
enough, or should more descriptive names be used?

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

Resource leak: If this allocation fails, are the previously allocated
dm entries from earlier loop iterations freed?

> +		dm->cores = rte_malloc(NULL, sizeof(uint16_t) * cpu_count, 0);
> +		if (!dm->cores)
> +			return -1;

Resource leak: If dm->cores allocation fails, is the dm struct freed?

> +				int cpu_core = lcore_to_core(cpu_id);
> +				if (cpu_core == -1)
> +					return -1;

Error handling: Does returning -1 here leak the allocated topology
structures?

> +					struct core_domain_mapping *dm = (*layer_ptr)[k];
> +					if (dm == NULL)
> +						return -2;

Return code: Is the distinction between -1 and -2 documented anywhere?

> +	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;

Error path: Should this call rte_eal_topology_release() to ensure
complete cleanup?

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

Magic number: Should 5 be replaced with RTE_DIM(layers) or similar?

> +		*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) {

Long line: Does this exceed 79 characters?

> +	hwloc_topology_destroy(topo_cnfg.topology);
> +	topo_cnfg.topology = NULL;

Memory ordering: Is setting to NULL after destroy necessary? Does the
destroy function leave a dangling pointer?

> +static void
> +release_domain(struct domain_store *d)
> +{
> +	if (!d->map) {
> +		d->count = 0;
> +		d->core_count = 0;
> +		return;
> +	}

Does this function need to handle partially initialized domains?

> +	for (int i = 0; i < d->count; i++) {
> +		if (!d->map[i])
> +			continue;
> +		rte_free(d->map[i]->cores);
> +		d->map[i]->cores = NULL;
> +		rte_free(d->map[i]);
> +		d->map[i] = NULL;
> +	}

Double free: Can d->map[i]->cores be NULL when d->map[i] is non-NULL?
Does the code handle that case?

---

## Patch 2/3: app test cases

> +#ifdef RTE_LIBHWLOC_PROBE
> +#include <rte_topology.h>
> +#endif

Whitespace: Should there be a blank line before the system includes and
this section?

> +	unsigned int id1 = RTE_MAX_LCORE, id2 = RTE_MAX_LCORE;
> +	unsigned int domain = 0;

Initialization: Is initializing domain to 0 necessary since it's
immediately reassigned in the loop?

> +		id1 = rte_topo_get_nth_lcore_from_domain(domain, 0, 0, RTE_TOPO_DOMAIN_L1);
> +		id2 = rte_topo_get_nth_lcore_from_domain(domain, 0, 0, RTE_TOPO_DOMAIN_L1);

Copy-paste error: Are both calls using position 0 intentional? Should
the second use position 1?

> +static int
> +get_same_l2_domains(struct lcore_pair *lcp)
> +{	if (rte_topo_get_domain_count(RTE_TOPO_DOMAIN_L2) == 0)

Whitespace: Should there be a newline after the opening brace?

> +	if (rte_lcore_count() > 2) {

Comment: Would it be helpful to document why more than 2 cores are
needed for these tests?

---

## Patch 3/3: Documentation

> +  [topology](@ref rte_topology.h),

Alphabetical order: Is the list of headers alphabetically sorted? Should
topology come after lcore?

> +    topology_lib
> +
> +
> +-How to Guides
> +-------------

Whitespace: Should there be only one blank line before the section?

> +- ``hwloc-dev`` tested on `2.10.0`

Version requirement: Should there be a minimum version documented if
2.10.0 is only tested, not required?

> +.. code-block:: c
> +
> +   uint32_t
> +   rte_topo_get_domain_count(enum rte_topo_domain_sel domain_sel);

Return type: Does the actual API return unsigned int but the doc shows
uint32_t?