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

[Eugene Loh]

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.

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?

*)  just throw it into the 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.

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

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