[PATCH v1 0/1] fix hardware stats interrupt nesting issue

[PATCH v1 0/1] fix hardware stats interrupt nesting issue

[Dimon Zhao]

fix dropped packets counter issue

Dimon Zhao (1):
  net/nbl: fix hardware stats interrupt nesting issue

 drivers/net/nbl/nbl_dev/nbl_dev.c | 52 ++++++++++++++++++++++++++++++-
 drivers/net/nbl/nbl_dev/nbl_dev.h |  2 ++
 2 files changed, 53 insertions(+), 1 deletion(-)

-- 
2.34.1

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

[Dimon Zhao]

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>
---
 drivers/net/nbl/nbl_dev/nbl_dev.c | 52 ++++++++++++++++++++++++++++++-
 drivers/net/nbl/nbl_dev/nbl_dev.h |  2 ++
 2 files changed, 53 insertions(+), 1 deletion(-)

diff --git a/drivers/net/nbl/nbl_dev/nbl_dev.c b/drivers/net/nbl/nbl_dev/nbl_dev.c
index 8a56939aa7..f389fa62e4 100644
--- a/drivers/net/nbl/nbl_dev/nbl_dev.c
+++ b/drivers/net/nbl/nbl_dev/nbl_dev.c
@@ -168,11 +168,37 @@ static int nbl_dev_update_hw_stats(struct rte_eth_dev *eth_dev)
 	return ret;
 }
 
+static uint32_t nbl_dev_thread_hw_stats_task(void *param)
+{
+	struct rte_eth_dev *eth_dev = (struct rte_eth_dev *)param;
+	struct nbl_adapter *adapter = ETH_DEV_TO_NBL_DEV_PF_PRIV(eth_dev);
+	struct nbl_dev_mgt *dev_mgt = NBL_ADAPTER_TO_DEV_MGT(adapter);
+	struct nbl_dev_net_mgt *net_dev = NBL_DEV_MGT_TO_NET_DEV(dev_mgt);
+	char unused[16];
+	ssize_t nr = 0;
+
+	while (true) {
+		nr = read(net_dev->fd[0], &unused, sizeof(unused));
+		if (nr <= 0)
+			break;
+
+		nbl_dev_update_hw_stats(eth_dev);
+	}
+
+	return 0;
+}
+
 static void nbl_dev_update_hw_stats_handler(void *param)
 {
 	struct rte_eth_dev *eth_dev = param;
+	struct nbl_adapter *adapter = ETH_DEV_TO_NBL_DEV_PF_PRIV(eth_dev);
+	struct nbl_dev_mgt *dev_mgt = NBL_ADAPTER_TO_DEV_MGT(adapter);
+	struct nbl_dev_net_mgt *net_dev = NBL_DEV_MGT_TO_NET_DEV(dev_mgt);
+	char notify_byte = 0;
+	ssize_t nw = 0;
 
-	nbl_dev_update_hw_stats(eth_dev);
+	nw = write(net_dev->fd[1], &notify_byte, 1);
+	RTE_SET_USED(nw);
 
 	rte_eal_alarm_set(NBL_ALARM_INTERNAL, nbl_dev_update_hw_stats_handler, eth_dev);
 }
@@ -187,6 +213,19 @@ static int nbl_dev_hw_stats_start(struct rte_eth_dev *eth_dev)
 	struct nbl_ustore_stats ustore_stats = {0};
 	int ret;
 
