[PATCH v3 0/1] dts: Add the ability to bind ports to drivers

[PATCH v3 0/1] dts: Add the ability to bind ports to drivers

[jspewock]

From: Jeremy Spewock <jspewock@iol.unh.edu>

Changes in this version address the comments on the last and change what
was necessary. Now, we no longer modprobe the driver, but the decision
was made to still make driver binding exclusive to the SUT for the time
being due to the uncertainty of what binding drivers on the traffic
generator will look like in the future when we need to do so.

I also decided to leave the os_udp test case in the patch as leaving it
does no harm really, all that is required for it to run is binding to
the os_driver before it runs and back to the DPDK driver after, and I
think it serves as somewhat of a "hello world" for ensuring that your
traffic generator is functioning. If it is decided that we no longer
want it in the future or want to make it a part of the hello_world
suite, another patch will be submitted at a later date.

Previous version sent the wrong commit, this is the updated and fixed
one.

v1: https://mails.dpdk.org/archives/dev/2023-November/281477.html

Jeremy Spewock (1):
  dts: bind to DPDK driver before running test suites

 dts/framework/testbed_model/sut_node.py | 33 +++++++++++++++++++++++++
 dts/tests/TestSuite_os_udp.py           |  4 +++
 dts/tests/TestSuite_smoke_tests.py      |  6 ++---
 3 files changed, 39 insertions(+), 4 deletions(-)

-- 
2.42.0

[PATCH v3 1/1] dts: bind to DPDK driver before running test suites

[jspewock]

From: Jeremy Spewock <jspewock@iol.unh.edu>

Modifies the current process so that we bind to os_driver_for_dpdk from
the configuration file before running test suites and bind back to the
os_driver afterwards. This allows test suites to assume that the ports
are bound to a DPDK supported driver or bind to either driver as needed.

Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
---
 dts/framework/testbed_model/sut_node.py | 33 +++++++++++++++++++++++++
 dts/tests/TestSuite_os_udp.py           |  4 +++
 dts/tests/TestSuite_smoke_tests.py      |  6 ++---
 3 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/dts/framework/testbed_model/sut_node.py b/dts/framework/testbed_model/sut_node.py
index 202aebfd06..4161d3a4d5 100644
--- a/dts/framework/testbed_model/sut_node.py
+++ b/dts/framework/testbed_model/sut_node.py
@@ -89,6 +89,7 @@ class SutNode(Node):
     _dpdk_version: str | None
     _node_info: NodeInfo | None
     _compiler_version: str | None
+    _path_to_devbind_script: PurePath | None
 
     def __init__(self, node_config: SutNodeConfiguration):
         super(SutNode, self).__init__(node_config)
@@ -105,6 +106,7 @@ def __init__(self, node_config: SutNodeConfiguration):
         self._dpdk_version = None
         self._node_info = None
         self._compiler_version = None
+        self._path_to_devbind_script = None
         self._logger.info(f"Created node: {self.name}")
 
     @property
@@ -155,6 +157,14 @@ def compiler_version(self) -> str:
                 return ""
         return self._compiler_version
 
