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

[Patrick Robb]

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

> On Fri, Oct 27, 2023 at 12:03 AM <jspewock at iol.unh.edu> wrote:
> >
> > From: Jeremy Spewock <jspewock at 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 at 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.
>

Agreed.


> > 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
>
> >
> >      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.
>
>
Jeremy and I discussed this and I'm not sure right now. With the SUT we
have a known default behavior (bind to os_driver_for_dpdk), but for the TG
we don't (really) know yet whether there will be any needs introduced by
adding dperf, trex, ixia, etc. as traffic generators. I prefer to approach
this question when we are adding TGs for perf testing and necessarily
seeing if we need to allow for any configuration we aren't thinking of yet.


> > +
> > +    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 don't have a strong opinion on this other than I like the conf file being
a source of truth, but 3 sounds fine too.


>
> >
> >      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.
>
>
I think I'm missing something but as we already depend on DPDK, we know we
will have dpdk/usertools/dpdk-devbind.py, right?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mails.dpdk.org/archives/dev/attachments/20231108/eed085c0/attachment.htm>