Re: [PATCH v2 0/3] EAL memory fixes

[Stephen Hemminger <stephen@networkplumber.org>]

On Thu, 14 Aug 2025 16:32:42 -0500
Jake Freeland <jfree@FreeBSD.org> wrote:

> Hi there,
> 
> This patchset contains a number of EAL-specific memory fixes that I've
> made over the last year. Two pertain to FreeBSD, one pertains to Linux.
> 
> v2:
> * Log messages are no longer split across multiple lines.
> * The patch titled "Do not index out of bounds in memseg list" has been
>   removed since there was no indexing out of bounds happening.
> * A new patch has been added per Anatoly's recommendation that starts
>   searching for memseg entries after the last used entry.
> 
> Jake Freeland (3):
>   eal/freebsd: Do not use prev_ms_idx for hole detection
>   eal/freebsd: Avoid claiming memseg holes
>   eal/linux: Check hugepage access permissions
> 
>  lib/eal/freebsd/eal_memory.c      | 24 +++++++++++++++++++-----
>  lib/eal/linux/eal_hugepage_info.c |  7 +++++++
>  2 files changed, 26 insertions(+), 5 deletions(-)
> 

Makes sense. I noticed that AI spotted same dead code as Anatoly.
You might want to fix that before Coverity complains.



## DPDK Patch Review: FreeBSD/Linux Memory Setup Fixes (v2)

**Series:** [PATCH v2 1-3/3] Memory segment and hugepage fixes  
**Author:** Jake Freeland <jfree@FreeBSD.org>  
**Acked-by:** Anatoly Burakov <anatoly.burakov@intel.com>

---

### Patch 1/3: eal/freebsd: Do not use prev_ms_idx for hole detection

**Subject Analysis:**
| Check | Status | Notes |
|-------|--------|-------|
| Length ≤60 chars | ✅ Pass | 55 characters |
| Format | ✅ Pass | `component: description` |
| Case after colon | ⚠️ Warning | "Do" should be "do" — lowercase required |
| No trailing period | ✅ Pass | |
| Signed-off-by | ✅ Pass | Present with real name |

**Commit Message Body:**
- ✅ Lines within 75 characters
- ✅ Does not start with "It"
- ✅ Clear explanation of the bug and fix

**Code Review:**
```c
-			if (need_hole && prev_ms_idx == ms_idx - 1)
+			if (need_hole &&
+			    rte_fbarray_is_used(arr, ms_idx - 1))
 				ms_idx++;
-			prev_ms_idx = ms_idx;
```

- ✅ Correctly uses `rte_fbarray_is_used()` API instead of fragile index tracking
- ✅ Fixes cross-memseg-list logic error
- ✅ Clean removal of now-unnecessary variable

**Verdict:** ✅ Acceptable — minor subject case warning

---

### Patch 2/3: eal/freebsd: Avoid claiming memseg holes

**Subject Analysis:**
| Check | Status | Notes |
|-------|--------|-------|
| Length ≤60 chars | ✅ Pass | 42 characters |
| Format | ✅ Pass | `component: description` |
| Case after colon | ⚠️ Warning | "Avoid" should be "avoid" |
| No trailing period | ✅ Pass | |
| Signed-off-by | ✅ Pass | |

**Commit Message Body:**
- ✅ Lines within 75 characters
- ✅ Does not start with "It"
- ✅ Clear problem/solution description

**Code Review:**

```c
start_idx = rte_fbarray_find_prev_n_used(
        arr, arr->len - 1, 1) + 1;
if (start_idx < 0)
    start_idx = 0;
```

⚠️ **Warning — Dead Code:** The condition `if (start_idx < 0)` can never be true. When `rte_fbarray_find_prev_n_used()` returns -1 (not found), adding 1 yields 0, which is not < 0. The check is ineffective. Consider either:
- Removing the dead check (since starting at 0 when nothing found is correct behavior), or
- Checking the return value before adding 1

```c
/* Multi-line comment style is correct */
/* begin our search after the last used
 * element in the list, skipping over
 * any previously placed holes
 */
```
- ✅ Comment formatting follows DPDK style

**Verdict:** ⚠️ Minor issues — dead code check, subject case

---

### Patch 3/3: eal/linux: Check hugepage access permissions

**Subject Analysis:**
| Check | Status | Notes |
|-------|--------|-------|
| Length ≤60 chars | ✅ Pass | 43 characters |
| Format | ✅ Pass | `component: description` |
| Case after colon | ⚠️ Warning | "Check" should be "check" |
| No trailing period | ✅ Pass | |
| Signed-off-by | ✅ Pass | |

**Commit Message Body:**
- ✅ Lines within 75 characters
- ✅ Does not start with "It"
- ✅ Clear description of the permission issue and fix

**Code Review:**
```c
+	if (access(splitstr[MOUNTPT], R_OK | W_OK) < 0) {
+		EAL_LOG(NOTICE,
+			"Skipping hugepage dir '%s': missing r/w perms",
+			splitstr[MOUNTPT]);
+		continue;
+	}
```

- ✅ Correct use of `access()` with R_OK | W_OK
- ✅ Appropriate NOTICE log level for skipping mountpoints
- ✅ Good defensive programming — fail gracefully rather than EACCES later
- ✅ Explicit comparison with `< 0` (not implicit boolean)

**Verdict:** ✅ Acceptable — minor subject case warning

---

## Summary

| Patch | Errors | Warnings | Info |
|-------|--------|----------|------|
| 1/3 | 0 | 1 (subject case) | 0 |
| 2/3 | 0 | 2 (subject case, dead code) | 0 |
| 3/3 | 0 | 1 (subject case) | 0 |

### Recommended Actions

**Should fix (Warnings):**

1. **All patches:** Fix subject line capitalization. Per DPDK guidelines, text after the colon should be lowercase except for acronyms:
   - `eal/freebsd: do not use prev_ms_idx for hole detection`
   - `eal/freebsd: avoid claiming memseg holes`
   - `eal/linux: check hugepage access permissions`

2. **Patch 2/3:** Remove or fix the dead `if (start_idx < 0)` check — it serves no purpose as written.

**Consider (Info):**

3. These appear to be bug fixes. If they fix regressions introduced by specific commits, consider adding `Fixes:` tags with the 12-character SHA and original subject.

4. If these are candidates for stable backport, consider adding `Cc: stable@dpdk.org`.

---

**Overall Assessment:** The series is technically sound and addresses real bugs in memory segment management. The fixes are well-designed and the commit messages explain the issues clearly. With the minor subject line case fixes and the dead code cleanup in patch 2, this series should be ready for merge.