Re: [PATCH v4 4/7] dts: rework DPDK attributes in SUT node config

[Nicholas Pratte <npratte@iol.unh.edu>]

Reviewed-by: Nicholas Pratte <npratte@iol.unh.edu>

On Fri, Jan 24, 2025 at 6:39 AM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>
> From: Nicholas Pratte <npratte@iol.unh.edu>
>
> Rework 'lcores' and 'memory_channels' into a new 'dpdk_config'
> subsection in an effort to make these attributes SUT specific; the
> traffic generator, more often than not, does not need this information.
> Ideally, if such information is needed, then it will be listed in the
> 'traffic_generator' component in TG Node configuration. Such logic is
> not introduced in this patch, but the framework can be rewritten to do
> so without any implications of extreme effort.
>
> To make this work, use_first_core has been removed from the framework
> entirely in favor of doing this within the LogicalCoreListFilter object.
> Since use_first_core was only ever activated when logical core 0 was
> explicitly defined, core 0 can be removed from the list of total logical
> cores assuming that it was not listed within filter_specifier.
>
> This patch also removes 'vdevs' from 'system_under_test_node' and moves
> it into 'test_runs'.
>
> Bugzilla ID: 1360
>
> Signed-off-by: Nicholas Pratte <npratte@iol.unh.edu>
> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
> Reviewed-by: Dean Marx <dmarx@iol.unh.edu>
> ---
>  dts/conf.yaml                                | 20 +++++-----
>  dts/framework/config/__init__.py             | 39 ++++++++++----------
>  dts/framework/runner.py                      |  6 +--
>  dts/framework/testbed_model/cpu.py           |  6 ++-
>  dts/framework/testbed_model/linux_session.py |  5 +--
>  dts/framework/testbed_model/node.py          | 27 +-------------
>  dts/framework/testbed_model/os_session.py    |  2 +-
>  dts/framework/testbed_model/sut_node.py      | 14 ++++++-
>  8 files changed, 54 insertions(+), 65 deletions(-)
>
> diff --git a/dts/conf.yaml b/dts/conf.yaml
> index c93eedbc94..bc78882d0d 100644
> --- a/dts/conf.yaml
> +++ b/dts/conf.yaml
> @@ -26,12 +26,11 @@ test_runs:
>      skip_smoke_tests: false # optional
>      test_suites: # the following test suites will be run in their entirety
>        - hello_world
> +    vdevs: # optional; if removed, vdevs won't be used in the execution
> +      - "crypto_openssl"
>      # The machine running the DPDK test executable
> -    system_under_test_node:
> -      node_name: "SUT 1"
> -      vdevs: # optional; if removed, vdevs won't be used in the test run
> -        - "crypto_openssl"
> -    # Traffic generator node to use for this test run
> +    system_under_test_node: "SUT 1"
> +    # Traffic generator node to use for this execution environment
>      traffic_generator_node: "TG 1"
>  nodes:
>    # Define a system under test node, having two network ports physically
> @@ -40,11 +39,6 @@ nodes:
>      hostname: sut1.change.me.localhost
>      user: dtsuser
>      os: linux
> -    lcores: "" # use all available logical cores (Skips first core)
> -    memory_channels: 4 # tells DPDK to use 4 memory channels
> -    hugepages_2mb: # optional; if removed, will use system hugepage configuration
> -        number_of: 256
> -        force_first_numa: false
>      ports:
>        # sets up the physical link between "SUT 1"@0000:00:08.0 and "TG 1"@0000:00:08.0
>        - pci: "0000:00:08.0"
> @@ -58,6 +52,12 @@ nodes:
>          os_driver: i40e
>          peer_node: "TG 1"
>          peer_pci: "0000:00:08.1"
> +    hugepages_2mb: # optional; if removed, will use system hugepage configuration
> +        number_of: 256
> +        force_first_numa: false
> +    dpdk_config:
> +        lcores: "" # use all available logical cores (Skips first core)
> +        memory_channels: 4 # tells DPDK to use 4 memory channels
>    # Define a Scapy traffic generator node, having two network ports
>    # physically connected to the corresponding ports in SUT 1 (the peer node).
>    - name: "TG 1"
> diff --git a/dts/framework/config/__init__.py b/dts/framework/config/__init__.py
> index 5dfa0cf0d4..2496f48e20 100644
> --- a/dts/framework/config/__init__.py
> +++ b/dts/framework/config/__init__.py
> @@ -161,15 +161,23 @@ class NodeConfiguration(FrozenModel):
>      password: str | None = None
>      #: The operating system of the :class:`~framework.testbed_model.node.Node`.
>      os: OS
> -    #: A comma delimited list of logical cores to use when running DPDK. ```any```, an empty
> -    #: string or omitting this field means use any core except for the first one. The first core
> -    #: will only be used if explicitly set.
> -    lcores: LogicalCores = ""
>      #: An optional hugepage configuration.
>      hugepages: HugepageConfiguration | None = Field(None, alias="hugepages_2mb")
>      #: The ports that can be used in testing.
>      ports: list[PortConfig] = Field(min_length=1)
>
> +
> +class DPDKConfiguration(FrozenModel):
> +    """Configuration of the DPDK EAL parameters."""
> +
> +    #: A comma delimited list of logical cores to use when running DPDK. ```any```, an empty
> +    #: string or omitting this field means use any core except for the first one. The first core
> +    #: will only be used if explicitly set.
> +    lcores: LogicalCores = ""
> +
> +    #: The number of memory channels to use when running DPDK.
> +    memory_channels: int = 1
> +
>      @property
>      def use_first_core(self) -> bool:
>          """Returns :data:`True` if `lcores` explicitly selects the first core."""
> @@ -179,8 +187,8 @@ def use_first_core(self) -> bool:
>  class SutNodeConfiguration(NodeConfiguration):
>      """:class:`~framework.testbed_model.sut_node.SutNode` specific configuration."""
>
> -    #: The number of memory channels to use when running DPDK.
> -    memory_channels: int = 1
> +    #: The runtime configuration for DPDK.
> +    dpdk_config: DPDKConfiguration
>
>
>  class TGNodeConfiguration(NodeConfiguration):
> @@ -405,15 +413,6 @@ def validate_names(self) -> Self:
>          return self
>
>
> -class TestRunSUTNodeConfiguration(FrozenModel):
> -    """The SUT node configuration of a test run."""
> -
> -    #: The SUT node to use in this test run.
> -    node_name: str
> -    #: The names of virtual devices to test.
> -    vdevs: list[str] = Field(default_factory=list)
> -
> -
>  class TestRunConfiguration(FrozenModel):
>      """The configuration of a test run.
>
> @@ -431,10 +430,12 @@ class TestRunConfiguration(FrozenModel):
>      skip_smoke_tests: bool = False
>      #: The names of test suites and/or test cases to execute.
>      test_suites: list[TestSuiteConfig] = Field(min_length=1)
> -    #: The SUT node configuration to use in this test run.
> -    system_under_test_node: TestRunSUTNodeConfiguration
> +    #: The SUT node name to use in this test run.
> +    system_under_test_node: str
>      #: The TG node name to use in this test run.
>      traffic_generator_node: str
> +    #: The names of virtual devices to test.
> +    vdevs: list[str] = Field(default_factory=list)
>      #: The seed to use for pseudo-random generation.
>      random_seed: int | None = None
>
> @@ -464,12 +465,12 @@ def test_runs_with_nodes(self) -> list[TestRunWithNodesConfiguration]:
>          test_runs_with_nodes = []
>
>          for test_run_no, test_run in enumerate(self.test_runs):
> -            sut_node_name = test_run.system_under_test_node.node_name
> +            sut_node_name = test_run.system_under_test_node
>              sut_node = next(filter(lambda n: n.name == sut_node_name, self.nodes), None)
>
>              assert sut_node is not None, (
>                  f"test_runs.{test_run_no}.sut_node_config.node_name "
> -                f"({test_run.system_under_test_node.node_name}) is not a valid node name"
> +                f"({test_run.system_under_test_node}) is not a valid node name"
>              )
>              assert isinstance(sut_node, SutNodeConfiguration), (
>                  f"test_runs.{test_run_no}.sut_node_config.node_name is a valid node name, "
> diff --git a/dts/framework/runner.py b/dts/framework/runner.py
> index 510be1a870..0cdbb07e06 100644
> --- a/dts/framework/runner.py
> +++ b/dts/framework/runner.py
> @@ -277,7 +277,7 @@ def _connect_nodes_and_run_test_run(
>                  tg_node = TGNode(tg_node_config)
>                  tg_nodes[tg_node.name] = tg_node
>          except Exception as e:
> -            failed_node = test_run_config.system_under_test_node.node_name
> +            failed_node = test_run_config.system_under_test_node
>              if sut_node:
>                  failed_node = test_run_config.traffic_generator_node
>              self._logger.exception(f"The Creation of node {failed_node} failed.")
> @@ -315,9 +315,7 @@ def _run_test_run(
>          Raises:
>              ConfigurationError: If the DPDK sources or build is not set up from config or settings.
>          """
> -        self._logger.info(
> -            f"Running test run with SUT '{test_run_config.system_under_test_node.node_name}'."
> -        )
> +        self._logger.info(f"Running test run with SUT '{test_run_config.system_under_test_node}'.")
>          test_run_result.ports = sut_node.ports
>          test_run_result.sut_info = sut_node.node_info
>          try:
> diff --git a/dts/framework/testbed_model/cpu.py b/dts/framework/testbed_model/cpu.py
> index d19fa5d597..b8bc601c22 100644
> --- a/dts/framework/testbed_model/cpu.py
> +++ b/dts/framework/testbed_model/cpu.py
> @@ -185,7 +185,6 @@ def __init__(
>
>          # sorting by core is needed in case hyperthreading is enabled
>          self._lcores_to_filter = sorted(lcore_list, key=lambda x: x.core, reverse=not ascending)
> -        self.filter()
>
>      @abstractmethod
>      def filter(self) -> list[LogicalCore]:
> @@ -228,6 +227,8 @@ def filter(self) -> list[LogicalCore]:
>          Returns:
>              The filtered cores.
>          """
> +        if 0 in self._lcores_to_filter:
> +            self._lcores_to_filter = self._lcores_to_filter[1:]
>          sockets_to_filter = self._filter_sockets(self._lcores_to_filter)
>          filtered_lcores = []
>          for socket_to_filter in sockets_to_filter:
> @@ -356,6 +357,9 @@ def filter(self) -> list[LogicalCore]:
>          Raises:
>              ValueError: If the specified lcore filter specifier is invalid.
>          """
> +        if 0 not in self._filter_specifier.lcore_list:
> +            self._lcores_to_filter = self._lcores_to_filter[1:]
> +
>          if not len(self._filter_specifier.lcore_list):
>              return self._lcores_to_filter
>
> diff --git a/dts/framework/testbed_model/linux_session.py b/dts/framework/testbed_model/linux_session.py
> index e3732f0827..41e7656b0f 100644
> --- a/dts/framework/testbed_model/linux_session.py
> +++ b/dts/framework/testbed_model/linux_session.py
> @@ -67,15 +67,12 @@ class LinuxSession(PosixSession):
>      def _get_privileged_command(command: str) -> str:
>          return f"sudo -- sh -c '{command}'"
>
> -    def get_remote_cpus(self, use_first_core: bool) -> list[LogicalCore]:
> +    def get_remote_cpus(self) -> list[LogicalCore]:
>          """Overrides :meth:`~.os_session.OSSession.get_remote_cpus`."""
>          cpu_info = self.send_command("lscpu -p=CPU,CORE,SOCKET,NODE|grep -v \\#").stdout
>          lcores = []
>          for cpu_line in cpu_info.splitlines():
>              lcore, core, socket, node = map(int, cpu_line.split(","))
> -            if core == 0 and socket == 0 and not use_first_core:
> -                self._logger.info("Not using the first physical core.")
> -                continue
>              lcores.append(LogicalCore(lcore, core, socket, node))
>          return lcores
>
> diff --git a/dts/framework/testbed_model/node.py b/dts/framework/testbed_model/node.py
> index b08b1cf14d..6c2dfd6185 100644
> --- a/dts/framework/testbed_model/node.py
> +++ b/dts/framework/testbed_model/node.py
> @@ -24,14 +24,7 @@
>  from framework.exception import ConfigurationError
>  from framework.logger import DTSLogger, get_dts_logger
>
> -from .cpu import (
> -    Architecture,
> -    LogicalCore,
> -    LogicalCoreCount,
> -    LogicalCoreList,
> -    LogicalCoreListFilter,
> -    lcore_filter,
> -)
> +from .cpu import Architecture, LogicalCore, LogicalCoreCount, LogicalCoreList, lcore_filter
>  from .linux_session import LinuxSession
>  from .os_session import OSSession
>  from .port import Port
> @@ -82,24 +75,8 @@ def __init__(self, node_config: NodeConfiguration):
>          self._logger = get_dts_logger(self.name)
>          self.main_session = create_session(self.config, self.name, self._logger)
>          self.arch = Architecture(self.main_session.get_arch_info())
> -
>          self._logger.info(f"Connected to node: {self.name}")
> -
>          self._get_remote_cpus()
> -        # filter the node lcores according to the test run configuration
> -        self.lcores = LogicalCoreListFilter(
> -            self.lcores, LogicalCoreList(self.config.lcores)
> -        ).filter()
> -
> -        if LogicalCore(lcore=0, core=0, socket=0, node=0) in self.lcores:
> -            self._logger.info(
> -                """
> -                WARNING: First core being used;
> -                using the first core is considered risky and should only
> -                be done by advanced users.
> -                """
> -            )
> -
>          self._other_sessions = []
>          self._init_ports()
>
> @@ -188,7 +165,7 @@ def filter_lcores(
>      def _get_remote_cpus(self) -> None:
>          """Scan CPUs in the remote OS and store a list of LogicalCores."""
>          self._logger.info("Getting CPU information.")
> -        self.lcores = self.main_session.get_remote_cpus(self.config.use_first_core)
> +        self.lcores = self.main_session.get_remote_cpus()
>
>      def _setup_hugepages(self) -> None:
>          """Setup hugepages on the node.
> diff --git a/dts/framework/testbed_model/os_session.py b/dts/framework/testbed_model/os_session.py
> index fcda9b3de1..e436886692 100644
> --- a/dts/framework/testbed_model/os_session.py
> +++ b/dts/framework/testbed_model/os_session.py
> @@ -445,7 +445,7 @@ def get_dpdk_version(self, version_path: str | PurePath) -> str:
>          """
>
>      @abstractmethod
> -    def get_remote_cpus(self, use_first_core: bool) -> list[LogicalCore]:
> +    def get_remote_cpus(self) -> list[LogicalCore]:
>          r"""Get the list of :class:`~.cpu.LogicalCore`\s on the remote node.
>
>          Args:
> diff --git a/dts/framework/testbed_model/sut_node.py b/dts/framework/testbed_model/sut_node.py
> index 11d4b22089..d8f1f9d452 100644
> --- a/dts/framework/testbed_model/sut_node.py
> +++ b/dts/framework/testbed_model/sut_node.py
> @@ -33,6 +33,7 @@
>  from framework.remote_session.remote_session import CommandResult
>  from framework.utils import MesonArgs, TarCompressionFormat
>
> +from .cpu import LogicalCore, LogicalCoreList
>  from .node import Node
>  from .os_session import OSSession, OSSessionInfo
>  from .virtual_device import VirtualDevice
> @@ -92,6 +93,17 @@ def __init__(self, node_config: SutNodeConfiguration):
>              node_config: The SUT node's test run configuration.
>          """
>          super().__init__(node_config)
> +        self.lcores = self.filter_lcores(LogicalCoreList(self.config.dpdk_config.lcores))
> +        if LogicalCore(lcore=0, core=0, socket=0, node=0) in self.lcores:
> +            self._logger.info(
> +                """
> +                WARNING: First core being used;
> +                using the first core is considered risky and should only
> +                be done by advanced users.
> +                """
> +            )
> +        else:
> +            self._logger.info("Not using first core")
>          self.virtual_devices = []
>          self.dpdk_prefix_list = []
>          self._env_vars = {}
> @@ -198,7 +210,7 @@ def set_up_test_run(
>              dpdk_build_config: The build configuration of DPDK.
>          """
>          super().set_up_test_run(test_run_config, dpdk_build_config)
> -        for vdev in test_run_config.system_under_test_node.vdevs:
> +        for vdev in test_run_config.vdevs:
>              self.virtual_devices.append(VirtualDevice(vdev))
>          self._set_up_dpdk(dpdk_build_config)
>
> --
> 2.43.0
>