|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

## Summary
This patch adds a new test suite for PVP (Physical-Virtual-Physical) vhost single-core performance testing. The code is primarily test infrastructure written in Python for the DTS framework.

## Errors

### Resource Leak - Missing Error Path Cleanup
**File:** `dts/framework/testbed_model/linux_session.py` and `os_session.py`

The `clean_up_vhost()` method calls `send_command("rm -rf /tmp/vhost-net*", privileged=True)` but does not check the return value. While `rm -rf` typically does not fail, if the command execution itself fails (e.g., permission denied despite `privileged=True`, session disconnected), the error is silently ignored. This could leave stale socket files that cause subsequent test runs to fail mysteriously.

**Suggested fix:**
```python
def clean_up_vhost(self) -> None:
    """Overrides :meth:`~.os_session.OSSession.clean_up_vhost`."""
    result = self.send_command("rm -rf /tmp/vhost-net*", privileged=True)
    # Consider verifying result.return_code == 0 or logging if non-zero
```

Note: This may be acceptable if `send_command` with `privileged=True` is documented to always succeed or raise an exception on failure. If that's the case, this is not an error.

---

## Warnings

### Incomplete Implementation of Abstract Method
**File:** `dts/framework/testbed_model/os_session.py`

The new abstract method `clean_up_vhost()` is added to `OSSession` base class but only implemented in `LinuxSession`. Other OS session implementations (if any exist, such as `WindowsSession`) will now fail at instantiation because the abstract method is not implemented. This breaks polymorphism guarantees.

**Suggested fix:**
Either:
1. Implement the method in all OS session subclasses (with no-op for non-Linux if appropriate), or
2. Make it non-abstract and provide a default implementation that does nothing, or
3. Document that this is Linux-only and add a runtime check

---

### Documentation Does Not Match Code
**File:** `dts/tests/TestSuite_pvp_multi_paths_vhost_single_core_perf.py`

The docstring for `_transmit()` states:
> "Returns: The MPPS (millions of packets per second) forwarded by the SUT."

However, the function computes an *average* MPPS over `repetitions` runs, not the MPPS from a single run. The docstring should clarify this:

**Suggested fix:**
```python
"""Transmit packets using testpmd instances and compute average MPPS.

Args:
    vhost: the running vhost testpmd shell.
    virtio: the running virtio testpmd shell.
    frame_size: The size of the frame to transmit.
    repetitions: The number of times to rerun the transmission.

Returns:
    The average MPPS (millions of packets per second) forwarded by the SUT
    across all repetitions.
"""
```

---

### Missing Test in Functional Test Suite
**File:** `dts/tests/TestSuite_pvp_multi_paths_vhost_single_core_perf.py`

The guidelines state:
> "New API functions must have hooks in `app/testpmd` and tests in the functional test suite"

While this is a performance test suite (not new API), the new `clean_up_vhost()` method in the framework is a new API for OS sessions and has no unit test or functional test coverage. Consider adding a test that verifies the method successfully removes socket files.

---

### Assertion on User-Provided Configuration
**File:** `dts/tests/TestSuite_pvp_multi_paths_vhost_single_core_perf.py`, line 175

```python
assert params["expected_mpps"] > 0, "Invalid expected mpps, must be greater than 0."
```

This assertion will crash the test run if the user provides invalid configuration. While configuration errors should fail fast, using an assertion here produces an unhelpful stack trace instead of a clear test failure message. 

**Suggested fix:**
```python
verify(
    params["expected_mpps"] > 0,
    f"Invalid test configuration: expected_mpps must be greater than 0, got {params['expected_mpps']}"
)
```

Similarly for line 83:
```python
verify(
    repetitions > 0,
    f"Invalid repetitions parameter: must be greater than 0, got {repetitions}"
)
```

---

### Missing Release Notes
**File:** Documentation

The patch adds new OS session methods (`clean_up_vhost()`) and a new test suite, but does not update release notes. While test-only changes do not require release notes according to the guidelines, the new framework method `clean_up_vhost()` is an API change to the testbed model and should be documented.

**Suggested action:**
Add a note to the current release notes file (e.g., `doc/guides/rel_notes/release_25_03.rst` or equivalent) mentioning:
- New `clean_up_vhost()` method in OS session API
- New PVP vhost single-core performance test suite

---

## Info

### Hardcoded Path `/tmp/vhost-net*`
**File:** Multiple locations

The vhost socket path `/tmp/vhost-net` is hardcoded in three places:
1. `linux_session.py` cleanup command
2. `VirtualDevice` definitions (twice) in test suite

Consider making this a constant at module or class level to ensure consistency and ease future changes:

```python
VHOST_SOCKET_PATH = "/tmp/vhost-net"
```

Then use it in all three locations.

---

### Consider Using Context Manager for Cleanup
**File:** `dts/tests/TestSuite_pvp_multi_paths_vhost_single_core_perf.py`

The `set_up_test_case()` method calls `clean_up_vhost()` before each test. If a test crashes or raises an exception partway through, the socket files may be left behind. Consider also cleaning up in a `tear_down_test_case()` method to ensure cleanup happens even on failure.

---

### Type Annotation Could Be More Specific
**File:** `dts/tests/TestSuite_pvp_multi_paths_vhost_single_core_perf.py`

The `test_parameters` type is `list[dict[str, int | float]]`, but the actual structure is more specific (known keys: `frame_size`, `num_descriptors`, `expected_mpps`, and dynamically added `measured_mpps`, `performance_delta`, `pass`).

Consider defining a `TypedDict` or dataclass for test parameters to improve type safety and IDE autocomplete:

```python
from typing import TypedDict

class TestParams(TypedDict):
    frame_size: int
    num_descriptors: int
    expected_mpps: float
    measured_mpps: float  # added at runtime
    performance_delta: float  # added at runtime
    pass: bool  # added at runtime
```

---

## Conclusion

**Primary concerns:**
1. **Error-level**: Missing error handling in `clean_up_vhost()` command execution (though may be acceptable depending on framework behavior)
2. **Warning-level**: Abstract method not implemented in all OS session subclasses
3. **Warning-level**: Assertions used for configuration validation instead of proper error messages

The test suite implementation is generally well-structured. The main issues are around error handling, polymorphism guarantees, and documentation clarity.