[DTrace-devel] [PATCH 5/6] Implement aggrate-based aggregation snapshots

[Eugene Loh]

Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
again with minor comments, many familiar from the switchrate review.

Again, there is the unfortunate interchanging on frequency/rate on the 
one hand and interval on the other.  They are inverses of each other.

The actual aggrate cannot be faster than the switchrate... not sure if 
that should be mentioned in the code.

On 2/22/23 10:17, Kris Van Hees via DTrace-devel wrote:
> Some aggregation operations like clear() depend on aggregation snapshot
> frequency.
>
> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> ---
>   libdtrace/dt_aggregate.c | 19 ++++++++++++++++++-
>   libdtrace/dt_consume.c   |  1 +
>   libdtrace/dt_impl.h      |  1 +
>   libdtrace/dt_open.c      |  1 +
>   libdtrace/dt_work.c      |  5 +++++
>   5 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/libdtrace/dt_aggregate.c b/libdtrace/dt_aggregate.c
> index 41fa0da1..2129c867 100644
> --- a/libdtrace/dt_aggregate.c
> +++ b/libdtrace/dt_aggregate.c
> @@ -608,15 +608,32 @@ dt_aggregate_snap_cpu(dtrace_hdl_t *dtp, processorid_t cpu, int fd)
>   int
>   dtrace_aggregate_snap(dtrace_hdl_t *dtp)
>   {
> +	dtrace_optval_t	interval = dtp->dt_options[DTRACEOPT_AGGRATE];
>   	dt_aggregate_t	*agp = &dtp->dt_aggregate;
> +	hrtime_t	now = gethrtime();

now is used only inside the "if (interval > 0)" block and so can be 
defined there.  (Unless that block is restructured as discussed below.)

>   	int		i, rval;
>   
> +	/* Has tracing started yet? */
> +	if (!dtp->dt_active)
> +		return dt_set_errno(dtp, EINVAL);
> +
>   	/*
>   	 * If we do not have a buffer initialized, we will not be processing
>   	 * aggregations, so there is nothing to be done here.
>   	 */
>   	if (agp->dtat_buf == NULL)
> -		return 0;
> +		return DTRACE_WORKSTATUS_OKAY;
> +
> +	/* Do not retrieve at a rate higher than 'aggrate'. */

"Higher" is open to interpretation if these "rates" are handled as 
intervals.  How about s/higher/faster/?

> +	if (interval > 0) {
> +		if (dtp->dt_lastagg != 0) {
> +			if (now - dtp->dt_lastagg < interval)
> +				return DTRACE_WORKSTATUS_OKAY;
> +
> +			dtp->dt_lastagg += interval;
> +		} else
> +			dtp->dt_lastagg = now;
> +	}

Seems like it'd be simpler to throw away the interval>0 test and just 
replace all of the above with:

         if (now < dtp->dt_lastagg + interval)
                 return DTRACE_WORKSTATUS_OKAY;
         dtp->dt_lastagg = now;

or

         if (now < dtp->dt_nextagg)
                 return DTRACE_WORKSTATUS_OKAY;
         dtp->dt_nextagg = now + interval;

>   	dtrace_aggregate_clear(dtp);
>   
> diff --git a/libdtrace/dt_consume.c b/libdtrace/dt_consume.c
> index 1c2223be..34009e6b 100644
> --- a/libdtrace/dt_consume.c
> +++ b/libdtrace/dt_consume.c
> @@ -3058,6 +3058,7 @@ dtrace_consume(dtrace_hdl_t *dtp, FILE *fp, dtrace_consume_probe_f *pf,
>   			return rval;
>   
>   		/* Force data retrieval since BEGIN was processed. */
> +		dtp->dt_lastagg = 0;
>   		dtp->dt_lastswitch = 0;
>   	}
>   
> diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
> index e9e4bbcf..9edef5ab 100644
> --- a/libdtrace/dt_impl.h
> +++ b/libdtrace/dt_impl.h
> @@ -392,6 +392,7 @@ struct dtrace_hdl {
>   	dtrace_status_t dt_status[2]; /* status cache */
>   	int dt_statusgen;	/* current status generation */
>   	hrtime_t dt_lastswitch;	/* last switch of buffer data */
> +	hrtime_t dt_lastagg;	/* last snapshot of aggregation data */

Again, I'm a fan of dt_nextagg instead of dt_lastagg, but no big deal.  
If you prefer dt_lastagg, that's fine.

>   	dt_list_t dt_spec_bufs_draining; /* List of spec bufs being drained */
>   	dt_htab_t *dt_spec_bufs;/* spec ID -> list of dt_spec_bufs_head_t */
>   	char *dt_sprintf_buf;	/* buffer for dtrace_sprintf() */
> diff --git a/libdtrace/dt_open.c b/libdtrace/dt_open.c
> index f282c900..d01bc9df 100644
> --- a/libdtrace/dt_open.c
> +++ b/libdtrace/dt_open.c
> @@ -800,6 +800,7 @@ dt_vopen(int version, int flags, int *errp,
>   	 * Set the default data rates.
>   	 */
>   	dtp->dt_options[DTRACEOPT_SWITCHRATE] = NANOSEC;
> +	dtp->dt_options[DTRACEOPT_AGGRATE] = NANOSEC;

Same thing about a comment being helpful to explain "NANOSEC" means "one 
second".  But if the switchrate line gets that comment in the previous 
patch, I suppose the comment does not need to be repeated for aggrate.

>   	dtp->dt_cpp_argv[0] = (char *)strbasename(dtp->dt_cpp_path);
>   
> diff --git a/libdtrace/dt_work.c b/libdtrace/dt_work.c
> index f1292761..9f79d875 100644
> --- a/libdtrace/dt_work.c
> +++ b/libdtrace/dt_work.c
> @@ -174,6 +174,7 @@ dtrace_work(dtrace_hdl_t *dtp, FILE *fp, dtrace_consume_probe_f *pfunc,
>   	case DTRACE_STATUS_EXITED:
>   	case DTRACE_STATUS_STOPPED:
>   		dtp->dt_lastswitch = 0;
> +		dtp->dt_lastagg = 0;
>   		rval = DTRACE_WORKSTATUS_DONE;
>   		break;
>   	case DTRACE_STATUS_NONE:
> @@ -184,6 +185,10 @@ dtrace_work(dtrace_hdl_t *dtp, FILE *fp, dtrace_consume_probe_f *pfunc,
>   		return DTRACE_WORKSTATUS_ERROR;
>   	}
>   
> +	if (dtrace_aggregate_snap(dtp) ==
> +	    DTRACE_WORKSTATUS_ERROR)

Why a line break?

> +		return DTRACE_WORKSTATUS_ERROR;
> +
>   	if (dtrace_consume(dtp, fp, pfunc, rfunc, arg) ==
>   	    DTRACE_WORKSTATUS_ERROR)
>   		return DTRACE_WORKSTATUS_ERROR;