[DTrace-devel] [PATCH 2/2] Snapshot aggregations just in time

[eugene.loh at oracle.com]

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

Currently, dtrace periodically calls dtrace_work(), which in turn calls
dtrace_consume(), which among other things calls dtrace_aggregate_snap().
But aggregations are kept in entirety in the kernel's BPF maps.  There
is no need to snapshot the aggregations into user space unless we're
actually going to do something with aggregations.

Snapshot aggregations just in time -- that is, if there is a clear(),
trunc(), or printa() or if aggregations are to be printed at the end
of a dtrace session.

Skip the aggrate-slow test.  Just-in-time snapshots mean the semantics
of aggrate have changed.  A fast aggrate means nothing.  A slow aggrate
means we are supposed to use stale aggregation data, which would be
baffling.  A future patch is advised to deprecate aggrate entirely.

Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
---
 libdtrace/dt_aggregate.c                 | 7 +++++++
 libdtrace/dt_consume.c                   | 9 +++++++--
 libdtrace/dt_impl.h                      | 1 +
 test/unittest/options/tst.aggrate-slow.d | 1 +
 4 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/libdtrace/dt_aggregate.c b/libdtrace/dt_aggregate.c
index 40c1ae44f..6c1b642ff 100644
--- a/libdtrace/dt_aggregate.c
+++ b/libdtrace/dt_aggregate.c
@@ -817,6 +817,10 @@ dtrace_aggregate_snap(dtrace_hdl_t *dtp)
 			dtp->dt_lastagg = now;
 	}
 
+	if (dtp->dt_haveagg)
+		return DTRACE_WORKSTATUS_OKAY;
+	dtp->dt_haveagg = 1;
+
 	dtrace_aggregate_clear(dtp);
 
 	for (i = 0; i < dtp->dt_conf.num_online_cpus; i++) {
@@ -1901,6 +1905,9 @@ dtrace_aggregate_print(dtrace_hdl_t *dtp, FILE *fp,
 {
 	dtrace_print_aggdata_t pd;
 
+	dtp->dt_haveagg = 0;
+	dtrace_aggregate_snap(dtp);
+
 	if (dtp->dt_maxaggdsize == 0)
 		return 0;
 
diff --git a/libdtrace/dt_consume.c b/libdtrace/dt_consume.c
index 8f50ebefc..a91413672 100644
--- a/libdtrace/dt_consume.c
+++ b/libdtrace/dt_consume.c
@@ -2353,11 +2353,15 @@ dt_consume_one_probe(dtrace_hdl_t *dtp, FILE *fp, char *data, uint32_t size,
 				i++;
 				continue;
 			case DT_ACT_CLEAR:
+				if (dtrace_aggregate_snap(dtp) == DTRACE_WORKSTATUS_ERROR)
+					return DTRACE_WORKSTATUS_ERROR;
 				if (dt_clear(dtp, data, rec) != 0)
 					return DTRACE_WORKSTATUS_ERROR;
 
 				continue;
 			case DT_ACT_TRUNC:
+				if (dtrace_aggregate_snap(dtp) == DTRACE_WORKSTATUS_ERROR)
+					return DTRACE_WORKSTATUS_ERROR;
 				if (i == epd->dtdd_nrecs - 1)
 					return dt_set_errno(dtp, EDT_BADTRUNC);
 
@@ -2519,6 +2523,8 @@ dt_consume_one_probe(dtrace_hdl_t *dtp, FILE *fp, char *data, uint32_t size,
 			func = dtrace_fprintf;
 			break;
 		case DTRACEACT_PRINTA:
+			if (dtrace_aggregate_snap(dtp) == DTRACE_WORKSTATUS_ERROR)
+				return DTRACE_WORKSTATUS_ERROR;
 			if (rec->dtrd_format != NULL)
 				func = dtrace_fprinta;
 			else
@@ -3113,8 +3119,7 @@ dtrace_consume(dtrace_hdl_t *dtp, FILE *fp, dtrace_consume_probe_f *pf,
 		}
 	}
 
-	if (dtrace_aggregate_snap(dtp) == DTRACE_WORKSTATUS_ERROR)
-		return DTRACE_WORKSTATUS_ERROR;
+	dtp->dt_haveagg = 0;
 
 	/*
 	 * If dtp->dt_beganon is not -1, we did not process the BEGIN probe
diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
index 1033154d9..2c376f59a 100644
--- a/libdtrace/dt_impl.h
+++ b/libdtrace/dt_impl.h
@@ -405,6 +405,7 @@ struct dtrace_hdl {
 	dt_percpu_drops_t *dt_drops; /* per-CPU drop counters cache */
 	uint64_t dt_specdrops;	/* consumer-side spec drops counter */
 	int dt_statusgen;	/* current status generation */
+	int dt_haveagg;		/* FIXME: figure out a good name for this */
 	hrtime_t dt_laststatus;	/* last status */
 	hrtime_t dt_lastswitch;	/* last switch of buffer data */
 	hrtime_t dt_lastagg;	/* last snapshot of aggregation data */
diff --git a/test/unittest/options/tst.aggrate-slow.d b/test/unittest/options/tst.aggrate-slow.d
index e2a0f2cb2..cd91c0a6c 100644
--- a/test/unittest/options/tst.aggrate-slow.d
+++ b/test/unittest/options/tst.aggrate-slow.d
@@ -9,6 +9,7 @@
  * When the aggrate is slower than the switchrate and the pace of printa()
  * actions, multiple printa() should all reflect the same stale count.
  */
+/* @@skip: aggrate makes no sense */
 /* @@trigger: periodic_output */
 /* @@nosort */
 
-- 
2.43.5