[4/4] dts: Rework DPDK Attributes In SUT Node Config

Message ID 20240613201831.9748-11-npratte@iol.unh.edu (mailing list archive)
State Superseded
Delegated to: Thomas Monjalon
Headers
Series dts: Remove Excess Attributes From User Config |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/intel-Functional success Functional PASS
ci/github-robot: build success github build: passed
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-unit-amd64-testing success Testing PASS
ci/iol-compile-amd64-testing success Testing PASS
ci/iol-sample-apps-testing success Testing PASS
ci/iol-unit-arm64-testing success Testing PASS
ci/iol-compile-arm64-testing success Testing PASS

Commit Message

Nicholas Pratte June 13, 2024, 8:18 p.m. UTC
  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 'executions.'

Bugzilla ID: 1360
Signed-off-by: Nicholas Pratte <npratte@iol.unh.edu>

---
 doc/guides/tools/dts.rst                     |  2 +
 dts/conf.yaml                                | 18 +++----
 dts/framework/config/__init__.py             | 45 +++++++++++-------
 dts/framework/config/conf_yaml_schema.json   | 49 ++++++++++----------
 dts/framework/config/types.py                | 30 ++++++------
 dts/framework/testbed_model/cpu.py           |  5 ++
 dts/framework/testbed_model/linux_session.py |  5 +-
 dts/framework/testbed_model/node.py          | 26 +----------
 dts/framework/testbed_model/os_session.py    |  2 +-
 dts/framework/testbed_model/sut_node.py      | 17 ++++++-
 10 files changed, 100 insertions(+), 99 deletions(-)
  

Comments

Jeremy Spewock June 14, 2024, 6:11 p.m. UTC | #1
Funny, this commit addresses a comment I had on the previous. I think
it makes a lot of sense to split the EAL parameter information into a
DPDK specific config that the TG doesn't have since it likely won't
need it.

On Thu, Jun 13, 2024 at 4:22 PM Nicholas Pratte <npratte@iol.unh.edu> wrote:
>
> 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

I think it makes more sense to do this in the commit where you removed
it from the config as well and just completely take it out. There
isn't really a need to keep it in the framework in that commit so I'd
be more in favor of removing it from there entirely and then this
commit won't need to since it's less relevant here.

> 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 'executions.'
>
> Bugzilla ID: 1360
> Signed-off-by: Nicholas Pratte <npratte@iol.unh.edu>
>
> ---
<snip>
> diff --git a/dts/framework/testbed_model/cpu.py b/dts/framework/testbed_model/cpu.py
> index 9e33b2825d..0c315a0da6 100644
> --- a/dts/framework/testbed_model/cpu.py
> +++ b/dts/framework/testbed_model/cpu.py
> @@ -210,6 +210,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:
> @@ -328,6 +330,9 @@ def filter(self) -> list[LogicalCore]:
>          Return:
>              The filtered logical CPU cores.
>          """
> +        if 0 not in self._filter_specifier.lcore_list:
> +            self._lcores_to_filter = self._lcores_to_filter[1:]
> +

I don't really understand what these two conditionals are doing. if 0
is in the lcore_list, why do we need to omit the first value from the
filter list? Or if it is in the cores to filter why do we need to
remove the first element from that list? Also, is this attempting to
omit core 0 in the list? I think where it appears in the list can be
different depending on if the list is ascending or descending which is
different depending on the EAL parameters function it seems.
Regardless, can we add a comment on why this is needed?

>          if not len(self._filter_specifier.lcore_list):
>              return self._lcores_to_filter
>
<snip>
> 2.44.0
>
  

Patch

diff --git a/doc/guides/tools/dts.rst b/doc/guides/tools/dts.rst
index 0453a15a73..9d780d5dcd 100644
--- a/doc/guides/tools/dts.rst
+++ b/doc/guides/tools/dts.rst
@@ -544,6 +544,8 @@  involved in the testing. These can be defined with the following mappings:
    +-----------------------+---------------------------------------------------------------------------------------+
    | ``os``                | The operating system of this node. See `OS`_ for supported values.                    |
    +-----------------------+---------------------------------------------------------------------------------------+
+   | ``dpdk_config``       | Configuration relating to DPDK (to be specified on SUT Nodes)                         |
+   +-----------------------+---------------------------------------------------------------------------------------+
    | ``lcores``            | | (*optional*, defaults to 1 if not used) *string* – Comma-separated list of logical  |
    |                       | | cores to use. An empty string means use all lcores except core 0. core 0 is used    |
    |                       | | only when explicitly specified                                                      |
diff --git a/dts/conf.yaml b/dts/conf.yaml
index b9f5704ca5..b7a2ed567d 100644
--- a/dts/conf.yaml
+++ b/dts/conf.yaml
@@ -14,11 +14,10 @@  executions:
     test_suites: # the following test suites will be run in their entirety
       - hello_world
       - os_udp
+    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 execution
-        - "crypto_openssl"
+    system_under_test_node: "SUT 1"
     # Traffic generator node to use for this execution environment
     traffic_generator_node: "TG 1"
 nodes:
@@ -28,11 +27,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:  # optional; if removed, will use system hugepage configuration
-        amount: 256
-        force_first_numa: false
     ports:
       # sets up the physical link between "SUT 1"@000:00:08.0 and "TG 1"@0000:00:08.0
       - pci: "0000:00:08.0"
@@ -46,6 +40,12 @@  nodes:
         os_driver: i40e
         peer_node: "TG 1"
         peer_pci: "0000:00:08.1"
+    hugepages:  # optional; if removed, will use system hugepage configuration
+        amount: 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 07b85a6afb..9ca70b3fdd 100644
--- a/dts/framework/config/__init__.py
+++ b/dts/framework/config/__init__.py
@@ -208,8 +208,6 @@  class NodeConfiguration:
         password: The password of the user. The use of passwords is heavily discouraged.
             Please use keys instead.
         os: The operating system of the :class:`~framework.testbed_model.node.Node`.
-        lcores: A comma delimited list of logical cores to use when running DPDK.
-        use_first_core: If :data:`True`, the first logical core won't be used.
         hugepages: An optional hugepage configuration.
         ports: The ports that can be used in testing.
     """
