Re: [hackers] [slstatus][PATCH] cpu_perc: Check for division by zero
Hi Ingo, great that you noticed the bug and thanks for the patch! Applied it right now. Cheers! Aaron -- Web: https://drkhsh.at/ or http://drkhsh5rv6pnahas.onion/ GPG: 0x7A65E38D55BE96FE Fingerprint: 4688 907C 8720 3318 0D9F AFDE 7A65 E38D 55BE 96FE
Re: [hackers] [slstatus][PATCH] cpu_perc: Check for division by zero
Ok managed to reproduce it on Linux by setting interval in config.h to 1ms and the patch fixes it. Thanks :) Am Fr., 15. Feb. 2019 um 00:20 Uhr schrieb Michael Buch : > > Ah I see, thanks for the example. Sorry, I overlooked your initial > email since it wasn't part of your patch mail. The patch seems fine > then, but I don't run OpenBSD at the moment and on the other platforms > this never occurred to me but perhaps someone who has encountered this > or can reproduce it with a trace can chime in. > > Thanks, > Michael > > Am Do., 14. Feb. 2019 um 23:41 Uhr schrieb Ingo Feinerer : > > > > On Thu, Feb 14, 2019 at 09:16:23PM +, Michael Buch wrote: > > > Thanks for changing, on second thought though, I have my doubts about this > > > situation being possible. The cp times are millisecond increments since > > > boot and would only not increment from our perspective if we sampled at > > > submillisecond frequency. You can maximally > > > configure slstatus to 1ms so I dont think we need a check here > > > Did this happen to you? > > > > Yes, as I wrote in my initial explanatory mail > > (https://lists.suckless.org/hackers/1902/16697.html) this happens > > regularly (but not always) after a suspend and resume cycle on OpenBSD. > > This causes slstatus to crash. > >
Re: [hackers] [slstatus][PATCH] cpu_perc: Check for division by zero
Ah I see, thanks for the example. Sorry, I overlooked your initial email since it wasn't part of your patch mail. The patch seems fine then, but I don't run OpenBSD at the moment and on the other platforms this never occurred to me but perhaps someone who has encountered this or can reproduce it with a trace can chime in. Thanks, Michael Am Do., 14. Feb. 2019 um 23:41 Uhr schrieb Ingo Feinerer : > > On Thu, Feb 14, 2019 at 09:16:23PM +, Michael Buch wrote: > > Thanks for changing, on second thought though, I have my doubts about this > > situation being possible. The cp times are millisecond increments since > > boot and would only not increment from our perspective if we sampled at > > submillisecond frequency. You can maximally > > configure slstatus to 1ms so I dont think we need a check here > > Did this happen to you? > > Yes, as I wrote in my initial explanatory mail > (https://lists.suckless.org/hackers/1902/16697.html) this happens > regularly (but not always) after a suspend and resume cycle on OpenBSD. > This causes slstatus to crash. >
Re: [hackers] [slstatus][PATCH] cpu_perc: Check for division by zero
On Thu, Feb 14, 2019 at 09:16:23PM +, Michael Buch wrote: > Thanks for changing, on second thought though, I have my doubts about this > situation being possible. The cp times are millisecond increments since > boot and would only not increment from our perspective if we sampled at > submillisecond frequency. You can maximally > configure slstatus to 1ms so I dont think we need a check here > Did this happen to you? Yes, as I wrote in my initial explanatory mail (https://lists.suckless.org/hackers/1902/16697.html) this happens regularly (but not always) after a suspend and resume cycle on OpenBSD. This causes slstatus to crash.
Re: [hackers] [slstatus][PATCH] cpu_perc: Check for division by zero
Thanks for changing, on second thought though, I have my doubts about this
situation being possible. The cp times are millisecond increments since
boot and would only not increment from our perspective if we sampled at
submillisecond frequency. You can maximally
configure slstatus to 1ms so I dont think we need a check here
Did this happen to you?
Am Do., 14. Feb. 2019 um 18:26 Uhr schrieb Ingo Feinerer :
>
> ---
> components/cpu.c | 45 +++--
> 1 file changed, 27 insertions(+), 18 deletions(-)
>
> diff --git a/components/cpu.c b/components/cpu.c
> index d9bd018..9e28003 100644
> --- a/components/cpu.c
> +++ b/components/cpu.c
> @@ -24,7 +24,7 @@
> cpu_perc(void)
> {
> static long double a[7];
> - long double b[7];
> + long double b[7], sum;
>
> memcpy(b, a, sizeof(b));
> /* cpu user nice system idle iowait irq softirq */
> @@ -37,13 +37,16 @@
> return NULL;
> }
>
> + sum = (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]);
> +
> + if (sum == 0) {
> + return NULL;
> + }
> +
> return bprintf("%d", (int)(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[0] + a[1] + a[2] + a[5] + a[6])) /
sum));
> }
> #elif defined(__OpenBSD__)
> #include
> @@ -75,7 +78,7 @@
> {
> int mib[2];
> static uintmax_t a[CPUSTATES];
> - uintmax_t b[CPUSTATES];
> + uintmax_t b[CPUSTATES], sum;
> size_t size;
>
> mib[0] = CTL_KERN;
> @@ -92,15 +95,18 @@
> return NULL;
> }
>
> + sum = (a[CP_USER] + a[CP_NICE] + a[CP_SYS] + a[CP_INTR] +
a[CP_IDLE]) -
> + (b[CP_USER] + b[CP_NICE] + b[CP_SYS] + b[CP_INTR] +
b[CP_IDLE]);
> +
> + if (sum == 0) {
> + return NULL;
> + }
> +
> return bprintf("%d", 100 *
>((a[CP_USER] + a[CP_NICE] + a[CP_SYS] +
> a[CP_INTR]) -
> (b[CP_USER] + b[CP_NICE] + b[CP_SYS] +
> -b[CP_INTR])) /
> - ((a[CP_USER] + a[CP_NICE] + a[CP_SYS] +
> -a[CP_INTR] + a[CP_IDLE]) -
> - (b[CP_USER] + b[CP_NICE] + b[CP_SYS] +
> -b[CP_INTR] + b[CP_IDLE])));
> +b[CP_INTR])) / sum);
> }
> #elif defined(__FreeBSD__)
> #include
> @@ -129,7 +135,7 @@
> {
> size_t size;
> static long a[CPUSTATES];
> - long b[CPUSTATES];
> + long b[CPUSTATES], sum;
>
> size = sizeof(a);
> memcpy(b, a, sizeof(b));
> @@ -142,14 +148,17 @@
> return NULL;
> }
>
> + sum = (a[CP_USER] + a[CP_NICE] + a[CP_SYS] + a[CP_INTR] +
a[CP_IDLE]) -
> + (b[CP_USER] + b[CP_NICE] + b[CP_SYS] + b[CP_INTR] +
b[CP_IDLE]);
> +
> + if (sum == 0) {
> + return NULL;
> + }
> +
> return bprintf("%d", 100 *
>((a[CP_USER] + a[CP_NICE] + a[CP_SYS] +
> a[CP_INTR]) -
> (b[CP_USER] + b[CP_NICE] + b[CP_SYS] +
> -b[CP_INTR])) /
> - ((a[CP_USER] + a[CP_NICE] + a[CP_SYS] +
> -a[CP_INTR] + a[CP_IDLE]) -
> - (b[CP_USER] + b[CP_NICE] + b[CP_SYS] +
> -b[CP_INTR] + b[CP_IDLE])));
> +b[CP_INTR])) / sum);
> }
> #endif
> --
> 2.20.1
>
>
[hackers] [slstatus][PATCH] cpu_perc: Check for division by zero
---
components/cpu.c | 45 +++--
1 file changed, 27 insertions(+), 18 deletions(-)
diff --git a/components/cpu.c b/components/cpu.c
index d9bd018..9e28003 100644
--- a/components/cpu.c
+++ b/components/cpu.c
@@ -24,7 +24,7 @@
cpu_perc(void)
{
static long double a[7];
- long double b[7];
+ long double b[7], sum;
memcpy(b, a, sizeof(b));
/* cpu user nice system idle iowait irq softirq */
@@ -37,13 +37,16 @@
return NULL;
}
+ sum = (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]);
+
+ if (sum == 0) {
+ return NULL;
+ }
+
return bprintf("%d", (int)(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[0] + a[1] + a[2] + a[5] + a[6])) / sum));
}
#elif defined(__OpenBSD__)
#include
@@ -75,7 +78,7 @@
{
int mib[2];
static uintmax_t a[CPUSTATES];
- uintmax_t b[CPUSTATES];
+ uintmax_t b[CPUSTATES], sum;
size_t size;
mib[0] = CTL_KERN;
@@ -92,15 +95,18 @@
return NULL;
}
+ sum = (a[CP_USER] + a[CP_NICE] + a[CP_SYS] + a[CP_INTR] +
a[CP_IDLE]) -
+ (b[CP_USER] + b[CP_NICE] + b[CP_SYS] + b[CP_INTR] +
b[CP_IDLE]);
+
+ if (sum == 0) {
+ return NULL;
+ }
+
return bprintf("%d", 100 *
((a[CP_USER] + a[CP_NICE] + a[CP_SYS] +
a[CP_INTR]) -
(b[CP_USER] + b[CP_NICE] + b[CP_SYS] +
-b[CP_INTR])) /
- ((a[CP_USER] + a[CP_NICE] + a[CP_SYS] +
-a[CP_INTR] + a[CP_IDLE]) -
- (b[CP_USER] + b[CP_NICE] + b[CP_SYS] +
-b[CP_INTR] + b[CP_IDLE])));
+b[CP_INTR])) / sum);
}
#elif defined(__FreeBSD__)
#include
@@ -129,7 +135,7 @@
{
size_t size;
static long a[CPUSTATES];
- long b[CPUSTATES];
+ long b[CPUSTATES], sum;
size = sizeof(a);
memcpy(b, a, sizeof(b));
@@ -142,14 +148,17 @@
return NULL;
}
+ sum = (a[CP_USER] + a[CP_NICE] + a[CP_SYS] + a[CP_INTR] +
a[CP_IDLE]) -
+ (b[CP_USER] + b[CP_NICE] + b[CP_SYS] + b[CP_INTR] +
b[CP_IDLE]);
+
+ if (sum == 0) {
+ return NULL;
+ }
+
return bprintf("%d", 100 *
((a[CP_USER] + a[CP_NICE] + a[CP_SYS] +
a[CP_INTR]) -
(b[CP_USER] + b[CP_NICE] + b[CP_SYS] +
-b[CP_INTR])) /
- ((a[CP_USER] + a[CP_NICE] + a[CP_SYS] +
-a[CP_INTR] + a[CP_IDLE]) -
- (b[CP_USER] + b[CP_NICE] + b[CP_SYS] +
-b[CP_INTR] + b[CP_IDLE])));
+b[CP_INTR])) / sum);
}
#endif
--
2.20.1
Re: [hackers] [slstatus][PATCH] cpu_perc: Check for division by zero
On Thu, Feb 14, 2019 at 12:31:46AM +, Michael Buch wrote: > Perhaps it might be cleaner to save the sum away in separate variables > instead of accessing the states several times. Or better extract into > a separate function/macro. Also mind doing it for FreeBSD and Linux? > Since they use the same logic I sent another version to the list that uses variables; for *BSD and Linux.
[hackers] [slstatus][PATCH] cpu_perc: Check for division by zero
---
components/cpu.c | 45 +++--
1 file changed, 27 insertions(+), 18 deletions(-)
diff --git a/components/cpu.c b/components/cpu.c
index d9bd018..9e28003 100644
--- a/components/cpu.c
+++ b/components/cpu.c
@@ -24,7 +24,7 @@
cpu_perc(void)
{
static long double a[7];
- long double b[7];
+ long double b[7], sum;
memcpy(b, a, sizeof(b));
/* cpu user nice system idle iowait irq softirq */
@@ -37,13 +37,16 @@
return NULL;
}
+ sum = (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]);
+
+ if (sum == 0) {
+ return NULL;
+ }
+
return bprintf("%d", (int)(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[0] + a[1] + a[2] + a[5] + a[6])) / sum));
}
#elif defined(__OpenBSD__)
#include
@@ -75,7 +78,7 @@
{
int mib[2];
static uintmax_t a[CPUSTATES];
- uintmax_t b[CPUSTATES];
+ uintmax_t b[CPUSTATES], sum;
size_t size;
mib[0] = CTL_KERN;
@@ -92,15 +95,18 @@
return NULL;
}
+ sum = (a[CP_USER] + a[CP_NICE] + a[CP_SYS] + a[CP_INTR] +
a[CP_IDLE]) -
+ (b[CP_USER] + b[CP_NICE] + b[CP_SYS] + b[CP_INTR] +
b[CP_IDLE]);
+
+ if (sum == 0) {
+ return NULL;
+ }
+
return bprintf("%d", 100 *
((a[CP_USER] + a[CP_NICE] + a[CP_SYS] +
a[CP_INTR]) -
(b[CP_USER] + b[CP_NICE] + b[CP_SYS] +
-b[CP_INTR])) /
- ((a[CP_USER] + a[CP_NICE] + a[CP_SYS] +
-a[CP_INTR] + a[CP_IDLE]) -
- (b[CP_USER] + b[CP_NICE] + b[CP_SYS] +
-b[CP_INTR] + b[CP_IDLE])));
+b[CP_INTR])) / sum);
}
#elif defined(__FreeBSD__)
#include
@@ -129,7 +135,7 @@
{
size_t size;
static long a[CPUSTATES];
- long b[CPUSTATES];
+ long b[CPUSTATES], sum;
size = sizeof(a);
memcpy(b, a, sizeof(b));
@@ -142,14 +148,17 @@
return NULL;
}
+ sum = (a[CP_USER] + a[CP_NICE] + a[CP_SYS] + a[CP_INTR] +
a[CP_IDLE]) -
+ (b[CP_USER] + b[CP_NICE] + b[CP_SYS] + b[CP_INTR] +
b[CP_IDLE]);
+
+ if (sum == 0) {
+ return NULL;
+ }
+
return bprintf("%d", 100 *
((a[CP_USER] + a[CP_NICE] + a[CP_SYS] +
a[CP_INTR]) -
(b[CP_USER] + b[CP_NICE] + b[CP_SYS] +
-b[CP_INTR])) /
- ((a[CP_USER] + a[CP_NICE] + a[CP_SYS] +
-a[CP_INTR] + a[CP_IDLE]) -
- (b[CP_USER] + b[CP_NICE] + b[CP_SYS] +
-b[CP_INTR] + b[CP_IDLE])));
+b[CP_INTR])) / sum);
}
#endif
--
2.20.1
Re: [hackers] [slstatus][PATCH] cpu_perc: Check for division by zero
Perhaps it might be cleaner to save the sum away in separate variables
instead of accessing the states several times. Or better extract into
a separate function/macro. Also mind doing it for FreeBSD and Linux?
Since they use the same logic
Regards,
Michael
Am Mi., 13. Feb. 2019 um 23:20 Uhr schrieb Ingo Feinerer :
>
> ---
> components/cpu.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/components/cpu.c b/components/cpu.c
> index d9bd018..47a10c5 100644
> --- a/components/cpu.c
> +++ b/components/cpu.c
> @@ -92,6 +92,11 @@
> return NULL;
> }
>
> + if (a[CP_USER] + a[CP_NICE] + a[CP_SYS] + a[CP_INTR] +
> a[CP_IDLE] ==
> + b[CP_USER] + b[CP_NICE] + b[CP_SYS] + b[CP_INTR] +
> b[CP_IDLE]) {
> + return NULL;
> + }
> +
> return bprintf("%d", 100 *
>((a[CP_USER] + a[CP_NICE] + a[CP_SYS] +
> a[CP_INTR]) -
> --
> 2.20.1
>
>
[hackers] [slstatus][PATCH] cpu_perc: Check for division by zero
---
components/cpu.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/components/cpu.c b/components/cpu.c
index d9bd018..47a10c5 100644
--- a/components/cpu.c
+++ b/components/cpu.c
@@ -92,6 +92,11 @@
return NULL;
}
+ if (a[CP_USER] + a[CP_NICE] + a[CP_SYS] + a[CP_INTR] +
a[CP_IDLE] ==
+ b[CP_USER] + b[CP_NICE] + b[CP_SYS] + b[CP_INTR] +
b[CP_IDLE]) {
+ return NULL;
+ }
+
return bprintf("%d", 100 *
((a[CP_USER] + a[CP_NICE] + a[CP_SYS] +
a[CP_INTR]) -
--
2.20.1
[hackers] [slstatus][PATCH] cpu_perc: Check for division by zero
Hi,
slstatus crashes (not always but often) after (suspend and) resume on
OpenBSD amd64.
The problem is in the OpenBSD section of components/cpu.c when `a` is
identical to `b` which leads to a division by zero. E.g., in one case I
had
a: CP_USER 28357, CP_NICE 0, CP_SYS 10376, CP_INTR 1681, CP_IDLE 1112152
b: CP_USER 28357, CP_NICE 0, CP_SYS 10376, CP_INTR 1681, CP_IDLE 1112152
So this patch checks this condition and returns.
Feel free to modify this patch (e.g., a variable could be introduced to
avoid the redundant sum (in the check and in the division) if you prefer
this).
Best regards,
Ingo
>From 8e2ec259cbc053647d982cc81039daa8c3832cbe Mon Sep 17 00:00:00 2001
From: Ingo Feinerer
Date: Wed, 13 Feb 2019 22:07:34 +0100
Subject: [PATCH] cpu_perc: Check for division by zero
---
components/cpu.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/components/cpu.c b/components/cpu.c
index d9bd018..47a10c5 100644
--- a/components/cpu.c
+++ b/components/cpu.c
@@ -92,6 +92,11 @@
return NULL;
}
+ if (a[CP_USER] + a[CP_NICE] + a[CP_SYS] + a[CP_INTR] +
a[CP_IDLE] ==
+ b[CP_USER] + b[CP_NICE] + b[CP_SYS] + b[CP_INTR] +
b[CP_IDLE]) {
+ return NULL;
+ }
+
return bprintf("%d", 100 *
((a[CP_USER] + a[CP_NICE] + a[CP_SYS] +
a[CP_INTR]) -
--
2.20.1
