Re: [PATCH v1 1/1] net/nbl: fix hardware stats interrupt nesting issue

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed, 14 Jan 2026 00:15:58 -0800
Dimon Zhao <dimon.zhao@nebula-matrix.com> 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: stable@dpdk.org
> 
> Signed-off-by: Dimon Zhao <dimon.zhao@nebula-matrix.com>
> ---

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: stable@dpdk.org | ✓ 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