|WARNING| [v2] eal: fix core_index for non-EAL registered threads

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/165612

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-06-17

# DPDK Patch Review

## Summary
This patch fixes a correctness bug where non-EAL threads registered via `rte_thread_register()` have their `core_index` left at -1, causing libraries that use `rte_lcore_index()` for per-lcore caching to fall back to a shared path with severe contention.

## Errors

### 1. Bitset declared as first member of `struct rte_config` causes ABI break
**Location:** `lib/eal/common/eal_private.h:48-49`

```c
struct rte_config {
	RTE_BITSET_DECLARE(core_indices,
			   RTE_MAX_LCORE); /**< bitset of currently allocated core_indices */
	uint32_t main_lcore;
```

Adding a new member at the **beginning** of `struct rte_config` shifts the offset of all existing fields (`main_lcore`, `lcore_count`, `numa_node_count`, etc.), breaking the ABI. Any code compiled against the old layout will access wrong offsets.

**Fix:** Add the new field at the **end** of the structure to preserve existing member offsets:

```c
struct rte_config {
	uint32_t main_lcore;         /**< Id of the main lcore */
	uint32_t lcore_count;        /**< Number of available logical cores. */
	uint32_t numa_node_count;    /**< Number of detected NUMA nodes. */
	/* ... existing fields ... */
	RTE_BITSET_DECLARE(core_indices,
			   RTE_MAX_LCORE); /**< bitset of currently allocated core_indices */
};
```

### 2. Resource leak on early error return in `eal_lcore_non_eal_allocate`
**Location:** `lib/eal/common/eal_common_lcore.c:383-387`

```c
	core_index = rte_bitset_find_first_clear(cfg->core_indices, RTE_MAX_LCORE);
	if (core_index == -1) {
		EAL_LOG(DEBUG, "No core_index available.");
		lcore_id = RTE_MAX_LCORE;
		goto out;
	}
```

The `rte_rwlock_write_lock(&lcore_lock)` at line 382 is never released if this error path is taken. The lock remains held, causing deadlock on the next call to any function that acquires `lcore_lock`.

**Fix:** Release the lock before returning:

```c
	core_index = rte_bitset_find_first_clear(cfg->core_indices, RTE_MAX_LCORE);
	if (core_index == -1) {
		EAL_LOG(DEBUG, "No core_index available.");
		lcore_id = RTE_MAX_LCORE;
		rte_rwlock_write_unlock(&lcore_lock);
		goto out;
	}
```

Alternatively, move the `rte_bitset_find_first_clear()` call before acquiring the lock if it does not require lock protection.

## Warnings

### 1. Missing explicit comparison in conditional
**Location:** `lib/eal/common/eal_common_lcore.c:383`

```c
	if (core_index == -1) {
```

This is correct per DPDK style (explicit comparison against -1). No issue.

### 2. Bitset initialization on first discovery
**Location:** `lib/eal/common/eal_common_lcore.c:188`

```c
	/* This is the first time we discover the lcores, so the bitset should be zeroed */
	rte_bitset_set(config->core_indices, count);
```

The comment states the bitset "should be zeroed," implying an assumption that `rte_config` is zero-initialized before `rte_eal_cpu_init()` runs. Verify that `rte_config` (whether static or dynamically allocated) is guaranteed to be zeroed at the point this function is called. If not, add an explicit `rte_bitset_clear_all()` before the loop.

### 3. Release notes missing
The patch fixes a significant performance regression (5x throughput drop) in a scenario where non-EAL threads use per-lcore caching. This is a **bug fix** that restores expected performance and should be documented in the release notes under the "Fixed Issues" section for the EAL subsystem.

**Suggested entry:**
```
* **eal: Fixed per-lcore cache contention for registered non-EAL threads**

  Non-EAL threads registered via ``rte_thread_register()`` now receive a valid
  ``core_index``, allowing libraries that use ``rte_lcore_index()`` for per-lcore
  caching (e.g., mlx5 indexed memory pool) to avoid fallback to a shared global
  path. This eliminates severe spinlock contention that caused up to 5x throughput
  regression under concurrent access from multiple registered threads.
```

### 4. Functional test or app/testpmd hook recommended
While not strictly required for an internal bug fix, consider adding a test case in `app/test` that:
- Registers multiple non-EAL threads via `rte_thread_register()`
- Verifies each gets a unique `rte_lcore_index()` >= 0
- Verifies indices are reused after `rte_thread_unregister()`

This would prevent regression and validate the fix.

## Info

### Code style observations (no action required)

1. Include order change in `eal_common_lcore.c` (adding `rte_bitset.h` before `rte_branch_prediction.h`) is acceptable per DPDK style (alphabetical within the DPDK misc library group).

2. The logic correctly handles:
   - Initial EAL lcore discovery setting the bitset
   - Lcore remapping on `-l`/`-c` options clearing and repopulating the bitset
   - Non-EAL allocation setting the bit and rollback clearing it on callback failure
   - Non-EAL release clearing the bit

3. Bitset size `RTE_MAX_LCORE` is appropriate -- matches the maximum possible `core_index` values.

---

## Summary of Required Changes

**Must fix (Errors):**
1. Move `core_indices` bitset to the end of `struct rte_config` to avoid ABI break
2. Release `lcore_lock` on the error path where no `core_index` is available

**Should fix (Warnings):**
3. Add release notes entry documenting the performance fix
4. Verify or ensure `rte_config` is zero-initialized before `rte_eal_cpu_init()`, or add explicit `rte_bitset_clear_all()` at function entry

**Consider (Info):**
5. Add a functional test for non-EAL thread `core_index` allocation/reuse