[PATCH v1 0/2] dts: dpdk shell bug fixes

[PATCH v1 0/2] dts: dpdk shell bug fixes

[Patrick Robb]

Support for the dpdk-test-crypto-perf application in DTS was added
recently. This work introduced a new bug (incorrectly setting the EAL
port allowlist when checking ethdev capabilities) and also exposed a
pre-existing bug (insuffient time delay between dpdk app runs causing
resource conflicts). Both of these bugs are resolved by the patches in
this series. The patches are very small, but also important for the DTS
user experience, so if there is time for these to be merged in for RC3 I
think it would be a big benefit. Thanks.

Patrick Robb (2):
  dts: always use ethdev ports to check capabilities
  dts: avoid dpdk resources conflict

 dts/framework/remote_session/dpdk_shell.py | 14 ++++++++++----
 dts/framework/testbed_model/capability.py  |  2 +-
 2 files changed, 11 insertions(+), 5 deletions(-)

-- 
2.49.0

[PATCH v1 1/2] dts: always use ethdev ports to check capabilities

[Patrick Robb]

DTS uses its capability check step to query ethdev devices for
their capabilities, and then run or skip testsuites according
to their respective capability requirements. So, we should be
running the capability check step with the ethernet devices
in all cases. However, when crypto ports are specified, a
crypto virtual function will be used in the capability check
step in error. So, this patch changes the default behavior
when computing EAL arguments, setting the port allow list to
the ethernet devices by default. In addition, as a defensive
coding measure, the capability check now specifies its usage
of the SUT ethernet devices when acquiring capabilities.

Fixes: 76e3fdd7b5c6 ("dts: add cryptodev package to DTS")