@@ -219,8 +217,6 @@  class NodeConfiguration:
     user: str
     password: str | None
     os: OS
-    lcores: str
-    use_first_core: bool
     hugepages: HugepageConfiguration | None
     ports: list[PortConfig]
 
@@ -243,9 +239,6 @@  def from_dict(
                 hugepage_config_dict["force_first_numa"] = False
             hugepage_config = HugepageConfiguration(**hugepage_config_dict)
 
-        lcores = "1" if "lcores" not in d else d["lcores"] if "any" not in d["lcores"] else ""
-        use_first_core = "0" in lcores
-
         # The calls here contain duplicated code which is here because Mypy doesn't
         # properly support dictionary unpacking with TypedDicts
         if "traffic_generator" in d:
@@ -255,36 +248,54 @@  def from_dict(
                 user=d["user"],
                 password=d.get("password"),
                 os=OS(d["os"]),
-                lcores=lcores,
-                use_first_core=use_first_core,
                 hugepages=hugepage_config,
                 ports=[PortConfig.from_dict(d["name"], port) for port in d["ports"]],
                 traffic_generator=TrafficGeneratorConfig.from_dict(d["traffic_generator"]),
             )
         else:
+            dpdk_config = d["dpdk_config"]
+            dpdk_config["lcores"] = (
+                "1"
+                if "lcores" not in dpdk_config
+                else dpdk_config["lcores"]
+                if "any" not in dpdk_config["lcores"]
+                else ""
+            )
+            dpdk_config["memory_channels"] = dpdk_config.get("memory_channels", 1)
             return SutNodeConfiguration(
                 name=d["name"],
                 hostname=d["hostname"],
                 user=d["user"],
                 password=d.get("password"),
                 os=OS(d["os"]),
-                lcores=lcores,
-                use_first_core=use_first_core,
+                dpdk_config=DPDKConfig(**dpdk_config),
                 hugepages=hugepage_config,
                 ports=[PortConfig.from_dict(d["name"], port) for port in d["ports"]],
-                memory_channels=d.get("memory_channels", 1),
             )
 
 
+@dataclass(slots=True, frozen=True)
+class DPDKConfig:
+    """EAL parameters for executing and running DPDK.
+
+    Attributes:
+        lcores: Logical cores to be used for DPDK execution.
+        memory_channels: Memory channels to be used for DPDK execution.
+    """
+
+    lcores: str
+    memory_channels: int
+
+
 @dataclass(slots=True, frozen=True)
 class SutNodeConfiguration(NodeConfiguration):
     """:class:`~framework.testbed_model.sut_node.SutNode` specific configuration.
 
     Attributes:
-        memory_channels: The number of memory channels to use when running DPDK.
+        dpdk_config: DPDK configuration attributes to be used during execution.
     """
 
-    memory_channels: int
+    dpdk_config: DPDKConfig
 
 
 @dataclass(slots=True, frozen=True)
@@ -447,7 +458,7 @@  def from_dict(
             map(BuildTargetConfiguration.from_dict, d["build_targets"])
         )
         test_suites: list[TestSuiteConfig] = list(map(TestSuiteConfig.from_dict, d["test_suites"]))
-        sut_name = d["system_under_test_node"]["node_name"]
+        sut_name = d["system_under_test_node"]
         skip_smoke_tests = d.get("skip_smoke_tests", False)
         assert sut_name in node_map, f"Unknown SUT {sut_name} in execution {d}"
         system_under_test_node = node_map[sut_name]
@@ -462,9 +473,7 @@  def from_dict(
             traffic_generator_node, TGNodeConfiguration
         ), f"Invalid TG configuration {traffic_generator_node}"
 
-        vdevs = (
-            d["system_under_test_node"]["vdevs"] if "vdevs" in d["system_under_test_node"] else []
-        )
+        vdevs = d["vdevs"] if "vdevs" in d else []
         return ExecutionConfiguration(
             build_targets=build_targets,
             perf=d["perf"],
diff --git a/dts/framework/config/conf_yaml_schema.json b/dts/framework/config/conf_yaml_schema.json
index 49db384967..58e308a76c 100644
--- a/dts/framework/config/conf_yaml_schema.json
+++ b/dts/framework/config/conf_yaml_schema.json
@@ -150,18 +150,25 @@ 
           "os": {
             "$ref": "#/definitions/OS"
           },
-          "lcores": {
-            "type": "string",
-            "pattern": "^(([0-9]+|([0-9]+-[0-9]+))(,([0-9]+|([0-9]+-[0-9]+)))*)?$|any",
-            "description": "Optional comma-separated list of logical cores to use, e.g.: 1,2,3,4,5,18-22. Defaults to 1. An empty string means use all lcores."
-          },
-          "memory_channels": {
-            "type": "integer",
-            "description": "How many memory channels to use. Optional, defaults to 1."
-          },
           "hugepages": {
             "$ref": "#/definitions/hugepages"
           },
+          "dpdk_config": {
+            "type": "object",
+            "description": "EAL arguments for DPDK execution",
+            "properties": {
+              "lcores": {
+                "type": "string",
+                "pattern": "^(([0-9]+|([0-9]+-[0-9]+))(,([0-9]+|([0-9]+-[0-9]+)))*)?$|any",
+                "description": "Optional comma-separated list of logical cores to use, e.g.: 1,2,3,4,5,18-22. Defaults to 1. An empty string means use all lcores."
+              },
+              "memory_channels": {
+                "type": "integer",
+                "description": "How many memory channels to use. Optional, defaults to 1."
+              }
+            },
+            "minimum": 1
+          },
           "ports": {
             "type": "array",
             "items": {
@@ -264,23 +271,15 @@ 
             "description": "Optional field that allows you to skip smoke testing",
             "type": "boolean"
           },
+          "vdevs": {
+            "description": "Optional list of names of vdevs to be used in execution",
+            "type": "array",
+            "items": {
+              "type": "string"
+            }
+          },
           "system_under_test_node": {
-            "type":"object",
-            "properties": {
-              "node_name": {
-                "$ref": "#/definitions/node_name"
-              },
-              "vdevs": {
-                "description": "Optional list of names of vdevs to be used in execution",
-                "type": "array",
-                "items": {
-                  "type": "string"
-                }
-              }
-            },
-            "required": [
-              "node_name"
-            ]
+            "$ref": "#/definitions/node_name"
           },
           "traffic_generator_node": {
             "$ref": "#/definitions/node_name"
diff --git a/dts/framework/config/types.py b/dts/framework/config/types.py
index c841ab2d7c..fb760daa9f 100644
--- a/dts/framework/config/types.py
+++ b/dts/framework/config/types.py
@@ -33,6 +33,15 @@  class TrafficGeneratorConfigDict(TypedDict):
     type: str
 
 
+class DPDKConfigDict(TypedDict):
+    """Allowed keys and values."""
+
+    #:
+    memory_channels: int
+    #:
+    lcores: str
+
+
 class HugepageConfigurationDict(TypedDict):
     """Allowed keys and values."""
 
@@ -58,15 +67,11 @@  class NodeConfigDict(TypedDict):
     #:
     os: str
     #:
-    lcores: str
-    #:
-    use_first_core: bool
-    #:
     ports: list[PortConfigDict]
     #:
-    memory_channels: int
-    #:
     traffic_generator: TrafficGeneratorConfigDict
+    #:
+    dpdk_config: DPDKConfigDict
 
 
 class BuildTargetConfigDict(TypedDict):
@@ -87,15 +92,6 @@  class TestSuiteConfigDict(TypedDict):
     cases: list[str]
 
 
-class ExecutionSUTConfigDict(TypedDict):
-    """Allowed keys and values."""
-
-    #:
-    node_name: str
-    #:
-    vdevs: list[str]
-
-
 class ExecutionConfigDict(TypedDict):
     """Allowed keys and values."""
 
@@ -110,9 +106,11 @@  class ExecutionConfigDict(TypedDict):
     #:
     test_suites: TestSuiteConfigDict
     #:
-    system_under_test_node: ExecutionSUTConfigDict
+    system_under_test_node: str
     #:
     traffic_generator_node: str
+    #:
+    vdevs: list[str]
 
 
 class ConfigurationDict(TypedDict):
diff --git a/dts/framework/testbed_model/cpu.py b/dts/framework/testbed_model/cpu.py
index 9e33b2825d..0c315a0da6 100644
--- a/dts/framework/testbed_model/cpu.py
+++ b/dts/framework/testbed_model/cpu.py
@@ -210,6 +210,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:
@@ -328,6 +330,9 @@  def filter(self) -> list[LogicalCore]:
         Return:
             The filtered logical CPU cores.
         """
+        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 5d24030c3d..9d887ef6db 100644
--- a/dts/framework/testbed_model/linux_session.py
+++ b/dts/framework/testbed_model/linux_session.py
@@ -68,15 +68,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 ee4577cf35..19be6c8c4f 100644
--- a/dts/framework/testbed_model/node.py
+++ b/dts/framework/testbed_model/node.py
@@ -27,13 +27,7 @@ 
 from framework.logger import DTSLogger, get_dts_logger
 from framework.settings import SETTINGS
 
-from .cpu import (
-    LogicalCore,
-    LogicalCoreCount,
-    LogicalCoreList,
-    LogicalCoreListFilter,
-    lcore_filter,
-)
+from .cpu import LogicalCore, LogicalCoreCount, LogicalCoreList, lcore_filter
 from .linux_session import LinuxSession
 from .os_session import InteractiveShellType, OSSession
 from .port import Port
@@ -87,24 +81,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.virtual_devices = []
         self._init_ports()
@@ -269,7 +247,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 e082102b00..07b0189368 100644
--- a/dts/framework/testbed_model/os_session.py
+++ b/dts/framework/testbed_model/os_session.py
@@ -314,7 +314,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 34213f6884..741d8f3cea 100644
--- a/dts/framework/testbed_model/sut_node.py
+++ b/dts/framework/testbed_model/sut_node.py
@@ -27,7 +27,7 @@ 
 from framework.settings import SETTINGS
 from framework.utils import MesonArgs
 
-from .cpu import LogicalCoreCount, LogicalCoreList
+from .cpu import LogicalCore, LogicalCoreCount, LogicalCoreList, LogicalCoreListFilter
 from .node import Node
 from .os_session import InteractiveShellType, OSSession
 from .port import Port
@@ -131,6 +131,19 @@  def __init__(self, node_config: SutNodeConfiguration):
             node_config: The SUT node's test run configuration.
         """
         super(SutNode, self).__init__(node_config)
+        self.lcores = LogicalCoreListFilter(
+            self.lcores, LogicalCoreList(self.config.dpdk_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.
+                """
+            )
+        else:
+            self._logger.info("Not using first core")
         self._dpdk_prefix_list = []
         self._build_target_config = None
         self._env_vars = {}
@@ -395,7 +408,7 @@  def create_eal_parameters(
 
         return EalParameters(
             lcore_list=lcore_list,
-            memory_channels=self.config.memory_channels,
+            memory_channels=self.config.dpdk_config.memory_channels,
             prefix=prefix,
             no_pci=no_pci,
             vdevs=vdevs,