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

[Jeremy Spewock <jspewock@iol.unh.edu>]

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

On Wed, Nov 8, 2023 at 6:03 AM Juraj Linkeš <juraj.linkes@pantheon.tech>
wrote:

> On Fri, Oct 27, 2023 at 12:03 AM <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/remote_session/linux_session.py |  3 ++
> >  dts/framework/remote_session/os_session.py    |  6 ++++
> >  dts/framework/testbed_model/sut_node.py       | 34 +++++++++++++++++++
> >  dts/tests/TestSuite_os_udp.py                 |  4 +++
> >  dts/tests/TestSuite_smoke_tests.py            |  6 ++--
> >  5 files changed, 49 insertions(+), 4 deletions(-)
> >
> > diff --git a/dts/framework/remote_session/linux_session.py
> b/dts/framework/remote_session/linux_session.py
> > index a3f1a6bf3b..7f2453c44c 100644
> > --- a/dts/framework/remote_session/linux_session.py
> > +++ b/dts/framework/remote_session/linux_session.py
> > @@ -199,3 +199,6 @@ def configure_port_ip_address(
> >      def configure_ipv4_forwarding(self, enable: bool) -> None:
> >          state = 1 if enable else 0
> >          self.send_command(f"sysctl -w net.ipv4.ip_forward={state}",
> privileged=True)
> > +
> > +    def probe_driver(self, driver_name: str) -> None:
> > +        self.send_command(f"modprobe {driver_name}", verify=True)
>
> This may not be something we want to do in DTS, but rather assume it's
> already been configured and maybe add a smoke test that would test
> whether the driver is there.
>

That makes sense. I included it originally because I just assumed it
wouldn't change much with the idempotence of modprobe, but this is a good
point, we can just check to see if it is loaded beforehand. I like the idea
of it being a smoke test but, because the smoke tests are run with the
other suites, it is actually the case that we bind to the driver for DPDK
before running smoke tests. We could alter this behaviour, but we need to
be bound to the DPDK driver in some cases for the checking if the devices
are listed in testpmd case.

So, I'm not sure where this check to see if it is loaded would go if not
smoke tests. I could add something that checks to see if the driver is
loaded before we bind to it and only then modprobes, but for now we can
assume that the user has probed the driver and just remove this step.


>
> > diff --git a/dts/framework/remote_session/os_session.py
> b/dts/framework/remote_session/os_session.py
> > index 8a709eac1c..719e815ac8 100644
> > --- a/dts/framework/remote_session/os_session.py
> > +++ b/dts/framework/remote_session/os_session.py
> > @@ -282,3 +282,9 @@ def configure_ipv4_forwarding(self, enable: bool) ->
> None:
> >          """
> >          Enable IPv4 forwarding in the underlying OS.
> >          """
> > +
> > +    @abstractmethod
> > +    def probe_driver(self, driver_name: str) -> None:
> > +        """
> > +        Load the module for the driver.
> > +        """
> > diff --git a/dts/framework/testbed_model/sut_node.py
> b/dts/framework/testbed_model/sut_node.py
> > index 202aebfd06..5a7dd91cac 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: PurePath | None
>
> I'd add script to the variable so that it's clearer:
> self._path_to_devbind_script
>
>
Good point, I will change this.


> >
> >      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 = 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(self) -> PurePath:
> > +        if self._path_to_devbind is None:
> > +            self._path_to_devbind = self.main_session.join_remote_path(
> > +                self._remote_dpdk_dir, "usertools", "dpdk-devbind.py"
> > +            )
> > +        return self._path_to_devbind
> > +
> >      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()
>
> This only binds the ports on the SUT node, should we also do this on
> the TG node? We may not have access to the devbind script on the TG
> node, but we can just copy it there.
>

It does only bind on the SUT but the reason I wrote it this way is we don't
really have a reason to change the drivers on the TG currently. Obviously
in terms of the scapy traffic generator we don't need to because we use the
OS driver, but even in the case of TREX, you don't need to bind to any
drivers, you just need to unbind. I think to incorporate this would add
slightly more complexity because we would need to copy the DPDK tarball
onto the TG and store a path to it much like we do in the SUT. We could
just do this by moving the other methods from the SUT that untar DPDK and
guess it's path to the Node class, but the reason I didn't is because we
don't really have a use for it currently.

I could incorporate it just in case we need it in the future, but from what
I understand about how binding to drivers would work on the TG, all we
would really need to do is unbind before running with TREX and then maybe
bind back to the OS driver for cleanup. Maybe other traffic generators
could have different requirements however. Do you think this added
complexity is worth adding now, or would it be better to incorporate it
when we start to write the new traffic generator class that might need it
so we can shape it around that use case?

I might lean more towards not adding this functionality to the TG yet
because it would lead to us copying DPDK over onto the TG and then just not
using it in the current framework. Again, there could be a case that would
be nice to have this on the TG, but we could also use /sys/bus/pci to
unbind the driver on the TG as well in the future if that ends up being
easier rather than copying DPDK over and using devbind. The only reason I
used devbind for the SUT was because binding to vfio-pci I found sometimes
gave me trouble using /sys/bus/pci but always worked with devbind.

What are your thoughts on this?


>
> > +
> > +    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)
>
> There are three possibilities here:
> 1. We unbind the port
> 2. We bind the port to the OS driver (like you're suggesting)
> 3. We bind the port to the state before self.bind_ports_to_driver()
>
> I don't think it's that big of a deal if we just bind it to the OS
> driver, but 3 is probably the best.
>

I think three would make sense, but I assumed that the purpose of the
os_driver would be "the driver that we use when we aren't testing DPDK" so
it would make sense to bind back to that driver. I think it makes the
method simpler to make it a binary choice and I also don't see the purpose
of the os_driver if it isn't the driver we expect to be on outside of
running testing but maybe I misunderstand its purpose. Initially in smoke
tests actually we verified that we were bound to this driver, so I assumed
that the expected state before running is supposed to be the OS driver.

I think for simplicity it makes sense to just bind back to the os_driver,
but if there is a use case for when we would run DTS on something that
isn't the os_driver and want to bind back to that we could make it the case.


>
> >
> >      def _configure_build_target(
> >          self, build_target_config: BuildTargetConfiguration
> > @@ -389,3 +407,19 @@ 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.probe_driver(driver)
> > +            self.main_session.send_command(
> > +                f"{self.path_to_devbind} -b {driver} --force
> {port.pci}",
> > +                privileged=True,
> > +                verify=True,
> > +            )
>
> Just a note: I was thinking about creating a dependency on the devbind
> script which is outside DTS and it actually looks like a benefit. Not
> only we don't need to implement anything, we're also testing the tool.
>
> > 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)
>
> This should be in the test suite setup.
>
> >          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)
>
> Looks like the whole change is needed just because of this test suite.
> If so, we can just remove the suite, as that was only a demonstration
> of traffic generator code.
>
> > diff --git a/dts/tests/TestSuite_smoke_tests.py
> b/dts/tests/TestSuite_smoke_tests.py
> > index 4a269df75b..0b839cba4e 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
>
> Now that I'm looking at this, there doesn't seem that much of a reason
> to access the path directly. Just having methods that would return
> structured output from the script makes way more sense to me. The
> methods should also be usable on the TG node, there's not really a
> reason to limit this to the SUT node.
>

I could refactor this to use other methods of getting the output and I
agree that this would likely be a cleaner solution, but I think making
methods to parse this output of devbind and filter for things like the
driver currently bound might be a little bit out of scope of this patch as
this patch is just looking to make it so that we bind to DPDK before
running the test suites.


>
> >
> >          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
> >
>

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