|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: [PATCH v17] graph: add optional profiling stats

### Summary
This patch adds optional graph node profiling statistics, configurable at build time via `RTE_GRAPH_PROFILE`. The feature tracks cycles spent processing various burst size ranges per node.

---

## ERRORS

### 1. Statistics accumulation using assignment instead of increment

**File:** `lib/graph/rte_graph_worker_common.h`  
**Lines:** 245-247

```c
node->usage_stats_01[rc].calls++;
node->usage_stats_01[rc].cycles += cycles;
```

and

```c
node->usage_stats[index].calls++;
node->usage_stats[index].cycles += cycles;
node->usage_stats[index].objs += rc;
```

**Issue:** While these lines correctly use `+=`, there is a **missing initialization** of these statistics arrays.
The `usage_stats_01` and `usage_stats` fields are declared but never explicitly zeroed.
Without zero-initialization, the first `+=` operates on uninitialized memory, producing wrong totals.

**Why it matters:** Uninitialized statistics fields will contain garbage values, causing all profiling data to be incorrect from the start of graph execution.

**Fix:** In `lib/graph/node.c` or wherever the `rte_node` structure is allocated/initialized, add:

```c
#ifdef RTE_GRAPH_PROFILE
memset(node->usage_stats_01, 0, sizeof(node->usage_stats_01));
memset(node->usage_stats, 0, sizeof(node->usage_stats));
#endif
```

Or use zero-initializing allocation (`rte_zmalloc_socket`) for the node structure.

---

### 2. Division by zero in release build with stats disabled at runtime

**File:** `lib/graph/graph_debug.c`  
**Lines:** 103-108

```c
if (rte_graph_has_stats_feature())
    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 * 100.0,
        n->total_calls == 0 ? 0.0 :
        (double)n->total_cycles / (double)n->total_calls);
```

**Issue:** When `RTE_LIBRTE_GRAPH_STATS` is disabled (stats feature off), `total_calls` and `total_cycles` are not updated.
The code guards the fprintf with `if (rte_graph_has_stats_feature())`, which is correct.
However, the calculation `(double)n->total_cycles / (double)n->total_calls` occurs **inside the fprintf** even when stats are disabled -- the ternary protects against division by zero only when `total_calls == 0`, but with stats disabled these fields may be uninitialized.

Actually, on closer inspection: if `rte_graph_has_stats_feature()` returns false, this fprintf is **not executed**.
So this is **not a bug** -- the guard prevents the uninitialized read.

**Correction:** No issue here.

---

### 3. `all_total_cycles` accumulation loop iterates over all nodes

**File:** `lib/graph/graph_debug.c`  
**Lines:** 76-80

```c
rte_graph_foreach_node(count, off, g, n) {
    if (!all && n->idx == 0)
        continue;
    all_total_cycles += n->total_cycles;
}
```

**Issue:** This loop sums `total_cycles` across all nodes (respecting the `all` flag).
However, if `RTE_LIBRTE_GRAPH_STATS` is disabled, `total_cycles` is uninitialized.
The code does not guard this loop with `if (rte_graph_has_stats_feature())`.

**Why it matters:** Reading uninitialized `total_cycles` values when stats are disabled will produce garbage sums, and later print garbage percentages.

**Fix:** Guard the entire `all_total_cycles` calculation:

```c
if (rte_graph_has_stats_feature()) {
    rte_graph_foreach_node(count, off, g, n) {
        if (!all && n->idx == 0)
            continue;
        all_total_cycles += n->total_cycles;
    }
}
```

---

## WARNINGS

### 1. Missing zero-initialization for profiling stats

**File:** `lib/graph/rte_graph_worker_common.h`  
**Lines:** 124-142

The `usage_stats_01` and `usage_stats` arrays are declared but not initialized.
While the code uses `+=` (correct for accumulation), the arrays are never zeroed before first use.

This is the same issue as Error #1 above, so flagging it as a warning here would be redundant.

**Correction:** Already covered in Error #1.

---

### 2. Cache alignment directive placement

**File:** `lib/graph/rte_graph_worker_common.h`  
**Line:** 126

```c
alignas(RTE_CACHE_LINE_MIN_SIZE)
struct {
    uint64_t calls;
    uint64_t cycles;
} usage_stats_01[2];
```

**Issue:** The `alignas` applies to the **array** `usage_stats_01`, not to the struct type.
This is actually correct C11 syntax for aligning the array itself, not individual elements.
However, the comment in the code says "array should be aligned, not struct" which matches this.

**Correction:** No issue here -- the alignment is correct as written.

---

### 3. Documentation clarity

**File:** `doc/guides/prog_guide/graph_lib.rst`  
**Lines:** 52-57

The documentation says "sample specific burst size ranges" but does not explain what these ranges mean or how they map to the three `BURST_*` config options.

**Suggestion:** Add a brief explanation, e.g.:

```rst
- ``RTE_GRAPH_PROFILE_BURST_SMALL`` - upper edge of the small burst range
- ``RTE_GRAPH_PROFILE_BURST_MEDIUM`` - upper edge of the medium burst range  
- ``RTE_GRAPH_PROFILE_BURST_LARGE`` - upper edge of the large burst range
  Profiling tracks cycles spent processing bursts in four ranges:
  micro (2 to SMALL-1), small (SMALL to MEDIUM-1), medium (MEDIUM to LARGE-1),
  and large (LARGE+).
```

---

## REVIEW SUMMARY

**Critical issues:**
1. Uninitialized profiling statistics arrays (`usage_stats_01`, `usage_stats`) -- must zero-initialize
2. `all_total_cycles` accumulation reads uninitialized data when stats are disabled -- must guard with `if (rte_graph_has_stats_feature())`

**Other findings:**
- Documentation could be clearer on the meaning of the `BURST_*` config options

The rest of the patch follows DPDK coding style and correctly uses build-time conditionals, static assertions, and explicit comparisons. The profiling logic itself is sound once the initialization issue is fixed.