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 67F1E432D6; Wed, 8 Nov 2023 12:03:26 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0052B40E40; Wed, 8 Nov 2023 12:03:25 +0100 (CET) Received: from mail-ed1-f45.google.com (mail-ed1-f45.google.com [209.85.208.45]) by mails.dpdk.org (Postfix) with ESMTP id 3707D402DA for ; Wed, 8 Nov 2023 12:03:24 +0100 (CET) Received: by mail-ed1-f45.google.com with SMTP id 4fb4d7f45d1cf-542d654d03cso11154519a12.1 for ; Wed, 08 Nov 2023 03:03:24 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1699441404; x=1700046204; darn=dpdk.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=0VROn8LdscZ/RkV14GfwYbAEfy5Ue9c2D9Ocm+OGOkA=; b=tQk4P9DVoWBNOn06ymEzLHnI/YTl4HW/uBjsEzQlv5os1gqhBo1Yn03T/sFuYDUBVp Vnkzbk2bEbc+rFceekDaFGIzjmzKVQGEWHDDTKQZLdBttUHHfESoqU809pOP5VKhlWjf jue3P+d/OorU2bDRGoYS4Bd3irsJMLNWWys3WH6a/SBh77dLciAS7hwjwTnTfIoI88wd ZGp6Ttfr8SJ+FWqwXH3ghgWok9QYX6Adhf02sk+aZXii2bDh8THO1pN9ou/OOnrvk+Bg GtOZvn39uG4Gzb7ELZOMdetvpAEF7Go+B0JmwC0++8wnSa0nHN52a8aesifJumdUIgF4 9MvA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1699441404; x=1700046204; h=content-transfer-encoding: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=0VROn8LdscZ/RkV14GfwYbAEfy5Ue9c2D9Ocm+OGOkA=; b=Bl4AMGZLKhrvQCVJSfC7catktrl6NosFxk6VQOzfKZ06Zla/HxpiKoJ4asGJujlXJt v2vyWQbR8N8p9Q1Z4npDdXZaZAGdSUh6Rh2/8ws/IGwo32fitoWjSfWRm1DB2D6uz4SH 5Dj8+qavgVrmq9KpPzyg6jZ1UchS/suwXZoDBOG8KaQdD+CmjYwaalLz/C2+Uo3/f+Zn 70pj06mPnE1JM0k4TRMllvyXfAv8FSn4G6Y+FZchdLZ09MBizC16emzS4ZYd/4vOwGd2 FSeDjMo1SU7QCFAxAk0i1cmR913y3xbjojpaDhIZ+XGJIgbRnVt7dAOZT8GVhVLLlLO7 TBDQ== X-Gm-Message-State: AOJu0Yyu/KOGjv5Ykof7bd2a4hoTccpG5xQv1QQpHNNqdPNvRbKG4yW1 qb2YyN41ZnrGIzjpERuwBU/YK7QqDiMz0xjFVlMIXg== X-Google-Smtp-Source: AGHT+IEbj3Qqnt4oEWOXRv3s+W188N5RdVI/bEwIIplp5MElUZ5aU5QvUGcrRdd0nwBFYKWvXxwToEOUKVTUNEdv3V4= X-Received: by 2002:a50:955c:0:b0:543:72cd:e14c with SMTP id v28-20020a50955c000000b0054372cde14cmr1234750eda.36.1699441403845; Wed, 08 Nov 2023 03:03:23 -0800 (PST) MIME-Version: 1.0 References: <20231026220059.10685-2-jspewock@iol.unh.edu> <20231026220059.10685-3-jspewock@iol.unh.edu> In-Reply-To: <20231026220059.10685-3-jspewock@iol.unh.edu> From: =?UTF-8?Q?Juraj_Linke=C5=A1?= Date: Wed, 8 Nov 2023 12:03:12 +0100 Message-ID: Subject: Re: [PATCH v1 1/1] dts: bind to DPDK driver before running test suites To: jspewock@iol.unh.edu Cc: Honnappa.Nagarahalli@arm.com, thomas@monjalon.net, wathsala.vithanage@arm.com, probb@iol.unh.edu, paul.szczepanek@arm.com, dev@dpdk.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 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/framewor= k/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}", pr= ivileged=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. > diff --git a/dts/framework/remote_session/os_session.py b/dts/framework/r= emote_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/test= bed_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_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=3Dself.dpdk_version, compiler_version=3Dself.co= mpiler_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. > + > + def _tear_down_build_target(self) -> None: > + """ > + This method exists to be optionally overwritten by derived class= es and > + is not decorated so that the derived class doesn't have to use t= he 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. > > 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. > diff --git a/dts/tests/TestSuite_os_udp.py b/dts/tests/TestSuite_os_udp.p= y > 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=3DFalse) 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=3DTrue) > + # Assume other suites will likely need dpdk driver > + self.sut_node.bind_ports_to_driver(for_dpdk=3DTrue) 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_smo= ke_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 th= e correct > driver. > """ > - path_to_devbind =3D self.sut_node.main_session.join_remote_path( > - self.sut_node._remote_dpdk_dir, "usertools", "dpdk-devbind.p= y" > - ) > + path_to_devbind =3D 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. > > all_nics_in_dpdk_devbind =3D self.sut_node.main_session.send_com= mand( > 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) =3D=3D nic.os_driver, > + devbind_info_for_nic.group(1) =3D=3D nic.os_driver_for_d= pdk, > f"Driver for device {nic.pci} does not match driver list= ed in " > f"configuration (bound to {devbind_info_for_nic.group(1)= })", > ) > -- > 2.42.0 >