Re: [dev] [PATCH 2/2] slstatus: determine the number of cpu's in runtime

From: Lucas Gabriel Vuotto <lvuotto92_AT_gmail.com>
Date: Fri, 15 Sep 2017 17:35:49 -0300

Hi Kurt,

On 09/15/17 14:50, Kurt Van Dijck wrote:
> This runtime probe is only used when cpu usage is reported relative to 1cpu.
> The default case reports cpu usage relative to 100%, and not runtime
> cpu counting is required.
>
> Signed-off-by: Kurt Van Dijck <dev.kurt_AT_vandijck-laurijssen.be>
> ---
> config.def.h | 7 ++++---
> slstatus.c | 17 +++++++++++++++++
> 2 files changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/config.def.h b/config.def.h
> index f099bd1..71880f6 100644
> --- a/config.def.h
> +++ b/config.def.h
> _AT_@ -4,10 +4,11 @@
> static const int interval = 1000;
>
> /* number of CPU's to scale CPU usage for
> - * ideally, this number is detected repeatedly in runtime
> - * since it may even change ...
> + * When ncpu=0, cpu usage is relative to 100%.
> + * When ncpu!=0, cpu usage is relative to 1cpu and the number of cpu's
> + * is determined on each measurement.
> */
> -static int ncpu = 1;
> +static int ncpu = 0;
>
> /* text to show if no value can be retrieved */
> static const char unknown_str[] = "n/a";
> diff --git a/slstatus.c b/slstatus.c
> index 79faeb3..ef44499 100644
> --- a/slstatus.c
> +++ b/slstatus.c
> _AT_@ -179,6 +179,19 @@ cpu_freq(void)
> bprintf("%d", (freq + 500) / 1000) : unknown_str;
> }
>
> +static int
> +cpu_count(void)
> +{
> + char cbuf[4];
> + int cpucnt, id;
> +
> + pscanf(NULL, "%*[^\n]");
> + for (cpucnt = 0; ; ++cpucnt)
> + if (pscanf(NULL, "cpu%i %*[^\n]", &id) != 1)
> + break;
> + return cpucnt;
> +}
> +
> static const char *
> cpu_perc(void)
> {
> _AT_@ -196,6 +209,8 @@ cpu_perc(void)
> valid = 1;

Where does this come from? It's not in upstream nor your prevoius patch.

> return unknown_str;
> }
> + if (ncpu)
> + ncpu = cpu_count();
>
> perc = 100 * ((b[0]+b[1]+b[2]+b[5]+b[6]) - (a[0]+a[1]+a[2]+a[5]+a[6])) /
> ((b[0]+b[1]+b[2]+b[3]+b[4]+b[5]+b[6]) - (a[0]+a[1]+a[2]+a[3]+a[4]+a[5]+a[6]));

a and b are defined as long double a[4], b[4] in upstream.

> _AT_@ -220,6 +235,8 @@ cpu_iowait(void)
> valid = 1;

Same as before.

> return unknown_str;
> }
> + if (ncpu)
> + ncpu = cpu_count();
>
> perc = 100 * ((b[4]) - (a[4])) /
> ((b[0]+b[1]+b[2]+b[3]+b[4]+b[5]+b[6]) - (a[0]+a[1]+a[2]+a[3]+a[4]+a[5]+a[6]));

Same here. Is this patch based on upstream/master?

Why do we need to get the number of cpus in runtime? It's a static value, which in worst case changes after a reboot when disabling hyperthreading.

Also, using ncpu for more than one thing (first it's flag, then it's a number) is a really bad idea.

I don't think this should be merged, neither the whole 100% vs per-core thing: I think that it gives little to no information, and it doesn't seems to play nice with multi-threaded CPUs (in a 2:2 setup, does having over 200% means that the cpu is over-the-top? Maybe it's the same core, so it can handle more load, but maybe not, so, what info did it gave to me?)
Received on Fri Sep 15 2017 - 22:35:49 CEST