On Wed, 14 Jan 2026 00:15:58 -0800
Dimon Zhao <[email protected]> wrote:
> The timer interrupt handler periodically queries and updates
> hw_stats via mailbox.
> Since mailbox operations rely on interrupts for packet reception,
> this causes interrupt nesting.
> To resolve this, trigger a task from the interrupt handler and
> start a dedicated thread to execute this task,
> eliminating the nested interrupt scenario.
>
> Fixes: c9726a719ca1 ("net/nbl: support dropped packets counter")
> Cc: [email protected]
>
> Signed-off-by: Dimon Zhao <[email protected]>
> ---
Automated patch review has some good observations, please address
these and resubmit.
## Patch Review: net/nbl: fix hardware stats interrupt nesting issue
### Commit Message Analysis
| Criterion | Status | Notes |
|-----------|--------|-------|
| Subject ≤60 chars | ✓ PASS | 51 characters |
| Lowercase after colon | ✓ PASS | "fix hardware stats..." |
| Correct prefix | ✓ PASS | `net/nbl:` |
| Imperative mood | ✓ PASS | "fix" |
| No trailing period | ✓ PASS | |
| Body ≤75 chars | ✓ PASS | All body lines within limit |
| Body does not start with "It" | ✓ PASS | Starts with "The" |
| Signed-off-by present | ✓ PASS | Valid name and email |
| Fixes tag format | ✓ PASS | 12-char SHA `c9726a719ca1` |
| Cc: [email protected] | ✓ PASS | Appropriate for bug fix |
| Tag order | ✓ PASS | Fixes → Cc → blank → Signed-off-by |
**Commit message: PASS**
---
### Code Review
#### **WARNING: Unnecessary cast of void pointer**
**Location:** Line 76 in patch (new function `nbl_dev_thread_hw_stats_task`)
```c
struct rte_eth_dev *eth_dev = (struct rte_eth_dev *)param;
```
Per AGENTS.md, casting `void *` is unnecessary in C. Should be:
```c
struct rte_eth_dev *eth_dev = param;
```
---
#### **WARNING: Unnecessary variable initialization**
**Location:** Line 101 in patch
```c
ssize_t nw = 0;
```
The variable is immediately assigned on line 104 (`nw = write(...)`), making
the `= 0` initialization unnecessary. Per AGENTS.md guidelines on unnecessary
initialization patterns.
---
#### **WARNING: Resource leak on error path**
**Location:** `nbl_dev_hw_stats_start()` around lines 213-224 in patch
If `pipe()` succeeds but `rte_thread_create_internal_control()` fails, the pipe
file descriptors are leaked:
```c
ret = pipe(net_dev->fd);
if (ret) {
NBL_LOG(ERR, "hw_stats pipe failed, ret %d", ret);
return ret;
}
ret = rte_thread_create_internal_control(&net_dev->tid, "nbl_hw_stats_thread",
nbl_dev_thread_hw_stats_task, eth_dev);
if (ret) {
NBL_LOG(ERR, "create hw_stats thread failed, ret %d", ret);
return ret; // <-- LEAK: net_dev->fd[0] and fd[1] not closed
}
```
**Suggested fix:**
```c
ret = rte_thread_create_internal_control(&net_dev->tid, "nbl_hw_stats_thread",
nbl_dev_thread_hw_stats_task, eth_dev);
if (ret) {
NBL_LOG(ERR, "create hw_stats thread failed, ret %d", ret);
close(net_dev->fd[0]);
close(net_dev->fd[1]);
return ret;
}
```
---
#### **INFO: Thread function return type**
**Location:** Line 74
```c
static uint32_t nbl_dev_thread_hw_stats_task(void *param)
```
The `rte_thread_create_internal_control()` callback expects signature `uint32_t
(*func)(void *)`, so this is correct. Just noting for completeness.
---
#### **INFO: Consider adding missing include**
The code uses `pipe()`, `read()`, `write()`, and `close()` which typically
require `<unistd.h>`. Ensure this header is included (either directly or
transitively through `nbl_dev.h` → `nbl_common.h`).
---
### Summary
| Severity | Count | Issues |
|----------|-------|--------|
| **Error** | 0 | — |
| **Warning** | 3 | Unnecessary void* cast, unnecessary initialization,
resource leak |
| **Info** | 2 | Return type note, include verification |
### Verdict: **Needs Minor Revision (v2)**
The patch logic is sound—moving the mailbox-dependent stats update from
interrupt context to a dedicated thread via pipe notification is the correct
approach to resolve interrupt nesting. However, three warnings should be
addressed:
1. Remove the unnecessary `(struct rte_eth_dev *)` cast
2. Remove `= 0` from `ssize_t nw` declaration
3. Add cleanup of pipe file descriptors if thread creation fails
