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

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Summary
This patch adds optional graph node profiling statistics to track cycles spent processing various burst size intervals. The feature is build-time configurable via `RTE_GRAPH_PROFILE`.

---

## Errors

### 1. Integer type mismatch in array size assertion (Correctness)

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

```c
static_assert(RTE_DIM(sample_sizes) == 2 + RTE_DIM(n->usage_stats),
		"usage_stats array size mismatch");
```

**Issue:** `RTE_DIM()` returns `size_t` (unsigned), but the comparison involves signed integer arithmetic. On some platforms where `size_t` is larger than `int`, this could produce unexpected results.

**Fix:** Use explicit cast or ensure both sides are the same type:
```c
static_assert(RTE_DIM(sample_sizes) == 2U + RTE_DIM(n->usage_stats),
		"usage_stats array size mismatch");
```

### 2. Missing bounds validation for profile burst size configuration (Correctness)

**Location:** `config/rte_config.h:109-111`

**Issue:** No validation that `RTE_GRAPH_PROFILE_BURST_SMALL < RTE_GRAPH_PROFILE_BURST_MEDIUM < RTE_GRAPH_PROFILE_BURST_LARGE`. If configured incorrectly, the interval classification in `__rte_node_process()` will produce wrong statistics without any compile-time or runtime error.

**Fix:** Add static assertions in `rte_graph_worker_common.h`:
```c
#ifdef RTE_GRAPH_PROFILE
static_assert(RTE_GRAPH_PROFILE_BURST_SMALL < RTE_GRAPH_PROFILE_BURST_MEDIUM,
	"BURST_SMALL must be less than BURST_MEDIUM");
static_assert(RTE_GRAPH_PROFILE_BURST_MEDIUM < RTE_GRAPH_PROFILE_BURST_LARGE,
	"BURST_MEDIUM must be less than BURST_LARGE");
static_assert(RTE_GRAPH_PROFILE_BURST_LARGE <= RTE_GRAPH_BURST_SIZE,
	"BURST_LARGE cannot exceed RTE_GRAPH_BURST_SIZE");
#endif
```

---

## Warnings

### 1. Implicit boolean comparison (Style)

**Location:** `lib/graph/graph_debug.c:76,107`

**Issue:** DPDK style requires explicit comparison for integers used as booleans.

**Current:**
```c
if (rte_graph_has_stats_feature()) {
```

**Should be:**
```c
if (rte_graph_has_stats_feature() != 0) {
```

**Note:** If `rte_graph_has_stats_feature()` returns `bool`, the current form is acceptable. Only flag if it returns `int`.

### 2. Division by zero checks rely on ternary without explicit validation comment (Code clarity)

**Location:** `lib/graph/graph_debug.c:86-90, 108-110`

**Issue:** Multiple division-by-zero guards using ternary operators. While correct, the pattern is dense and repeated. Consider a helper macro for readability.

**Suggestion:**
```c
#define SAFE_PERCENT(num, denom) \
	((denom) == 0 ? 0.0 : ((double)(num) / (double)(denom) * 100.0))

fprintf(f, "       total_cycles=%" PRIu64 " (%.1f%% of all nodes), avg cycles/call=%.1f\n",
	n->total_cycles,
	SAFE_PERCENT(n->total_cycles, all_total_cycles),
	n->total_calls == 0 ? 0.0 : (double)n->total_cycles / (double)n->total_calls);
```

This is a suggestion, not a requirement.

### 3. Cache alignment on small structures may waste memory (Performance consideration)

**Location:** `lib/graph/rte_graph_worker_common.h:125-140`

**Issue:** Each `usage_stats_01` entry and `usage_stats` entry is marked `__rte_cache_aligned`, forcing 64-byte alignment per structure instance. For the small 2-element `usage_stats_01` array, this wastes considerable memory.

**Consideration:** Remove `__rte_cache_aligned` from individual array entries unless profiling shows false sharing issues. The outer `struct rte_node` is already cache-aligned.

---

## Info

### 1. Comment states feature is disabled by default

**Location:** `doc/guides/rel_notes/release_26_11.rst:61`

The documentation correctly notes the feature is "disabled by default," which matches `config/rte_config.h:109` where `RTE_GRAPH_PROFILE` is commented out.

### 2. Static assertion correctly matches array dimensions

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

The assertion `RTE_DIM(sample_sizes) == 2 + RTE_DIM(n->usage_stats)` correctly validates that `sample_sizes[]` has 6 elements (0, 1, 2, SMALL, MEDIUM, LARGE) to match `usage_stats_01[2]` (2 elements) + `usage_stats[4]` (4 elements). The logic is sound aside from the signedness issue flagged above.

### 3. Fast path impact assessment

When `RTE_GRAPH_PROFILE` is enabled, `__rte_node_process()` adds a conditional ladder with up to 5 comparisons per call. This is acceptable for profiling builds but confirms the feature should remain off by default for production.

---

## Summary of Required Changes

1. **Error #1:** Add `U` suffix to literal in `static_assert` to ensure unsigned comparison
2. **Error #2:** Add static assertions validating `RTE_GRAPH_PROFILE_BURST_*` ordering

The warnings are style suggestions that improve consistency but are not blocking.