From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 3D168432D8; Wed, 8 Nov 2023 17:45:09 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id BBD5D402BB; Wed, 8 Nov 2023 17:45:08 +0100 (CET) Received: from mail-oo1-f44.google.com (mail-oo1-f44.google.com [209.85.161.44]) by mails.dpdk.org (Postfix) with ESMTP id E8661402A7 for ; Wed, 8 Nov 2023 17:45:06 +0100 (CET) Received: by mail-oo1-f44.google.com with SMTP id 006d021491bc7-586ba7cdb6bso3838271eaf.2 for ; Wed, 08 Nov 2023 08:45:06 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1699461906; x=1700066706; darn=dpdk.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=MHV34Jnur6U3rAZyk+LJkEYpEnztQyuYDiunP5omZCI=; b=N5h4UBIWuCMG61BI8/cmLBw+iVCiyozScNj6J1l7y10bgF2x/4jJHx1x1JDOx+etUR XkxvUJZIzSLLH+ypjK0rJdYJ3O/R5Dxawk5N9P4lcA77n0t4fmnJl9SDPTbKoDjXIv/p 6YJwOmDYLvZ6uQDJNG9LGUHLVIkOyp51ODZBE= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1699461906; x=1700066706; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=MHV34Jnur6U3rAZyk+LJkEYpEnztQyuYDiunP5omZCI=; b=h0swSj09Lf6t8stOtaZi3K8IBiEe+SFBHvwa4FY8pO+wyx0Zdtc1WGViJXonL3iqBK o+OX7F5V4LNdoFKlSPi9aiC1ZBypksbDDO3i2wz8K35+gLWDAhLOkdTbpPmn5t4OCI1W 3r5Goxh8jnzXGefnvX9yLatFHQRm5DhR0nn2SE3ShQk/XL46t+Rpwc9ULZ53f2AZlTQT Kft1CoL9Shes3j9sg0/lE/vLoorVGu9QMQBgoSZ5Mvch1fE4oxPbhl9Zh3j9RTDj3pqe AVOizi338aD+SmHDApV5fWeUmPpPNXIDTICX7Arqxyv8Ihf2wJup1xuRBwBrYJPIT4fk A++g== X-Gm-Message-State: AOJu0Ywg4YtlOI2d2jOxgGw118WqDat1w1+IWhtlVTUPMNH/WIgjLvpg gxpVeUMO2HUEsquLHjkZrs87XY2q+mEiOkFNJLinWg== X-Google-Smtp-Source: AGHT+IGQtMK+qKUxK7KrBQHu57M7EQvWohHfKw/POpD54Lg5ExIwU6NhfSJAKqptiXq+WVI4Q2/jUz2Onlz2j5UxcXE= X-Received: by 2002:a4a:dc94:0:b0:586:8c18:ddd9 with SMTP id g20-20020a4adc94000000b005868c18ddd9mr2339344oou.9.1699461906120; Wed, 08 Nov 2023 08:45:06 -0800 (PST) MIME-Version: 1.0 References: <20231026220059.10685-2-jspewock@iol.unh.edu> <20231026220059.10685-3-jspewock@iol.unh.edu> In-Reply-To: From: Patrick Robb Date: Wed, 8 Nov 2023 11:44:55 -0500 Message-ID: Subject: Re: [PATCH v1 1/1] dts: bind to DPDK driver before running test suites To: =?UTF-8?Q?Juraj_Linke=C5=A1?= Cc: jspewock@iol.unh.edu, Honnappa.Nagarahalli@arm.com, thomas@monjalon.net, wathsala.vithanage@arm.com, paul.szczepanek@arm.com, dev@dpdk.org Content-Type: multipart/alternative; boundary="0000000000003959280609a6cf9f" X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org --0000000000003959280609a6cf9f Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, Nov 8, 2023 at 6:03=E2=80=AFAM Juraj Linke=C5=A1 wrote: > On Fri, Oct 27, 2023 at 12:03=E2=80=AFAM wrote: > > > > From: Jeremy Spewock > > > > 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 > > --- > > 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 =3D 1 if enable else 0 > > self.send_command(f"sysctl -w net.ipv4.ip_forward=3D{state}", > privileged=3DTrue) > > + > > + def probe_driver(self, driver_name: str) -> None: > > + self.send_command(f"modprobe {driver_name}", verify=3DTrue) > > 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 =3D None > > self._node_info =3D None > > self._compiler_version =3D None > > + self._path_to_devbind =3D 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 =3D self.main_session.join_remote_pa= th( > > + self._remote_dpdk_dir, "usertools", "dpdk-devbind.py" > > + ) > > + return self._path_to_devbind > > + > > def get_build_target_info(self) -> BuildTargetInfo: > > return BuildTargetInfo( > > dpdk_version=3Dself.dpdk_version, > compiler_version=3Dself.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=3DFalse) > > 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 =3D 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 =3D 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=3DTrue, > > + verify=3DTrue, > > + ) > > 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? --0000000000003959280609a6cf9f Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable

