public inbox for dtrace@lists.linux.dev
 help / color / mirror / Atom feed
From: eugene.loh@oracle.com
To: dtrace@lists.linux.dev, dtrace-devel@oss.oracle.com
Subject: [PATCH 5/8] Fix stddev() carryover computation
Date: Tue,  4 Jun 2024 14:00:05 -0400	[thread overview]
Message-ID: <20240604180008.11331-5-eugene.loh@oracle.com> (raw)
In-Reply-To: <20240604180008.11331-1-eugene.loh@oracle.com>

From: Eugene Loh <eugene.loh@oracle.com>

The stddev() aggregation function squares 64-bit data values.  A
value is split into 32-bit high and low values.  Then, (high + low)
is squared to produce high*high, 2*high*low, and low*low.  Each is
managed in its own 64-bit register, with the final result residing
in two 64-bit registers.

When the 2*high*low portion is combined with the low*low portion,
care is exercised in case the combination has a carryover portion to
the higher bits.  This check was broken in the case where low==0.
That is, data values whose lowest 32 bits were 0 resulted in
outrageously bad stddev() results.

Fix the check and add a test for such cases.

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
 libdtrace/dt_cg.c                |  2 +-
 test/unittest/aggs/tst.stddev2.d | 45 ++++++++++++++++++++++++++++++++
 test/unittest/aggs/tst.stddev2.r | 13 +++++++++
 3 files changed, 59 insertions(+), 1 deletion(-)
 create mode 100644 test/unittest/aggs/tst.stddev2.d
 create mode 100644 test/unittest/aggs/tst.stddev2.r

diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
index a1c24e37..fc2cebf0 100644
--- a/libdtrace/dt_cg.c
+++ b/libdtrace/dt_cg.c
@@ -8290,7 +8290,7 @@ dt_cg_agg_stddev(dt_pcb_t *pcb, dt_ident_t *aid, dt_node_t *dnp,
 	/* Add low value part from mid to lowreg */
 	emit(dlp,  BPF_ALU64_REG(BPF_ADD, lowreg, lmdreg));
 	/* Handle the overflow/carry case */
-	emit(dlp,  BPF_BRANCH_REG(BPF_JLT, lmdreg, lowreg, Lncy));
+	emit(dlp,  BPF_BRANCH_REG(BPF_JLE, lmdreg, lowreg, Lncy));
 	emit(dlp,  BPF_ALU64_IMM(BPF_ADD, hi_reg, 1)) /* account for carry */;
 
 	/* Sum high value; no overflow expected nor accounted for */
diff --git a/test/unittest/aggs/tst.stddev2.d b/test/unittest/aggs/tst.stddev2.d
new file mode 100644
index 00000000..994bc3e2
--- /dev/null
+++ b/test/unittest/aggs/tst.stddev2.d
@@ -0,0 +1,45 @@
+/*
+ * Oracle Linux DTrace.
+ * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
+ * Licensed under the Universal Permissive License v 1.0 as shown at
+ * http://oss.oracle.com/licenses/upl.
+ */
+
+/*
+ * ASSERTION: Positive stddev() test
+ *
+ * SECTION: Aggregations/Aggregations
+ *
+ * NOTES: This is a simple verifiable positive test of the stddev() function.
+ */
+
+#pragma D option quiet
+
+BEGIN
+{
+	@a = stddev(          0); @a = stddev(          0);
+	@b = stddev(       0x10); @b = stddev(       0x20);
+	@c = stddev(      0x100); @c = stddev(      0x200);
+	@d = stddev(     0x1000); @d = stddev(     0x2000);
+	@e = stddev(    0x10000); @e = stddev(    0x20000);
+	@f = stddev(   0x100000); @f = stddev(   0x200000);
+	@g = stddev(  0x1000000); @g = stddev(  0x2000000);
+	@h = stddev( 0x10000000); @h = stddev( 0x20000000);
+	@i = stddev( 0x20000000); @i = stddev( 0x40000000);
+	@j = stddev( 0x40000000); @j = stddev( 0x80000000);
+	@k = stddev( 0x80000000); @k = stddev(0x100000000);
+	@l = stddev(0x100000000); @l = stddev(0x200000000);
+	printa("%9@x\n", @a);
+	printa("%9@x\n", @b);
+	printa("%9@x\n", @c);
+	printa("%9@x\n", @d);
+	printa("%9@x\n", @e);
+	printa("%9@x\n", @f);
+	printa("%9@x\n", @g);
+	printa("%9@x\n", @h);
+	printa("%9@x\n", @i);
+	printa("%9@x\n", @j);
+	printa("%9@x\n", @k);
+	printa("%9@x\n", @l);
+	exit(0);
+}
diff --git a/test/unittest/aggs/tst.stddev2.r b/test/unittest/aggs/tst.stddev2.r
new file mode 100644
index 00000000..16e17736
--- /dev/null
+++ b/test/unittest/aggs/tst.stddev2.r
@@ -0,0 +1,13 @@
+
+        0
+        8
+       80
+      800
+     8000
+    80000
+   800000
+  8000000
+ 10000000
+ 20000000
+ 40000000
+ 80000000
-- 
2.18.4


  parent reply	other threads:[~2024-06-04 18:00 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-06-04 18:00 [PATCH 1/8] test: Allow aggpercpu test to omit some CPUs eugene.loh
2024-06-04 18:00 ` [PATCH 2/8] Reduce stack depth if kernel returns NULL frames eugene.loh
2024-08-19 23:30   ` [DTrace-devel] " Kris Van Hees
2024-08-28 20:11     ` Eugene Loh
2024-08-28 20:17       ` Kris Van Hees
2024-08-28 20:23         ` Eugene Loh
2024-08-28 20:37           ` Kris Van Hees
2025-08-13  5:12             ` Eugene Loh
2024-06-04 18:00 ` [PATCH 3/8] test: Fix nonexistent @@sort option eugene.loh
2024-08-19 23:15   ` [DTrace-devel] " Kris Van Hees
2024-06-04 18:00 ` [PATCH 4/8] test: Remove unneeded -w option eugene.loh
2024-08-19 23:12   ` [DTrace-devel] " Kris Van Hees
2024-06-04 18:00 ` eugene.loh [this message]
2024-08-19 23:13   ` [DTrace-devel] [PATCH 5/8] Fix stddev() carryover computation Kris Van Hees
2024-06-04 18:00 ` [PATCH 6/8] test: Check dtrace return status eugene.loh
2024-06-04 18:00 ` [PATCH 7/8] Clean up double semicolons eugene.loh
2024-08-19 23:34   ` [DTrace-devel] " Kris Van Hees
2024-06-04 18:00 ` [PATCH 8/8] Turn some leading spaces into tabs eugene.loh
2024-08-19 23:34   ` [DTrace-devel] " Kris Van Hees
2024-08-19 23:11 ` [PATCH 1/8] test: Allow aggpercpu test to omit some CPUs Kris Van Hees

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240604180008.11331-5-eugene.loh@oracle.com \
    --to=eugene.loh@oracle.com \
    --cc=dtrace-devel@oss.oracle.com \
    --cc=dtrace@lists.linux.dev \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox