[2/4] dts: Use First Core Logic Change

Message ID 20240613201831.9748-7-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

Commit Message

Nicholas Pratte June 13, 2024, 8:18 p.m. UTC
  Removed use_first_core from the conf.yaml in favor of determining this
within the framework. use_first_core continue to serve a purpose in that
it is only enabled when core 0 is explicitly provided in the
configuration. Any other configuration, including "" or "any," will
omit core 0.

Documentation reworks are included to reflect the changes made.

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

---
 doc/guides/tools/dts.rst                   |  9 +++------
 dts/conf.yaml                              |  3 +--
 dts/framework/config/__init__.py           | 11 +++++++----
 dts/framework/config/conf_yaml_schema.json |  6 +-----
 dts/framework/testbed_model/node.py        |  9 +++++++++
 5 files changed, 21 insertions(+), 17 deletions(-)
  

Comments

Jeremy Spewock June 14, 2024, 6:09 p.m. UTC | #1
On Thu, Jun 13, 2024 at 4:21 PM Nicholas Pratte <npratte@iol.unh.edu> wrote:
>
> Removed use_first_core from the conf.yaml in favor of determining this
> within the framework. use_first_core continue to serve a purpose in that
> it is only enabled when core 0 is explicitly provided in the
> configuration. Any other configuration, including "" or "any," will
> omit core 0.
>
> Documentation reworks are included to reflect the changes made.
>
> Bugzilla ID: 1360
> Signed-off-by: Nicholas Pratte <npratte@iol.unh.edu>
>
> ---
>  doc/guides/tools/dts.rst                   |  9 +++------
>  dts/conf.yaml                              |  3 +--
>  dts/framework/config/__init__.py           | 11 +++++++----
>  dts/framework/config/conf_yaml_schema.json |  6 +-----
>  dts/framework/testbed_model/node.py        |  9 +++++++++
>  5 files changed, 21 insertions(+), 17 deletions(-)
>
> diff --git a/doc/guides/tools/dts.rst b/doc/guides/tools/dts.rst
> index da85295db9..fbb5c6f17b 100644
> --- a/doc/guides/tools/dts.rst
> +++ b/doc/guides/tools/dts.rst
> @@ -546,15 +546,12 @@ involved in the testing. These can be defined with the following mappings:
>     +-----------------------+---------------------------------------------------------------------------------------+
>     | ``os``                | The operating system of this node. See `OS`_ for supported values.                    |
>     +-----------------------+---------------------------------------------------------------------------------------+
> -   | ``lcores``            | | (*optional*, defaults to 1) *string* – Comma-separated list of logical              |
> -   |                       | | cores to use. An empty string means use all lcores.                                 |
> +   | ``lcores``            | | (*optional*, defaults to 1 if not used) *string* – Comma-separated list of logical  |

I think just leaving this as "defaults to 1" is fine here. It's more
explicit to say "if it isn't used", but just saying it defaults I
think implies that enough.

