[DTrace-devel] [PATCH] Add support for aggpercpu option

[Eugene Loh]

On 1/24/24 15:42, Kris Van Hees wrote:

> On Tue, Nov 07, 2023 at 11:40:39PM -0500, eugene.loh at oracle.com wrote:
>> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> Some suggested changes below.
>
>> diff --git a/libdtrace/dt_aggregate.c b/libdtrace/dt_aggregate.c
>> @@ -578,8 +581,40 @@ hashnext:
>>   
>>   	memcpy(ptr, data, size);
>>   	agd->dtada_data = ptr;
>> +	if (dtp->dt_aggregate.dtat_flags & DTRACE_A_PERCPU) {
>> +		int i, max_cpus = dtp->dt_conf.max_cpuid + 1;
>> +		dtrace_recdesc_t	*rec = &agg->dtagd_drecs[DT_AGGDATA_RECORD];
>> +		dtrace_actkind_t	act = rec->dtrd_action;
>> +		uint32_t		siz = rec->dtrd_size;
>> +
>> +		agd->dtada_percpu = dt_alloc(dtp, max_cpus * sizeof(caddr_t));
>> +		if (agd->dtada_percpu == NULL)
>> +			return dt_set_errno(dtp, EDT_NOMEM);
>> +
>> +		for (i = 0; i < max_cpus; i++) {
>> +			int64_t	*vals;
>> +
>> +			agd->dtada_percpu[i] = dt_alloc(dtp, size);
>> +			if (agd->dtada_percpu[i] == NULL)
>> +				return dt_set_errno(dtp, EDT_NOMEM);
>> +
>> +			vals = (int64_t *) &agd->dtada_percpu[i][rec->dtrd_offset];
>> +			switch (act) {
>> +			case DT_AGG_MIN:
>> +				*vals = INT64_MAX;
>> +				break;
>> +			case DT_AGG_MAX:
>> +				*vals = INT64_MIN;
>> +				break;
>> +			default:
>> +				memset(vals, 0, siz);
>> +				break;
>> +			}
>> +		}
>> +		memcpy(agd->dtada_percpu[cpu], data, size);
>> +	}
> I wonder whether it might be better to just allocate everything in this loop,
> and then call dt_aggregate_clear_one() to do the actual initialization?  Or
> split out the initialization code from dt_aggregate_clear_one() and call it
> from there and from here?  It contains enough complexity in indirections and
> array indexing that I think that would be warranted.

I'm not convinced but I posted a v2.  If that looks good to you, I'm 
fine with it.

>> -	/* Add the new entru to the hashtable. */
>> +	/* Add the new entry to the hashtable. */
>>   	if (agh->dtah_hash[ndx] != NULL)
>>   		agh->dtah_hash[ndx]->dtahe_prev = h;
>>   
>> @@ -1564,7 +1599,7 @@ dtrace_aggregate_walk_joined(dtrace_hdl_t *dtp, dtrace_aggid_t *aggvars,
>>   
>>   		if ((zdata = dt_zalloc(dtp, zsize)) == NULL) {
>>   			/*
>> -			 * If we failed to allocated some zero-filled data, we
>> +			 * If we failed to allocate some zero-filled data, we
>>   			 * need to zero out the remaining dtada_data pointers
>>   			 * to prevent the wrong data from being freed below.
>>   			 */
>> diff --git a/libdtrace/dt_consume.c b/libdtrace/dt_consume.c
>> @@ -1745,6 +1745,15 @@ dt_print_aggs(const dtrace_aggdata_t **aggsdata, int naggvars, void *arg)
>>   		if (dt_print_datum(dtp, fp, rec, aggdata->dtada_data, normal,
>>   				   agg->dtagd_sig) < 0)
>>   			return DTRACE_AGGWALK_ERROR;
>> +		if (aggdata->dtada_percpu != NULL) {
>> +			int j, max_cpus = aggdata->dtada_hdl->dt_conf.max_cpuid + 1;
>> +			for (j = 0; j < max_cpus; j++) {
>> +				if (dt_printf(dtp, fp, "\nagg per cpu %d", aggdata->dtada_hdl->dt_conf.cpus[j].cpu_id) < 0)
> I think this makes the output look rather weird.  How about:
>
> 	if (dt_printf(dtp, fp, "\n    [CPU %d]", aggdata->dtada_hdl->dt_conf.cpus[j].cpu_id) < 0)
>
> That makes it more clear I think that this is the data entry for that specific
> CPU (for the aggregation for which the overall aggregated value was just
> printed above).  As far as I can see that still works OK with aggregations
> that are indexed.

Okay.  I made that change in the v2 (along with the corresponding test 
change).

>> +					return DTRACE_AGGWALK_ERROR;
>> +				if (dt_print_datum(dtp, fp, rec, aggdata->dtada_percpu[j], normal, agg->dtagd_sig) < 0)
>> +					return DTRACE_AGGWALK_ERROR;
>> +			}
>> +		}
>>   
>>   		if (dt_buffered_flush(dtp, NULL, rec, aggdata,
>>   				      DTRACE_BUFDATA_AGGVAL) < 0)