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

[Eugene Loh]

On 8/14/24 18:28, Kris Van Hees wrote:

> 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;

Hmm.  Then both dt_consume.c and dt_aggregate.c need to know about this 
new struct.  So, copy it to both files?  Introduce a new 
dt_aggregate.h?  Ick.

Maybe the thing to do is use dtrace_aggregate_walk() with a callback in 
dt_consume.c.  The callback can filter on the agg id and call 
dt_aggregate_clear_one() accordingly... I *think* that works.  Let me 
know if you're on board with this approach.

> 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.