+	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;
+	}
+
 	if (!common->is_vf) {
 		ret = disp_ops->get_ustore_total_pkt_drop_stats(NBL_DEV_MGT_TO_DISP_PRIV(dev_mgt),
 						common->eth_id, &ustore_stats);
@@ -261,8 +300,19 @@ static void nbl_dev_txrx_stop(struct rte_eth_dev *eth_dev)
 
 static int nbl_dev_hw_stats_stop(struct rte_eth_dev *eth_dev)
 {
+	struct nbl_adapter *adapter = ETH_DEV_TO_NBL_DEV_PF_PRIV(eth_dev);
+	struct nbl_dev_mgt *dev_mgt = NBL_ADAPTER_TO_DEV_MGT(adapter);
+	struct nbl_dev_net_mgt *net_dev = NBL_DEV_MGT_TO_NET_DEV(dev_mgt);
+
 	rte_eal_alarm_cancel(nbl_dev_update_hw_stats_handler, eth_dev);
 
+	/* closing pipe to cause hw_stats thread to exit */
+	close(net_dev->fd[0]);
+	close(net_dev->fd[1]);
+	net_dev->fd[0] = -1;
+	net_dev->fd[1] = -1;
+	rte_thread_join(net_dev->tid, NULL);
+
 	return 0;
 }
 
diff --git a/drivers/net/nbl/nbl_dev/nbl_dev.h b/drivers/net/nbl/nbl_dev/nbl_dev.h
index 21d87a372d..bfe2b06deb 100644
--- a/drivers/net/nbl/nbl_dev/nbl_dev.h
+++ b/drivers/net/nbl/nbl_dev/nbl_dev.h
@@ -60,6 +60,8 @@ struct nbl_dev_net_mgt {
 	u8 rsv:6;
 	struct nbl_hw_stats hw_stats;
 	bool hw_stats_inited;
+	rte_thread_t tid;
+	int fd[2];
 };
 
 struct nbl_dev_mgt {
-- 
2.34.1

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

[Stephen Hemminger]

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

[PATCH v2 0/1] fix hardware stats interrupt nesting issue

[Dimon Zhao]

fix dropped packets counter issue

Dimon Zhao (1):
  net/nbl: fix hardware stats interrupt nesting issue

 drivers/net/nbl/nbl_dev/nbl_dev.c | 56 ++++++++++++++++++++++++++++++-
 drivers/net/nbl/nbl_dev/nbl_dev.h |  2 ++
 2 files changed, 57 insertions(+), 1 deletion(-)

-- 
2.34.1

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

[Dimon Zhao]

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>
---
 drivers/net/nbl/nbl_dev/nbl_dev.c | 56 ++++++++++++++++++++++++++++++-
 drivers/net/nbl/nbl_dev/nbl_dev.h |  2 ++
 2 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/drivers/net/nbl/nbl_dev/nbl_dev.c b/drivers/net/nbl/nbl_dev/nbl_dev.c
index e926c06456..0afd5a37b1 100644
--- a/drivers/net/nbl/nbl_dev/nbl_dev.c
+++ b/drivers/net/nbl/nbl_dev/nbl_dev.c
@@ -168,11 +168,37 @@ static int nbl_dev_update_hw_stats(struct rte_eth_dev *eth_dev)
 	return ret;
 }
 
+static uint32_t nbl_dev_thread_hw_stats_task(void *param)
+{
+	struct rte_eth_dev *eth_dev = param;
+	struct nbl_adapter *adapter = ETH_DEV_TO_NBL_DEV_PF_PRIV(eth_dev);
+	struct nbl_dev_mgt *dev_mgt = NBL_ADAPTER_TO_DEV_MGT(adapter);
+	struct nbl_dev_net_mgt *net_dev = NBL_DEV_MGT_TO_NET_DEV(dev_mgt);
+	char unused[16];
+	ssize_t nr;
+
+	while (true) {
+		nr = read(net_dev->fd[0], &unused, sizeof(unused));
+		if (nr <= 0)
+			break;
+
+		nbl_dev_update_hw_stats(eth_dev);
+	}
+
+	return 0;
+}
+
 static void nbl_dev_update_hw_stats_handler(void *param)
 {
 	struct rte_eth_dev *eth_dev = param;
+	struct nbl_adapter *adapter = ETH_DEV_TO_NBL_DEV_PF_PRIV(eth_dev);
+	struct nbl_dev_mgt *dev_mgt = NBL_ADAPTER_TO_DEV_MGT(adapter);
+	struct nbl_dev_net_mgt *net_dev = NBL_DEV_MGT_TO_NET_DEV(dev_mgt);
+	char notify_byte = 0;
+	ssize_t nw;
 
-	nbl_dev_update_hw_stats(eth_dev);
+	nw = write(net_dev->fd[1], &notify_byte, 1);
+	RTE_SET_USED(nw);
 
 	rte_eal_alarm_set(NBL_ALARM_INTERNAL, nbl_dev_update_hw_stats_handler, eth_dev);
 }
@@ -187,6 +213,23 @@ static int nbl_dev_hw_stats_start(struct rte_eth_dev *eth_dev)
 	struct nbl_ustore_stats ustore_stats = {0};
 	int ret;
 
+	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);
+		close(net_dev->fd[0]);
+		close(net_dev->fd[1]);
+		net_dev->fd[0] = -1;
+		net_dev->fd[1] = -1;
+		return ret;
+	}
+
 	if (!common->is_vf) {
 		ret = disp_ops->get_ustore_total_pkt_drop_stats(NBL_DEV_MGT_TO_DISP_PRIV(dev_mgt),
 						common->eth_id, &ustore_stats);
@@ -261,8 +304,19 @@ static void nbl_dev_txrx_stop(struct rte_eth_dev *eth_dev)
 
 static int nbl_dev_hw_stats_stop(struct rte_eth_dev *eth_dev)
 {
+	struct nbl_adapter *adapter = ETH_DEV_TO_NBL_DEV_PF_PRIV(eth_dev);
+	struct nbl_dev_mgt *dev_mgt = NBL_ADAPTER_TO_DEV_MGT(adapter);
+	struct nbl_dev_net_mgt *net_dev = NBL_DEV_MGT_TO_NET_DEV(dev_mgt);
+
 	rte_eal_alarm_cancel(nbl_dev_update_hw_stats_handler, eth_dev);
 
+	/* closing pipe to cause hw_stats thread to exit */
+	close(net_dev->fd[0]);
+	close(net_dev->fd[1]);
+	net_dev->fd[0] = -1;
+	net_dev->fd[1] = -1;
+	rte_thread_join(net_dev->tid, NULL);
+
 	return 0;
 }
 
diff --git a/drivers/net/nbl/nbl_dev/nbl_dev.h b/drivers/net/nbl/nbl_dev/nbl_dev.h
index 21d87a372d..bfe2b06deb 100644
--- a/drivers/net/nbl/nbl_dev/nbl_dev.h
+++ b/drivers/net/nbl/nbl_dev/nbl_dev.h
@@ -60,6 +60,8 @@ struct nbl_dev_net_mgt {
 	u8 rsv:6;
 	struct nbl_hw_stats hw_stats;
 	bool hw_stats_inited;
+	rte_thread_t tid;
+	int fd[2];
 };
 
 struct nbl_dev_mgt {
-- 
2.34.1

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

[Stephen Hemminger]

On Wed, 14 Jan 2026 22:18:07 -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>
> ---

Queued to next-net.

You may want to consider the suggestions from AI patch review

### ✅ COMMIT MESSAGE

**Subject Line** (Line 29):
- Length: 53 characters ✓ (limit: 60)
- Format: `net/nbl: fix hardware stats interrupt nesting issue` ✓
- Lowercase with imperative mood ✓
- No trailing period ✓
- Correct component prefix ✓

**Body** (Lines 50-56):
- Lines within 75 character limit ✓
- Doesn't start with "It" ✓
- Clearly explains the problem: interrupt nesting from mailbox operations ✓
- Describes solution: dedicated thread eliminates nested interrupts ✓
- Good technical rationale ✓

**Tags** (Lines 58-61):
- `Fixes:` tag with 12-char SHA and exact subject ✓
- `Cc: stable@dpdk.org` present ✓
- `Signed-off-by:` with full name and email ✓
- Correct tag order ✓

### ✅ CODE CHANGES

**Architecture**: Producer-consumer pattern using pipe for interrupt-to-thread communication - solid design ✓

**nbl_dev_thread_hw_stats_task()** (Lines 75-93):
- Worker thread that reads from pipe and updates stats
- Uses `read()` with proper return value checking (line 86-87) ✓
- Clean loop termination on pipe closure ✓

**nbl_dev_update_hw_stats_handler()** (Lines 95-109):
- Interrupt handler now writes to pipe instead of direct mailbox call ✓
- **Line 106-107**: `write()` return value captured but marked unused with `RTE_SET_USED(nw)`

**nbl_dev_hw_stats_start()** (Lines 213-229):
- Creates pipe with error checking ✓
- Spawns thread with proper error handling ✓
- Cleans up pipe on thread creation failure (lines 124-127) ✓
- Sets fd to -1 on error - good defensive programming ✓

**nbl_dev_hw_stats_stop()** (Lines 336-350):
- Cancels alarm ✓
- Closes pipe to signal thread exit ✓
- **Line 145-146**: `close()` calls without return value checks
- **Line 149**: `rte_thread_join()` without return value check
- Sets fd to -1 after close - good practice ✓

### ⚠️ WARNINGS

**Warning 1**: Line 106-107 - Write return value handling
```c
nw = write(net_dev->fd[1], &notify_byte, 1);
RTE_SET_USED(nw);
```
**Issue**: In interrupt context, the pipe write is intentionally best-effort (if write fails, one stats update is skipped but alarm reschedules). However, this should have a comment explaining the intentional ignore, or consider at least logging on failure if safe in interrupt context.

**Suggested improvement**:
```c
/* Best-effort notification; if write fails, stats update skipped this cycle */
nw = write(net_dev->fd[1], &notify_byte, 1);
if (unlikely(nw != 1))
    NBL_LOG(DEBUG, "hw_stats notification write failed");
```

**Warning 2**: Line 149 - Thread join without error check
```c
rte_thread_join(net_dev->tid, NULL);
```
**Issue**: Return value should be checked per AGENTS.md guidelines on checking system calls.

**Suggested improvement**:
```c
ret = rte_thread_join(net_dev->tid, NULL);
if (ret)
    NBL_LOG(ERR, "hw_stats thread join failed, ret %d", ret);
```

**Warning 3**: Lines 145-146 - Close without error check
```c
close(net_dev->fd[0]);
close(net_dev->fd[1]);
```
**Issue**: While errors on `close()` in cleanup paths are typically non-fatal, AGENTS.md emphasizes checking return values. At minimum, could check for -1 and log.

### ✅ CODE STYLE

- Line length under 100 characters ✓
- Proper indentation ✓
- No trailing whitespace visible ✓
- Variable naming conventions followed ✓
- Comment on line 144 uses correct style ✓

### 📋 FINAL ASSESSMENT

**Errors:** None  
**Warnings:** 3 (return value checking in edge cases)  
**Info:** Excellent architectural solution to interrupt nesting problem

The patch solves a real concurrency issue with a clean producer-consumer design. The interrupt-to-thread communication via pipe is the right approach. The warnings are minor - mostly about defensive error checking in contexts where failures are unlikely or non-fatal.

**Recommendation:** ✅ **APPROVED with minor suggestions**

The patch is fundamentally sound and ready for merge. The suggested improvements around return value checking would make it even more robust, but the current implementation is acceptable given that:
1. Pipe write failure in interrupt context has limited recovery options
2. Thread join failure during cleanup is rare
3. Close failures are typically informational

**Priority for improvements**: Low - consider for follow-up polish but not blocking.