|WARNING| [v17] graph: add optional profiling stats

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/168252

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-08-23

# DPDK Patch Review

## Patch: graph: add optional profiling stats

### Summary
This patch adds optional build-time configurable graph node profiling statistics. The implementation tracks cycles spent processing various burst size ranges per node, with build-time configurable range boundaries.

---

## Errors

### 1. Unsafe cast in percentage calculation (use-after-free risk if node freed)
In `graph_debug.c` lines 105-107, the code dereferences `n->total_cycles` and `n->total_calls` after potentially being freed in the loop. However, on closer inspection, the loop iterates over graph nodes that remain valid during the dump operation, so this is not a correctness bug.

**Correction: No issue here - nodes remain valid during graph traversal.**

### 2. Missing validation of config option relationships
The static assertions in `rte_graph_worker_common.h` (lines 237-242) validate burst size relationships, but there's no validation that these config values are positive or that `BURST_LARGE <= RTE_GRAPH_BURST_SIZE`.

**Add validation:**
```c
static_assert(RTE_GRAPH_PROFILE_BURST_LARGE <= RTE_GRAPH_BURST_SIZE,
	"BURST_LARGE must be <= RTE_GRAPH_BURST_SIZE");
```

---

## Warnings

### 1. Missing experimental API markers
The new configuration options `RTE_GRAPH_PROFILE`, `RTE_GRAPH_PROFILE_BURST_SMALL`, `RTE_GRAPH_PROFILE_BURST_MEDIUM`, and `RTE_GRAPH_PROFILE_BURST_LARGE` are new build-time API but are not marked experimental. Build-time configuration is part of the public API.

**Recommendation:** Document in the release notes that these are experimental configuration options, or mark them appropriately in the code comments.

### 2. Inconsistent comment style in array declaration
Line 132-135 in `graph_debug.c`:
```c
static const uint16_t sample_sizes[] = {
		0, 1, 2, RTE_GRAPH_PROFILE_BURST_SMALL,
		RTE_GRAPH_PROFILE_BURST_MEDIUM, RTE_GRAPH_PROFILE_BURST_LARGE};
```

The indentation uses tabs + spaces inconsistently. Should use tabs only for indentation.

**Fix:**
```c
	static const uint16_t sample_sizes[] = {
		0, 1, 2, RTE_GRAPH_PROFILE_BURST_SMALL,
		RTE_GRAPH_PROFILE_BURST_MEDIUM, RTE_GRAPH_PROFILE_BURST_LARGE};
```

### 3. Implicit comparison with zero
Line 103 in `graph_debug.c`:
```c
all_total_cycles == 0 ? 0.0 :
```

This is acceptable for integer comparison, but for consistency with DPDK style that requires explicit comparisons, suggest:
```c
all_total_cycles != 0 ? ... : 0.0
```

However, this is already correctly using explicit comparison, so no change needed.

**Correction: This is already correct.**

### 4. Division by zero protection pattern
Lines 105-107 use ternary operators to protect against division by zero. While correct, the pattern could be clearer with early guards:

```c
if (all_total_cycles != 0 && n->total_calls != 0) {
	fprintf(f, "       total_cycles=%" PRIu64 " (%.1f%% of all nodes), avg cycles/call=%.1f\n",
		...);
}
```

However, the current ternary pattern is acceptable and maintains readability.

**No change required.**

### 5. Release notes placement
The release notes are correctly placed in `doc/guides/rel_notes/release_26_11.rst`, which matches the patch date (2026-08-23). No issue.

### 6. Alignment directive placement
Line 126 in `rte_graph_worker_common.h`:
```c
alignas(RTE_CACHE_LINE_MIN_SIZE)
struct {
```

The `alignas` should appear after the `struct` keyword per DPDK style for alignment attributes. However, `alignas` is a C11 keyword, not a DPDK attribute macro, so standard C syntax applies here.

**Actually correct** - `alignas` is a C11 keyword with its own syntax rules, not a DPDK macro.

---

## Info

### 1. Performance consideration
The burst size bucketing logic (lines 247-257 in `rte_graph_worker_common.h`) uses a sequence of comparisons. For very hot paths, a lookup table might be slightly faster, but the current approach is clear and the overhead is only when `RTE_GRAPH_PROFILE` is enabled.

### 2. Documentation clarity
The documentation in `graph_lib.rst` could benefit from explaining what information the profiling provides and when to enable it (development vs production).

### 3. Statistics counter width
All statistics use `uint64_t` counters. On a very long-running system, these could theoretically overflow, but this is acceptable for profiling statistics (overflow would take years at typical cycle counts).

---

## Summary

The patch is well-structured and correctly implements optional profiling statistics. The main issue is the missing validation that `BURST_LARGE <= RTE_GRAPH_BURST_SIZE`. The code is otherwise correct with no resource leaks, use-after-free, or other correctness bugs identified.

**Recommendation:** Add the suggested static assertion for `BURST_LARGE` validation. The patch can proceed with that minor addition.