<div dir="ltr"><div dir="ltr"><br></div><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Nov 8, 2023 at 6:03 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Fri, Oct 27, 2023 at 12:03 AM <<a href="mailto:jspewock@iol.unh.edu" target="_blank">jspewock@iol.unh.edu</a>> wrote:<br>
><br>
> From: Jeremy Spewock <<a href="mailto:jspewock@iol.unh.edu" target="_blank">jspewock@iol.unh.edu</a>><br>
><br>
> Modifies the current process so that we bind to os_driver_for_dpdk from<br>
> the configuration file before running test suites and bind back to the<br>
> os_driver afterwards. This allows test suites to assume that the ports<br>
> are bound to a DPDK supported driver or bind to either driver as needed.<br>
><br>
> Signed-off-by: Jeremy Spewock <<a href="mailto:jspewock@iol.unh.edu" target="_blank">jspewock@iol.unh.edu</a>><br>
> ---<br>
> dts/framework/remote_session/linux_session.py | 3 ++<br>
> dts/framework/remote_session/os_session.py | 6 ++++<br>
> dts/framework/testbed_model/sut_node.py | 34 +++++++++++++++++++<br>
> dts/tests/TestSuite_os_udp.py | 4 +++<br>
> dts/tests/TestSuite_smoke_tests.py | 6 ++--<br>
> 5 files changed, 49 insertions(+), 4 deletions(-)<br>
><br>
> diff --git a/dts/framework/remote_session/linux_session.py b/dts/framework/remote_session/linux_session.py<br>
> index a3f1a6bf3b..7f2453c44c 100644<br>
> --- a/dts/framework/remote_session/linux_session.py<br>
> +++ b/dts/framework/remote_session/linux_session.py<br>
> @@ -199,3 +199,6 @@ def configure_port_ip_address(<br>
> def configure_ipv4_forwarding(self, enable: bool) -> None:<br>
> state = 1 if enable else 0<br>
> self.send_command(f"sysctl -w net.ipv4.ip_forward={state}", privileged=True)<br>
> +<br>
> + def probe_driver(self, driver_name: str) -> None:<br>
> + self.send_command(f"modprobe {driver_name}", verify=True)<br>
<br>
This may not be something we want to do in DTS, but rather assume it's<br>
already been configured and maybe add a smoke test that would test<br>
whether the driver is there.<br></blockquote><div> </div><div>Agreed.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> diff --git a/dts/framework/remote_session/os_session.py b/dts/framework/remote_session/os_session.py<br>
> index 8a709eac1c..719e815ac8 100644<br>
> --- a/dts/framework/remote_session/os_session.py<br>
> +++ b/dts/framework/remote_session/os_session.py<br>
> @@ -282,3 +282,9 @@ def configure_ipv4_forwarding(self, enable: bool) -> None:<br>
> """<br>
> Enable IPv4 forwarding in the underlying OS.<br>
> """<br>
> +<br>
> + @abstractmethod<br>
> + def probe_driver(self, driver_name: str) -> None:<br>
> + """<br>
> + Load the module for the driver.<br>
> + """<br>
> diff --git a/dts/framework/testbed_model/sut_node.py b/dts/framework/testbed_model/sut_node.py<br>
> index 202aebfd06..5a7dd91cac 100644<br>
> --- a/dts/framework/testbed_model/sut_node.py<br>
> +++ b/dts/framework/testbed_model/sut_node.py<br>
> @@ -89,6 +89,7 @@ class SutNode(Node):<br>
> _dpdk_version: str | None<br>
> _node_info: NodeInfo | None<br>
> _compiler_version: str | None<br>
> + _path_to_devbind: PurePath | None<br>
<br>
I'd add script to the variable so that it's clearer:<br>
self._path_to_devbind_script<br>
<br>
><br>
> def __init__(self, node_config: SutNodeConfiguration):<br>
> super(SutNode, self).__init__(node_config)<br>
> @@ -105,6 +106,7 @@ def __init__(self, node_config: SutNodeConfiguration):<br>
> self._dpdk_version = None<br>
> self._node_info = None<br>
> self._compiler_version = None<br>
> + self._path_to_devbind = None<br>
> self._<a href="http://logger.info" rel="noreferrer" target="_blank">logger.info</a>(f"Created node: {<a href="http://self.name" rel="noreferrer" target="_blank">self.name</a>}")<br>
><br>
> @property<br>
> @@ -155,6 +157,14 @@ def compiler_version(self) -> str:<br>
> return ""<br>
> return self._compiler_version<br>
><br>
> + @property<br>
> + def path_to_devbind(self) -> PurePath:<br>
> + if self._path_to_devbind is None:<br>
> + self._path_to_devbind = self.main_session.join_remote_path(<br>
> + self._remote_dpdk_dir, "usertools", "dpdk-devbind.py"<br>
> + )<br>
> + return self._path_to_devbind<br>
> +<br>
> def get_build_target_info(self) -> BuildTargetInfo:<br>
> return BuildTargetInfo(<br>
> dpdk_version=self.dpdk_version, compiler_version=self.compiler_version<br>
> @@ -176,6 +186,14 @@ def _set_up_build_target(<br>
> self._configure_build_target(build_target_config)<br>
> self._copy_dpdk_tarball()<br>
> self._build_dpdk()<br>
> + self.bind_ports_to_driver()<br>
<br>
This only binds the ports on the SUT node, should we also do this on<br>
the TG node? We may not have access to the devbind script on the TG<br>
node, but we can just copy it there.<br>
<br></blockquote><div><br></div><div>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. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +<br>
> + def _tear_down_build_target(self) -> None:<br>
> + """<br>
> + This method exists to be optionally overwritten by derived classes and<br>
> + is not decorated so that the derived class doesn't have to use the decorator.<br>
> + """<br>
> + self.bind_ports_to_driver(for_dpdk=False)<br>
<br>
There are three possibilities here:<br>
1. We unbind the port<br>
2. We bind the port to the OS driver (like you're suggesting)<br>
3. We bind the port to the state before self.bind_ports_to_driver()<br>
<br>
I don't think it's that big of a deal if we just bind it to the OS<br>
driver, but 3 is probably the best.<br></blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
><br>
> def _configure_build_target(<br>
> self, build_target_config: BuildTargetConfiguration<br>
> @@ -389,3 +407,19 @@ def create_interactive_shell(<br>
> return super().create_interactive_shell(<br>
> shell_cls, timeout, privileged, str(eal_parameters)<br>
> )<br>
> +<br>
> + def bind_ports_to_driver(self, for_dpdk: bool = True) -> None:<br>
> + """Bind all ports on the SUT to a driver.<br>
> +<br>
> + Args:<br>
> + for_dpdk: Boolean that, when True, binds ports to os_driver_for_dpdk<br>
> + or, when False, binds to os_driver. Defaults to True.<br>
> + """<br>
> + for port in self.ports:<br>
> + driver = port.os_driver_for_dpdk if for_dpdk else port.os_driver<br>
> + self.main_session.probe_driver(driver)<br>
> + self.main_session.send_command(<br>
> + f"{self.path_to_devbind} -b {driver} --force {port.pci}",<br>
> + privileged=True,<br>
> + verify=True,<br>
> + )<br>
<br>
Just a note: I was thinking about creating a dependency on the devbind<br>
script which is outside DTS and it actually looks like a benefit. Not<br>
only we don't need to implement anything, we're also testing the tool.<br>
<br></blockquote><div><br></div><div>I think I'm missing something but as we already depend on DPDK, we know we will have dpdk/usertools/dpdk-devbind.py, right?</div><div> </div></div></div>