[DTrace-devel] [PATCH] Implementation of the printa() action

[Kris Van Hees]

On Tue, Dec 08, 2020 at 01:34:16PM -0800, Eugene Loh wrote:
> On 12/07/2020 01:07 PM, Kris Van Hees wrote:
> 
> > Given the design for the handling of aggregation data, there is no
> > further need for cummulative data retrieval using snapshots.  When we
> > are ready to display aggregation data, we can retrieve the data by
> > performing a key 0 BPF map lookup on the 'aggs' map.  It gives us all
> > data for all CPUs in a single buffer.
> 
> I'm trying to review this, but the patch is big and covers a lot of code 
> that's unfamiliar to me.  Here are some preliminary comments. They 
> aren't meant to be important, but only what I've gotten to so far.
> 
> These "new" tests have 2006 (-2020) copyrights.
>      test/unittest/actions/printa/err.D_PRINTA_PROTO.fmt_no_arg.d
>      test/unittest/actions/printa/err.D_PROTO_LEN.no_arg.d
>      test/unittest/aggs/err.D_AGG_REDEF.idx_diff_funcs.d

Thanks.  Corrected in my copy.

> There is an orphaned string.  Try "git grep idhash_nextoff".

Yes, that comment is going away.  Thanks.

> Are these obsolete?
>      dtat_ncpus
>      dtat_ncpu
>      dtat_cpus
> dtat_maxcpu seems to be used, but I cannot tell where it is set.  It 
> used to be set in aggregate_go().

They remain for now because they may still be needed in terms of support for
tracing on specific CPU(s).

> Should DTRACEOPT_AGGRATE be decommissioned?
> How about aggrate (still present in many tests)?
> How about dt_lastagg?
> How about dtrace_bufdesc and dtrace_bufdesc_t?

Probably, but that is cleanup that can be done in future patches.  Or, well,
I'll just add it in because I am making changes anyway.  In general, there are
likely to be some things that are not getting used that still in the code.
Some of that is because it is likely to be needed when we support indexed
aggregations, and other stuff is because there still needs to be a further
cleanup to handle some of the extensive changes in how the code is structured.

> Are we changing support for "cpu" (CPU on which to enable tracing)?

Right now, this version does not support tracing on a specific CPU so we do
not do that for aggregations eother.

> In general, I would suggest leaving stylistic refactoring (such as 
> eliminating "(void)", changing spacing around "sizeof", use of 
> parentheses on "return", etc.) out of big functional patches like this.  
> Better to have patches for particular purposes.  If such changes should 
> be made, they can be made consistently and as their own patch, 
> simplifying review, etc.  That's water under the bridge as far as this 
> patch goes, but if you like I can prepare such a patch ("not this week") 
> so that there is consistent style in the code base and such refactoring 
> will not be part of other patches. Let me know.

Yes, it is a habit I have because it makes it easier to deal with the code I
am touching.  Doing a more general run through all the source code files and
dealing with some of these stylistic changes would certainly be welcome.

I'll put it on our TODO list.