|WARNING| [v3] dts: add pvp multi paths vhost performance testsuite

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-09-29

# DPDK Patch Review

## Errors

### 1. Use-after-free potential in test parameterization
**File:** `TestSuite_pvp_multi_paths_vhost_single_core_perf.py:182-183`

The code modifies dictionary entries in `self.test_parameters` list during test execution (`params["measured_mpps"] = ...`, `params["performance_delta"] = ...`, `params["pass"] = ...`). These dictionaries are shared references to entries in `self.config.test_parameters`. If a test case is run multiple times (e.g., via test retry mechanisms), subsequent runs will see stale data from previous runs because the original config dictionaries have been mutated.

**Fix:** Create a deep copy of test parameters at the start of each test case:
```python
import copy

def _create_and_transmit(...) -> None:
    # At start of function:
    test_params = copy.deepcopy(self.test_parameters)
    
    for params in test_params:
        # ... existing code using params ...
    
    self._produce_stats_table(test_params)
    for params in test_params:
        verify(...)
```

### 2. Assertion failure does not clean up TestPmd instances
**File:** `TestSuite_pvp_multi_paths_vhost_single_core_perf.py:84, 179`

The `assert` statement at line 84 (`assert repetitions > 0`) occurs after TestPmd instances are started at lines 92-93. If this assertion fails (though unlikely in practice), the `vhost` and `virtio` TestPmd processes remain running because the context managers at lines 167-170 never exit via their `__exit__` methods. Similarly, line 179 has an assertion while TestPmd instances are active.

**Fix:** Move assertions outside the TestPmd context managers or replace with `verify()` which integrates with the test framework's cleanup:
```python
def _transmit(...) -> float:
    verify(repetitions > 0, "Invalid number of repetitions, must be greater than 0.")
    # ... packet construction ...
    
    vhost.start()
    virtio.start()
    # ... rest of code

# And at line 179:
verify(params["expected_mpps"] > 0, "Invalid expected mpps, must be greater than 0.")
params["measured_mpps"] = ...
```

### 3. Missing error handling for `assess_performance_by_packet()`
**File:** `TestSuite_pvp_multi_paths_vhost_single_core_perf.py:97-99`

The `assess_performance_by_packet()` function can fail (network issues, traffic generator errors, timeout). No error checking is performed on `stats` before accessing `stats.rx_pps`. If the traffic generator fails mid-test, this will result in an unhandled exception that may leave TestPmd instances and vhost sockets in an inconsistent state.

**Fix:** Check the return value and handle failures gracefully:
```python
stats = assess_performance_by_packet(packet=packet, duration=5)
if stats is None or not hasattr(stats, 'rx_pps'):
    raise RuntimeError("Traffic generation failed - unable to collect statistics")
rx_avg += stats.rx_pps
```

## Warnings

### 1. Missing release notes entry
**File:** Missing `doc/guides/rel_notes/release_*.rst`

This patch adds a new test suite but does not update the release notes. New test suites should be documented in the "Tested Platforms" or "Tests" section of the current release notes.

**Fix:** Add an entry to the current release notes file under an appropriate section.

### 2. Hardcoded magic numbers for lcore assignments
**File:** `TestSuite_pvp_multi_paths_vhost_single_core_perf.py:150, 162`

Lcore lists are hardcoded: `LogicalCoreList([1, 2])` for vhost and `LogicalCoreList([3, 4, 5])` for virtio. This assumes a specific system topology and will fail on systems where these cores are unavailable or assigned to other purposes.

**Suggestion:** Derive lcore assignments from `self._ctx.sut_node` available cores or make them configurable via test config:
```python
# In Config class:
vhost_lcores: list[int] = [1, 2]
virtio_lcores: list[int] = [3, 4, 5]

# In test:
vhost_params["lcore_list"] = LogicalCoreList(self.config.vhost_lcores)
virtio_params["lcore_list"] = LogicalCoreList(self.config.virtio_lcores)
```

### 3. `clean_up_vhost()` uses broad wildcard with `rm -rf`
**File:** `linux_session.py:465-466`

The command `rm -rf /tmp/vhost-net*` with wildcard expansion could unintentionally delete files if there are naming collisions (e.g., user creates `/tmp/vhost-network-config.txt`).

**Fix:** Use the exact socket path that was created:
```python
def clean_up_vhost(self) -> None:
    """Overrides :meth:`~.os_session.OSSession.clean_up_vhost`."""
    # Remove only the known socket file
    self.send_command("rm -f /tmp/vhost-net", privileged=True)
```

Or make the socket path a parameter to avoid hardcoding.

### 4. Inconsistent `extra_args` usage across test cases
**File:** Multiple test methods

Some test cases include `tx_offloads`, `enable_hw_vlan_strip`, and `rss` in `extra_args`, while others (e.g., `test_perf_vhost_single_core_virtio_vectorized`) omit them. This inconsistency makes it unclear whether the differences are intentional (testing different offload configurations) or accidental.

**Suggestion:** Document why each test case uses its specific parameter set, or standardize the base parameters if the variations are unintentional.

### 5. Missing functional test for `clean_up_vhost()`
**File:** `os_session.py:723-725`

The new abstract method `clean_up_vhost()` has no corresponding unit test or functional test to verify it correctly removes stale socket files without side effects.

**Fix:** Add a test case in the test suite (or a separate unit test) that verifies:
- Socket files are removed after test execution
- The method handles missing files gracefully (no error if socket doesn't exist)
- It doesn't remove unrelated files

### 6. Boolean integer parameters not using `bool` type
**File:** `TestSuite_pvp_multi_paths_vhost_single_core_perf.py:103-107`

Parameters `ring_format`, `in_order`, `buffers`, and `vectorized` are declared as `int` but only hold binary values (0 or 1). They should use `bool` type for clarity.

**Fix:**
```python
def _create_and_transmit(
    self,
    ring_format: bool,
    in_order: bool,
    buffers: bool,
    vectorized: bool,
    extra_args: TestPmdParamsDict,
) -> None:
    """...
    Args:
        ring_format: True for packed virtqueue (1), False for split (0).
        in_order: True to enable in-order packet processing.
        buffers: True to enable mergeable rx buffers.
        vectorized: True to enable SIMD vectorized data paths.
        ...
    """
    # Then convert to int when building the vdev string:
    virtio_user_vdev = VirtualDevice(
        f"net_virtio_user0,"
        f"...,packed_vq={int(ring_format)},"
        f"mrg_rxbuf={int(buffers)},"
        f"in_order={int(in_order)},"
        f"vectorized={int(vectorized)}"
    )
```

And update all call sites:
```python
self._create_and_transmit(
    ring_format=True,  # was: ring_format=1
    buffers=True,      # was: buffers=1
    ...
)
```

## Summary

**Critical issues:**
- Dictionary mutation causing stale data in repeated test runs (Error #1)
- Missing error handling for traffic generation failure (Error #3)

**Important improvements:**
- Replace assertions inside TestPmd context managers with `verify()` (Error #2)
- Add release notes entry (Warning #1)
- Make lcore assignments configurable (Warning #2)

**Code quality:**
- Use `bool` type for binary flag parameters (Warning #6)
- Improve `clean_up_vhost()` implementation to avoid wildcard risks (Warning #3)
- Add tests for new infrastructure methods (Warning #5)

The patch provides good test coverage for vhost performance across multiple virtio configurations. The primary concerns are around error handling and resource cleanup to ensure test reliability.