|WARNING| kernel/freebsd: add NUMA domain selection for contigmem

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-10-24

# DPDK Patch Review

## ERRORS

**Error 1: Resource leak on error path**

Line 181, `kernel/freebsd/contigmem/contigmem.c`:
```c
		goto error;
```

When `contigmalloc()` or `contigmalloc_domainset()` fails partway through the loop (buffer 5 of 10, for example), the patch jumps to the `error:` label. However, the code does not show cleanup of the already-allocated buffers 0 through i-1. If the existing `error:` label does not free these partial allocations, this is a resource leak. The patch adds new failure paths (NUMA domain validation, domain-specific allocation) but does not show that cleanup handles these cases.

To verify this is correct, the existing `error:` label implementation must be checked to ensure it iterates through `contigmem_buffers[]` and frees any non-NULL entries. If it does not, every new allocation failure leaks all prior buffers. The patch should either show that cleanup, or if relying on existing code, that code must be verified to handle partial allocation.

**Recommended fix:**
Verify the `error:` label cleanup logic. If it does not already free allocated buffers, add:
```c
error:
	for (int j = 0; j < i; j++) {
		if (contigmem_buffers[j].addr != NULL) {
			contigfree(contigmem_buffers[j].addr,
				contigmem_buffer_size, M_CONTIGMEM);
			contigmem_buffers[j].addr = NULL;
		}
	}
	/* existing error cleanup */
```

---

## WARNINGS

**Warning 1: Missing documentation of sysctl read-only behavior**

Line 85-86, `kernel/freebsd/contigmem/contigmem.c`:
```c
SYSCTL_INT(_hw_contigmem, OID_AUTO, numa_domain, CTLFLAG_RD,
	&contigmem_numa_domain, 0,
```

The tunable is exposed as read-only via sysctl (`CTLFLAG_RD`), meaning runtime changes via `sysctl hw.contigmem.numa_domain=X` have no effect. The documentation in `doc/guides/freebsd_gsg/build_dpdk.rst` states "The tunable takes effect only when the module is loaded" (line 122) but does not explicitly say the sysctl is read-only. Users accustomed to runtime-writable sysctls may attempt `sysctl -w hw.contigmem.numa_domain=1` and be confused when it fails or appears to succeed but has no effect on subsequent allocations.

**Recommended fix:**
Add to the documentation (after line 122):
```rst
The setting is read-only after module load;
it can only be changed by unloading and reloading the module with a new tunable value.
```

---

## INFO

**Info 1: Pre-existing issue - variable declarations in loop initializers**

The new loop at line 169 uses C99-style declaration (`for (i = 0; ...)`), but `i` is declared at function scope earlier in the code (not visible in patch). This is consistent with existing DPDK kernel module style but differs from the preference stated in AGENTS.md for declaring variables at point of use. This is **pre-existing** style in the FreeBSD kernel modules and not introduced by this patch.

---

## FINAL VERIFICATION

### Pass 1: Correctness bugs
- Resource leak on error path: INCLUDED (Error 1)
- No use-after-free detected
- No missing error checks (existing code must already check `addr == NULL`)
- All error paths traced

### Pass 2: Style/process false positives
- No items concluded as "acceptable" or "no issue"

### Pass 3: Evidence verification
- Error 1: Line 181 shown, error path jumps to `error:` label without visible cleanup
- Warning 1: Lines 85-86 show `CTLFLAG_RD`, documentation at line 122 does not mention read-only nature

### Pass 4: Correctness bugs >= 50% confidence
- Error 1: Resource leak depends on unseen error label code; >80% confident this is a risk worth reporting given partial allocation pattern

Review-Result: ERROR