[DTrace-devel] [PATCH v2 5/5] Use array-of-maps as storage for aggregations

[Eugene Loh]

On 8/24/22 22:51, Kris Van Hees wrote:

> On Wed, Aug 24, 2022 at 06:43:04PM -0400, Eugene Loh wrote:
>> Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
>>
>> We have three copies of aggregation data:  the BPF map, the snapshot, and the
>> user-space copy.  So, in dt_aggregate_go(), how about
>> -       /* Allocate a buffer to hold the aggregation data for a CPU.  */
>> +       /* Allocate a buffer to hold the snapshot data for a CPU.  */
> I am not convinced this makes a difference.  The snapshot is the aggregation
> data.  Sure, it is the state of the aggregation data at a given moment, but
> does it really matter how specific we get in our comments?  In the end, it is
> a buffer sized to hold the aggregation data and it gets copied into that buffer
> using the map lookup elem bpf syscall.


It seems like an easy change to make, a change that makes the comment 
more precise.  Strikes me as a win.  How about:
         /* Allocate the aggregation snapshot buffer. */
It's what we call dtat_buf in dt_impl.h, so there's at least greater 
coherence.

>> From: Kris Van Hees via DTrace-devel <dtrace-devel at oss.oracle.com>
>> Sent: Tuesday, August 23, 2022 2:49 PM
>> To: dtrace-devel at oss.oracle.com <dtrace-devel at oss.oracle.com>
>> Subject: [DTrace-devel] [PATCH v2 5/5] Use array-of-maps as storage for
>> aggregations
>>
>> diff --git a/libdtrace/dt_aggregate.c b/libdtrace/dt_aggregate.c
>> @@ -999,41 +994,22 @@ dt_aggregate_go(dtrace_hdl_t *dtp)
>>   
>> -       /*
>> -        * Allocate a buffer to hold the aggregation data for all possible
>> -        * CPUs, and initialize the per-CPU data pointers for CPUs that are
>> -        * currently enabled.
>> -        */
>> -       agp->dtat_buf = dt_zalloc(dtp, dtp->dt_conf.num_possible_cpus * aggsz);
>> +       /* Allocate a buffer to hold the aggregation data for a CPU.  */
>> +       agp->dtat_buf = dt_zalloc(dtp, aggsz);
>>           if (agp->dtat_buf == NULL)
>>                   return dt_set_errno(dtp, EDT_NOMEM);
>>   
>> -       agp->dtat_cpu_buf = dt_calloc(dtp, dtp->dt_conf.max_cpuid + 1,
>> -                                     sizeof(char *));
>> -       if (agp->dtat_cpu_buf == NULL) {
>> -               dt_free(dtp, agp->dtat_buf);
>> -               return dt_set_errno(dtp, EDT_NOMEM);
>> -       }
>> -
>> -       for (i = 0; i < dtp->dt_conf.num_online_cpus; i++) {
>> -               int     cpu = dtp->dt_conf.cpus[i].cpu_id;
>> -
>> -               agp->dtat_cpu_buf[cpu] = agp->dtat_buf + cpu * aggsz;
>> -       }