> +   |                       | | cores to use. An empty string means use all lcores except core 0. core 0 is used    |
> +   |                       | | only when explicitly specified                                                      |
>     |                       |                                                                                       |
>     |                       | **Example**: ``1,2,3,4,5,18-22``                                                      |
>     +-----------------------+---------------------------------------------------------------------------------------+
> -   | ``use_first_core``    | (*optional*, defaults to ``false``) *boolean*                                         |
> -   |                       |                                                                                       |
> -   |                       | Indicates whether DPDK should use only the first physical core or not.                |
> -   +-----------------------+---------------------------------------------------------------------------------------+
>     | ``memory_channels``   | (*optional*, defaults to 1) *integer*                                                 |
>     |                       |                                                                                       |
>     |                       | The number of the memory channels to use.                                             |
<snip>
> diff --git a/dts/framework/config/__init__.py b/dts/framework/config/__init__.py
> index 5a127a1207..6bc290a56a 100644
> --- a/dts/framework/config/__init__.py
> +++ b/dts/framework/config/__init__.py
> @@ -245,6 +245,9 @@ 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,8 +258,8 @@ def from_dict(
>                  password=d.get("password"),
>                  arch=Architecture(d["arch"]),
>                  os=OS(d["os"]),
> -                lcores=d.get("lcores", "1"),
> -                use_first_core=d.get("use_first_core", False),
> +                lcores=lcores,
> +                use_first_core=use_first_core,

I wonder if we could completely remove the "use_first_core" attribute
from the config classes. It seems like it's only used when you're
getting remote CPUs to skip core 0 based on the condition. With these
new changes it seems like we just assume that if 0 is in the list, we
will use it, otherwise we simply won't, so I don't think we need this
condition to detect whether we should skip or not anymore, do we?

>                  hugepages=hugepage_config,
>                  ports=[PortConfig.from_dict(d["name"], port) for port in d["ports"]],
>                  traffic_generator=TrafficGeneratorConfig.from_dict(d["traffic_generator"]),
> @@ -269,8 +272,8 @@ def from_dict(
>                  password=d.get("password"),
>                  arch=Architecture(d["arch"]),
>                  os=OS(d["os"]),
> -                lcores=d.get("lcores", "1"),
> -                use_first_core=d.get("use_first_core", False),
> +                lcores=lcores,
> +                use_first_core=use_first_core,
>                  hugepages=hugepage_config,
>                  ports=[PortConfig.from_dict(d["name"], port) for port in d["ports"]],
>                  memory_channels=d.get("memory_channels", 1),
<snip>
> diff --git a/dts/framework/testbed_model/node.py b/dts/framework/testbed_model/node.py
> index 74061f6262..470cd18e30 100644
> --- a/dts/framework/testbed_model/node.py
> +++ b/dts/framework/testbed_model/node.py
> @@ -93,6 +93,15 @@ def __init__(self, node_config: NodeConfiguration):
>              self.lcores, LogicalCoreList(self.config.lcores)
>          ).filter()
>
> +        if LogicalCore(lcore=0, core=0, socket=0, node=0) in self.lcores:

This condition fits if you completely remove the `use_first_core`
boolean from the NodeConfiguration, but if we decide not to I think it
could be shortened to just:

if config.use_first_core:



> +            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()
> --
> 2.44.0
>
  
Nicholas Pratte June 20, 2024, 1:41 p.m. UTC | #2
On Fri, Jun 14, 2024 at 2:09 PM Jeremy Spewock <jspewock@iol.unh.edu> wrote:
>
> On Thu, Jun 13, 2024 at 4:21 PM Nicholas Pratte <npratte@iol.unh.edu> wrote:
> >
> > Removed use_first_core from the conf.yaml in favor of determining this
> > within the framework. use_first_core continue to serve a purpose in that
> > it is only enabled when core 0 is explicitly provided in the
> > configuration. Any other configuration, including "" or "any," will
> > omit core 0.
> >
> > Documentation reworks are included to reflect the changes made.
> >
> > Bugzilla ID: 1360
> > Signed-off-by: Nicholas Pratte <npratte@iol.unh.edu>
> >
> > ---
> >  doc/guides/tools/dts.rst                   |  9 +++------
> >  dts/conf.yaml                              |  3 +--
> >  dts/framework/config/__init__.py           | 11 +++++++----
> >  dts/framework/config/conf_yaml_schema.json |  6 +-----
> >  dts/framework/testbed_model/node.py        |  9 +++++++++
> >  5 files changed, 21 insertions(+), 17 deletions(-)
> >
> > diff --git a/doc/guides/tools/dts.rst b/doc/guides/tools/dts.rst
> > index da85295db9..fbb5c6f17b 100644
> > --- a/doc/guides/tools/dts.rst
> > +++ b/doc/guides/tools/dts.rst
> > @@ -546,15 +546,12 @@ involved in the testing. These can be defined with the following mappings:
> >     +-----------------------+---------------------------------------------------------------------------------------+
> >     | ``os``                | The operating system of this node. See `OS`_ for supported values.                    |
> >     +-----------------------+---------------------------------------------------------------------------------------+
> > -   | ``lcores``            | | (*optional*, defaults to 1) *string* – Comma-separated list of logical              |
> > -   |                       | | cores to use. An empty string means use all lcores.                                 |
> > +   | ``lcores``            | | (*optional*, defaults to 1 if not used) *string* – Comma-separated list of logical  |
>
> I think just leaving this as "defaults to 1" is fine here. It's more
> explicit to say "if it isn't used", but just saying it defaults I
> think implies that enough.

Good point, and you're right. '*optional* and 'if not used' are redundant.

>
> > +   |                       | | cores to use. An empty string means use all lcores except core 0. core 0 is used    |
> > +   |                       | | only when explicitly specified                                                      |
> >     |                       |                                                                                       |
> >     |                       | **Example**: ``1,2,3,4,5,18-22``                                                      |
> >     +-----------------------+---------------------------------------------------------------------------------------+
> > -   | ``use_first_core``    | (*optional*, defaults to ``false``) *boolean*                                         |
> > -   |                       |                                                                                       |
> > -   |                       | Indicates whether DPDK should use only the first physical core or not.                |
> > -   +-----------------------+---------------------------------------------------------------------------------------+
> >     | ``memory_channels``   | (*optional*, defaults to 1) *integer*                                                 |
> >     |                       |                                                                                       |
> >     |                       | The number of the memory channels to use.                                             |
> <snip>
> > diff --git a/dts/framework/config/__init__.py b/dts/framework/config/__init__.py
> > index 5a127a1207..6bc290a56a 100644
> > --- a/dts/framework/config/__init__.py
> > +++ b/dts/framework/config/__init__.py
> > @@ -245,6 +245,9 @@ 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,8 +258,8 @@ def from_dict(
> >                  password=d.get("password"),
> >                  arch=Architecture(d["arch"]),
> >                  os=OS(d["os"]),
> > -                lcores=d.get("lcores", "1"),
> > -                use_first_core=d.get("use_first_core", False),
> > +                lcores=lcores,
> > +                use_first_core=use_first_core,
>
> I wonder if we could completely remove the "use_first_core" attribute
> from the config classes. It seems like it's only used when you're
> getting remote CPUs to skip core 0 based on the condition. With these
> new changes it seems like we just assume that if 0 is in the list, we
> will use it, otherwise we simply won't, so I don't think we need this
> condition to detect whether we should skip or not anymore, do we?

It's an interesting point, and it's one that I honestly thought about
right after I completed the last patch in this series (DPDK_config
config attribute). It wasn't until I tried implementing the
'dpdk_config' attribute that I realized the broader scope of lcore
filtering in the framework. The framework is currently filtering a
list of lcores in two spots of the framework. The last patch, as you
saw, addresses the aforementioned problem by removing 'use_first_core'
from the framework completely.

In retrospect, I could have dropped this commit and just included the
'dpdk_config' patch with the 'use_first_core' changes included, but I
suppose it's not bad for others to see both implementations and
discuss what the better option is.

>
> >                  hugepages=hugepage_config,
> >                  ports=[PortConfig.from_dict(d["name"], port) for port in d["ports"]],
> >                  traffic_generator=TrafficGeneratorConfig.from_dict(d["traffic_generator"]),
> > @@ -269,8 +272,8 @@ def from_dict(
> >                  password=d.get("password"),
> >                  arch=Architecture(d["arch"]),
> >                  os=OS(d["os"]),
> > -                lcores=d.get("lcores", "1"),
> > -                use_first_core=d.get("use_first_core", False),
> > +                lcores=lcores,
> > +                use_first_core=use_first_core,
> >                  hugepages=hugepage_config,
> >                  ports=[PortConfig.from_dict(d["name"], port) for port in d["ports"]],
> >                  memory_channels=d.get("memory_channels", 1),
> <snip>
> > diff --git a/dts/framework/testbed_model/node.py b/dts/framework/testbed_model/node.py
> > index 74061f6262..470cd18e30 100644
> > --- a/dts/framework/testbed_model/node.py
> > +++ b/dts/framework/testbed_model/node.py
> > @@ -93,6 +93,15 @@ def __init__(self, node_config: NodeConfiguration):
> >              self.lcores, LogicalCoreList(self.config.lcores)
> >          ).filter()
> >
> > +        if LogicalCore(lcore=0, core=0, socket=0, node=0) in self.lcores:
>
> This condition fits if you completely remove the `use_first_core`
> boolean from the NodeConfiguration, but if we decide not to I think it
> could be shortened to just:
>
> if config.use_first_core:

Noted, and good catch! I'll keep this in mind depending on which way
the wind blows.
  

Patch

diff --git a/doc/guides/tools/dts.rst b/doc/guides/tools/dts.rst
index da85295db9..fbb5c6f17b 100644
--- a/doc/guides/tools/dts.rst
+++ b/doc/guides/tools/dts.rst
@@ -546,15 +546,12 @@  involved in the testing. These can be defined with the following mappings:
    +-----------------------+---------------------------------------------------------------------------------------+
    | ``os``                | The operating system of this node. See `OS`_ for supported values.                    |
    +-----------------------+---------------------------------------------------------------------------------------+
-   | ``lcores``            | | (*optional*, defaults to 1) *string* – Comma-separated list of logical              |
-   |                       | | cores to use. An empty string means use all lcores.                                 |
+   | ``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                                                      |
    |                       |                                                                                       |
    |                       | **Example**: ``1,2,3,4,5,18-22``                                                      |
    +-----------------------+---------------------------------------------------------------------------------------+
-   | ``use_first_core``    | (*optional*, defaults to ``false``) *boolean*                                         |
-   |                       |                                                                                       |
-   |                       | Indicates whether DPDK should use only the first physical core or not.                |
-   +-----------------------+---------------------------------------------------------------------------------------+
    | ``memory_channels``   | (*optional*, defaults to 1) *integer*                                                 |
    |                       |                                                                                       |
    |                       | The number of the memory channels to use.                                             |
diff --git a/dts/conf.yaml b/dts/conf.yaml
index 672e6f92b6..c20afb9621 100644
--- a/dts/conf.yaml
+++ b/dts/conf.yaml
@@ -29,8 +29,7 @@  nodes:
     user: dtsuser
     arch: x86_64
     os: linux
-    lcores: "" # use all the available logical cores
-    use_first_core: false # tells DPDK to use any physical core
+    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
diff --git a/dts/framework/config/__init__.py b/dts/framework/config/__init__.py
index 5a127a1207..6bc290a56a 100644
--- a/dts/framework/config/__init__.py
+++ b/dts/framework/config/__init__.py
@@ -245,6 +245,9 @@  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,8 +258,8 @@  def from_dict(
                 password=d.get("password"),
                 arch=Architecture(d["arch"]),
                 os=OS(d["os"]),
-                lcores=d.get("lcores", "1"),
-                use_first_core=d.get("use_first_core", False),
+                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"]),
@@ -269,8 +272,8 @@  def from_dict(
                 password=d.get("password"),
                 arch=Architecture(d["arch"]),
                 os=OS(d["os"]),
-                lcores=d.get("lcores", "1"),
-                use_first_core=d.get("use_first_core", False),
+                lcores=lcores,
+                use_first_core=use_first_core,
                 hugepages=hugepage_config,
                 ports=[PortConfig.from_dict(d["name"], port) for port in d["ports"]],
                 memory_channels=d.get("memory_channels", 1),
diff --git a/dts/framework/config/conf_yaml_schema.json b/dts/framework/config/conf_yaml_schema.json
index bf0be28176..7c8429abbc 100644
--- a/dts/framework/config/conf_yaml_schema.json
+++ b/dts/framework/config/conf_yaml_schema.json
@@ -163,13 +163,9 @@ 
           },
           "lcores": {
             "type": "string",
-            "pattern": "^(([0-9]+|([0-9]+-[0-9]+))(,([0-9]+|([0-9]+-[0-9]+)))*)?$",
+            "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."
           },
-          "use_first_core": {
-            "type": "boolean",
-            "description": "Indicate whether DPDK should use the first physical core. It won't be used by default."
-          },
           "memory_channels": {
             "type": "integer",
             "description": "How many memory channels to use. Optional, defaults to 1."
diff --git a/dts/framework/testbed_model/node.py b/dts/framework/testbed_model/node.py
index 74061f6262..470cd18e30 100644
--- a/dts/framework/testbed_model/node.py
+++ b/dts/framework/testbed_model/node.py
@@ -93,6 +93,15 @@  def __init__(self, node_config: NodeConfiguration):
             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()