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

[Kris Van Hees]

I decided against moving this forward.  Looking closer at the legacy behaviour
(which we cannot easily mimic here) I decided that it is better to report an
error when aggsize is too small rather than automatically increasing it.  In
the legacy version, a too small aggsize would result in any aggregation
activity to be reported as a drop.  But since this condition is definitely
known at compilation time *and* since this situation (aggsize too small) is
almost always accidental *and* since it is very difficult (and ugly) to mimic
the always-drop behaviour, reporting the error is best I think.

v2 of the series will have a patch that ensures we report an error (and
include a test to see to it that we do).

On Mon, May 01, 2023 at 10:32:05PM -0400, Eugene Loh via DTrace-devel wrote:
> 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
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel