[DTrace-devel] [PATCH 3/5] agg: ensure there is always space for at least one aggregation

[Eugene Loh]

Well,

1)  Shouldn't this depend on -xbufresize?

2)  And/or why not just complain about insufficient space and let the 
user ask for more if that is what is desired?

In general, it's unclear to me what aggsize even means.  I think the 
nelem*(dt_maxtuplesize+dt_maxaggdsize) memory model is very approximate 
(that's from memory, so I may be wrong) and there is probably some 
rounding to page size, etc.  I didn't try this, but I suspect that, in 
the test that says that aggsize was raised to 285, the user will be 
hard-pressed to see anything that is 285. So why are we telling the user 
about stuff that maps to nothing they see?  We could rationalize some 
explanation, but it would be kind of convoluted from a user's point of view.

Another way of thinking about it is that the legacy parameters 
describing buffer sizing don't really work well for what we're doing 
anymore.  Trying to hang onto them may not make much sense. This patch 
doesn't feel to me to be a meaningful step either in size or direction.

On 5/1/23 15:51, Kris Van Hees via DTrace-devel wrote:
> When aggregations are used, raise the aggsize option value if needed to
> ensure there is room for at least one aggregation.
>
> diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
> @@ -520,9 +520,11 @@ gmap_create_aggs(dtrace_hdl_t *dtp)
>   
>   	nelems = dtp->dt_options[DTRACEOPT_AGGSIZE] /
>   		 (dtp->dt_maxtuplesize + dtp->dt_maxaggdsize);
> -
> -	if (nelems == 0)
> -		return 0;
> +	if (nelems == 0) {
> +		dtp->dt_options[DTRACEOPT_AGGSIZE] =
> +				dtp->dt_maxtuplesize + dtp->dt_maxaggdsize;
> +		nelems = 1;
> +	}
>   
>   	dtp->dt_aggmap_fd = create_gmap_of_maps(dtp, "aggs",
>   						BPF_MAP_TYPE_ARRAY_OF_MAPS,
> diff --git a/test/unittest/aggs/tst.aggsize-too-small.d b/test/unittest/aggs/tst.aggsize-too-small.d
> new file mode 100644
> index 00000000..3298d600
> --- /dev/null
> +++ b/test/unittest/aggs/tst.aggsize-too-small.d
> @@ -0,0 +1,28 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
> + * Licensed under the Universal Permissive License v 1.0 as shown at
> + * http://oss.oracle.com/licenses/upl.
> + */
> +
> +/*
> + * ASSERTION: If aggsize is too small to hold a single aggregation, the size is
> + *	      increased to make it fit.
> + *
> + * SECTION: Aggregations/Aggregations
> + */
> +
> +#pragma D option quietresize=no
> +#pragma D option aggsize=256
> +#pragma D option strsize=256
> +
> +BEGIN
> +{
> +	@["a"] = count();
> +	exit(0);
> +}
> +
> +ERROR
> +{
> +	exit(0);
> +}
> diff --git a/test/unittest/aggs/tst.aggsize-too-small.r b/test/unittest/aggs/tst.aggsize-too-small.r
> new file mode 100644
> index 00000000..504e0ed0
> --- /dev/null
> +++ b/test/unittest/aggs/tst.aggsize-too-small.r
> @@ -0,0 +1,7 @@
> +                   FUNCTION:NAME
> +                          :BEGIN
> +
> +  a                                                                 1
> +-- @@stderr --
> +dtrace: script 'test/unittest/aggs/tst.aggsize-too-small.d' matched 2 probes
> +dtrace: aggregation size raised to 285