+    @property
+    def path_to_devbind_script(self) -> PurePath:
+        if self._path_to_devbind_script is None:
+            self._path_to_devbind_script = self.main_session.join_remote_path(
+                self._remote_dpdk_dir, "usertools", "dpdk-devbind.py"
+            )
+        return self._path_to_devbind_script
+
     def get_build_target_info(self) -> BuildTargetInfo:
         return BuildTargetInfo(
             dpdk_version=self.dpdk_version, compiler_version=self.compiler_version
@@ -176,6 +186,14 @@ def _set_up_build_target(
         self._configure_build_target(build_target_config)
         self._copy_dpdk_tarball()
         self._build_dpdk()
+        self.bind_ports_to_driver()
+
+    def _tear_down_build_target(self) -> None:
+        """
+        This method exists to be optionally overwritten by derived classes and
+        is not decorated so that the derived class doesn't have to use the decorator.
+        """
+        self.bind_ports_to_driver(for_dpdk=False)
 
     def _configure_build_target(
         self, build_target_config: BuildTargetConfiguration
@@ -389,3 +407,18 @@ def create_interactive_shell(
         return super().create_interactive_shell(
             shell_cls, timeout, privileged, str(eal_parameters)
         )
+
+    def bind_ports_to_driver(self, for_dpdk: bool = True) -> None:
+        """Bind all ports on the SUT to a driver.
+
+        Args:
+            for_dpdk: Boolean that, when True, binds ports to os_driver_for_dpdk
+            or, when False, binds to os_driver. Defaults to True.
+        """
+        for port in self.ports:
+            driver = port.os_driver_for_dpdk if for_dpdk else port.os_driver
+            self.main_session.send_command(
+                f"{self.path_to_devbind_script} -b {driver} --force {port.pci}",
+                privileged=True,
+                verify=True,
+            )
diff --git a/dts/tests/TestSuite_os_udp.py b/dts/tests/TestSuite_os_udp.py
index 9b5f39711d..bf6b93deb5 100644
--- a/dts/tests/TestSuite_os_udp.py
+++ b/dts/tests/TestSuite_os_udp.py
@@ -19,6 +19,8 @@ def set_up_suite(self) -> None:
             Configure SUT ports and SUT to route traffic from if1 to if2.
         """
 
+        # This test uses kernel drivers
+        self.sut_node.bind_ports_to_driver(for_dpdk=False)
         self.configure_testbed_ipv4()
 
     def test_os_udp(self) -> None:
@@ -43,3 +45,5 @@ def tear_down_suite(self) -> None:
             Remove the SUT port configuration configured in setup.
         """
         self.configure_testbed_ipv4(restore=True)
+        # Assume other suites will likely need dpdk driver
+        self.sut_node.bind_ports_to_driver(for_dpdk=True)
diff --git a/dts/tests/TestSuite_smoke_tests.py b/dts/tests/TestSuite_smoke_tests.py
index 4a269df75b..e8016d1b54 100644
--- a/dts/tests/TestSuite_smoke_tests.py
+++ b/dts/tests/TestSuite_smoke_tests.py
@@ -84,9 +84,7 @@ def test_device_bound_to_driver(self) -> None:
             Ensure that all drivers listed in the config are bound to the correct
             driver.
         """
-        path_to_devbind = self.sut_node.main_session.join_remote_path(
-            self.sut_node._remote_dpdk_dir, "usertools", "dpdk-devbind.py"
-        )
+        path_to_devbind = self.sut_node.path_to_devbind_script
 
         all_nics_in_dpdk_devbind = self.sut_node.main_session.send_command(
             f"{path_to_devbind} --status | awk '{REGEX_FOR_PCI_ADDRESS}'",
@@ -108,7 +106,7 @@ def test_device_bound_to_driver(self) -> None:
             # We know this isn't None, but mypy doesn't
             assert devbind_info_for_nic is not None
             self.verify(
-                devbind_info_for_nic.group(1) == nic.os_driver,
+                devbind_info_for_nic.group(1) == nic.os_driver_for_dpdk,
                 f"Driver for device {nic.pci} does not match driver listed in "
                 f"configuration (bound to {devbind_info_for_nic.group(1)})",
             )
-- 
2.42.0

Re: [PATCH v3 1/1] dts: bind to DPDK driver before running test suites

[Patrick Robb]

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

On Thu, Nov 9, 2023 at 6:17 PM <jspewock@iol.unh.edu> wrote:

> From: Jeremy Spewock <jspewock@iol.unh.edu>
>
> Modifies the current process so that we bind to os_driver_for_dpdk from
> the configuration file before running test suites and bind back to the
> os_driver afterwards. This allows test suites to assume that the ports
> are bound to a DPDK supported driver or bind to either driver as needed.
>
> Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
> ---
>  dts/framework/testbed_model/sut_node.py | 33 +++++++++++++++++++++++++
>  dts/tests/TestSuite_os_udp.py           |  4 +++
>  dts/tests/TestSuite_smoke_tests.py      |  6 ++---
>  3 files changed, 39 insertions(+), 4 deletions(-)
>
> diff --git a/dts/framework/testbed_model/sut_node.py
> b/dts/framework/testbed_model/sut_node.py
> index 202aebfd06..4161d3a4d5 100644
> --- a/dts/framework/testbed_model/sut_node.py
> +++ b/dts/framework/testbed_model/sut_node.py
> @@ -89,6 +89,7 @@ class SutNode(Node):
>      _dpdk_version: str | None
>      _node_info: NodeInfo | None
>      _compiler_version: str | None
> +    _path_to_devbind_script: PurePath | None
>
>      def __init__(self, node_config: SutNodeConfiguration):
>          super(SutNode, self).__init__(node_config)
> @@ -105,6 +106,7 @@ def __init__(self, node_config: SutNodeConfiguration):
>          self._dpdk_version = None
>          self._node_info = None
>          self._compiler_version = None
> +        self._path_to_devbind_script = None
>          self._logger.info(f"Created node: {self.name}")
>
>      @property
> @@ -155,6 +157,14 @@ def compiler_version(self) -> str:
>                  return ""
>          return self._compiler_version
>
> +    @property
> +    def path_to_devbind_script(self) -> PurePath:
> +        if self._path_to_devbind_script is None:
> +            self._path_to_devbind_script =
> self.main_session.join_remote_path(
> +                self._remote_dpdk_dir, "usertools", "dpdk-devbind.py"
> +            )
> +        return self._path_to_devbind_script
> +
>      def get_build_target_info(self) -> BuildTargetInfo:
>          return BuildTargetInfo(
>              dpdk_version=self.dpdk_version,
> compiler_version=self.compiler_version
> @@ -176,6 +186,14 @@ def _set_up_build_target(
>          self._configure_build_target(build_target_config)
>          self._copy_dpdk_tarball()
>          self._build_dpdk()
> +        self.bind_ports_to_driver()
> +
> +    def _tear_down_build_target(self) -> None:
> +        """
> +        This method exists to be optionally overwritten by derived
> classes and
> +        is not decorated so that the derived class doesn't have to use
> the decorator.
> +        """
> +        self.bind_ports_to_driver(for_dpdk=False)
>
>      def _configure_build_target(
>          self, build_target_config: BuildTargetConfiguration
> @@ -389,3 +407,18 @@ def create_interactive_shell(
>          return super().create_interactive_shell(
>              shell_cls, timeout, privileged, str(eal_parameters)
>          )
> +
> +    def bind_ports_to_driver(self, for_dpdk: bool = True) -> None:
> +        """Bind all ports on the SUT to a driver.
> +
> +        Args:
> +            for_dpdk: Boolean that, when True, binds ports to
> os_driver_for_dpdk
> +            or, when False, binds to os_driver. Defaults to True.
> +        """
> +        for port in self.ports:
> +            driver = port.os_driver_for_dpdk if for_dpdk else
> port.os_driver
> +            self.main_session.send_command(
> +                f"{self.path_to_devbind_script} -b {driver} --force
> {port.pci}",
> +                privileged=True,
> +                verify=True,
> +            )
>

This all looks consistent with the understanding we came to during the CI
meeting and on the v2 discussion thread.


> diff --git a/dts/tests/TestSuite_os_udp.py b/dts/tests/TestSuite_os_udp.py
> index 9b5f39711d..bf6b93deb5 100644
> --- a/dts/tests/TestSuite_os_udp.py
> +++ b/dts/tests/TestSuite_os_udp.py
> @@ -19,6 +19,8 @@ def set_up_suite(self) -> None:
>              Configure SUT ports and SUT to route traffic from if1 to if2.
>          """
>
> +        # This test uses kernel drivers
> +        self.sut_node.bind_ports_to_driver(for_dpdk=False)
>          self.configure_testbed_ipv4()
>
>      def test_os_udp(self) -> None:
> @@ -43,3 +45,5 @@ def tear_down_suite(self) -> None:
>              Remove the SUT port configuration configured in setup.
>          """
>          self.configure_testbed_ipv4(restore=True)
> +        # Assume other suites will likely need dpdk driver
> +        self.sut_node.bind_ports_to_driver(for_dpdk=True)
>

Thanks for refactoring this to make it survive the binding approach change.
We'll still want to revisit this for 24.03, as Juraj mentioned that it's
really just to demonstrate the TG is performant, and may not be needed in
the future.


> diff --git a/dts/tests/TestSuite_smoke_tests.py
> b/dts/tests/TestSuite_smoke_tests.py
> index 4a269df75b..e8016d1b54 100644
> --- a/dts/tests/TestSuite_smoke_tests.py
> +++ b/dts/tests/TestSuite_smoke_tests.py
> @@ -84,9 +84,7 @@ def test_device_bound_to_driver(self) -> None:
>              Ensure that all drivers listed in the config are bound to the
> correct
>              driver.
>          """
> -        path_to_devbind = self.sut_node.main_session.join_remote_path(
> -            self.sut_node._remote_dpdk_dir, "usertools", "dpdk-devbind.py"
> -        )
> +        path_to_devbind = self.sut_node.path_to_devbind_script
>
>          all_nics_in_dpdk_devbind =
> self.sut_node.main_session.send_command(
>              f"{path_to_devbind} --status | awk '{REGEX_FOR_PCI_ADDRESS}'",
> @@ -108,7 +106,7 @@ def test_device_bound_to_driver(self) -> None:
>              # We know this isn't None, but mypy doesn't
>              assert devbind_info_for_nic is not None
>              self.verify(
> -                devbind_info_for_nic.group(1) == nic.os_driver,
> +                devbind_info_for_nic.group(1) == nic.os_driver_for_dpdk,
>                  f"Driver for device {nic.pci} does not match driver
> listed in "
>                  f"configuration (bound to
> {devbind_info_for_nic.group(1)})",
>              )
> --
> 2.42.0
>
> We discussed this aspect of binding during last week's CI meeting and I
understood Juraj to be consenting to returning to DTS running the binding
to the dpdk driver (so, what you're doing here), as opposed to relying on
the user to do it, and making it a smoke test. As we've discussed, that's
how the old DTS framework ran, and I prefer to stick to this approach. One
aspect I raised was how in a lab context it's desirable for us to define as
much as possible within config files, and have environmental configuration
be handled by DTS. So, since there was basically agreement here, I think
your changes here are appropriate.

Acked-by: Patrick Robb <probb@iol.unh.edu>

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

Re: [PATCH v3 1/1] dts: bind to DPDK driver before running test suites

[Thomas Monjalon]

13/11/2023 18:56, Patrick Robb:
> On Thu, Nov 9, 2023 at 6:17 PM <jspewock@iol.unh.edu> wrote:
> 
> > From: Jeremy Spewock <jspewock@iol.unh.edu>
> >
> > Modifies the current process so that we bind to os_driver_for_dpdk from
> > the configuration file before running test suites and bind back to the
> > os_driver afterwards. This allows test suites to assume that the ports
> > are bound to a DPDK supported driver or bind to either driver as needed.
> >
> > Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
> >
> > We discussed this aspect of binding during last week's CI meeting and I
> understood Juraj to be consenting to returning to DTS running the binding
> to the dpdk driver (so, what you're doing here), as opposed to relying on
> the user to do it, and making it a smoke test. As we've discussed, that's
> how the old DTS framework ran, and I prefer to stick to this approach. One
> aspect I raised was how in a lab context it's desirable for us to define as
> much as possible within config files, and have environmental configuration
> be handled by DTS. So, since there was basically agreement here, I think
> your changes here are appropriate.
> 
> Acked-by: Patrick Robb <probb@iol.unh.edu>

Not sure it is a good idea to add something knowing it will be reworked,
but you agreed, so I apply.

PS: please make all versions of the same patch in the same email thread.

Re: [PATCH v3 1/1] dts: bind to DPDK driver before running test suites

[Jeremy Spewock]

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

On Tue, Nov 14, 2023 at 4:49 PM Thomas Monjalon <thomas@monjalon.net> wrote:

> 13/11/2023 18:56, Patrick Robb:
> > On Thu, Nov 9, 2023 at 6:17 PM <jspewock@iol.unh.edu> wrote:
> >
> > > From: Jeremy Spewock <jspewock@iol.unh.edu>
> > >
> > > Modifies the current process so that we bind to os_driver_for_dpdk from
> > > the configuration file before running test suites and bind back to the
> > > os_driver afterwards. This allows test suites to assume that the ports
> > > are bound to a DPDK supported driver or bind to either driver as
> needed.
> > >
> > > Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
> > >
> > > We discussed this aspect of binding during last week's CI meeting and I
> > understood Juraj to be consenting to returning to DTS running the binding
> > to the dpdk driver (so, what you're doing here), as opposed to relying on
> > the user to do it, and making it a smoke test. As we've discussed, that's
> > how the old DTS framework ran, and I prefer to stick to this approach.
> One
> > aspect I raised was how in a lab context it's desirable for us to define
> as
> > much as possible within config files, and have environmental
> configuration
> > be handled by DTS. So, since there was basically agreement here, I think
> > your changes here are appropriate.
> >
> > Acked-by: Patrick Robb <probb@iol.unh.edu>
>
> Not sure it is a good idea to add something knowing it will be reworked,
> but you agreed, so I apply.
>
>

I believe logically the methods I am adding here wouldn't end up needing to
be refactored, the refactor part would be of the already existing logic in
the TGNode to allow for it to be able to use the method I wrote here. There
are a few things that would need to change if we wanted to be able to
support doing this on the TGNode because the current framework doesn't
exactly allow for it since the devbind script doesn't exist on that node.
The main reason I refrained from doing that rework in this patch is due to
the lack of need for the support of it on the TGNode at this time (and
potentially in the future) and the lack of existing support.

The methods I am writing here however would likely not need to be reworked,
just moved to their superclass if we decided we wanted to do the TGNode
rework and add support. Apologies if I made it sound like I was submitting
these changes just for them to be completely overridden and changed in the
near future and hopefully that makes a little more sense.

Thank you for applying!



> PS: please make all versions of the same patch in the same email thread.
>

I will start doing this in future patches, sorry for any difficulty it may
have caused trying to see previous comments.

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