On Fri, Jun 14, 2024 at 2:09 PM Jeremy Spewock <jspew...@iol.unh.edu> wrote:
>
> On Thu, Jun 13, 2024 at 4:21 PM Nicholas Pratte <npra...@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 <npra...@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.
