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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On Wed, 6 Mar 2024, tglozar@xxxxxxxxxx wrote:

> From: Tomas Glozar <tglozar@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx>
> ---
>  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





[Index of Archives]     [RT Stable]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]

  Powered by Linux