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

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-07-03

# DPDK Patch Review

## Summary
This patch adds optional graph node profiling statistics. Overall quality is good with well-structured code. Found one **Error** (resource management), several **Warnings** (documentation and code quality), and multiple **Info** items (style preferences).

---

## Errors

### 1. Missing bounds check on `RTE_GRAPH_PROFILE_BURST_SIZE`

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

The code assumes `RTE_GRAPH_PROFILE_BURST_SIZE < RTE_GRAPH_BURST_SIZE` but does not verify this at compile time. If a user misconfigures `rte_config.h` with `RTE_GRAPH_PROFILE_BURST_SIZE >= RTE_GRAPH_BURST_SIZE`, the profiling logic becomes incorrect: a burst of exactly `RTE_GRAPH_BURST_SIZE` objects would increment both `usage_stats[2]` (the profile sample size) and `usage_stats[3]` (the max burst size), or worse, only increment `usage_stats[2]` if the comparisons are ordered differently. This produces incorrect statistics.

**Suggested fix:**

Add a compile-time assertion in `rte_graph_worker_common.h` after the `usage_stats` definition:

```c
#ifdef RTE_GRAPH_PROFILE
	struct __rte_cache_aligned {
		uint64_t calls;
		uint64_t cycles;
	} usage_stats[4];

	static_assert(RTE_GRAPH_PROFILE_BURST_SIZE < RTE_GRAPH_BURST_SIZE,
		"RTE_GRAPH_PROFILE_BURST_SIZE must be less than RTE_GRAPH_BURST_SIZE");
	static_assert(RTE_GRAPH_PROFILE_BURST_SIZE > 1,
		"RTE_GRAPH_PROFILE_BURST_SIZE must be greater than 1");
#endif
```

---

## Warnings

### 1. Inconsistent formatting in release notes

**Location:** `doc/guides/rel_notes/release_26_07.rst:86`

The release note body indents with 2 spaces per the existing file, but new text should match surrounding entries' indentation style.

**Suggested change:**

Verify indentation matches other entries in the file (typically 2 spaces for description text after the `*` bullet).

---

### 2. `RTE_GRAPH_PROFILE_BURST_SIZE` should have a validity range documented

**Location:** `config/rte_config.h:109`, `doc/guides/prog_guide/graph_lib.rst:52`

The configuration option `RTE_GRAPH_PROFILE_BURST_SIZE` has no documented valid range or constraints. Users might set it to 0, 1, or >= `RTE_GRAPH_BURST_SIZE`, all of which break the profiling logic or waste histogram entries.

**Suggested fix:**

Add a comment in `config/rte_config.h`:

```c
/* RTE_GRAPH_PROFILE is not set */
/* RTE_GRAPH_PROFILE_BURST_SIZE: sample size between 2 and RTE_GRAPH_BURST_SIZE-1 */
#define RTE_GRAPH_PROFILE_BURST_SIZE 32
```

And update `graph_lib.rst` to document the constraint:

```rst
- Enable the ``RTE_GRAPH_PROFILE`` config option for more profiling details.
  Set the ``RTE_GRAPH_PROFILE_BURST_SIZE`` config option (must be greater than 1
  and less than ``RTE_GRAPH_BURST_SIZE``) to sample a specific burst size.
```

---

### 3. Variable initialization hides potential compiler warnings

**Location:** `lib/graph/graph_debug.c:102-104`

Variables `calls`, `cycles`, and `objs_per_call` are declared at the top of the loop and initialized in every branch. This defeats compiler "used-before-initialized" warnings if a code path fails to assign them.

**Suggested fix:**

Move declarations to point of first assignment:

```c
for (int idx = 0; idx < RTE_DIM(n->usage_stats) + 1; idx++) {
	if (idx < RTE_DIM(n->usage_stats)) {
		uint16_t idx_objs = profile_sample_sizes[idx];
		fprintf(f, "       objs[%u]\n", idx_objs);
		uint64_t calls = n->usage_stats[idx].calls;
		uint64_t cycles = n->usage_stats[idx].cycles;
		double objs_per_call = (double)idx_objs;
		/* ... */
	} else {
		fprintf(f, "       objs[other]\n");
		if (calls_other > 0 && cycles_other > 0 && objs_other > 0) {
			uint64_t calls = calls_other;
			uint64_t cycles = cycles_other;
			double objs_per_call = /* ... */;
			/* ... */
		}
	}
}
```

Or restructure to eliminate the "other" entry logic from the main loop.

---

### 4. Documentation does not explain "objs[other]" histogram entry

**Location:** `doc/guides/prog_guide/graph_lib.rst:52-54`, release notes

The documentation mentions sampling at 0, 1, `RTE_GRAPH_PROFILE_BURST_SIZE`, and `RTE_GRAPH_BURST_SIZE`, but the debug output includes an `objs[other]` entry for all other burst sizes. Users may be confused about what this represents.

