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

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Patch Summary
This patch adds optional graph node profiling statistics that can be enabled via `RTE_GRAPH_PROFILE` in `rte_config.h`. It tracks cycles spent processing different burst size intervals per node.

---

## Errors

### 1. Potential uninitialized variable access in graph_debug.c

**Location:** `lib/graph/graph_debug.c:106`

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

If `rte_graph_has_stats_feature()` returns false in the first loop (lines 76-80) but true in the second loop (line 105), `all_total_cycles` is used uninitialized (still 0 from line 63, but not intentionally computed).

**Fix:** Initialize `all_total_cycles = 0` unconditionally is correct, but the logic should ensure both loops see the same feature state, or compute `all_total_cycles` unconditionally:

```c
uint64_t all_total_cycles = 0;

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

Remove the `if (rte_graph_has_stats_feature())` guard around the first loop (lines 76-80). The `total_cycles` field exists regardless; the guard is redundant.

### 2. Non-const static array in hot path

**Location:** `lib/graph/rte_graph_worker_common.h:114`

```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 array is defined inside `rte_graph_obj_dump()` (not the hot path), so this is acceptable. However, in `__rte_node_process()` (lines 232-237), the static_assert checks reference the constants directly, which is correct. No issue here.

**Correction:** After re-reading, the `sample_sizes` array is only in `graph_debug.c:114` (dump function), not in the fast path. This is fine. Disregard this item.

---

## Warnings

### 1. Missing error propagation context in documentation

**Location:** `doc/guides/prog_guide/graph_lib.rst:52-56`

The documentation states that the feature is "build-time configurable" but does not mention that it is **disabled by default** (which is stated in the release notes). For consistency and user clarity, mention the default state in the prog_guide as well.

**Suggested addition:**

```rst
- Enable the ``RTE_GRAPH_PROFILE`` config option for more profiling details
  (disabled by default).
```

### 2. RTE_BUILD_BUG_ON disabled under RTE_GRAPH_PROFILE

**Location:** `lib/graph/node.c:113-117`

```c
#ifndef RTE_GRAPH_PROFILE
    /* Limit Node specific metadata to one cacheline on 64B CL machine */
    RTE_BUILD_BUG_ON((offsetof(struct rte_node, nodes) -
              offsetof(struct rte_node, ctx)) !=
             RTE_CACHE_LINE_MIN_SIZE);
#endif
```

When `RTE_GRAPH_PROFILE` is enabled, the structure grows beyond one cacheline, and the build-time check is disabled. This is intentional, but the comment should clarify **why** it's acceptable to exceed one cacheline when profiling is enabled (profiling trades space for observability). Consider adding a comment:

```c
#ifndef RTE_GRAPH_PROFILE
    /* Limit Node specific metadata to one cacheline on 64B CL machine.
     * When profiling is enabled, the structure is intentionally larger
     * to accommodate profiling stats.
     */
    RTE_BUILD_BUG_ON(...);
#endif
```

### 3. Usage stats not zeroed on node reset

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

The new `usage_stats_01` and `usage_stats` arrays are added to `struct rte_node`, but there is no evidence in the patch that these are zeroed when a node is initialized or reset. Verify that `node.c` or the graph creation path zero-initializes these fields. If using `rte_zmalloc_socket()` for node allocation, this is safe; if not, an explicit `memset()` or zeroing loop may be required.

**Action:** Confirm that node structures are allocated with `rte_zmalloc_socket()` or explicitly zeroed. If not, add initialization code.

---

## Info

### 1. Double computation of percentage in nested fprintf

**Location:** `lib/graph/graph_debug.c:148-151`

The percentage computation `(double)calls / (double)n->total_calls * 100.0` is repeated in the same block. For readability, consider computing once and storing in a `double calls_pct` variable.

**Suggested refactor:**

```c
double calls_pct = n->total_calls == 0 ? 0.0 :
    (double)calls / (double)n->total_calls * 100.0;
fprintf(f, " (%.1f%% of this node)\n", calls_pct);
```

### 2. Histogram bucket boundaries hardcoded in three places

**Location:** `rte_graph_worker_common.h:232-244`, `graph_debug.c:114-115`

The burst size thresholds (`BURST_SMALL`, `BURST_MEDIUM`, `BURST_LARGE`) are used in both the hot path (`__rte_node_process`) and the dump function. If these thresholds change, both locations must be updated. Consider centralizing the threshold logic (e.g., a macro or inline function that maps `rc` to a histogram index).

**Example:**

```c
static inline unsigned int
__rte_node_usage_bucket(uint16_t rc)
{
    if (rc < 2) return rc;
    if (rc < RTE_GRAPH_PROFILE_BURST_SMALL) return 2;
    if (rc < RTE_GRAPH_PROFILE_BURST_MEDIUM) return 3;
    if (rc < RTE_GRAPH_PROFILE_BURST_LARGE) return 4;
    return 5;
}
```

This is a maintainability suggestion, not a correctness issue.

---

## Summary

**Correctness:** One potential issue (all_total_cycles guard redundancy) requiring verification.

**Style/Process:** No violations. The code follows DPDK style, uses appropriate `static_assert` checks, and documents the feature in both prog_guide and release notes.

**Documentation:** Minor omission (default state not mentioned in prog_guide).

**Testing:** The patch does not add tests. New profiling stats should be exercised in `app/test` (e.g., verify that counters increment, histogram buckets populate correctly). This is required per the guidelines for new functionality.