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

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon, 12 Jan 2026 18:36:47 +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
> 
> Signed-off-by: Dariusz Sosnowski <dsosnowski@nvidia.com>
> ---

Much better, but there some suggestions.
Yes this AI "bikeshedding" which is a first...

## Patch Review: app/testpmd: fix flow queue job leaks

### Summary
This patch fixes memory leaks in testpmd's async flow API by tracking `queue_job` structs in a linked list and ensuring proper cleanup when flushing flow queues or closing ports.

---

### ✅ Commit Message Analysis

| Check | Status | Notes |
|-------|--------|-------|
| Subject ≤60 chars | ✅ Pass | `app/testpmd: fix flow queue job leaks` = 37 chars |
| Correct prefix | ✅ Pass | `app/testpmd:` is correct |
| Lowercase after colon | ✅ Pass | |
| Imperative mood | ✅ Pass | "fix" is imperative |
| No trailing period | ✅ Pass | |
| No forbidden punctuation | ✅ Pass | |
| Body doesn't start with "It" | ✅ Pass | Starts with "Each enqueued..." |
| `Signed-off-by:` present | ✅ Pass | Real name with valid email |
| `Fixes:` tags present | ✅ Pass | 5 Fixes tags with 12-char SHAs |
| `Cc: stable@dpdk.org` present | ✅ Pass | Correctly included for bug fix |
| Blank line before Signed-off-by | ✅ Pass | |
| Tag order | ✅ Pass | Fixes → Cc → blank → Signed-off-by |

---

### ⚠️ Warnings

**1. Unnecessary cast of `void *` (line ~420)**
```c
job = (struct queue_job *)res[i].user_data;
```
If `user_data` is `void *`, this cast is unnecessary in C and should be removed:
```c
job = res[i].user_data;
```

---

### ℹ️ Informational Notes

**1. Function naming could be clearer**

The function `port_alloc_queue_job()` doesn't actually allocate anything—it inserts a job into a linked list. A clearer name might be `port_register_queue_job()` or `port_track_queue_job()`.

**2. Commit body line lengths**

The ASAN stack trace in the commit message appears to have lines that may approach or exceed 75 characters due to indentation. While the content itself is useful, you may want to verify line lengths or consider trimming the stack trace to the most relevant frames.

---

### ✅ Code Style Verification

| Check | Status |
|-------|--------|
| Lines ≤100 characters | ✅ Pass |
| Explicit NULL comparisons | ✅ Pass |
| Explicit zero comparisons | ✅ Pass |
| No forbidden APIs | ✅ Pass |
| Function return type on own line | ✅ Pass |
| Proper comment style | ✅ Pass |
| No trailing whitespace (visible) | ✅ Pass |
| Uses DPDK list macros correctly | ✅ Pass |

---

### Code Quality Notes

The implementation is sound:
- Properly tracks jobs per queue using `LIST_HEAD`/`LIST_ENTRY` macros
- Bounded iteration count (`FLOW_QUEUE_FLUSH_MAX_ITERS`) prevents infinite loops
- Graceful error handling with cleanup on failure paths
- Memory is freed both on normal completion and timeout conditions
- Uses `RTE_ASSERT` with explicit NULL comparison for debugging

---

### Verdict

**Acceptable with minor suggestions.** The patch follows DPDK coding standards correctly. The only actionable item is the unnecessary `void *` cast, which is a warning-level issue. The naming suggestion is purely stylistic.