**Suggested addition to `graph_lib.rst`:**

```rst
When enabled, the profiler tracks cycles spent processing bursts of 0, 1,
``RTE_GRAPH_PROFILE_BURST_SIZE``, and ``RTE_GRAPH_BURST_SIZE`` objects per node.
Bursts of other sizes are accumulated in the ``objs[other]`` histogram entry.
```

And update the release note to match.

---

### 5. `profile_sample_sizes` array could be `const` at file scope

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

The array `profile_sample_sizes` is declared `static const` inside the function. Moving it to file scope (or just above the function) would make it clear that it's a compile-time constant, allow the compiler to place it in `.rodata`, and avoid redeclaring it on every call.

**Suggested change:**

Move before `rte_graph_obj_dump()`:

```c
#ifdef RTE_GRAPH_PROFILE
static const uint16_t profile_sample_sizes[] = {
	0, 1, RTE_GRAPH_PROFILE_BURST_SIZE, RTE_GRAPH_BURST_SIZE
};
static_assert(RTE_DIM(profile_sample_sizes) == 4,
	"usage_stats array size mismatch");
#endif
```

Then use it in the function without redeclaring.

---

## Info

### 1. Consider `bool` for conditional flags

**Location:** `lib/graph/graph_debug.c:97` (and elsewhere using `rte_graph_has_stats_feature()`)

The `rte_graph_has_stats_feature()` appears to be a predicate function. If it returns `int` (0/1), consider changing it to `bool` for clarity. (If it's already `bool`, this is fine.)

---

### 2. Explicit comparison in statistics check

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

```c
if (rte_graph_has_stats_feature())
```

If `rte_graph_has_stats_feature()` returns `int`, DPDK style prefers explicit comparison:

```c
if (rte_graph_has_stats_feature() != 0)
```

However, if the function returns `bool`, the current form is acceptable per the guidelines.

---

### 3. Loop index increment could be more idiomatic

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

Looping over `RTE_DIM(n->usage_stats) + 1` to handle both histogram entries and the "other" entry is non-idiomatic. Consider separating the histogram loop from the "other" entry handling:

```c
for (int idx = 0; idx < RTE_DIM(n->usage_stats); idx++) {
	/* print histogram entry */
}
/* then handle "other" separately */
if (calls_other > 0) {
	fprintf(f, "       objs[other]\n");
	/* ... */
}
```

This makes the control flow clearer.

---

### 4. Variable shadowing in nested scope

**Location:** `lib/graph/graph_debug.c:102-139`

If the suggestion in Warning #3 is applied, `calls`, `cycles`, and `objs_per_call` would be declared in both branches of the `if`/`else`, shadowing each other. This is acceptable C but can confuse readers. Consider distinct names or a refactored structure.

---

### 5. Formatting: space before array subscript

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

```c
uint16_t idx_objs = profile_sample_sizes[idx];
```

DPDK style generally does not require a space before `[` in array subscripts. The current code is acceptable.

---

### 6. Consider consolidating redundant `fprintf` calls

**Location:** `lib/graph/graph_debug.c:134-140`

The logic building up a single line of output across multiple `fprintf` calls could be consolidated into a single formatted string for clarity:

```c
if (calls != 0 && objs_per_call != 0.0) {
	fprintf(f, "         calls=%" PRIu64 ", cycles=%" PRIu64
		", avg cycles/call=%.1f, avg cycles/obj=%.1f\n",
		calls, cycles,
		(double)cycles / (double)calls,
		(double)cycles / (double)calls / objs_per_call);
} else if (calls != 0) {
	fprintf(f, "         calls=%" PRIu64 ", cycles=%" PRIu64
		", avg cycles/call=%.1f\n",
		calls, cycles, (double)cycles / (double)calls);
} else {
	fprintf(f, "         calls=0\n");
}
```

This avoids the incremental line-building pattern.

---

### 7. Division by zero safety

**Location:** `lib/graph/graph_debug.c:137-140`

The code checks `calls != 0` before dividing by `calls`, which is correct. The additional check for `objs_per_call != 0.0` prevents division by zero in the cycles/obj calculation. This is good defensive programming.

---

### 8. `static_assert` in function body

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

C11 allows `static_assert` in function bodies, but placing it at file scope (or with the `profile_sample_sizes` array as suggested in Warning #5) would be more conventional.

---

## Summary of Findings

- **1 Error**: Missing compile-time constraint on `RTE_GRAPH_PROFILE_BURST_SIZE`
- **5 Warnings**: Documentation gaps, unvalidated config, variable initialization pattern, missing explanation of "other" entry, array scope
- **8 Info**: Code style preferences, minor refactoring suggestions

**Overall:** The patch is well-implemented with good profiling logic. The main correctness issue is the lack of validation that `RTE_GRAPH_PROFILE_BURST_SIZE` is in a valid range relative to `RTE_GRAPH_BURST_SIZE`. The warnings are primarily documentation and code clarity improvements.