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

[Kris Van Hees]

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)?

> > +	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?

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