[PATCH] rteval: Add relative cpulists for loads

[PATCH] rteval: Add relative cpulists for loads

[tglozar]

From: Tomas Glozar <tglozar@redhat.com>

Relative cpulists were added for measurements in 64ce7848 ("rteval: Add
relative cpulists for measurements").

Add support for relative cpulists also for loads. This works the same
way as for measurements using parse_cpulist_from_config, only difference
is there is no --loads-run-on-isolcpus option.

Signed-off-by: Tomas Glozar <tglozar@redhat.com>
---
 rteval-cmd | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/rteval-cmd b/rteval-cmd
index a5e8746..8572022 100755
--- a/rteval-cmd
+++ b/rteval-cmd
@@ -340,28 +340,29 @@ if __name__ == '__main__':
         ldcfg = config.GetSection('loads')
         msrcfg = config.GetSection('measurement')
         msrcfg_cpulist_present = msrcfg.cpulist != ""
-        # Parse measurement cpulist using parse_cpulist_from_config to account for run-on-isolcpus
+        ldcfg_cpulist_present = ldcfg.cpulist != ""
+        # Parse cpulists using parse_cpulist_from_config to account for run-on-isolcpus
         # and relative cpusets
         cpulist = parse_cpulist_from_config(msrcfg.cpulist, msrcfg.run_on_isolcpus)
         if msrcfg_cpulist_present and not cpulist_utils.is_relative(msrcfg.cpulist) and msrcfg.run_on_isolcpus:
             logger.log(Log.WARN, "ignoring --measurement-run-on-isolcpus, since cpulist is specified")
         msrcfg.cpulist = collapse_cpulist(cpulist)
-        if ldcfg.cpulist:
-            ldcfg.cpulist = remove_offline(ldcfg.cpulist)
+        cpulist = parse_cpulist_from_config(ldcfg.cpulist, run_on_isolcpus=False)
+        ldcfg.cpulist = collapse_cpulist(cpulist)
         # if we only specified one set of cpus (loads or measurement)
         # default the other to the inverse of the specified list
-        if not ldcfg.cpulist and msrcfg_cpulist_present:
+        if not ldcfg_cpulist_present and msrcfg_cpulist_present:
             tmplist = expand_cpulist(msrcfg.cpulist)
             tmplist = SysTopology().invert_cpulist(tmplist)
             tmplist = cpulist_utils.online_cpulist(tmplist)
             ldcfg.cpulist = collapse_cpulist(tmplist)
-        if not msrcfg_cpulist_present and ldcfg.cpulist:
+        if not msrcfg_cpulist_present and ldcfg_cpulist_present:
             tmplist = expand_cpulist(ldcfg.cpulist)
             tmplist = SysTopology().invert_cpulist(tmplist)
             tmplist = cpulist_utils.online_cpulist(tmplist)
             msrcfg.cpulist = collapse_cpulist(tmplist)
 
-        if ldcfg.cpulist:
+        if ldcfg_cpulist_present:
             logger.log(Log.DEBUG, f"loads cpulist: {ldcfg.cpulist}")
         # if --onlyload is specified msrcfg.cpulist is unused
         if msrcfg_cpulist_present and not rtevcfg.onlyload:
-- 
2.44.0

Re: [PATCH] rteval: Add relative cpulists for loads

[John Kacur]

On Wed, 6 Mar 2024, tglozar@redhat.com wrote:

> From: Tomas Glozar <tglozar@redhat.com>
> 
> Relative cpulists were added for measurements in 64ce7848 ("rteval: Add
> relative cpulists for measurements").
> 
> Add support for relative cpulists also for loads. This works the same
> way as for measurements using parse_cpulist_from_config, only difference
> is there is no --loads-run-on-isolcpus option.

You need a better description here for people who don't know what this is 
about. For example, why would this be useful? There is no mention of 
--only-loads




> 
> Signed-off-by: Tomas Glozar <tglozar@redhat.com>
> ---
>  rteval-cmd | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/rteval-cmd b/rteval-cmd
> index a5e8746..8572022 100755
> --- a/rteval-cmd
> +++ b/rteval-cmd
> @@ -340,28 +340,29 @@ if __name__ == '__main__':
>          ldcfg = config.GetSection('loads')
>          msrcfg = config.GetSection('measurement')
>          msrcfg_cpulist_present = msrcfg.cpulist != ""
> -        # Parse measurement cpulist using parse_cpulist_from_config to account for run-on-isolcpus
> +        ldcfg_cpulist_present = ldcfg.cpulist != ""

I'm not a fan of this syntactic sugar it makes things wordier and less 
readable IMO for programmers. I see I let you do that for msrcfg.cpulist, 
but I'd prefer you put that back too

> +        # Parse cpulists using parse_cpulist_from_config to account for run-on-isolcpus

I'm not religious about the 80 char term, but if you're already modifying 
a comment, that overflows to the next line, why not neaten this up?

>          # and relative cpusets
>          cpulist = parse_cpulist_from_config(msrcfg.cpulist, msrcfg.run_on_isolcpus)
>          if msrcfg_cpulist_present and not cpulist_utils.is_relative(msrcfg.cpulist) and msrcfg.run_on_isolcpus:
>              logger.log(Log.WARN, "ignoring --measurement-run-on-isolcpus, since cpulist is specified")
>          msrcfg.cpulist = collapse_cpulist(cpulist)
> -        if ldcfg.cpulist:
> -            ldcfg.cpulist = remove_offline(ldcfg.cpulist)
> +        cpulist = parse_cpulist_from_config(ldcfg.cpulist, run_on_isolcpus=False)

Since run_on_isolcpus are False by default you could also just do

cpulist = parse_cpulist_from_config(ldcfg.cpulist)

since we know that we are reusing the interface in which run_on_isolcpus 
are relevant, but in this case they are not, so explitly setting them to 
False is confusing

> +        ldcfg.cpulist = collapse_cpulist(cpulist)
>          # if we only specified one set of cpus (loads or measurement)
>          # default the other to the inverse of the specified list
> -        if not ldcfg.cpulist and msrcfg_cpulist_present:
> +        if not ldcfg_cpulist_present and msrcfg_cpulist_present:
>              tmplist = expand_cpulist(msrcfg.cpulist)
>              tmplist = SysTopology().invert_cpulist(tmplist)
>              tmplist = cpulist_utils.online_cpulist(tmplist)
>              ldcfg.cpulist = collapse_cpulist(tmplist)
> -        if not msrcfg_cpulist_present and ldcfg.cpulist:
> +        if not msrcfg_cpulist_present and ldcfg_cpulist_present:
>              tmplist = expand_cpulist(ldcfg.cpulist)
>              tmplist = SysTopology().invert_cpulist(tmplist)
>              tmplist = cpulist_utils.online_cpulist(tmplist)
>              msrcfg.cpulist = collapse_cpulist(tmplist)
>  
> -        if ldcfg.cpulist:
> +        if ldcfg_cpulist_present:
>              logger.log(Log.DEBUG, f"loads cpulist: {ldcfg.cpulist}")
>          # if --onlyload is specified msrcfg.cpulist is unused
>          if msrcfg_cpulist_present and not rtevcfg.onlyload:
> -- 
> 2.44.0


This works, but it needs to be more robust. For example
rteval-cmd --measurement-run-on-isolcpus --measurement-cpulist -2,4 -d5s
fails with
rteval-cmd: error: argument --measurement-cpulist: expected one argument
although this works
rteval-cmd --measurement-run-on-isolcpus --measurement-cpulist +0,-2,4 
-d5s

Also, ranges are not supported, it would be nice to do
-2-5 instead of -2,3,4,5

Thanks

John