|SUCCESS| [v2,2/2] dts: avoid dpdk resources conflict

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/162613

_AI Code Review_

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

This review is based on the patch series that modifies DTS (DPDK Test Suite) to fix issues with capability checking and resource conflicts when using crypto ports alongside ethernet ports.

git sha: 740203-2-probb (from Message-ID)
Author: Patrick Robb <probb at iol.unh.edu>
Subject: [PATCH v2 1/2] dts: always use ethdev ports to check capabilities

The first patch ensures that capability checks always use ethernet
devices rather than crypto VFs. The second patch adds a delay after
closing DPDK applications to prevent resource conflicts.

> diff --git a/dts/framework/remote_session/dpdk_shell.py b/dts/framework/remote_session/dpdk_shell.py
> index b94d336d4e..51b97d4ff6 100644
> --- a/dts/framework/remote_session/dpdk_shell.py
> +++ b/dts/framework/remote_session/dpdk_shell.py
> @@ -46,10 +46,7 @@ def compute_eal_params(
>      params.prefix = prefix
>  
>      if params.allowed_ports is None:
> -        if ctx.topology.crypto_vf_ports:
> -            params.allowed_ports = [ctx.topology.crypto_vf_ports[0]]
> -        else:
> -            params.allowed_ports = ctx.topology.sut_dpdk_ports
> +        params.allowed_ports = ctx.topology.sut_dpdk_ports

Does this change break any existing tests that rely on crypto_vf_ports
being selected by default when they are available? If crypto tests need
specific ports, would they still be able to override the allowed_ports
parameter?

> diff --git a/dts/framework/testbed_model/capability.py b/dts/framework/testbed_model/capability.py
> index b166014e0c..960370fc72 100644
> --- a/dts/framework/testbed_model/capability.py
> +++ b/dts/framework/testbed_model/capability.py
> @@ -384,7 +384,7 @@ def get_supported_capabilities(
>          )
>          if cls.capabilities_to_check:
>              capabilities_to_check_map = cls._get_decorated_capabilities_map()
> -            with TestPmd() as testpmd:
> +            with TestPmd(allowed_ports=topology.sut_dpdk_ports) as testpmd:

This explicit specification is now redundant given the compute_eal_params()
change above. When TestPmd() is instantiated without allowed_ports, does
it call compute_eal_params() which now defaults to sut_dpdk_ports anyway?

[ ... ]

git sha: 740203-3-probb (from Message-ID)
Author: Patrick Robb <probb at iol.unh.edu>
Subject: [PATCH v2 2/2] dts: avoid dpdk resources conflict

> diff --git a/dts/api/testpmd/__init__.py b/dts/api/testpmd/__init__.py
> index 703cae487e..e9187440bb 100644
> --- a/dts/api/testpmd/__init__.py
> +++ b/dts/api/testpmd/__init__.py
> @@ -1174,7 +1174,7 @@ def clear_port_stats_all(self, verify: bool = True) -> None:
>  
>      @only_active
>      def close(self) -> None:
> -        """Overrides :meth:`~.interactive_shell.close`."""
> +        """Overrides :meth:`~.dpdk_shell.close`."""
>          self.stop()
>          self.send_command("quit", "Bye...")
>          return super().close()

Does this docstring change accurately reflect the new inheritance chain
after adding close() to DpdkShell? When TestPmd.close() calls
super().close(), does it now invoke DpdkShell.close() which then sleeps
and calls InteractiveShell.close()?

> diff --git a/dts/framework/remote_session/dpdk_shell.py b/dts/framework/remote_session/dpdk_shell.py
> index 51b97d4ff6..f926c3fa4d 100644
> --- a/dts/framework/remote_session/dpdk_shell.py
> +++ b/dts/framework/remote_session/dpdk_shell.py
> @@ -6,13 +6,14 @@
>  Provides a base class to create interactive shells based on DPDK.
>  """
>  
> +import time
>  from abc import ABC, abstractmethod
>  from pathlib import PurePath
>  
>  from framework.context import get_ctx
>  from framework.params.eal import EalParams
>  from framework.remote_session.interactive_shell import (
> -    InteractiveShell,
> +    InteractiveShell, only_active,
>  )

The import formatting appears inconsistent. The coding style guide
indicates that multiple imports from the same module should be on
separate lines, not comma-separated on one line.

> @@ -84,3 +85,12 @@ def _make_real_path(self) -> PurePath:
>          Adds the remote DPDK build directory to the path.
>          """
>          return get_ctx().dpdk_build.remote_dpdk_build_dir.joinpath(self.path)
> +
> +    @only_active
> +    def close(self) -> None:
> +        """Overrides :meth:`~.interactive_shell.close`."""
> +        # Allow time for VFIO and hardware resources to be released by the kernel after the
> +        # DPDK process exits. Without this delay, sequential testpmd instances may have EAL
> +        # errors when trying to acquire the same devices.
> +        time.sleep(1)
> +        return super().close()

Does this one-second hardcoded sleep value accommodate all platforms and
resource types? Would a shorter sleep still be sufficient, or could some
slower systems require more time? Has polling for resource availability
been considered as an alternative to a fixed delay?

Is there a risk that the sleep occurs even if the application failed to
start properly, potentially adding unnecessary delays to error cases?