[DTrace-devel] [PATCH 5/6] Implement aggrate-based aggregation snapshots
Kris Van Hees
kris.van.hees at oracle.com
Fri Feb 24 02:33:01 UTC 2023
On Thu, Feb 23, 2023 at 09:19:05PM -0500, Eugene Loh via DTrace-devel wrote:
> 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.
Existing issue - one option used for both.
> 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.)
Yes.
> > 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/?
Sure.
> > + 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;
This fails to update the lastagg value to the next boundary, so that is not
correct.
> or
>
> if (now < dtp->dt_nextagg)
> return DTRACE_WORKSTATUS_OKAY;
> dtp->dt_nextagg = now + interval;
That is also wrong because the next lastagg value is lastagg + interval, not
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;
>
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel
More information about the DTrace-devel
mailing list