Re: [PATCH v2] app/testpmd: fix flow queue job leaks

[Stephen Hemminger <stephen@networkplumber.org>]

On Fri, 9 Jan 2026 16:26:07 +0100
Dariusz Sosnowski <dsosnowski@nvidia.com> wrote:

> Each enqueued async flow operation in testpmd has an associated
> queue_job struct. It is passed in user data and used to determine
> the type of operation when operation results are pulled on a given
> queue. This information informs the necessary additional handling
> (e.g., freeing flow struct or dumping the queried action state).
> 
> If async flow operations were enqueued and results were not pulled
> before quitting testpmd, these queue_job structs were leaked as reported
> by ASAN:
> 
> Direct leak of 88 byte(s) in 1 object(s) allocated from:
>     #0 0x7f7539084a37 in __interceptor_calloc
>         ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:154
>     #1 0x55a872c8e512 in port_queue_flow_create
>         (/download/dpdk/install/bin/dpdk-testpmd+0x4cd512)
>     #2 0x55a872c28414 in cmd_flow_cb
>         (/download/dpdk/install/bin/dpdk-testpmd+0x467414)
>     #3 0x55a8734fa6a3 in __cmdline_parse
>         (/download/dpdk/install/bin/dpdk-testpmd+0xd396a3)
>     #4 0x55a8734f6130 in cmdline_valid_buffer
>         (/download/dpdk/install/bin/dpdk-testpmd+0xd35130)
>     #5 0x55a873503b4f in rdline_char_in
>         (/download/dpdk/install/bin/dpdk-testpmd+0xd42b4f)
>     #6 0x55a8734f62ba in cmdline_in
>         (/download/dpdk/install/bin/dpdk-testpmd+0xd352ba)
>     #7 0x55a8734f65eb in cmdline_interact
>         (/download/dpdk/install/bin/dpdk-testpmd+0xd355eb)
>     #8 0x55a872c19b8e in prompt
>         (/download/dpdk/install/bin/dpdk-testpmd+0x458b8e)
>     #9 0x55a872be425a in main
>         (/download/dpdk/install/bin/dpdk-testpmd+0x42325a)
>     #10 0x7f7538756d8f in __libc_start_call_main
>         ../sysdeps/nptl/libc_start_call_main.h:58
> 
> This patch addresses that by registering all queue_job structs, for a
> given queue, on a linked list. Whenever operation results are pulled
> and result is handled, queue_job struct will be removed from that list
> and freed.
> Before port is closed, during flow flush, testpmd will pull
> all of the expected results
> (based on the number of queue_job on the list).
> 
> Fixes: c9dc03840873 ("ethdev: add indirect action async query")
> Fixes: 99231e480b69 ("ethdev: add template table resize")
> Fixes: 77e7939acf1f ("app/testpmd: add flow rule update command")
> Fixes: 3e3edab530a1 ("ethdev: add flow quota")
> Fixes: 966eb55e9a00 ("ethdev: add queue-based API to report aged flow rules")
> Cc: stable@dpdk.org
> 

Surprisingly AI code spotted a minor bug which I did not see.


---

## DPDK Patch Evaluation: `app/testpmd: fix flow queue job leaks`

### Summary

| Category | Status |
|----------|--------|
| Commit Message | ✅ Pass |
| Tags | ✅ Pass |
| Code Style | ⚠️ Minor issues |
| Code Correctness | ❌ **Bug found** |

---

### Commit Message Analysis

**Subject Line:** `app/testpmd: fix flow queue job leaks`

| Criterion | Result |
|-----------|--------|
| Length (≤60 chars) | ✅ 37 characters |
| Format `component: description` | ✅ Correct |
| Component prefix | ✅ `app/testpmd:` (not `testpmd:`) |
| Lowercase after colon | ✅ Yes |
| Imperative mood | ✅ "fix" |
| No trailing period | ✅ Correct |
| No forbidden punctuation | ✅ Clean |

**Body:**
- ✅ Does not start with "It"
- ✅ Provides clear context (ASAN leak report, explanation of queue_job lifecycle)
- ✅ Lines appear within 75 character limit
- ✅ Well-structured explanation of the problem and solution

**Tags:**
- ✅ Multiple `Fixes:` tags with 12-character SHAs and quoted subjects
- ✅ `Cc: stable@dpdk.org` present (appropriate for bug fix)
- ✅ `Signed-off-by:` with real name and valid email
- ✅ Tag order correct: Fixes → Cc → blank line → Signed-off-by

---

### Code Style Analysis

**Positive:**
- ✅ Function return types on separate lines
- ✅ Explicit comparisons (`res == NULL`, `ret < 0`, `ret == 0`)
- ✅ Proper brace style (single statements without braces)
- ✅ Lowercase function names with underscores
- ✅ Comment style follows guidelines
- ✅ Lines within 100 characters

**Info (minor observations):**
- C99-style loop variable declarations (`for (int i = 0; ...)` and `for (queueid_t queue_id = 0; ...)`). This is acceptable in DPDK but some maintainers prefer traditional style.

---

### ❌ Critical Bug Found: Format String Argument Mismatch

There are **two printf format string bugs** where arguments are in the wrong order:

**Bug 1 (line ~428-429):**
```c
printf("Flushed flow queue %u on port %u (%u failed, %u succeeded).\n",
       port_id, queue_id, polled - success, success);
```
Format says "queue %u on port %u" but arguments are `port_id, queue_id` — **reversed**.

**Bug 2 (line ~437-438):**
```c
TESTPMD_LOG(ERR, "Unable to fully flush flow queue %u on port %u (left ops %u)\n",
            port_id, queue_id, expected_ops);
```
Same issue — format expects queue then port, but arguments are port then queue.

**Correct version should be:**
```c
printf("Flushed flow queue %u on port %u (%u failed, %u succeeded).\n",
       queue_id, port_id, polled - success, success);

TESTPMD_LOG(ERR, "Unable to fully flush flow queue %u on port %u (left ops %u)\n",
            queue_id, port_id, expected_ops);
```

Note: Line ~475-476 has the correct order (`queue_id, port_id`), which makes this inconsistency more apparent.

---

### Verdict

| Severity | Issue |
|----------|-------|
| **Error** | Format string argument order bug (2 instances) — will print misleading debug output |
| Info | C99 loop variable declarations |

**Recommendation:** Request v3 to fix the printf argument order. The fix itself is sound in design — tracking queue_job entries on a list and flushing on port close is the right approach. The ASAN leak is properly addressed. However, the swapped format arguments would cause confusing output during debugging ("Flushed flow queue 0 on port 1" when it should say "queue 1 on port 0").