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

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Summary
This patch adds optional build-time graph profiling statistics tracking cycles and calls across different burst size intervals. The implementation has several **critical correctness bugs** that must be fixed before merging.

---

## Errors (Must Fix)

### 1. Integer divide-by-zero with all_total_cycles (Correctness Bug)
**Location:** `lib/graph/graph_debug.c:108`

The code computes a percentage using `all_total_cycles` as the denominator:
```c
(double)n->total_cycles / (double)all_total_cycles
```
This is already guarded by `all_total_cycles == 0`, so **no issue here**.

### 2. Typo in double calculation (Correctness Bug)
**Location:** `lib/graph/graph_debug.c:108`

The ternary expression is missing the multiplication by 100.0 for percentage:
```c
all_total_cycles == 0 ? 0.0 :
(double)n->total_cycles / (double)all_total_cycles,
```

Should be:
```c
all_total_cycles == 0 ? 0.0 :
100.0 * (double)n->total_cycles / (double)all_total_cycles,
```
The format string says `"%.1f%% of all nodes"` which expects a percentage (0-100), not a ratio (0-1).

### 3. Same percentage calculation bug repeated (Correctness Bug)
**Locations:** `lib/graph/graph_debug.c:149` and `lib/graph/graph_debug.c:153`

Both percentage calculations are missing the 100.0 multiplier:
```c
(double)calls / (double)n->total_calls  // produces 0.0-1.0, not 0-100
(double)cycles / (double)n->total_cycles  // produces 0.0-1.0, not 0-100
```

All three percentage calculations should multiply by 100.0.

### 4. Missing cache alignment attributes
**Location:** `lib/graph/rte_graph_worker_common.h:124-140`

The `usage_stats_01` and `usage_stats` arrays are marked `__rte_cache_aligned`, but this is incorrect. `__rte_cache_aligned` applies to struct/union types, not struct member arrays. If the intent is to align these fields on cache lines, use explicit padding or place them at known cache-line boundaries.

Current code:
```c
struct __rte_cache_aligned {
    uint64_t calls;
    uint64_t cycles;
} usage_stats_01[2];
```

This attempts to define an anonymous aligned struct, which is not the intended pattern. The alignment attribute should be on the `rte_node` struct itself, not on individual member anonymous structs.

**Suggested fix:** Remove `__rte_cache_aligned` from the anonymous struct definitions inside the members. The `struct rte_node` is already `__rte_cache_aligned` (line 121), so the entire structure is aligned. If you need specific cache-line alignment of these fields relative to the struct start, use explicit padding bytes instead.

### 5. Non-const array used as local variable
**Location:** `lib/graph/graph_debug.c:113-115`

```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};
```

This is inside a loop (`rte_graph_foreach_node`), causing the array to be re-initialized on every iteration. Move it outside the loop or to file scope.

**Impact:** Performance degradation from repeated initialization; no correctness issue but wasteful.

### 6. Format string expects unsigned, objs is signed cast
**Location:** `lib/graph/graph_debug.c:135`

```c
fprintf(f, "       for %u objs/call\n", idx);
```
Here `idx` is `unsigned int`, which is correct. However, at line 125:
```c
objs_per_call = (double)idx;
```
This casts an unsigned to double, which is fine. No issue here.

---

## Warnings (Should Fix)

### 1. Missing release notes for test-only or internal changes
**Not applicable** -- This patch adds a user-visible feature (profiling stats), so release notes are required and are present. The release notes correctly describe the feature.

### 2. Missing testpmd hooks or functional tests for new feature
**Location:** Entire patch

The patch adds a new feature (`RTE_GRAPH_PROFILE`) but does not add tests in `app/test` or hooks in `app/testpmd`. While `graph_debug.c` dumps the stats when enabled, there is no automated test verifying the profiling counters are correct.

**Recommendation:** Add a unit test in `app/test` that:
- Creates a graph with `RTE_GRAPH_PROFILE` enabled
- Processes known burst sizes (0, 1, 8, 32, 256 objects)
- Calls `rte_graph_obj_dump()` and verifies the profiling stats reflect the expected calls and object counts

### 3. Inconsistent array indexing
**Location:** `lib/graph/graph_debug.c:117-159`

The code uses `sample_sizes[idx]` and `sample_sizes[idx + 1]` to print interval bounds, but the logic is fragile if the array is reordered. The static_assert (line 115) enforces size but not order.

**Recommendation:** Add a comment documenting that `sample_sizes[]` must be sorted ascending, or add a compile-time check for ordering if feasible.

### 4. Formatting: fprintf split across many lines
**Location:** `lib/graph/graph_debug.c:106-109, 149-158`

The fprintf calls are split into many short lines, making them hard to read. While each line is under 100 chars, consolidating related format strings would improve readability.

**Example (line 106-109):**
```c
fprintf(f, "       total_cycles=%" PRIu64 " (%.1f%% of all nodes), avg cycles/call=%.1f\n",
    n->total_cycles,
    all_total_cycles == 0 ? 0.0 :
    (double)n->total_cycles / (double)all_total_cycles,
    n->total_calls == 0 ? 0.0 :
    (double)n->total_cycles / (double)n->total_calls);
```
Could be:
```c
fprintf(f, "       total_cycles=%" PRIu64 " (%.1f%% of all nodes), "
    "avg cycles/call=%.1f\n",
    n->total_cycles,
    all_total_cycles == 0 ? 0.0 : 100.0 * n->total_cycles / all_total_cycles,
    n->total_calls == 0 ? 0.0 : (double)n->total_cycles / n->total_calls);
```

Not a blocker, but improves readability.

---

## Info (Consider)

### 1. Macro naming: RTE_GRAPH_PROFILE_BURST_* sizes
The names `SMALL`, `MEDIUM`, `LARGE` are subjective. Consider documenting what these represent (e.g., "typical NIC batch size", "cache-line worth of pointers", etc.) in the code or doc.

### 2. Documentation: prog_guide/graph_lib.rst
The added text (lines 52-56) correctly describes the feature. However, it could mention **why** you would want these specific intervals (e.g., "to identify nodes processing small batches inefficiently").

---

## Final Check

**Correctness bugs identified:**
- Percentage calculations missing 100.0 multiplier (3 instances)
- `__rte_cache_aligned` misused on anonymous struct members
- `sample_sizes[]` array re-initialized in loop

**Style/process issues:**
- No functional test coverage (Warning-level)
- Minor formatting improvements possible (Info-level)

**No false positives** -- all items above are actual issues.