linux-kernel - Re: [PATCH v2 4/6] rtla/timerlat: Add --deepest-idle-state for top

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAP4=nvRnaM6ViT6t67prij+DLBso+0DB971G7ke+7OqEkPQpig@mail.gmail.com>
Date: Thu, 1 Aug 2024 12:20:32 +0200
From: Tomas Glozar <tglozar@...hat.com>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: linux-trace-kernel@...r.kernel.org, linux-kernel@...r.kernel.org, 
	jwyatt@...hat.com, jkacur@...hat.com
Subject: Re: [PATCH v2 4/6] rtla/timerlat: Add --deepest-idle-state for top

>
> Could probably do:
>
> #ifdef HAVE_LIBCPUPOWER_SUPPORT
> > +             "            --deepest-idle-state n: only go down to idle state n on cpus used by timerlat to reduce exit from idle latency",
> #else
> +               "            --deepest-idle-state n: [rtla built without libcpupower, --deepest-idle-state is not supported]",
> #endif
>
> >               NULL,
> >       };
> >

I would still include what the option does, even if not building with
libcpupower. I'm not too sure if the help is the place to say the
option is unsupported either. I see two perspectives on this matter:
one is that the binary does not support libcpupower and should state
it in the help, the other is that the version of rtla as a whole does
support it, you are just using a build that does not, and as such it
should be in the help (you will know it is unsupported when trying to
use the option). I suppose we can add a note like this, keeping the
help message to inform the user what the option does so that they will
rebuild if that want to use it:

```
#ifdef HAVE_LIBCPUPOWER_SUPPORT
             "            --deepest-idle-state n: only go down to idle
state n on cpus used by timerlat to reduce exit from idle latency",
#else
             "            --deepest-idle-state n: only go down to idle
state n on cpus used by timerlat to reduce exit from idle latency (not
supported due to rtla built without libcpupower)",
#endif
```

What do you think about that?

>
> We could get rid of most of the ifdefs if you changed the header file to be:
>

That's a neat idea, thank you! I know this approach (with defining
functions that do nothing when some feature is unavailable) is very
commonly used in the kernel to keep the API consistent across
different configs, but I didn't think about using it like this here in
rtla.

>
>You would think gcc may optimize it, but I don't have that much confidence
>it can or would. You may want to change that to:
>
>                int nr_cpus = sysconf(_SC_NPROCESSORS_CONF);
>
>                for (i = 0; i < nr_cpus; i++) {
>
>Otherwise you may be calling that sysconf() for each iteration of the loop.
>

Nah, that is simply an oversight. If GCC optimized that, it would be
in fact a GCC bug, since the value of sysconf(_SC_NPROCESSORS_CONF) is
external environment and can technically change during the runtime of
a program (think of CRIU live migration of the process from one
machine to another with a different number of CPUs).

Tomas