[DTrace-devel] [PATCH] Action clear() should clear only one aggregation

[Kris Van Hees]

On Wed, Aug 14, 2024 at 05:50:27PM -0400, Eugene Loh wrote:
> On 8/14/24 11:57, Kris Van Hees wrote:
> 
> > On Tue, Aug 13, 2024 at 10:59:25PM -0400, eugene.loh at oracle.com wrote:
> > > From: Eugene Loh <eugene.loh at oracle.com>
> > > 
> > > FIXME:  Is libdtrace/libdtrace.ver correctly updated?
> 
> Incidentally, what has to be done in this file?
> 
> > I think that keeping dtrace_aggregate_walk() as-is would be better, along with
> > adding a function dtrace_aggregate_clear_one(dtp, aid) that performs the
> > lookup from aid to dtrae_aggdata_t *agd, and then calls
> > dt__aggregate_clear_one(agd, dtp) to do the actual clearing.  There is no need
> > to go through a walker function when we already know we need to operate on
> > just one aggregation.
> 
> I'm not convinced.
> 
> "Just one aggregation"?  Yes, just one aggid, but possibly many keys.  To
> narrow down to which agd we want, we need to know the key -- and clear()
> doesn't give us the key, plus there will generally be more than one key
> anyhow.

Ah true.  I blanked on the fact that clear(@a) clears that aggregation, and
any indexed aggregations with the same name (and thus same aid).

Anyway, given that dtrace_aggregate_walk() is an API function that is exported
by libdtrace, we should not change it.

So, I'd propose that instead of introducing a new function, we can make use
of the existing dtrace_aggregate_walk() by passing both dtp and the aid as an
arg struct.  E.g.

typedef struct dt_clear_arg {
	dtrace_hdl_t	*dtp;
	dtrace_aggid_t	aid;
} dt_clear_arg_t;

void
dtrace_aggregate_clear(dtrace_hdl_t *dtp)
{
	dt_clear_arg_t	arg = { dtp, aid };

        dtrace_aggregate_walk(dtp, dt_aggregate_clear_one, &arg);
}

nd then dt_aggregate_clear_one() can check the aid and return if the aid
is not IDNOE and does not match the one for the aggregation it is called for.
That means you also need to change dt_aggregate_snap_one() but there you can
also define that arg and set aid as IDNONE in that because we know that if we
call dt_aggregate_clear_one() from here weknow it is for an aggregation that
needs to be cleared.

> Now, one can argue that in the current implementation and its current hash
> function, the aggid tells us at least which hash bucket to search.  So we
> could write a special, reduced loop.  Maybe so, but that would be wed to
> this particular choice of hash function, and I'd argue the current choice is
> a bad one.  E.g., consider:
> 
>         tick-10 { @pc[uregs[R_PC]] = count() }
>         syscall::write:entry { @writeentry = count() }
> 
> Lots of agd's will be sharing one bucket and another agd is sitting all by
> itself.  We ought to change the hash function, and then we'd have to undo
> the "search just one bucket" optimization.
> 
> PS  Here is an oddity:
> 
>     # /usr/sbin/dtrace -n 'BEGIN { @a[1] = count(); @a[2] = count();
> clear(@a[1]); exit(0); }'
>     dtrace: description 'BEGIN ' matched 1 probe
>      CPU     ID                    FUNCTION:NAME
>       34      1                           :BEGIN
> 
>             1                0
>             2                0
> 
> You *can* specify the key in the clear()!  But dtrace ignores the key. 
> Silently.  Same behavior in Solaris.