Re: [PATCH v2 2/6] dts: Use First Core Logic Change

["Juraj Linkeš" <juraj.linkes@pantheon.tech>]

On 5. 7. 2024 19:13, Nicholas Pratte 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>
> ---
>   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 +++++++++
>   4 files changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/dts/conf.yaml b/dts/conf.yaml
> index 56cc08ced2..53192e0761 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)

The message in parentheses could be confusing, let's make it explicit 
that an empty string skips first core.

And the comments in this file are all lowercase so let's do that in 
parentheses as well.

>       memory_channels: 4 # tells DPDK to use 4 memory channels
>       hugepages_2mb: # optional; if removed, will use system hugepage configuration
>           number_of: 256
> diff --git a/dts/framework/config/__init__.py b/dts/framework/config/__init__.py
> index 456a8a83ab..4c05373ef3 100644
> --- a/dts/framework/config/__init__.py
> +++ b/dts/framework/config/__init__.py
> @@ -246,6 +246,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
> +

I'm wondering whether we want to do this in config. The logic seems to 
be better placed elsewhere. My thinking is this:
1. Removing use_first_core from Node._get_remote_cpus() (and the 
downstream OSSession methods) makes sense since the method really looks 
like it should be getting all remote CPUs. A benefit here is the first 
core filtering doesn't happen in OSSession where it currently is - it 
just doesn't make sense there.
2. Putting use_first_core to LogicalCoreList also doesn't make much 
sense, it's supposed to be a list and not supposed to do any filtering.
3. The two above are used to compose the final list of usable cores on a 
node. We could either filter the first core from 1. after getting the 
cores, we could filter it from 2. after getting the core list or filter 
it after using 1. and 2. with LogicalCoreListFilter().

Or in other words:
remote_lcores = self.main_session.get_remote_cpus()
lcore_list_config = LogicalCoreList(self.config.lcores)
self.lcores = LogicalCoreListFilter(remote_lcores, 
lcore_list_config).filter()

We can either remove the first core from remote_lcores, 
lcore_list_config or from self.lcores. The result is the same if we 
remove it from any of these, so maybe we just work with self.lcores as 
that is also what you've used in the patch to issue the warning.

The logic of whether to remove the core or not would be in Node (if 
self.config.lcores is either "" or "any").

> diff --git a/dts/framework/config/conf_yaml_schema.json b/dts/framework/config/conf_yaml_schema.json
> index 3f7bc2acae..01a6afdc72 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",

This should be added to the description.

>               "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."
>             },

> diff --git a/dts/framework/testbed_model/node.py b/dts/framework/testbed_model/node.py
> index 12a40170ac..9b3f01f1e9 100644
> --- a/dts/framework/testbed_model/node.py
> +++ b/dts/framework/testbed_model/node.py
> @@ -86,6 +86,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(

If this is a warning, then we should call self._logger.warning()

> +                """
> +                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()
>