Signed-off-by: Patrick Robb <probb@iol.unh.edu>
Tested-by: Patrick Robb <probb@iol.unh.edu>
---
 dts/framework/remote_session/dpdk_shell.py | 5 +----
 dts/framework/testbed_model/capability.py  | 2 +-
 2 files changed, 2 insertions(+), 5 deletions(-)

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
 
     return params
 
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:
                 for (
                     conditional_capability_fn,
                     capabilities,
-- 
2.49.0

Re: [PATCH v1 1/2] dts: always use ethdev ports to check capabilities

[Andrew Bailey]

[-- Attachment #1: Type: text/plain, Size: 96 bytes --]

Tested-by: Andrew Bailey <abailey@iol.unh.edu>
Reviewed-by: Andrew Bailey <abailey@iol.unh.edu>

[-- Attachment #2: Type: text/html, Size: 252 bytes --]

[PATCH v1 2/2] dts: avoid dpdk resources conflict

[Patrick Robb]

When starting and stopping DPDK applications in rapid succession,
there may be a resource conflicts on ports, memory, VFIO, etc.
Even after we send "quit" to the testpmd CLI, some resource
cleanup will happen asynchronously in the background. To avoid a
resource conflict upon starting a subsequent DPDK application,
add a very brief sleep after quitting DPDK applications.

Fixes: bfad0948df75 ("dts: rework interactive shells")

Signed-off-by: Patrick Robb <probb@iol.unh.edu>
Tested-by: Patrick Robb <probb@iol.unh.edu>
---
 dts/framework/remote_session/dpdk_shell.py | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/dts/framework/remote_session/dpdk_shell.py b/dts/framework/remote_session/dpdk_shell.py
index 51b97d4ff6..7c79f33908 100644
--- a/dts/framework/remote_session/dpdk_shell.py
+++ b/dts/framework/remote_session/dpdk_shell.py
@@ -6,6 +6,7 @@
 Provides a base class to create interactive shells based on DPDK.
 """
 
+import time
 from abc import ABC, abstractmethod
 from pathlib import PurePath
 
@@ -84,3 +85,11 @@ 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)
+
+    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()
-- 
2.49.0

Re: [PATCH v1 2/2] dts: avoid dpdk resources conflict

[Andrew Bailey]

[-- Attachment #1: Type: text/plain, Size: 95 bytes --]

Tested-by: Andrew Bailey <abailey@iol.unh.edu>
Reviewed-by Andrew Bailey <abailey@iol.unh.edu>

[-- Attachment #2: Type: text/html, Size: 220 bytes --]

Re: [PATCH v1 2/2] dts: avoid dpdk resources conflict

[Andrew Bailey]

[-- Attachment #1: Type: text/plain, Size: 73 bytes --]

oops, I forgot a colon.
Reviewed-by: Andrew Bailey <abailey@iol.unh.edu>

[-- Attachment #2: Type: text/html, Size: 168 bytes --]

[PATCH v2 0/2] dts: dpdk shell bug fixes

[Patrick Robb]

Support for the dpdk-test-crypto-perf application in DTS was added
recently. This work introduced a new bug (incorrectly setting the EAL
port allowlist when checking ethdev capabilities) and also exposed a
pre-existing bug (insuffient time delay between dpdk app runs causing
resource conflicts). Both of these bugs are resolved by the patches in
this series. The patches are very small, but also important for the DTS
user experience, so if there is time for these to be merged in for RC3 I
think it would be a big benefit. Thanks.

v2:
Adding missing only_active decorator to the DPDKShell close method,
which resolves a doc build issue.

Patrick Robb (2):
  dts: always use ethdev ports to check capabilities
  dts: avoid dpdk resources conflict

 dts/api/testpmd/__init__.py                |  2 +-
 dts/framework/remote_session/dpdk_shell.py | 17 ++++++++++++-----
 dts/framework/testbed_model/capability.py  |  2 +-
 3 files changed, 14 insertions(+), 7 deletions(-)

-- 
2.49.0

[PATCH v2 1/2] dts: always use ethdev ports to check capabilities

[Patrick Robb]

DTS uses its capability check step to query ethdev devices for
their capabilities, and then run or skip testsuites according
to their respective capability requirements. So, we should be
running the capability check step with the ethernet devices
in all cases. However, when crypto ports are specified, a
crypto virtual function will be used in the capability check
step in error. So, this patch changes the default behavior
when computing EAL arguments, setting the port allow list to
the ethernet devices by default. In addition, as a defensive
coding measure, the capability check now specifies its usage
of the SUT ethernet devices when acquiring capabilities.

Fixes: 76e3fdd7b5c6 ("dts: add cryptodev package to DTS")

Signed-off-by: Patrick Robb <probb@iol.unh.edu>
Tested-by: Patrick Robb <probb@iol.unh.edu>
---
 dts/framework/remote_session/dpdk_shell.py | 5 +----
 dts/framework/testbed_model/capability.py  | 2 +-
 2 files changed, 2 insertions(+), 5 deletions(-)

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
 
     return params
 
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:
                 for (
                     conditional_capability_fn,
                     capabilities,
-- 
2.49.0

[PATCH v2 2/2] dts: avoid dpdk resources conflict

[Patrick Robb]

When starting and stopping DPDK applications in rapid succession,
there may be a resource conflicts on ports, memory, VFIO, etc.
Even after we send "quit" to the testpmd CLI, some resource
cleanup will happen asynchronously in the background. To avoid a
resource conflict upon starting a subsequent DPDK application,
add a very brief sleep after quitting DPDK applications.

Fixes: bfad0948df75 ("dts: rework interactive shells")

Signed-off-by: Patrick Robb <probb@iol.unh.edu>
Tested-by: Patrick Robb <probb@iol.unh.edu>
---
 dts/api/testpmd/__init__.py                |  2 +-
 dts/framework/remote_session/dpdk_shell.py | 12 +++++++++++-
 2 files changed, 12 insertions(+), 2 deletions(-)

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()
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,
 )
 from framework.testbed_model.cpu import LogicalCoreList
 
@@ -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()
-- 
2.49.0

[PATCH v3 1/2] dts: always use ethdev ports to check capabilities

[Patrick Robb]

DTS uses its capability check step to query ethdev devices for
their capabilities, and then run or skip testsuites according
to their respective capability requirements. So, we should be
running the capability check step with the ethernet devices
in all cases. However, when crypto ports are specified, a
crypto virtual function will be used in the capability check
step in error. So, this patch changes the default behavior
when computing EAL arguments, setting the port allow list to
the ethernet devices by default. In addition, as a defensive
coding measure, the capability check now specifies its usage
of the SUT ethernet devices when acquiring capabilities.

Fixes: 76e3fdd7b5c6 ("dts: add cryptodev package to DTS")

Signed-off-by: Patrick Robb <probb@iol.unh.edu>
Reviewed-by: Andrew Bailey <abailey@iol.unh.edu>
Tested-by: Andrew Bailey <abailey@iol.unh.edu>
---
 dts/framework/remote_session/dpdk_shell.py | 5 +----
 dts/framework/testbed_model/capability.py  | 2 +-
 2 files changed, 2 insertions(+), 5 deletions(-)

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
 
     return params
 
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:
                 for (
                     conditional_capability_fn,
                     capabilities,
-- 
2.49.0

[PATCH v3 2/2] dts: avoid dpdk resources conflict

[Patrick Robb]

When starting and stopping DPDK applications in rapid succession,
there may be a resource conflicts on ports, memory, VFIO, etc.
Even after we send "quit" to the testpmd CLI, some resource
cleanup will happen asynchronously in the background. To avoid a
resource conflict upon starting a subsequent DPDK application,
add a very brief sleep after quitting DPDK applications.

Fixes: bfad0948df75 ("dts: rework interactive shells")

Signed-off-by: Patrick Robb <probb@iol.unh.edu>
Reviewed-by: Andrew Bailey <abailey@iol.unh.edu>
Tested-by: Andrew Bailey <abailey@iol.unh.edu>
---
 dts/api/testpmd/__init__.py                |  2 +-
 dts/framework/remote_session/dpdk_shell.py | 11 +++++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

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()
diff --git a/dts/framework/remote_session/dpdk_shell.py b/dts/framework/remote_session/dpdk_shell.py
index 51b97d4ff6..269c2cada4 100644
--- a/dts/framework/remote_session/dpdk_shell.py
+++ b/dts/framework/remote_session/dpdk_shell.py
@@ -6,6 +6,7 @@
 Provides a base class to create interactive shells based on DPDK.
 """
 
+import time
 from abc import ABC, abstractmethod
 from pathlib import PurePath
 
@@ -13,6 +14,7 @@
 from framework.params.eal import EalParams
 from framework.remote_session.interactive_shell import (
     InteractiveShell,
+    only_active,
 )
 from framework.testbed_model.cpu import LogicalCoreList
 
@@ -84,3 +86,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()
-- 
2.49.0