On Wed, Nov 8, 2023 at 6:03=E2=80=AFAM Ju= raj Linke=C5=A1 <juraj.linkes@pantheon.tech> wrote:
On Fri, Oct 27, 2023 at 12:03=E2= =80=AFAM <jspe= wock@iol.unh.edu> wrote:
>
> From: Jeremy Spewock <jspewock@iol.unh.edu>
>
> Modifies the current process so that we bind to os_driver_for_dpdk fro= m
> 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 neede= d.
>
> Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
> ---
>=C2=A0 dts/framework/remote_session/linux_session.py |=C2=A0 3 ++
>=C2=A0 dts/framework/remote_session/os_session.py=C2=A0 =C2=A0 |=C2=A0 = 6 ++++
>=C2=A0 dts/framework/testbed_model/sut_node.py=C2=A0 =C2=A0 =C2=A0 =C2= =A0| 34 +++++++++++++++++++
>=C2=A0 dts/tests/TestSuite_os_udp.py=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0|=C2=A0 4 +++
>=C2=A0 dts/tests/TestSuite_smoke_tests.py=C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 |=C2=A0 6 ++--
>=C2=A0 5 files changed, 49 insertions(+), 4 deletions(-)
>
> diff --git a/dts/framework/remote_session/linux_session.py b/dts/frame= work/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(
>=C2=A0 =C2=A0 =C2=A0 def configure_ipv4_forwarding(self, enable: bool) = -> None:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 state =3D 1 if enable else 0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.send_command(f"sysctl -w n= et.ipv4.ip_forward=3D{state}", privileged=3DTrue)
> +
> +=C2=A0 =C2=A0 def probe_driver(self, driver_name: str) -> None: > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.send_command(f"modprobe {driver= _name}", verify=3DTrue)

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
whether the driver is there.
=C2=A0
Agreed.<= /div>


> diff --git a/dts/framework/remote_session/os_session.py b/dts/framewor= k/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:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Enable IPv4 forwarding in the underl= ying OS.
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 """
> +
> +=C2=A0 =C2=A0 @abstractmethod
> +=C2=A0 =C2=A0 def probe_driver(self, driver_name: str) -> None: > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Load the module for the driver.
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
> diff --git a/dts/framework/testbed_model/sut_node.py b/dts/framework/t= estbed_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):
>=C2=A0 =C2=A0 =C2=A0 _dpdk_version: str | None
>=C2=A0 =C2=A0 =C2=A0 _node_info: NodeInfo | None
>=C2=A0 =C2=A0 =C2=A0 _compiler_version: str | None
> +=C2=A0 =C2=A0 _path_to_devbind: PurePath | None

I'd add script to the variable so that it's clearer:
self._path_to_devbind_script

>
>=C2=A0 =C2=A0 =C2=A0 def __init__(self, node_config: SutNodeConfigurati= on):
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 super(SutNode, self).__init__(node_c= onfig)
> @@ -105,6 +106,7 @@ def __init__(self, node_config: SutNodeConfigurati= on):
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._dpdk_version =3D None
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._node_info =3D None
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._compiler_version =3D None
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self._path_to_devbind =3D None
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._logger.info(f"Created node: = {self.nam= e}")
>
>=C2=A0 =C2=A0 =C2=A0 @property
> @@ -155,6 +157,14 @@ def compiler_version(self) -> str:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return &= quot;"
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return self._compiler_version
>
> +=C2=A0 =C2=A0 @property
> +=C2=A0 =C2=A0 def path_to_devbind(self) -> PurePath:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 if self._path_to_devbind is None:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._path_to_devbind =3D s= elf.main_session.join_remote_path(
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._remote_= dpdk_dir, "usertools", "dpdk-devbind.py"
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 )
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return self._path_to_devbind
> +
>=C2=A0 =C2=A0 =C2=A0 def get_build_target_info(self) -> BuildTargetI= nfo:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return BuildTargetInfo(
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 dpdk_version=3Dself.dp= dk_version, compiler_version=3Dself.compiler_version
> @@ -176,6 +186,14 @@ def _set_up_build_target(
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._configure_build_target(build_t= arget_config)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._copy_dpdk_tarball()
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._build_dpdk()
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 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 t= o os_driver_for_dpdk), but for the TG we don't (really) know yet whethe= r there will be any needs introduced by adding dperf, trex, ixia, etc. as t= raffic generators. I prefer to approach this question when we are adding TG= s for perf testing and necessarily seeing if we need to allow for any confi= guration we aren't thinking of yet.=C2=A0
=C2=A0
> +
> +=C2=A0 =C2=A0 def _tear_down_build_target(self) -> None:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 This method exists to be optionally overw= ritten by derived classes and
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 is not decorated so that the derived clas= s doesn't have to use the decorator.
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.bind_ports_to_driver(for_dpdk=3DFals= e)

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<= br> driver, but 3 is probably the best.

I d= on't have a strong opinion on this other than I like the conf file bein= g a source of truth, but 3 sounds fine too.
=C2=A0

>
>=C2=A0 =C2=A0 =C2=A0 def _configure_build_target(
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self, build_target_config: BuildTarg= etConfiguration
> @@ -389,3 +407,19 @@ def create_interactive_shell(
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return super().create_interactive_sh= ell(
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 shell_cls, timeout, pr= ivileged, str(eal_parameters)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 )
> +
> +=C2=A0 =C2=A0 def bind_ports_to_driver(self, for_dpdk: bool =3D True)= -> None:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """Bind all ports on the S= UT to a driver.
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Args:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 for_dpdk: Boolean that, whe= n True, binds ports to os_driver_for_dpdk
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 or, when False, binds to os= _driver. Defaults to True.
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 for port in self.ports:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 driver =3D port.os_driver_f= or_dpdk if for_dpdk else port.os_driver
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.main_session.probe_dri= ver(driver)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.main_session.send_comm= and(
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 f"{self.= path_to_devbind} -b {driver} --force {port.pci}",
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 privileged=3D= True,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 verify=3DTrue= ,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 )

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 to= ol.


I think I'm missing something but = as we already depend on DPDK, we know we will have dpdk/usertools/dpdk-devb= ind.py, right?
=C2=A0
--0000000000003959280609a6cf9f--