[DTrace-devel] [PATCH 1/3] Clear agg keys

[Kris Van Hees]

On Mon, Aug 14, 2023 at 03:17:15PM -0400, Eugene Loh via DTrace-devel wrote:
> On 8/14/23 13:15, Kris Van Hees wrote:
> 
> > On Fri, Aug 11, 2023 at 01:04:05AM -0400, Eugene Loh via DTrace-devel wrote:
> > > On 8/8/23 17:24, eugene.loh at oracle.com wrote:
> > > 
> > > > From: Eugene Loh <eugene.loh at oracle.com>
> > > > 
> > > > Aggregations use a "map of maps" to access per-CPU BPF maps whose
> > > > keys are of size dt_maxtuplesize to accommodate keys of various
> > > > sizes.  Care was exercised on the cg side to ensure that a key
> > > > would be zeroed out before filling so that garbage bytes would not
> > > > pollute the key.
> > > > 
> > > > Exercise similar caution on the consumer side.
> > > > 
> > > > THIS PATCH NEEDS TESTS.
> > > Reminder:  if this gets worked into the trunc patch, then the trunc tests
> > > could be said to be the tests for this "Clear agg keys" stuff as well.
> > > Also...
> > > 
> > > > Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> > > > ---
> > > >    libdtrace/dt_aggregate.c | 9 +++++++--
> > > >    1 file changed, 7 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/libdtrace/dt_aggregate.c b/libdtrace/dt_aggregate.c
> > > > index 24faf1c1..1b8167d6 100644
> > > > --- a/libdtrace/dt_aggregate.c
> > > > +++ b/libdtrace/dt_aggregate.c
> > > > @@ -570,11 +570,16 @@ hashnext:
> > > >    	/* Copy the key. */
> > > >    	size = agg->dtagd_ksize;
> > > > -	ptr = dt_alloc(dtp, size);
> > > > +	ptr = dt_alloc(dtp, dtp->dt_maxtuplesize);
> > > >    	if (ptr == NULL)
> > > >    		return dt_set_errno(dtp, EDT_NOMEM);
> > > > -	memcpy(ptr, key, size);
> > > > +	memset(ptr, 0, dtp->dt_maxtuplesize);
> > > The dt_alloc() and memset() are dt_maxtuplesize because that's how big the
> > > keys for the aggs maps are.  But I don't think we ever use the last 8 bytes
> > > of dt_maxtuplesize for aggs, do we?  If not, then the key size for the aggs
> > > map can be reduced by 8 bytes.  Again, though, where that goes depends on
> > > whether this patch is integrated into the trunc() patch.
> > Why not use dt_zalloc(dtp, dtp->dt_maxtuplesize)?
> 
> Yeah, but actually I guess we don't need to zero anything out at all.  We
> can just memcpy() the entire dt_maxtuplesize in.

Actually, I belive that the following is sufficient:

 static int
 dt_aggwalk_remove(dtrace_hdl_t *dtp, dt_ahashent_t *h)
 {
        dtrace_aggdata_t        *agd = &h->dtahe_data;
        int                     i, ncpus = dtp->dt_conf.num_online_cpus;
+       char                    *key = dtp->dt_aggregate.dtat_key;
+
+       memset(key, 0, dtp->dt_maxtuplesize);
+       memcpy(key, agd->dtada_key, agd->dtada_desc->dtagd_ksize);
 
        for (i = 0; i < ncpus; i++) {
                int     cpu = dtp->dt_conf.cpus[i].cpu_id;
                int     fd = dt_bpf_map_lookup_fd(dtp->dt_aggmap_fd, &cpu);
 
                if (fd < 0)
                        return DTRACE_WORKSTATUS_ERROR;
 
-               dt_bpf_map_delete(fd, agd->dtada_key);
+               dt_bpf_map_delete(fd, key);
                close(fd);
        }
 
        return DTRACE_WORKSTATUS_OKAY;
 }

> The main question is whether this should:
> 
> *)  live as an independent patch.  Maybe that makes sense since it's not
> specific to trunc().  But if so, whether to add any tests for it... maybe it
> can be demonstrated with "clear" or something else?

Hm, actually, I think it is specific to trunc().  We know that tuples are
always constructed in a 0-filled buffer so there will not be garbage between
the elements of a tuple.

> *)  just throw it into the trunc patch.

Per the above, I think it should be in my trunc patch.

> Let me know.  But if separate patches, this should probably go in front of
> trunc().
> 
> And again there is the side issue about using an agg key that's
> dt_maxtuplesize-8.

I think there is banefit from using the same tuple code for associative arrays
and aggregations so the loss of 8 bytes is acceptable.  On the other hand, I've
taken another look at the way I implemented tuples and it seems that I may be
able to optimize it a bit in a way that all tuples benefit from a size
reduction.

But that would be a separate patch.

> > > > +	for (i = 0; i < agg->dtagd_nkrecs; i++) {
> > > > +		rec = &agg->dtagd_krecs[i];
> > > > +		off = rec->dtrd_offset;
> > > > +		memcpy(&ptr[off], &key[off], rec->dtrd_size);
> > > > +	}
> > Why do you copy the individual elements rather than memcpy(ptr, key, size),
> > which should be safe because we already know that the destination buffer is
> > all 0s.  Isn't your loop essentially just performing the same memcpy, but
> > in chunks rather than all at once?
> 
> Yeah.  I was trying to be extra careful now (also guard against gaps between
> key tuple members).  Plus, earlier, in dt_aggregate_snap_one(), this
> "chunk-by-chunk" approach is used to protect against garbage when comparing
> keys for a match.  But I guess both there and here it's overkill.  The
> matter is handled properly now, and neither place needs the chunk-by-chunk
> approach.

Yes, so an independent patch optimizing dt_aggregate_snap_one() might be in
order (not part if the trunc() work).

> > > >    	agd->dtada_key = ptr;
> > > >    	/* Copy the data. */
> > > _______________________________________________
> > > DTrace-devel mailing list
> > > DTrace-devel at oss.oracle.com
> > > https://oss.oracle.com/mailman/listinfo/dtrace-devel
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel