Re: [PATCH v2 10/17] libtraceeval histogram: Add updating of stats

[Steven Rostedt <rostedt@goodmis.org>]

On Tue, 15 Aug 2023 14:25:54 -0600
Ross Zwisler <zwisler@google.com> wrote:

> On Fri, Aug 11, 2023 at 01:39:33AM -0400, Steven Rostedt wrote:
> > From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> > 
> > Whenever an entry is added that already exists (overwriting the values)
> > keep track of the stats for the number values (max, min, total, count).
> > 
> > Also move the stat structure out of the public view. We may want to modify
> > this structure in the future, and so it should not become an API.
> > 
> > Add accessor functions to get to the stat values.
> > 
> > Add traceeval_stat() to acquire a stat handle from a specific key for a
> > specific value.
> > 
> > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> > ---
> >  include/traceeval-hist.h |  17 ++--
> >  src/eval-local.h         |   9 ++
> >  src/histograms.c         | 182 +++++++++++++++++++++++++++++++++++----
> >  3 files changed, 183 insertions(+), 25 deletions(-)
> > 
> > diff --git a/include/traceeval-hist.h b/include/traceeval-hist.h
> > index 1c02f3039809..d061a4532b06 100644
> > --- a/include/traceeval-hist.h
> > +++ b/include/traceeval-hist.h
> > @@ -130,13 +130,7 @@ struct traceeval_type {
> >  };
> >  
> >  /* Statistics about a given entry element */
> > -struct traceeval_stat {
> > -	unsigned long long	max;
> > -	unsigned long long	min;
> > -	unsigned long long	total;
> > -	unsigned long long	avg;
> > -	unsigned long long	std;
> > -};
> > +struct traceeval_stat;
> >  
> >  /* Iterator over aggregated data */
> >  struct traceeval_iterator;
> > @@ -160,4 +154,13 @@ int traceeval_query(struct traceeval *teval, const union traceeval_data *keys,
> >  void traceeval_results_release(struct traceeval *teval,
> >  			       union traceeval_data *results);
> >  
> > +struct traceeval_stat *traceeval_stat(struct traceeval *teval,
> > +				      const union traceeval_data *keys,
> > +				      struct traceeval_type *type);
> > +
> > +unsigned long long traceeval_stat_max(struct traceeval_stat *stat);
> > +unsigned long long traceeval_stat_min(struct traceeval_stat *stat);
> > +unsigned long long traceeval_stat_total(struct traceeval_stat *stat);
> > +unsigned long long traceeval_stat_count(struct traceeval_stat *stat);
> > +
> >  #endif /* __LIBTRACEEVAL_HIST_H__ */
> > diff --git a/src/eval-local.h b/src/eval-local.h
> > index 820d7ad096e8..190b19db14d2 100644
> > --- a/src/eval-local.h
> > +++ b/src/eval-local.h
> > @@ -45,11 +45,20 @@ struct hash_table {
> >  	struct hash_iter	iter;
> >  };
> >  
> > +struct traceeval_stat {
> > +	unsigned long long	max;
> > +	unsigned long long	min;
> > +	unsigned long long	total;
> > +	unsigned long long	std;
> > +	size_t			count;
> > +};
> > +
> >  /* A key-value pair */
> >  struct entry {
> >  	struct hash_item	hash;
> >  	union traceeval_data	*keys;
> >  	union traceeval_data	*vals;
> > +	struct traceeval_stat	*val_stats;  
> 
> I'm confused about why 'val_stats' is in 'struct entry', instead of in
> 'struct traceeval'?
> 
> I would think that we'd want to keep our stats on a per-histogram basis, where
> we have an array of 'struct traceeval_stat' structs with the same size as our
> 'val_types' array, so that for a given compound value, say 
> 
> { TRACEVAL_TYPE_NUMBER_64, TRACEEVAL_TYPE_NUMBER_16, TRACEEVAL_NUMBER_8}
> 
> we'd have stats for each of these entries, like min, max, average, etc.?
> 
> With the stats associated with each entry, I don't see how we keep all the
> stats for all the entries up to date as we do insertions & removals.

Because the stats are for each key, not the total of the traceeval. If you
look at the code in task-eval, I want to know the max,min,total,count,avg
of the sleep time for a task when it is blocked, when it is sleeping, when
it is running, etc:

Task: migrate
      Total run time (us):       4645364
      Total blocked time (us):         0
      Total preempt time (us):     98540
      Total sleep time (us):      115559

    thread id: 1884
      Total run time (us):           808

    thread id: 1885
      Total run time (us):       2920763
      Total preempt time (us):    245570
      Total sleep time (us):   224289277

    thread id: 1886
      Total run time (us):       3601375
      Total preempt time (us):   1308185
      Total sleep time (us):    12692630

    thread id: 1887
      Total run time (us):       3740300
      Total preempt time (us):   2620102
      Total sleep time (us):     4806722

That "Task: migrate" is just one entry in the traceeval, the threads are
entries in the nested traceeval that is saved per task.

Actually, task-eval is only looking at the total time, but I could easily
add the max/min/avg times too. If I were to write a latency program, and
was tracing the wake up latency of a task, you probably want these stats
for each key, so I can see the max latency per task, and not just the total
for all tasks.

-- Steve