|WARNING| eal: fix multi-process disabling

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-11-03

# DPDK Patch Review

## ERRORS

**Error 1: Race condition in `eal_mp_is_enabled()`**

In `lib/eal/common/eal_common_proc.c` line 1424:
```c
return rte_atomic_load_explicit(&mcfg->mp_status,
		rte_memory_order_relaxed) != MP_STATUS_DISABLED;
```

The `relaxed` memory order is insufficient here. The function is called by `rte_dev_probe()` and `rte_dev_remove()` which make decisions about whether to perform IPC operations based on the MP status. When `rte_mp_disable()` is called, it updates `mp_status` with `release` ordering, but this read uses `relaxed`, which provides no synchronization. A thread could read stale MP status, proceed to attempt IPC, and race with the disabling thread that is tearing down IPC state.

**Fix:** Use `acquire` ordering to synchronize with the `release` store in `rte_mp_disable()`:
```c
return rte_atomic_load_explicit(&mcfg->mp_status,
		rte_memory_order_acquire) != MP_STATUS_DISABLED;
```

---

**Error 2: Missing error check on `rte_mp_disable()` call**

In `lib/eal/common/eal_common_mcfg.c` line 70:
```c
if (internal_conf->no_shconf)
	rte_mp_disable(); /* multi-process requires shared config */
```

`rte_mp_disable()` returns `bool` indicating success/failure (it can fail if called after `__rte_mp_enable()` has already been called and IPC is in use). The return value is ignored here. If the disable fails, multi-process will remain enabled despite `no_shconf` being set, leading to the exact inconsistency the patch is trying to fix.

**Fix:** Check the return value and handle the failure case (at minimum, log an error; ideally, fail EAL init):
```c
if (internal_conf->no_shconf) {
	if (!rte_mp_disable()) {
		EAL_LOG(ERR, "Failed to disable multi-process for no_shconf mode");
		return -1;
	}
}
```

---

## WARNINGS

**Warning 1: Missing release notes**

This patch fixes bugs in the multi-process disable API (`rte_mp_disable()`) and the `--no-shconf` option interaction with IPC/hotplug. These are user-visible behavior changes: applications using `rte_mp_disable()` and then attempting hotplug will now behave differently, and `--no-shconf` will now correctly prevent IPC where it previously did not. This requires a release notes entry documenting the fix.

**Fix:** Add an entry to `doc/guides/rel_notes/release_XX_YY.rst` under "Fixed Issues" describing the corrected behavior.

---

**Warning 2: Function missing Doxygen documentation**

In `lib/eal/common/eal_private.h` line 745:
```c
/**
 * Check whether multi-process is enabled.
 */
bool eal_mp_is_enabled(void);
```

The Doxygen comment does not document the return value (what `true` and `false` mean). For internal API, this is less critical than for public API, but for consistency with DPDK documentation standards, the return value should be documented.

**Fix:**
```c
/**
 * Check whether multi-process is enabled.
 *
 * @return
 *   true if multi-process is enabled, false otherwise.
 */
bool eal_mp_is_enabled(void);
```

---

## INFO

**Info 1: Pre-existing -- `mp_mutex_action` not released on early return in `rte_mp_action_unregister()`**

In `lib/eal/common/eal_common_proc.c` at line 278 (context, not introduced by patch):
```c
pthread_mutex_lock(&mp_mutex_action);
entry = find_action_entry_by_name(name);
if (entry == NULL) {
	pthread_mutex_unlock(&mp_mutex_action);
	return;
}
```
The mutex is correctly unlocked on the `NULL` path. However, if `find_action_entry_by_name()` were to fail in a way that bypasses the NULL check, there would be a leak. This is a pre-existing pattern; the patch correctly preserves the existing lock discipline but does not introduce new issues here.

---

**Info 2: Consider renaming for clarity**

The function name `eal_mp_is_enabled()` could be confused with checking whether the `__rte_mp_enable()` call succeeded. The function actually checks whether multi-process *support* is enabled (i.e., not disabled via `rte_mp_disable()` or `--no-shconf`). A name like `eal_mp_is_supported()` or `eal_mp_status_check()` might be clearer. This is a minor style suggestion, not a defect.

---

Review-Result: ERROR