| Message ID | 20230811053940.1408424-11-rostedt@goodmis.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | libtraceeval histogram: Updates | expand |
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.
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
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; }; /* Histogram */ diff --git a/src/histograms.c b/src/histograms.c index a1a2daaa2e9b..9a8ec4d85301 100644 --- a/src/histograms.c +++ b/src/histograms.c @@ -506,21 +506,85 @@ static int get_entry(struct traceeval *teval, const union traceeval_data *keys, * Return 0 on success, -1 on error. */ static int copy_traceeval_data(struct traceeval_type *type, - const union traceeval_data *orig, - union traceeval_data *copy) + struct traceeval_stat *stat, + const union traceeval_data *orig, + union traceeval_data *copy) { + unsigned long long val; + *copy = *orig; - if (type->type == TRACEEVAL_TYPE_STRING) { + switch(type->type) { + case TRACEEVAL_TYPE_NUMBER: + if (type->flags & TRACEEVAL_FL_SIGNED) + val = (long long)copy->number; + else + val = (unsigned long long)copy->number; + break; + + case TRACEEVAL_TYPE_NUMBER_64: + if (type->flags & TRACEEVAL_FL_SIGNED) + val = (long long)copy->number_64; + else + val = (unsigned long long)copy->number_64; + break; + + case TRACEEVAL_TYPE_NUMBER_32: + if (type->flags & TRACEEVAL_FL_SIGNED) + val = (long long)copy->number_32; + else + val = (unsigned long long)copy->number_32; + break; + + case TRACEEVAL_TYPE_NUMBER_16: + if (type->flags & TRACEEVAL_FL_SIGNED) + val = (long long)copy->number_16; + else + val = (unsigned long long)copy->number_16; + break; + + case TRACEEVAL_TYPE_NUMBER_8: + if (type->flags & TRACEEVAL_FL_SIGNED) + val = (long long)copy->number_8; + else + val = (unsigned long long)copy->number_8; + break; + + case TRACEEVAL_TYPE_STRING: copy->string = NULL; if (orig->string) copy->string = strdup(orig->string); - else - return 0; if (!copy->string) return -1; + return 0; + default: + return 0; + } + + if (!stat) + return 0; + + if (!stat->count++) { + stat->max = val; + stat->min = val; + stat->total = val; + return 0; + } + + if (type->flags & TRACEEVAL_FL_SIGNED) { + if ((long long)stat->max < (long long)val) + stat->max = val; + if ((long long)stat->min > (long long)val) + stat->min = val; + stat->total += (long long)val; + } else { + if (stat->max < val) + stat->max = val; + if (stat->min > val) + stat->min = val; + stat->total += val; } return 0; @@ -549,6 +613,7 @@ static void data_release(size_t size, union traceeval_data **data, */ static int copy_traceeval_data_set(size_t size, struct traceeval_type *type, const union traceeval_data *orig, + struct traceeval_stat *stats, union traceeval_data **copy) { size_t i; @@ -562,7 +627,8 @@ static int copy_traceeval_data_set(size_t size, struct traceeval_type *type, return -1; for (i = 0; i < size; i++) { - if (copy_traceeval_data(type + i, orig + i, (*copy) + i)) + if (copy_traceeval_data(type + i, stats ? stats + i : NULL, + orig + i, (*copy) + i)) goto fail; } @@ -605,7 +671,7 @@ int traceeval_query(struct traceeval *teval, const union traceeval_data *keys, return check; return copy_traceeval_data_set(teval->nr_val_types, teval->val_types, - entry->vals, results); + entry->vals, NULL, results); } /* @@ -660,18 +726,22 @@ static int create_entry(struct traceeval *teval, union traceeval_data *new_vals; struct entry *entry; + entry = create_hist_entry(teval, keys); + if (!entry) + return -1; + + entry->val_stats = calloc(teval->nr_key_types, sizeof(*entry->val_stats)); + if (!entry->val_stats) + goto fail_entry; + /* copy keys */ if (copy_traceeval_data_set(teval->nr_key_types, teval->key_types, - keys, &new_keys) == -1) - return -1; + keys, NULL, &new_keys) == -1) + goto fail_stats; /* copy vals */ if (copy_traceeval_data_set(teval->nr_val_types, teval->val_types, - vals, &new_vals) == -1) - goto fail_vals; - - entry = create_hist_entry(teval, keys); - if (!entry) + vals, entry->val_stats, &new_vals) == -1) goto fail; entry->keys = new_keys; @@ -680,10 +750,13 @@ static int create_entry(struct traceeval *teval, return 0; fail: - data_release(teval->nr_val_types, &new_vals, teval->val_types); - -fail_vals: data_release(teval->nr_key_types, &new_keys, teval->key_types); + +fail_stats: + free(entry->val_stats); + +fail_entry: + free(entry); return -1; } @@ -700,7 +773,7 @@ static int update_entry(struct traceeval *teval, struct entry *entry, union traceeval_data *new_vals; if (copy_traceeval_data_set(teval->nr_val_types, teval->val_types, - vals, &new_vals) == -1) + vals, entry->val_stats, &new_vals) == -1) return -1; clean_data_set(entry->vals, teval->val_types, teval->nr_val_types); @@ -708,6 +781,79 @@ static int update_entry(struct traceeval *teval, struct entry *entry, return 0; } +struct traceeval_stat *traceeval_stat(struct traceeval *teval, + const union traceeval_data *keys, + struct traceeval_type *type) +{ + struct entry *entry; + int ret; + + /* Only value numbers have stats */ + if (!(type->flags & TRACEEVAL_FL_VALUE)) + return NULL; + + switch (type->type) { + case TRACEEVAL_TYPE_NUMBER: + case TRACEEVAL_TYPE_NUMBER_64: + case TRACEEVAL_TYPE_NUMBER_32: + case TRACEEVAL_TYPE_NUMBER_16: + case TRACEEVAL_TYPE_NUMBER_8: + break; + default: + return NULL; + } + + ret = get_entry(teval, keys, &entry); + if (ret <= 0) + return NULL; + + return &entry->val_stats[type->index]; +} + +/** + * traceeval_stat_max - return max value of stat + * @stat: The stat structure that holds the stats + * + * Returns the max value within @stat. + */ +unsigned long long traceeval_stat_max(struct traceeval_stat *stat) +{ + return stat->max; +} + +/** + * traceeval_stat_min - return min value of stat + * @stat: The stat structure that holds the stats + * + * Returns the min value within @stat. + */ +unsigned long long traceeval_stat_min(struct traceeval_stat *stat) +{ + return stat->min; +} + +/** + * traceeval_stat_total - return total value of stat + * @stat: The stat structure that holds the stats + * + * Returns the total value within @stat. + */ +unsigned long long traceeval_stat_total(struct traceeval_stat *stat) +{ + return stat->total; +} + +/** + * traceeval_stat_count - return count value of stat + * @stat: The stat structure that holds the stats + * + * Returns the count value within @stat. + */ +unsigned long long traceeval_stat_count(struct traceeval_stat *stat) +{ + return stat->count; +} + /* * traceeval_insert - insert an item into the traceeval descriptor * @teval: The descriptor to insert into