[PATCH 0/9] libtraceeval: Even more updates!

[PATCH 0/9] libtraceeval: Even more updates!

[Steven Rostedt]

From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

More updates to libtraceeval

- As the results and stats are often needed in the iterator loops, add helper
  functions to quickly retrieve them. The iterator has a handle on the current
  entry, no need to do a key lookup to find it.

- The iterator can be used to search for items to remove or update. Add a
  helper function to remove the current iterator item. This will also update
  the iterator so that other functions called are still safe, even if the removed
  entry is the current one.

- Convert traceeval_data into a proper structure with a "type" field and a
  union data portion. This will allow for checking the type of data being
  passed into the functions to make sure they match their corresponding types.

- Add checks to make sure that the data matches the types.

- Make traceeval_insert() and traceeval_query() into macros and add a
  traceeval_query_size() and traceeval_insert_size() that take the size of the
  list of data being passed into it. The macros work when the arrays are defined.

- Add checks to make sure the array sizes passed in match the sizes internally.

- Add back the STATS flag, and only do STATS on values that are numeric and
  have this set. This should speed things up, and also allow for optimizations
  in the future.

Steven Rostedt (Google) (9):
  libtraceeval: Add check for updates to know to recreate iter array
  libtraceeval: Add traceeval_iterator_query()
  libtraceeval: Add traceeval_iterator_stat()
  libtraceeval: Add traceeval_iterator_remove()
  libtraceeval histogram: Add type to traceeval_data and make it a
    structure
  libtraceveal: Add type checks to traceeval_data vals and keys
  libtraceeval: Add size checks to insert and query functions
  libtraceeval: Add checks to traceeval_insert() and query()
  libtraceeval: Only do stats on values marked with the STAT flag

 include/traceeval-hist.h | 102 +++++++++----
 samples/task-eval.c      | 146 ++++++++-----------
 src/eval-local.h         |   6 +-
 src/histograms.c         | 300 ++++++++++++++++++++++++++++++---------
 4 files changed, 372 insertions(+), 182 deletions(-)

-- 
2.40.1

[PATCH 1/9] libtraceeval: Add check for updates to know to recreate iter array

[Steven Rostedt]

From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

When an iterator is created, it creates an array of pointers to point to
all the elements in the traceeval. This is used to index through the
entities in an nice order. But if an event is added or removed from the
traceeval, the size and count of this array will be off in the iterator.

Add an "update_counter" that gets incremented every time an item is added
or removed (doesn't need to keep track of updates to existing entries). If
the counter is different from the last time the iterator created the sort
array, it will need to delete and recreate the list again before it can do
a sort.

Note: It is safe to use the iterator to remove times, so a removal (or
even insert) should not affect the traceeval_iterator_next(). But it
should be explained in the man pages (soon to be written) that doing so
must be done with care. And maybe a helper function should be used
instead!

Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 src/eval-local.h |  2 ++
 src/histograms.c | 91 ++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 74 insertions(+), 19 deletions(-)

diff --git a/src/eval-local.h b/src/eval-local.h
index 5c3918f17cc1..73f52efdf0da 100644
--- a/src/eval-local.h
+++ b/src/eval-local.h
@@ -68,6 +68,7 @@ struct traceeval {
 	struct hash_table		*hist;
 	size_t				nr_key_types;
 	size_t				nr_val_types;
+	size_t				update_counter;
 };
 
 struct traceeval_iterator {
@@ -75,6 +76,7 @@ struct traceeval_iterator {
 	struct entry			**entries;
 	struct traceeval_type		**sort;
 	bool				*direction;
+	size_t				update_counter;
 	size_t				nr_entries;
 	size_t				nr_sort;
 	size_t				next;
diff --git a/src/histograms.c b/src/histograms.c
index 96f0926f062c..66bdc1769985 100644
--- a/src/histograms.c
+++ b/src/histograms.c
@@ -757,6 +757,8 @@ static int create_entry(struct traceeval *teval,
 	entry->keys = new_keys;
 	entry->vals = new_vals;
 
+	teval->update_counter++;
+
 	return 0;
 
 fail:
@@ -960,6 +962,9 @@ int traceeval_remove(struct traceeval *teval,
 		return check;
 
 	hash_remove(hist, &entry->hash);
+
+	teval->update_counter++;
+
 	return 1;
 }
 
@@ -981,6 +986,37 @@ void traceeval_iterator_put(struct traceeval_iterator *iter)
 	free(iter);
 }
 
+static int create_iter_array(struct traceeval_iterator *iter)
+{
+	struct traceeval *teval = iter->teval;
+	struct hash_table *hist = teval->hist;
+	struct hash_iter *hiter;
+	struct hash_item *item;
+	int i;
+
+	iter->nr_entries = hash_nr_items(hist);
+	iter->entries = calloc(iter->nr_entries, sizeof(*iter->entries));
+	if (!iter->entries)
+		return -1;
+
+	for (i = 0, hiter = hash_iter_start(hist); (item = hash_iter_next(hiter)); i++) {
+		struct entry *entry = container_of(item, struct entry, hash);
+
+		iter->entries[i] = entry;
+	}
+
+	/* Loop must match entries */
+	if (i != iter->nr_entries) {
+		free(iter->entries);
+		iter->entries = NULL;
+		return -1;
+	}
+
+	iter->update_counter = teval->update_counter;
+
+	return 0;
+}
+
 /**
  * traceeval_iterator_get - get a handle to iterate over a given traceeval
  * @teval: The traceeval handle to iterate over
@@ -997,33 +1033,19 @@ void traceeval_iterator_put(struct traceeval_iterator *iter)
 struct traceeval_iterator *traceeval_iterator_get(struct traceeval *teval)
 {
 	struct traceeval_iterator *iter;
-	struct hash_table *hist = teval->hist;
-	struct hash_iter *hiter;
-	struct hash_item *item;
-	int i;
+	int ret;
 
 	iter = calloc(1, sizeof(*iter));
 	if (!iter)
 		return NULL;
 
 	iter->teval = teval;
-	iter->nr_entries = hash_nr_items(hist);
-	iter->entries = calloc(iter->nr_entries, sizeof(*iter->entries));
-	if (!iter->entries) {
-		free(iter);
-		return NULL;
-	}
-
-	for (i = 0, hiter = hash_iter_start(hist); (item = hash_iter_next(hiter)); i++) {
-		struct entry *entry = container_of(item, struct entry, hash);
 
-		iter->entries[i] = entry;
-	}
+	ret = create_iter_array(iter);
 
-	/* Loop must match entries */
-	if (i != iter->nr_entries) {
-		traceeval_iterator_put(iter);
-		return NULL;
+	if (ret < 0) {
+		free(iter);
+		iter = NULL;
 	}
 
 	return iter;
@@ -1168,6 +1190,31 @@ static int iter_cmp(const void *A, const void *B, void *data)
 	return 0;
 }
 
+static int check_update(struct traceeval_iterator *iter)
+{
+	struct entry **entries;
+	size_t nr_entries;
+	int ret;
+
+	/* Was something added or removed from the teval? */
+	if (iter->teval->update_counter == iter->update_counter)
+		return 0;
+
+	entries = iter->entries;
+	nr_entries = iter->nr_entries;
+
+	/* Something changed, need to recreate the array */
+	ret = create_iter_array(iter);
+	if (ret < 0) {
+		iter->entries = entries;
+		iter->nr_entries = nr_entries;
+			return -1;
+	}
+	free(entries);
+
+	return 0;
+}
+
 static int sort_iter(struct traceeval_iterator *iter)
 {
 	int i;
@@ -1178,6 +1225,9 @@ static int sort_iter(struct traceeval_iterator *iter)
 			return -1;
 	}
 
+	if (check_update(iter) < 0)
+		return -1;
+
 	qsort_r(iter->entries, iter->nr_entries, sizeof(*iter->entries),
 		iter_cmp, iter);
 
@@ -1214,6 +1264,9 @@ int traceeval_iterator_sort_custom(struct traceeval_iterator *iter,
 		.data = data
 	};
 
+	if (check_update(iter) < 0)
+		return -1;
+
 	qsort_r(iter->entries, iter->nr_entries, sizeof(*iter->entries),
 		iter_custom_cmp, &cust_data);
 
-- 
2.40.1

Re: [PATCH 1/9] libtraceeval: Add check for updates to know to recreate iter array

[Ross Zwisler]

On Thu, Aug 17, 2023 at 06:24:14PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> When an iterator is created, it creates an array of pointers to point to
> all the elements in the traceeval. This is used to index through the
> entities in an nice order. But if an event is added or removed from the
> traceeval, the size and count of this array will be off in the iterator.
> 
> Add an "update_counter" that gets incremented every time an item is added
> or removed (doesn't need to keep track of updates to existing entries). If
> the counter is different from the last time the iterator created the sort
> array, it will need to delete and recreate the list again before it can do
> a sort.
> 
> Note: It is safe to use the iterator to remove times, so a removal (or
                                                 items
> even insert) should not affect the traceeval_iterator_next(). But it
> should be explained in the man pages (soon to be written) that doing so
> must be done with care. And maybe a helper function should be used
> instead!
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
>  src/eval-local.h |  2 ++
>  src/histograms.c | 91 ++++++++++++++++++++++++++++++++++++++----------
>  2 files changed, 74 insertions(+), 19 deletions(-)
<>
> @@ -1168,6 +1190,31 @@ static int iter_cmp(const void *A, const void *B, void *data)
>  	return 0;
>  }
>  
> +static int check_update(struct traceeval_iterator *iter)
> +{
> +	struct entry **entries;
> +	size_t nr_entries;
> +	int ret;
> +
> +	/* Was something added or removed from the teval? */
> +	if (iter->teval->update_counter == iter->update_counter)
> +		return 0;
> +
> +	entries = iter->entries;
> +	nr_entries = iter->nr_entries;
> +
> +	/* Something changed, need to recreate the array */
> +	ret = create_iter_array(iter);
> +	if (ret < 0) {
> +		iter->entries = entries;
> +		iter->nr_entries = nr_entries;
> +			return -1;

                ^^ extra tab

Aside from these 2 nits:

Reviewed-by: Ross Zwisler <zwisler@google.com>

[PATCH 2/9] libtraceeval: Add traceeval_iterator_query()

[Steven Rostedt]

From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

Add a query to the iterator that will return the values of the last entry
that matches the keys returned by the last call of traceeval_iterator_next().

Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 include/traceeval-hist.h |  2 ++
 samples/task-eval.c      |  2 +-
 src/histograms.c         | 30 ++++++++++++++++++++++++++++++
 3 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/include/traceeval-hist.h b/include/traceeval-hist.h
index 0cee9bcbeb83..837a74f61a66 100644
--- a/include/traceeval-hist.h
+++ b/include/traceeval-hist.h
@@ -186,5 +186,7 @@ int traceeval_iterator_sort_custom(struct traceeval_iterator *iter,
 				   traceeval_cmp_fn sort_fn, void *data);
 int traceeval_iterator_next(struct traceeval_iterator *iter,
 			    const union traceeval_data **keys);
+int traceeval_iterator_query(struct traceeval_iterator *iter,
+			     const union traceeval_data **results);
 
 #endif /* __LIBTRACEEVAL_HIST_H__ */
diff --git a/samples/task-eval.c b/samples/task-eval.c
index 66d0c40dc0c8..18e07f44e11e 100644
--- a/samples/task-eval.c
+++ b/samples/task-eval.c
@@ -836,7 +836,7 @@ static void display_processes(struct traceeval *teval,
 		struct process_data *pdata = NULL;
 		const char *comm = keys[0].cstring;
 
-		ret = traceeval_query(teval_data, keys, &results);
+		ret = traceeval_iterator_query(iter, &results);
 		if (ret < 0)
 			pdie("Could not query iterator");
 		if (ret < 1)
diff --git a/src/histograms.c b/src/histograms.c
index 66bdc1769985..4f603ce36c8c 100644
--- a/src/histograms.c
+++ b/src/histograms.c
@@ -1306,3 +1306,33 @@ int traceeval_iterator_next(struct traceeval_iterator *iter,
 	*keys = entry->keys;
 	return 1;
 }
+
+/**
+ * traceeval_iterator_query - return the results of the last entry in the iterator
+ * @iter: The iterator to retrieve the entry results from
+ * @results: The returned results of the last entry (if exists)
+ *
+ * This returns the @results of the values from the last instance of
+ * traceeval_iterator_next(). It is equivalent of calling:
+ *
+ * traceeval_query_size() with the keys returned by traceeval_iterator_next().
+ *
+ * Except that it will always quickly return the last entry, whereas the
+ * traceeval_query_size() will reset the cached next_entry and do a full
+ * lookup again.
+ *
+ * Returns 1 if another entry is returned, or 0 if not (or negative on error)
+ */
+int traceeval_iterator_query(struct traceeval_iterator *iter,
+			     const union traceeval_data **results)
+{
+	struct entry *entry;
+
+	if (iter->next < 1 || iter->next > iter->nr_entries)
+		return 0;
+
+	entry = iter->entries[iter->next - 1];
+	*results = entry->vals;
+
+	return 1;
+}
-- 
2.40.1

Re: [PATCH 2/9] libtraceeval: Add traceeval_iterator_query()

[Steven Rostedt]

On Thu, 17 Aug 2023 18:24:15 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> @@ -186,5 +186,7 @@ int traceeval_iterator_sort_custom(struct traceeval_iterator *iter,
>  				   traceeval_cmp_fn sort_fn, void *data);
>  int traceeval_iterator_next(struct traceeval_iterator *iter,
>  			    const union traceeval_data **keys);
> +int traceeval_iterator_query(struct traceeval_iterator *iter,
> +			     const union traceeval_data **results);
>  

And while porting this to trace-flames.c, I also realize I need a
traceeval_iterator_results_release() to match it.

The traceeval_results_release() works, but it's nicer not to have to
remember what teval you used.

For example:

	while (traceeval_iterator_next(iter, &keys) > 0) {
		const struct traceeval_data *results;

		if (traceeval_iterator_query(iter, &results) < 1)
			continue;

		stack = results[0].pointer;
		traceeval_results_release(fstack->partial_stacks, results);


Would be much nice with:

	while (traceeval_iterator_next(iter, &keys) > 0) {
		const struct traceeval_data *results;

		if (traceeval_iterator_query(iter, &results) < 1)
			continue;

		stack = results[0].pointer;
		traceeval_iterator_results_release(iter, results);


-- Steve

Re: [PATCH 2/9] libtraceeval: Add traceeval_iterator_query()

[Stevie Alvarez]

On Thu, Aug 17, 2023 at 06:24:15PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> Add a query to the iterator that will return the values of the last entry
> that matches the keys returned by the last call of traceeval_iterator_next().
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
>  include/traceeval-hist.h |  2 ++
>  samples/task-eval.c      |  2 +-
>  src/histograms.c         | 30 ++++++++++++++++++++++++++++++
>  3 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/include/traceeval-hist.h b/include/traceeval-hist.h
> index 0cee9bcbeb83..837a74f61a66 100644
> --- a/include/traceeval-hist.h
> +++ b/include/traceeval-hist.h
> @@ -186,5 +186,7 @@ int traceeval_iterator_sort_custom(struct traceeval_iterator *iter,
>  				   traceeval_cmp_fn sort_fn, void *data);
>  int traceeval_iterator_next(struct traceeval_iterator *iter,
>  			    const union traceeval_data **keys);
> +int traceeval_iterator_query(struct traceeval_iterator *iter,
> +			     const union traceeval_data **results);
>  
>  #endif /* __LIBTRACEEVAL_HIST_H__ */
> diff --git a/samples/task-eval.c b/samples/task-eval.c
> index 66d0c40dc0c8..18e07f44e11e 100644
> --- a/samples/task-eval.c
> +++ b/samples/task-eval.c
> @@ -836,7 +836,7 @@ static void display_processes(struct traceeval *teval,
>  		struct process_data *pdata = NULL;
>  		const char *comm = keys[0].cstring;
>  
> -		ret = traceeval_query(teval_data, keys, &results);
> +		ret = traceeval_iterator_query(iter, &results);
>  		if (ret < 0)
>  			pdie("Could not query iterator");
>  		if (ret < 1)
> diff --git a/src/histograms.c b/src/histograms.c
> index 66bdc1769985..4f603ce36c8c 100644
> --- a/src/histograms.c
> +++ b/src/histograms.c
> @@ -1306,3 +1306,33 @@ int traceeval_iterator_next(struct traceeval_iterator *iter,
>  	*keys = entry->keys;
>  	return 1;
>  }
> +
> +/**
> + * traceeval_iterator_query - return the results of the last entry in the iterator

Nit, 'last entry' made me think you were referring to the *last* entry.
Maybe say previous entry instead?

-- Stevie

> + * @iter: The iterator to retrieve the entry results from
> + * @results: The returned results of the last entry (if exists)
> + *
> + * This returns the @results of the values from the last instance of
> + * traceeval_iterator_next(). It is equivalent of calling:
> + *
> + * traceeval_query_size() with the keys returned by traceeval_iterator_next().
> + *
> + * Except that it will always quickly return the last entry, whereas the
> + * traceeval_query_size() will reset the cached next_entry and do a full
> + * lookup again.
> + *
> + * Returns 1 if another entry is returned, or 0 if not (or negative on error)
> + */
> +int traceeval_iterator_query(struct traceeval_iterator *iter,
> +			     const union traceeval_data **results)
> +{
> +	struct entry *entry;
> +
> +	if (iter->next < 1 || iter->next > iter->nr_entries)
> +		return 0;
> +
> +	entry = iter->entries[iter->next - 1];
> +	*results = entry->vals;
> +
> +	return 1;
> +}
> -- 
> 2.40.1
> 

Re: [PATCH 2/9] libtraceeval: Add traceeval_iterator_query()

[Steven Rostedt]

On Sun, 20 Aug 2023 14:18:01 -0400
Stevie Alvarez <stevie.6strings@gmail.com> wrote:

> > +/**
> > + * traceeval_iterator_query - return the results of the last entry in the iterator  
> 
> Nit, 'last entry' made me think you were referring to the *last* entry.
> Maybe say previous entry instead?

Good point. hmm, what about:

  - return the results from the current entry in the iterator

as traceeval_iterator_next() really just updates the "current" entry.

-- Steve

Re: [PATCH 2/9] libtraceeval: Add traceeval_iterator_query()

[Ross Zwisler]

On Thu, Aug 17, 2023 at 06:24:15PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> Add a query to the iterator that will return the values of the last entry
> that matches the keys returned by the last call of traceeval_iterator_next().
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
>  include/traceeval-hist.h |  2 ++
>  samples/task-eval.c      |  2 +-
>  src/histograms.c         | 30 ++++++++++++++++++++++++++++++
>  3 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/include/traceeval-hist.h b/include/traceeval-hist.h
> index 0cee9bcbeb83..837a74f61a66 100644
> --- a/include/traceeval-hist.h
> +++ b/include/traceeval-hist.h
> @@ -186,5 +186,7 @@ int traceeval_iterator_sort_custom(struct traceeval_iterator *iter,
>  				   traceeval_cmp_fn sort_fn, void *data);
>  int traceeval_iterator_next(struct traceeval_iterator *iter,
>  			    const union traceeval_data **keys);
> +int traceeval_iterator_query(struct traceeval_iterator *iter,
> +			     const union traceeval_data **results);
>  
>  #endif /* __LIBTRACEEVAL_HIST_H__ */
> diff --git a/samples/task-eval.c b/samples/task-eval.c
> index 66d0c40dc0c8..18e07f44e11e 100644
> --- a/samples/task-eval.c
> +++ b/samples/task-eval.c
> @@ -836,7 +836,7 @@ static void display_processes(struct traceeval *teval,
>  		struct process_data *pdata = NULL;
>  		const char *comm = keys[0].cstring;
>  
> -		ret = traceeval_query(teval_data, keys, &results);
> +		ret = traceeval_iterator_query(iter, &results);
>  		if (ret < 0)
>  			pdie("Could not query iterator");
>  		if (ret < 1)
> diff --git a/src/histograms.c b/src/histograms.c
> index 66bdc1769985..4f603ce36c8c 100644
> --- a/src/histograms.c
> +++ b/src/histograms.c
> @@ -1306,3 +1306,33 @@ int traceeval_iterator_next(struct traceeval_iterator *iter,
>  	*keys = entry->keys;
>  	return 1;
>  }
> +
> +/**
> + * traceeval_iterator_query - return the results of the last entry in the iterator
> + * @iter: The iterator to retrieve the entry results from
> + * @results: The returned results of the last entry (if exists)
> + *
> + * This returns the @results of the values from the last instance of
> + * traceeval_iterator_next(). It is equivalent of calling:
> + *
> + * traceeval_query_size() with the keys returned by traceeval_iterator_next().

      traceeval_query()

> + *
> + * Except that it will always quickly return the last entry, whereas the
> + * traceeval_query_size() will reset the cached next_entry and do a full

      traceeval_query()

Aside from these you can add:

Reviewed-by: Ross Zwisler <zwisler@google.com>

I do agree that having a dedicated traceeval_iterator_results_release() would
be nice.

> + * lookup again.
> + *
> + * Returns 1 if another entry is returned, or 0 if not (or negative on error)
> + */
> +int traceeval_iterator_query(struct traceeval_iterator *iter,
> +			     const union traceeval_data **results)
> +{
> +	struct entry *entry;
> +
> +	if (iter->next < 1 || iter->next > iter->nr_entries)
> +		return 0;
> +
> +	entry = iter->entries[iter->next - 1];
> +	*results = entry->vals;
> +
> +	return 1;
> +}
> -- 
> 2.40.1
> 

[PATCH 3/9] libtraceeval: Add traceeval_iterator_stat()

[Steven Rostedt]

From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

Add traceeval_iterator_stat() that will return a stat of a value of the
current entry from a traceeval_iterator_next() loop.

Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 include/traceeval-hist.h |  2 ++
 samples/task-eval.c      |  6 ++---
 src/histograms.c         | 47 ++++++++++++++++++++++++++++++++--------
 3 files changed, 43 insertions(+), 12 deletions(-)

diff --git a/include/traceeval-hist.h b/include/traceeval-hist.h
index 837a74f61a66..d511c9c5f14c 100644
--- a/include/traceeval-hist.h
+++ b/include/traceeval-hist.h
@@ -188,5 +188,7 @@ int traceeval_iterator_next(struct traceeval_iterator *iter,
 			    const union traceeval_data **keys);
 int traceeval_iterator_query(struct traceeval_iterator *iter,
 			     const union traceeval_data **results);
+struct traceeval_stat *traceeval_iterator_stat(struct traceeval_iterator *iter,
+					       struct traceeval_type *type);
 
 #endif /* __LIBTRACEEVAL_HIST_H__ */
diff --git a/samples/task-eval.c b/samples/task-eval.c
index 18e07f44e11e..bde167d1219b 100644
--- a/samples/task-eval.c
+++ b/samples/task-eval.c
@@ -706,7 +706,7 @@ static void display_cpus(struct traceeval *teval)
 		int state = keys[1].number;
 		int cpu = keys[0].number;
 
-		stat = traceeval_stat(teval, keys, &delta_vals[0]);
+		stat = traceeval_iterator_stat(iter, &delta_vals[0]);
 		if (!stat)
 			continue; // die?
 
@@ -773,7 +773,7 @@ static void display_threads(struct traceeval *teval)
 		int state = keys[1].number;
 		int tid = keys[0].number;
 
-		stat = traceeval_stat(teval, keys, &delta_vals[0]);
+		stat = traceeval_iterator_stat(iter, &delta_vals[0]);
 		if (!stat)
 			continue; // die?
 
@@ -875,7 +875,7 @@ static void display(struct task_data *tdata)
 	while (traceeval_iterator_next(iter, &keys) > 0) {
 		int state = keys[1].number;
 
-		stat = traceeval_stat(teval, keys, &delta_vals[0]);
+		stat = traceeval_iterator_stat(iter, &delta_vals[0]);
 		if (!stat)
 			continue;
 
diff --git a/src/histograms.c b/src/histograms.c
index 4f603ce36c8c..fddd0f3587e2 100644
--- a/src/histograms.c
+++ b/src/histograms.c
@@ -812,16 +812,11 @@ static int update_entry(struct traceeval *teval, struct entry *entry,
 	return -1;
 }
 
-struct traceeval_stat *traceeval_stat(struct traceeval *teval,
-				      const union traceeval_data *keys,
-				      struct traceeval_type *type)
+static bool is_stat_type(struct traceeval_type *type)
 {
-	struct entry *entry;
-	int ret;
-
 	/* Only value numbers have stats */
 	if (!(type->flags & TRACEEVAL_FL_VALUE))
-		return NULL;
+		return false;
 
 	switch (type->type) {
 	case TRACEEVAL_TYPE_NUMBER:
@@ -829,10 +824,21 @@ struct traceeval_stat *traceeval_stat(struct traceeval *teval,
 	case TRACEEVAL_TYPE_NUMBER_32:
 	case TRACEEVAL_TYPE_NUMBER_16:
 	case TRACEEVAL_TYPE_NUMBER_8:
-		break;
+		return true;
 	default:
-		return NULL;
+		return false;
 	}
+}
+
+struct traceeval_stat *traceeval_stat(struct traceeval *teval,
+				      const union traceeval_data *keys,
+				      struct traceeval_type *type)
+{
+	struct entry *entry;
+	int ret;
+
+	if (!is_stat_type(type))
+		return NULL;
 
 	ret = get_entry(teval, keys, &entry);
 	if (ret <= 0)
@@ -1336,3 +1342,26 @@ int traceeval_iterator_query(struct traceeval_iterator *iter,
 
 	return 1;
 }
+
+/**
+ * traceeval_iterator_stat - return the stats from the last iterator entry
+ * @iter: The iterator to retrieve the stats from
+ * @type: The value type to get the stat from
+ *
+ * Returns the stats of the @type for the current iterator entry on success,
+ * or NULL if not found or an error occurred.
+ */
+struct traceeval_stat *traceeval_iterator_stat(struct traceeval_iterator *iter,
+					       struct traceeval_type *type)
+{
+	struct entry *entry;
+
+	if (!is_stat_type(type))
+		return NULL;
+
+	if (iter->next < 1 || iter->next > iter->nr_entries)
+		return NULL;
+
+	entry = iter->entries[iter->next - 1];
+	return &entry->val_stats[type->index];
+}
-- 
2.40.1

Re: [PATCH 3/9] libtraceeval: Add traceeval_iterator_stat()

[Ross Zwisler]

On Thu, Aug 17, 2023 at 06:24:16PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> Add traceeval_iterator_stat() that will return a stat of a value of the
> current entry from a traceeval_iterator_next() loop.
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>

Reviewed-by: Ross Zwisler <zwisler@google.com>

[PATCH 4/9] libtraceeval: Add traceeval_iterator_remove()

[Steven Rostedt]

From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

Add an API traceeval_iterator_remove() that is safe to call in the
traceeval_iterator_next() loop. Currently, traceeval_remove() can also be
called "safely", but that may change in the future.

The main difference between traceeval_remove() and traceeval_iterator_remove()
is that that traceeval_iterator_remove() will NULL out the entry in the
sort array, and use this in the other iterator functions. If the entry is
NULL, it will not be returned.

Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 include/traceeval-hist.h |  1 +
 src/histograms.c         | 48 ++++++++++++++++++++++++++++++++++++----
 2 files changed, 45 insertions(+), 4 deletions(-)

diff --git a/include/traceeval-hist.h b/include/traceeval-hist.h
index d511c9c5f14c..7d67673ce7e5 100644
--- a/include/traceeval-hist.h
+++ b/include/traceeval-hist.h
@@ -190,5 +190,6 @@ int traceeval_iterator_query(struct traceeval_iterator *iter,
 			     const union traceeval_data **results);
 struct traceeval_stat *traceeval_iterator_stat(struct traceeval_iterator *iter,
 					       struct traceeval_type *type);
+int traceeval_iterator_remove(struct traceeval_iterator *iter);
 
 #endif /* __LIBTRACEEVAL_HIST_H__ */
diff --git a/src/histograms.c b/src/histograms.c
index fddd0f3587e2..0fbd9e0a353e 100644
--- a/src/histograms.c
+++ b/src/histograms.c
@@ -1305,10 +1305,13 @@ int traceeval_iterator_next(struct traceeval_iterator *iter,
 		iter->next = 0;
 	}
 
-	if (iter->next >= iter->nr_entries)
-		return 0;
+	do {
+		if (iter->next >= iter->nr_entries)
+			return 0;
+
+		entry = iter->entries[iter->next++];
+	} while (!entry);
 
-	entry = iter->entries[iter->next++];
 	*keys = entry->keys;
 	return 1;
 }
@@ -1338,6 +1341,9 @@ int traceeval_iterator_query(struct traceeval_iterator *iter,
 		return 0;
 
 	entry = iter->entries[iter->next - 1];
+	if (!entry)
+		return 0;
+
 	*results = entry->vals;
 
 	return 1;
@@ -1363,5 +1369,39 @@ struct traceeval_stat *traceeval_iterator_stat(struct traceeval_iterator *iter,
 		return NULL;
 
 	entry = iter->entries[iter->next - 1];
-	return &entry->val_stats[type->index];
+	return entry ? &entry->val_stats[type->index] : NULL;
+}
+
+/**
+ * traceeval_iterator_remove - remove the current iterator entry
+ * @iter: The iterator to remove the entry from
+ *
+ * This will remove the current entry from the histogram.
+ * This is useful if the current entry should be removed. It will not
+ * affect the traceeval_iterator_next().
+ *
+ * Returns 1 if it successfully removed the entry, 0 if for some reason
+ * there was no "current entry" (called before traceeval_iterator_next()).
+ * or -1 on error.
+ */
+int traceeval_iterator_remove(struct traceeval_iterator *iter)
+{
+	struct traceeval *teval = iter->teval;
+	struct hash_table *hist = teval->hist;
+	struct entry *entry;
+
+	if (iter->next < 1 || iter->next > iter->nr_entries)
+		return 0;
+
+	entry = iter->entries[iter->next - 1];
+	if (!entry)
+		return 0;
+
+	hash_remove(hist, &entry->hash);
+
+	/* The entry no longer exists */
+	iter->entries[iter->next - 1] = NULL;
+	teval->update_counter++;
+
+	return 1;
 }
-- 
2.40.1

Re: [PATCH 4/9] libtraceeval: Add traceeval_iterator_remove()

[Ross Zwisler]

On Thu, Aug 17, 2023 at 06:24:17PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> Add an API traceeval_iterator_remove() that is safe to call in the
> traceeval_iterator_next() loop. Currently, traceeval_remove() can also be
> called "safely", but that may change in the future.
> 
> The main difference between traceeval_remove() and traceeval_iterator_remove()
> is that that traceeval_iterator_remove() will NULL out the entry in the
> sort array, and use this in the other iterator functions. If the entry is
> NULL, it will not be returned.
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
>  include/traceeval-hist.h |  1 +
>  src/histograms.c         | 48 ++++++++++++++++++++++++++++++++++++----
>  2 files changed, 45 insertions(+), 4 deletions(-)
> 
> diff --git a/include/traceeval-hist.h b/include/traceeval-hist.h
> index d511c9c5f14c..7d67673ce7e5 100644
> --- a/include/traceeval-hist.h
> +++ b/include/traceeval-hist.h
> @@ -190,5 +190,6 @@ int traceeval_iterator_query(struct traceeval_iterator *iter,
>  			     const union traceeval_data **results);
>  struct traceeval_stat *traceeval_iterator_stat(struct traceeval_iterator *iter,
>  					       struct traceeval_type *type);
> +int traceeval_iterator_remove(struct traceeval_iterator *iter);
>  
>  #endif /* __LIBTRACEEVAL_HIST_H__ */
> diff --git a/src/histograms.c b/src/histograms.c
> index fddd0f3587e2..0fbd9e0a353e 100644
> --- a/src/histograms.c
> +++ b/src/histograms.c
> @@ -1305,10 +1305,13 @@ int traceeval_iterator_next(struct traceeval_iterator *iter,
>  		iter->next = 0;
>  	}
>  
> -	if (iter->next >= iter->nr_entries)
> -		return 0;
> +	do {
> +		if (iter->next >= iter->nr_entries)
> +			return 0;
> +
> +		entry = iter->entries[iter->next++];
> +	} while (!entry);
>  
> -	entry = iter->entries[iter->next++];
>  	*keys = entry->keys;
>  	return 1;
>  }
> @@ -1338,6 +1341,9 @@ int traceeval_iterator_query(struct traceeval_iterator *iter,
>  		return 0;
>  
>  	entry = iter->entries[iter->next - 1];
> +	if (!entry)
> +		return 0;
> +
>  	*results = entry->vals;
>  
>  	return 1;
> @@ -1363,5 +1369,39 @@ struct traceeval_stat *traceeval_iterator_stat(struct traceeval_iterator *iter,
>  		return NULL;
>  
>  	entry = iter->entries[iter->next - 1];
> -	return &entry->val_stats[type->index];
> +	return entry ? &entry->val_stats[type->index] : NULL;
> +}
> +
> +/**
> + * traceeval_iterator_remove - remove the current iterator entry
> + * @iter: The iterator to remove the entry from
> + *
> + * This will remove the current entry from the histogram.
> + * This is useful if the current entry should be removed. It will not
> + * affect the traceeval_iterator_next().
> + *
> + * Returns 1 if it successfully removed the entry, 0 if for some reason
> + * there was no "current entry" (called before traceeval_iterator_next()).
> + * or -1 on error.
> + */
> +int traceeval_iterator_remove(struct traceeval_iterator *iter)
> +{
> +	struct traceeval *teval = iter->teval;
> +	struct hash_table *hist = teval->hist;
> +	struct entry *entry;
> +
> +	if (iter->next < 1 || iter->next > iter->nr_entries)
> +		return 0;
> +
> +	entry = iter->entries[iter->next - 1];
> +	if (!entry)
> +		return 0;
> +
> +	hash_remove(hist, &entry->hash);

Are we leaking 'entry' after we've removed it from the hash?

I think we need to call free_entry() in both traceeval_iterator_remove() as
well as traceeval_remove(), just like we do in the loop in
hist_table_release().

> +
> +	/* The entry no longer exists */
> +	iter->entries[iter->next - 1] = NULL;
> +	teval->update_counter++;
> +
> +	return 1;
>  }
> -- 
> 2.40.1
> 

Re: [PATCH 4/9] libtraceeval: Add traceeval_iterator_remove()

[Ross Zwisler]

On Thu, Aug 24, 2023 at 02:19:06PM -0600, Ross Zwisler wrote:
> On Thu, Aug 17, 2023 at 06:24:17PM -0400, Steven Rostedt wrote:
> > From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> > 
> > Add an API traceeval_iterator_remove() that is safe to call in the
> > traceeval_iterator_next() loop. Currently, traceeval_remove() can also be
> > called "safely", but that may change in the future.
> > 
> > The main difference between traceeval_remove() and traceeval_iterator_remove()
> > is that that traceeval_iterator_remove() will NULL out the entry in the
> > sort array, and use this in the other iterator functions. If the entry is
> > NULL, it will not be returned.
> > 
> > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> > ---
> >  include/traceeval-hist.h |  1 +
> >  src/histograms.c         | 48 ++++++++++++++++++++++++++++++++++++----
> >  2 files changed, 45 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/traceeval-hist.h b/include/traceeval-hist.h
> > index d511c9c5f14c..7d67673ce7e5 100644
> > --- a/include/traceeval-hist.h
> > +++ b/include/traceeval-hist.h
> > @@ -190,5 +190,6 @@ int traceeval_iterator_query(struct traceeval_iterator *iter,
> >  			     const union traceeval_data **results);
> >  struct traceeval_stat *traceeval_iterator_stat(struct traceeval_iterator *iter,
> >  					       struct traceeval_type *type);
> > +int traceeval_iterator_remove(struct traceeval_iterator *iter);
> >  
> >  #endif /* __LIBTRACEEVAL_HIST_H__ */
> > diff --git a/src/histograms.c b/src/histograms.c
> > index fddd0f3587e2..0fbd9e0a353e 100644
> > --- a/src/histograms.c
> > +++ b/src/histograms.c
> > @@ -1305,10 +1305,13 @@ int traceeval_iterator_next(struct traceeval_iterator *iter,
> >  		iter->next = 0;
> >  	}
> >  
> > -	if (iter->next >= iter->nr_entries)
> > -		return 0;
> > +	do {
> > +		if (iter->next >= iter->nr_entries)
> > +			return 0;
> > +
> > +		entry = iter->entries[iter->next++];
> > +	} while (!entry);
> >  
> > -	entry = iter->entries[iter->next++];
> >  	*keys = entry->keys;
> >  	return 1;
> >  }
> > @@ -1338,6 +1341,9 @@ int traceeval_iterator_query(struct traceeval_iterator *iter,
> >  		return 0;
> >  
> >  	entry = iter->entries[iter->next - 1];
> > +	if (!entry)
> > +		return 0;
> > +
> >  	*results = entry->vals;
> >  
> >  	return 1;
> > @@ -1363,5 +1369,39 @@ struct traceeval_stat *traceeval_iterator_stat(struct traceeval_iterator *iter,
> >  		return NULL;
> >  
> >  	entry = iter->entries[iter->next - 1];
> > -	return &entry->val_stats[type->index];
> > +	return entry ? &entry->val_stats[type->index] : NULL;
> > +}
> > +
> > +/**
> > + * traceeval_iterator_remove - remove the current iterator entry
> > + * @iter: The iterator to remove the entry from
> > + *
> > + * This will remove the current entry from the histogram.
> > + * This is useful if the current entry should be removed. It will not
> > + * affect the traceeval_iterator_next().
> > + *
> > + * Returns 1 if it successfully removed the entry, 0 if for some reason
> > + * there was no "current entry" (called before traceeval_iterator_next()).
> > + * or -1 on error.

Nit: we never actually return -1.  Only 1 and 0.

> > + */
> > +int traceeval_iterator_remove(struct traceeval_iterator *iter)
> > +{
> > +	struct traceeval *teval = iter->teval;
> > +	struct hash_table *hist = teval->hist;
> > +	struct entry *entry;
> > +
> > +	if (iter->next < 1 || iter->next > iter->nr_entries)
> > +		return 0;
> > +
> > +	entry = iter->entries[iter->next - 1];
> > +	if (!entry)
> > +		return 0;
> > +
> > +	hash_remove(hist, &entry->hash);
> 
> Are we leaking 'entry' after we've removed it from the hash?
> 
> I think we need to call free_entry() in both traceeval_iterator_remove() as
> well as traceeval_remove(), just like we do in the loop in
> hist_table_release().
> 
> > +
> > +	/* The entry no longer exists */
> > +	iter->entries[iter->next - 1] = NULL;
> > +	teval->update_counter++;
> > +
> > +	return 1;
> >  }
> > -- 
> > 2.40.1
> > 

Re: [PATCH 4/9] libtraceeval: Add traceeval_iterator_remove()

[Steven Rostedt]

On Thu, 24 Aug 2023 14:23:24 -0600
Ross Zwisler <zwisler@google.com> wrote:

> > > + * Returns 1 if it successfully removed the entry, 0 if for some reason
> > > + * there was no "current entry" (called before traceeval_iterator_next()).
> > > + * or -1 on error.  
> 
> Nit: we never actually return -1.  Only 1 and 0.
> 

Yeah, I usually just throw the -1 in there in case I want to add more
error checking, but since 1 is success, I'll just make it 1 and 0
anyway.

Thanks,

-- Steve

Re: [PATCH 4/9] libtraceeval: Add traceeval_iterator_remove()

[Steven Rostedt]

On Thu, 24 Aug 2023 14:19:06 -0600
Ross Zwisler <zwisler@google.com> wrote:

> > +int traceeval_iterator_remove(struct traceeval_iterator *iter)
> > +{
> > +	struct traceeval *teval = iter->teval;
> > +	struct hash_table *hist = teval->hist;
> > +	struct entry *entry;
> > +
> > +	if (iter->next < 1 || iter->next > iter->nr_entries)
> > +		return 0;
> > +
> > +	entry = iter->entries[iter->next - 1];
> > +	if (!entry)
> > +		return 0;
> > +
> > +	hash_remove(hist, &entry->hash);  
> 
> Are we leaking 'entry' after we've removed it from the hash?

Yeah, I guess we are. Good catch!

> 
> I think we need to call free_entry() in both traceeval_iterator_remove() as
> well as traceeval_remove(), just like we do in the loop in
> hist_table_release().

Yep, thanks Ross!

-- Steve

> 
> > +
> > +	/* The entry no longer exists */
> > +	iter->entries[iter->next - 1] = NULL;
> > +	teval->update_counter++;
> > +
> > +	return 1;
> >  }
> > -- 
> > 2.40.1
> >   

[PATCH 5/9] libtraceeval histogram: Add type to traceeval_data and make it a structure

[Steven Rostedt]

From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

Having a straight union for passing in the data that must match the types
is dangerous and prone for buggy code. If some data doesn't match its
type, there's nothing to catch it.

Instead of having a union traceeval_data of each type, have it be a
structure with a "type" field follow by a union, where the type defines
what kind of data it is.

Add helper macros to make it easy to define the data when using it.

Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 include/traceeval-hist.h |  88 ++++++++++++++++---------
 samples/task-eval.c      | 137 ++++++++++++++++-----------------------
 src/eval-local.h         |   4 +-
 src/histograms.c         |  68 +++++++++----------
 4 files changed, 150 insertions(+), 147 deletions(-)

diff --git a/include/traceeval-hist.h b/include/traceeval-hist.h
index 7d67673ce7e5..38d5dc878d3b 100644
--- a/include/traceeval-hist.h
+++ b/include/traceeval-hist.h
@@ -52,45 +52,75 @@ struct traceeval_dynamic {
  * Trace data entry for a traceeval histogram
  * Constitutes keys and values.
  */
-union traceeval_data {
-	struct traceeval_dynamic	dyn_data;
-	char				*string;
-	const char			*cstring;
-	void				*pointer;
-	unsigned long			number;
-	unsigned long long		number_64;
-	unsigned int			number_32;
-	unsigned short			number_16;
-	unsigned char			number_8;
+struct traceeval_data {
+	enum traceeval_data_type		type;
+	union {
+		struct traceeval_dynamic	dyn_data;
+		char				*string;
+		const char			*cstring;
+		void				*pointer;
+		unsigned long			number;
+		unsigned long long		number_64;
+		unsigned int			number_32;
+		unsigned short			number_16;
+		unsigned char			number_8;
+	};
 };
 
+#define __TRACEEVAL_DATA(data_type, member, data)			\
+	{  .type = TRACEEVAL_TYPE_##data_type, .member = (data) }
+
+#define DEFINE_TRACEEVAL_NUMBER(data)	   __TRACEEVAL_DATA(NUMBER, number, data)
+#define DEFINE_TRACEEVAL_NUMBER_8(data)	   __TRACEEVAL_DATA(NUMBER_8, number_8, data)
+#define DEFINE_TRACEEVAL_NUMBER_16(data)   __TRACEEVAL_DATA(NUMBER_16, number_16, data)
+#define DEFINE_TRACEEVAL_NUMBER_32(data)   __TRACEEVAL_DATA(NUMBER_32, number_32, data)
+#define DEFINE_TRACEEVAL_NUMBER_64(data)   __TRACEEVAL_DATA(NUMBER_64, number_64, data)
+#define DEFINE_TRACEEVAL_STRING(data)	   __TRACEEVAL_DATA(STRING, string, data)
+#define DEFINE_TRACEEVAL_CSTRING(data)	   __TRACEEVAL_DATA(STRING, cstring, data)
+#define DEFINE_TRACEEVAL_POINTER(data)	   __TRACEEVAL_DATA(POINTER, pointer, data)
+
+#define __TRACEEVAL_SET(data, data_type, member, val)		\
+	do {							\
+		(data).type = TRACEEVAL_TYPE_##data_type;	\
+		(data).member = (val);				\
+	} while (0)
+
+#define TRACEEVAL_SET_NUMBER(data, val)	     __TRACEEVAL_SET(data, NUMBER, number, val)
+#define TRACEEVAL_SET_NUMBER_8(data, val)    __TRACEEVAL_SET(data, NUMBER_8, number_8, val)
+#define TRACEEVAL_SET_NUMBER_16(data, val)   __TRACEEVAL_SET(data, NUMBER_16, number_16, val)
+#define TRACEEVAL_SET_NUMBER_32(data, val)   __TRACEEVAL_SET(data, NUMBER_32, number_32, val)
+#define TRACEEVAL_SET_NUMBER_64(data, val)   __TRACEEVAL_SET(data, NUMBER_64, number_64, val)
+#define TRACEEVAL_SET_STRING(data, val)	     __TRACEEVAL_SET(data, STRING, string, val)
+#define TRACEEVAL_SET_CSTRING(data, val)     __TRACEEVAL_SET(data, STRING, cstring, val)
+#define TRACEEVAL_SET_POINTER(data, val)     __TRACEEVAL_SET(data, POINTER, pointer, val)
+
 struct traceeval_type;
 struct traceeval;
 
 /* release function callback on traceeval_data */
 typedef void (*traceeval_data_release_fn)(const struct traceeval_type *type,
-					  union traceeval_data *data);
+					  struct traceeval_data *data);
 
 /* compare function callback to compare traceeval_data */
 typedef int (*traceeval_data_cmp_fn)(struct traceeval *teval,
 				     const struct traceeval_type *type,
-				     const union traceeval_data *A,
-				     const union traceeval_data *B);
+				     const struct traceeval_data *A,
+				     const struct traceeval_data *B);
 
 /* make a unique value */
 typedef int (*traceeval_data_hash_fn)(struct traceeval *teval,
 				      const struct traceeval_type *type,
-				      const union traceeval_data *data);
+				      const struct traceeval_data *data);
 
 typedef int (*traceeval_data_copy_fn)(const struct traceeval_type *type,
-				      union traceeval_data *dst,
-				      const union traceeval_data *src);
+				      struct traceeval_data *dst,
+				      const struct traceeval_data *src);
 
 typedef int (*traceeval_cmp_fn)(struct traceeval *teval,
-				const union traceeval_data *Akeys,
-				const union traceeval_data *Avals,
-				const union traceeval_data *Bkeys,
-				const union traceeval_data *Bvals,
+				const struct traceeval_data *Akeys,
+				const struct traceeval_data *Avals,
+				const struct traceeval_data *Bkeys,
+				const struct traceeval_data *Bvals,
 				void *data);
 
 /*
@@ -157,20 +187,20 @@ struct traceeval *traceeval_init(struct traceeval_type *keys,
 void traceeval_release(struct traceeval *teval);
 
 int traceeval_insert(struct traceeval *teval,
-		     const union traceeval_data *keys,
-		     const union traceeval_data *vals);
+		     const struct traceeval_data *keys,
+		     const struct traceeval_data *vals);
 
 int traceeval_remove(struct traceeval *teval,
-		     const union traceeval_data *keys);
+		     const struct traceeval_data *keys);
 
-int traceeval_query(struct traceeval *teval, const union traceeval_data *keys,
-		    const union traceeval_data **results);
+int traceeval_query(struct traceeval *teval, const struct traceeval_data *keys,
+		    const struct traceeval_data **results);
 
 void traceeval_results_release(struct traceeval *teval,
-			       const union traceeval_data *results);
+			       const struct traceeval_data *results);
 
 struct traceeval_stat *traceeval_stat(struct traceeval *teval,
-				      const union traceeval_data *keys,
+				      const struct traceeval_data *keys,
 				      struct traceeval_type *type);
 
 unsigned long long traceeval_stat_max(struct traceeval_stat *stat);
@@ -185,9 +215,9 @@ int traceeval_iterator_sort(struct traceeval_iterator *iter, const char *sort_fi
 int traceeval_iterator_sort_custom(struct traceeval_iterator *iter,
 				   traceeval_cmp_fn sort_fn, void *data);
 int traceeval_iterator_next(struct traceeval_iterator *iter,
-			    const union traceeval_data **keys);
+			    const struct traceeval_data **keys);
 int traceeval_iterator_query(struct traceeval_iterator *iter,
-			     const union traceeval_data **results);
+			     const struct traceeval_data **results);
 struct traceeval_stat *traceeval_iterator_stat(struct traceeval_iterator *iter,
 					       struct traceeval_type *type);
 int traceeval_iterator_remove(struct traceeval_iterator *iter);
diff --git a/samples/task-eval.c b/samples/task-eval.c
index bde167d1219b..ed3a1a95f097 100644
--- a/samples/task-eval.c
+++ b/samples/task-eval.c
@@ -190,21 +190,15 @@ static void update_process(struct task_data *tdata, const char *comm,
 			   enum sched_state state, enum command cmd,
 			   unsigned long long ts)
 {
-	union traceeval_data keys[] = {
-		{
-			.cstring	= comm,
-		},
-		{
-			.number		= state,
-		},
+	struct traceeval_data keys[] = {
+		DEFINE_TRACEEVAL_CSTRING(	comm	),
+		DEFINE_TRACEEVAL_NUMBER(	state	),
 	};
-	union traceeval_data vals[] = {
-		{
-			.number_64	= ts,
-		},
+	struct traceeval_data vals[] = {
+		DEFINE_TRACEEVAL_NUMBER_64(	ts	),
 	};
-	union traceeval_data new_vals[1] = { };
-	const union traceeval_data *results;
+	struct traceeval_data new_vals[1] = { };
+	const struct traceeval_data *results;
 	int ret;
 
 	switch (cmd) {
@@ -222,14 +216,15 @@ static void update_process(struct task_data *tdata, const char *comm,
 		if (!results[0].number_64)
 			break;
 
-		new_vals[0].number_64 = ts - results[0].number_64;
+		TRACEEVAL_SET_NUMBER_64(new_vals[0], ts - results[0].number_64);
 
 		ret = traceeval_insert(tdata->teval_processes.stop, keys, new_vals);
 		if (ret < 0)
 			pdie("Could not stop process");
 
 		/* Reset the start */
-		new_vals[0].number_64 = 0;
+		TRACEEVAL_SET_NUMBER_64(new_vals[0], 0);
+
 		ret = traceeval_insert(tdata->teval_processes.start, keys, new_vals);
 		if (ret < 0)
 			pdie("Could not start CPU");
@@ -253,15 +248,11 @@ static void stop_process(struct task_data *tdata, const char *comm,
 static struct process_data *
 get_process_data(struct task_data *tdata, const char *comm)
 {
-	union traceeval_data keys[] = {
-		{
-			.cstring = comm,
-		},
-		{
-			.number = RUNNING,
-		}
+	struct traceeval_data keys[] = {
+		DEFINE_TRACEEVAL_CSTRING(	comm	),
+		DEFINE_TRACEEVAL_NUMBER(	RUNNING	),
 	};
-	const union traceeval_data *results;
+	const struct traceeval_data *results;
 	void *data;
 	int ret;
 
@@ -278,16 +269,12 @@ get_process_data(struct task_data *tdata, const char *comm)
 
 void set_process_data(struct task_data *tdata, const char *comm, void *data)
 {
-	union traceeval_data keys[] = {
-		{
-			.cstring = comm,
-		},
-		{
-			.number = RUNNING,
-		}
+	struct traceeval_data keys[] = {
+		DEFINE_TRACEEVAL_CSTRING(	comm	),
+		DEFINE_TRACEEVAL_NUMBER(	RUNNING	),
 	};
-	union traceeval_data new_vals[1] = { };
-	const union traceeval_data *results;
+	struct traceeval_data new_vals[1] = { };
+	const struct traceeval_data *results;
 	int ret;
 
 	ret = traceeval_query(tdata->teval_processes_data, keys, &results);
@@ -296,7 +283,7 @@ void set_process_data(struct task_data *tdata, const char *comm, void *data)
 	if (ret < 0)
 		pdie("Could not query process data");
 
-	new_vals[0].pointer = data;
+	TRACEEVAL_SET_POINTER(new_vals[0], data);
 	ret = traceeval_insert(tdata->teval_processes_data, keys, new_vals);
 	if (ret < 0)
 		pdie("Failed to set process data");
@@ -309,21 +296,15 @@ static void update_cpu(struct teval_pair *teval_pair, int cpu,
 		       enum sched_state state, enum command cmd,
 		       unsigned long long ts)
 {
-	const union traceeval_data *results;
-	union traceeval_data keys[] = {
-		{
-			.number = cpu,
-		},
-		{
-			.number = state,
-		}
+	const struct traceeval_data *results;
+	struct traceeval_data keys[] = {
+		DEFINE_TRACEEVAL_NUMBER(	cpu	),
+		DEFINE_TRACEEVAL_NUMBER(	state	),
 	};
-	union traceeval_data vals[] = {
-		{
-			.number_64	= ts,
-		},
+	struct traceeval_data vals[] = {
+		DEFINE_TRACEEVAL_NUMBER_64(	ts	),
 	};
-	union traceeval_data new_vals[1] = { };
+	struct traceeval_data new_vals[1] = { };
 	int ret;
 
 	switch (cmd) {
@@ -350,14 +331,14 @@ static void update_cpu(struct teval_pair *teval_pair, int cpu,
 		if (!results[0].number_64)
 			break;
 
-		new_vals[0].number_64 = ts - results[0].number_64;
+		TRACEEVAL_SET_NUMBER_64(new_vals[0], ts - results[0].number_64);
 
 		ret = traceeval_insert(teval_pair->stop, keys, new_vals);
 		if (ret < 0)
 			pdie("Could not stop CPU");
 
 		/* Reset the start */
-		new_vals[0].number_64 = 0;
+		TRACEEVAL_SET_NUMBER_64(new_vals[0], 0);
 		ret = traceeval_insert(teval_pair->start, keys, new_vals);
 		if (ret < 0)
 			pdie("Could not start CPU");
@@ -385,21 +366,15 @@ static void update_thread(struct process_data *pdata, int tid,
 			  enum sched_state state, enum command cmd,
 			  unsigned long long ts)
 {
-	const union traceeval_data *results;
-	union traceeval_data keys[] = {
-		{
-			.number = tid,
-		},
-		{
-			.number = state,
-		}
+	const struct traceeval_data *results;
+	struct traceeval_data keys[] = {
+		DEFINE_TRACEEVAL_NUMBER(	tid	),
+		DEFINE_TRACEEVAL_NUMBER(	state	),
 	};
-	union traceeval_data vals[] = {
-		{
-			.number_64	= ts,
-		},
+	struct traceeval_data vals[] = {
+		DEFINE_TRACEEVAL_NUMBER_64(	ts	),
 	};
-	union traceeval_data new_vals[1] = { };
+	struct traceeval_data new_vals[1] = { };
 	int ret;
 
 	switch (cmd) {
@@ -415,7 +390,7 @@ static void update_thread(struct process_data *pdata, int tid,
 		if (ret == 0)
 			return;
 
-		new_vals[0].number_64 = ts - results[0].number_64;
+		TRACEEVAL_SET_NUMBER_64(new_vals[0], ts - results[0].number_64);
 
 		ret = traceeval_insert(pdata->teval_threads.stop, keys, new_vals);
 		traceeval_results_release(pdata->teval_threads.start, results);
@@ -646,17 +621,19 @@ static void print_microseconds(int idx, unsigned long long nsecs)
  *  If RUNNING is equal, then sort by COMM.
  */
 static int compare_pdata(struct traceeval *teval_data,
-				const union traceeval_data *Akeys,
-				const union traceeval_data *Avals,
-				const union traceeval_data *Bkeys,
-				const union traceeval_data *Bvals,
+				const struct traceeval_data *Akeys,
+				const struct traceeval_data *Avals,
+				const struct traceeval_data *Bkeys,
+				const struct traceeval_data *Bvals,
 				void *data)
 {
 	struct traceeval *teval = data; /* The deltas are here */
-	union traceeval_data keysA[] = {
-		{ .cstring = Akeys[0].cstring }, { .number = RUNNING } };
-	union traceeval_data keysB[] = {
-		{ .cstring = Bkeys[0].cstring }, { .number = RUNNING } };
+	struct traceeval_data keysA[] = {
+		DEFINE_TRACEEVAL_CSTRING(	Akeys[0].cstring	),
+		DEFINE_TRACEEVAL_NUMBER(	RUNNING			), };
+	struct traceeval_data keysB[] = {
+		DEFINE_TRACEEVAL_CSTRING(	Bkeys[0].cstring	),
+		DEFINE_TRACEEVAL_NUMBER(	RUNNING			), };
 	struct traceeval_stat *statA;
 	struct traceeval_stat *statB;
 	unsigned long long totalA = -1;
@@ -690,7 +667,7 @@ static int compare_pdata(struct traceeval *teval_data,
 static void display_cpus(struct traceeval *teval)
 {
 	struct traceeval_iterator *iter = traceeval_iterator_get(teval);
-	const union traceeval_data *keys;
+	const struct traceeval_data *keys;
 	struct traceeval_stat *stat;
 	int last_cpu = -1;
 
@@ -762,7 +739,7 @@ static void display_state_times(int state, unsigned long long total)
 static void display_threads(struct traceeval *teval)
 {
 	struct traceeval_iterator *iter = traceeval_iterator_get(teval);
-	const union traceeval_data *keys;
+	const struct traceeval_data *keys;
 	struct traceeval_stat *stat;
 	int last_tid = -1;
 
@@ -802,13 +779,9 @@ static void display_process_stats(struct traceeval *teval,
 {
 	struct traceeval_stat *stat;
 	unsigned long long delta;
-	union traceeval_data keys[] = {
-		{
-			.cstring = comm,
-		},
-		{
-			.number = RUNNING,
-		}
+	struct traceeval_data keys[] = {
+		DEFINE_TRACEEVAL_CSTRING(	comm		),
+		DEFINE_TRACEEVAL_NUMBER(	RUNNING		),
 	};
 
 	for (int i = 0; i < OTHER; i++) {
@@ -826,13 +799,13 @@ static void display_processes(struct traceeval *teval,
 			      struct traceeval *teval_data)
 {
 	struct traceeval_iterator *iter = traceeval_iterator_get(teval_data);
-	const union traceeval_data *keys;
+	const struct traceeval_data *keys;
 	int ret;
 
 	traceeval_iterator_sort_custom(iter, compare_pdata, teval);
 
 	while (traceeval_iterator_next(iter, &keys) > 0) {
-		const union traceeval_data *results;
+		const struct traceeval_data *results;
 		struct process_data *pdata = NULL;
 		const char *comm = keys[0].cstring;
 
@@ -857,7 +830,7 @@ static void display(struct task_data *tdata)
 {
 	struct traceeval *teval = tdata->teval_cpus.stop;
 	struct traceeval_iterator *iter = traceeval_iterator_get(teval);
-	const union traceeval_data *keys;
+	const struct traceeval_data *keys;
 	struct traceeval_stat *stat;
 	unsigned long long total_time = 0;
 	unsigned long long idle_time = 0;
diff --git a/src/eval-local.h b/src/eval-local.h
index 73f52efdf0da..26b3c9b29929 100644
--- a/src/eval-local.h
+++ b/src/eval-local.h
@@ -56,8 +56,8 @@ struct traceeval_stat {
 /* A key-value pair */
 struct entry {
 	struct hash_item	hash;
-	union traceeval_data	*keys;
-	union traceeval_data	*vals;
+	struct traceeval_data	*keys;
+	struct traceeval_data	*vals;
 	struct traceeval_stat	*val_stats;
 };
 
diff --git a/src/histograms.c b/src/histograms.c
index 0fbd9e0a353e..7c0eef6cf421 100644
--- a/src/histograms.c
+++ b/src/histograms.c
@@ -37,8 +37,8 @@ static void print_err(const char *fmt, ...)
  */
 static int compare_traceeval_data(struct traceeval *teval,
 				  struct traceeval_type *type,
-				  const union traceeval_data *orig,
-				  const union traceeval_data *copy)
+				  const struct traceeval_data *orig,
+				  const struct traceeval_data *copy)
 {
 	int i;
 
@@ -86,14 +86,14 @@ static int compare_traceeval_data(struct traceeval *teval,
 }
 
 /*
- * Compare arrays of union traceeval_data's with respect to @def.
+ * Compare arrays of struct traceeval_data's with respect to @def.
  *
  * Return 1 if @orig and @copy are the same, 0 if not, and -1 on error.
  */
 static int compare_traceeval_data_set(struct traceeval *teval,
 				      struct traceeval_type *defs,
-				      union traceeval_data *orig,
-				      const union traceeval_data *copy, size_t size)
+				      struct traceeval_data *orig,
+				      const struct traceeval_data *copy, size_t size)
 {
 	int check;
 	size_t i;
@@ -335,7 +335,7 @@ fail:
 /*
  * Frees dynamic data in @data if @type specifies a dynamic data type.
  */
-static void clean_data(union traceeval_data *data, struct traceeval_type *type)
+static void clean_data(struct traceeval_data *data, struct traceeval_type *type)
 {
 		if (type->release)
 			type->release(type, data);
@@ -352,7 +352,7 @@ static void clean_data(union traceeval_data *data, struct traceeval_type *type)
 /*
  * Free any specified dynamic data in @data.
  */
-static void clean_data_set(union traceeval_data *data, struct traceeval_type *defs,
+static void clean_data_set(struct traceeval_data *data, struct traceeval_type *defs,
 		       size_t size)
 {
 	size_t i;
@@ -431,7 +431,7 @@ void traceeval_release(struct traceeval *teval)
 	free(teval);
 }
 
-static unsigned make_hash(struct traceeval *teval, const union traceeval_data *keys,
+static unsigned make_hash(struct traceeval *teval, const struct traceeval_data *keys,
 			  int bits)
 {
 	const struct traceeval_type *types = teval->key_types;
@@ -470,7 +470,7 @@ static unsigned make_hash(struct traceeval *teval, const union traceeval_data *k
  *
  * Returns 1 on success, 0 if no match found, -1 on error.
  */
-static int get_entry(struct traceeval *teval, const union traceeval_data *keys,
+static int get_entry(struct traceeval *teval, const struct traceeval_data *keys,
 		     struct entry **result)
 {
 	struct hash_table *hist = teval->hist;
@@ -506,8 +506,8 @@ static int get_entry(struct traceeval *teval, const union traceeval_data *keys,
  */
 static int copy_traceeval_data(struct traceeval_type *type,
 			       struct traceeval_stat *stat,
-			       union traceeval_data *dst,
-			       const union traceeval_data *src)
+			       struct traceeval_data *dst,
+			       const struct traceeval_data *src)
 {
 	unsigned long long val;
 
@@ -597,7 +597,7 @@ static int copy_traceeval_data(struct traceeval_type *type,
  *
  * Does not call the release() callback if a copy() exists.
  */
-static void data_release(size_t size, union traceeval_data *data,
+static void data_release(size_t size, struct traceeval_data *data,
 			 struct traceeval_type *type)
 {
 	for (size_t i = 0; i < size; i++) {
@@ -610,7 +610,7 @@ static void data_release(size_t size, union traceeval_data *data,
 	}
 }
 
-static void data_release_and_free(size_t size, union traceeval_data **data,
+static void data_release_and_free(size_t size, struct traceeval_data **data,
 				struct traceeval_type *type)
 {
 	data_release(size, *data, type);
@@ -625,8 +625,8 @@ static void data_release_and_free(size_t size, union traceeval_data **data,
  */
 static int dup_traceeval_data_set(size_t size, struct traceeval_type *type,
 				  struct traceeval_stat *stats,
-				  const union traceeval_data *orig,
-				  union traceeval_data **copy)
+				  const struct traceeval_data *orig,
+				  struct traceeval_data **copy)
 {
 	size_t i;
 
@@ -669,8 +669,8 @@ fail:
  *
  * Returns 1 if found, 0 if not found, and -1 on error.
  */
-int traceeval_query(struct traceeval *teval, const union traceeval_data *keys,
-		    const union traceeval_data **results)
+int traceeval_query(struct traceeval *teval, const struct traceeval_data *keys,
+		    const struct traceeval_data **results)
 {
 	struct entry *entry;
 	int check;
@@ -698,7 +698,7 @@ int traceeval_query(struct traceeval *teval, const union traceeval_data *keys,
  * allow traceeval to clean up its references.
  */
 void traceeval_results_release(struct traceeval *teval,
-			       const union traceeval_data *results)
+			       const struct traceeval_data *results)
 {
 	if (!teval || !results) {
 		if (!teval)
@@ -708,7 +708,7 @@ void traceeval_results_release(struct traceeval *teval,
 }
 
 static struct entry *create_hist_entry(struct traceeval *teval,
-				       const union traceeval_data *keys)
+				       const struct traceeval_data *keys)
 {
 	struct hash_table *hist = teval->hist;
 	unsigned key = make_hash(teval, keys, hist->bits);
@@ -729,11 +729,11 @@ static struct entry *create_hist_entry(struct traceeval *teval,
  * Returns 0 on success, -1 on error.
  */
 static int create_entry(struct traceeval *teval,
-			const union traceeval_data *keys,
-			const union traceeval_data *vals)
+			const struct traceeval_data *keys,
+			const struct traceeval_data *vals)
 {
-	union traceeval_data *new_keys;
-	union traceeval_data *new_vals;
+	struct traceeval_data *new_keys;
+	struct traceeval_data *new_vals;
 	struct entry *entry;
 
 	entry = create_hist_entry(teval, keys);
@@ -780,12 +780,12 @@ fail_entry:
  * Return 0 on success, -1 on error.
  */
 static int update_entry(struct traceeval *teval, struct entry *entry,
-			const union traceeval_data *vals)
+			const struct traceeval_data *vals)
 {
 	struct traceeval_stat *stats = entry->val_stats;
 	struct traceeval_type *types = teval->val_types;
-	union traceeval_data *copy = entry->vals;
-	union traceeval_data old[teval->nr_val_types];
+	struct traceeval_data *copy = entry->vals;
+	struct traceeval_data old[teval->nr_val_types];
 	size_t size = teval->nr_val_types;
 	size_t i;
 
@@ -831,7 +831,7 @@ static bool is_stat_type(struct traceeval_type *type)
 }
 
 struct traceeval_stat *traceeval_stat(struct traceeval *teval,
-				      const union traceeval_data *keys,
+				      const struct traceeval_data *keys,
 				      struct traceeval_type *type)
 {
 	struct entry *entry;
@@ -923,8 +923,8 @@ unsigned long long traceeval_stat_count(struct traceeval_stat *stat)
  * Returns 0 on success, and -1 on error.
  */
 int traceeval_insert(struct traceeval *teval,
-		     const union traceeval_data *keys,
-		     const union traceeval_data *vals)
+		     const struct traceeval_data *keys,
+		     const struct traceeval_data *vals)
 {
 	struct entry *entry;
 	int check;
@@ -955,7 +955,7 @@ int traceeval_insert(struct traceeval *teval,
  *        -1 if there was an error.
  */
 int traceeval_remove(struct traceeval *teval,
-		     const union traceeval_data *keys)
+		     const struct traceeval_data *keys)
 {
 	struct hash_table *hist = teval->hist;
 	struct entry *entry;
@@ -1174,8 +1174,8 @@ static int iter_cmp(const void *A, const void *B, void *data)
 
 	for (int i = 0; i < iter->nr_sort; i++) {
 		struct traceeval_type *type;
-		union traceeval_data *dataA;
-		union traceeval_data *dataB;
+		struct traceeval_data *dataA;
+		struct traceeval_data *dataB;
 
 		type = iter->sort[i];
 
@@ -1293,7 +1293,7 @@ int traceeval_iterator_sort_custom(struct traceeval_iterator *iter,
  * Returns 1 if another entry is returned, or 0 if not (or negative on error)
  */
 int traceeval_iterator_next(struct traceeval_iterator *iter,
-			    const union traceeval_data **keys)
+			    const struct traceeval_data **keys)
 {
 	struct entry *entry;
 	int ret;
@@ -1333,7 +1333,7 @@ int traceeval_iterator_next(struct traceeval_iterator *iter,
  * Returns 1 if another entry is returned, or 0 if not (or negative on error)
  */
 int traceeval_iterator_query(struct traceeval_iterator *iter,
-			     const union traceeval_data **results)
+			     const struct traceeval_data **results)
 {
 	struct entry *entry;
 
-- 
2.40.1

Re: [PATCH 5/9] libtraceeval histogram: Add type to traceeval_data and make it a structure

[Ross Zwisler]

On Thu, Aug 17, 2023 at 06:24:18PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> Having a straight union for passing in the data that must match the types
> is dangerous and prone for buggy code. If some data doesn't match its
> type, there's nothing to catch it.
> 
> Instead of having a union traceeval_data of each type, have it be a
> structure with a "type" field follow by a union, where the type defines
> what kind of data it is.
> 
> Add helper macros to make it easy to define the data when using it.
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
>  include/traceeval-hist.h |  88 ++++++++++++++++---------
>  samples/task-eval.c      | 137 ++++++++++++++++-----------------------
>  src/eval-local.h         |   4 +-
>  src/histograms.c         |  68 +++++++++----------
>  4 files changed, 150 insertions(+), 147 deletions(-)
> 
<>
> @@ -802,13 +779,9 @@ static void display_process_stats(struct traceeval *teval,
>  {
>  	struct traceeval_stat *stat;
>  	unsigned long long delta;
> -	union traceeval_data keys[] = {
> -		{
> -			.cstring = comm,
> -		},
> -		{
> -			.number = RUNNING,
> -		}
> +	struct traceeval_data keys[] = {
> +		DEFINE_TRACEEVAL_CSTRING(	comm		),
> +		DEFINE_TRACEEVAL_NUMBER(	RUNNING		),
>  	};
>  
>  	for (int i = 0; i < OTHER; i++) {

Just after this in display_process_stats we have:

> 	for (int i = 0; i < OTHER; i++) {
> 		keys[1].number = i;

Which I think we want to set with the new macro TRACEEVAL_SET_NUMBER().

                TRACEEVAL_SET_NUMBER(keys[1], i)

Other than that you can add:

Reviewed-by: Ross Zwisler <zwisler@google.com>

[PATCH 6/9] libtraceveal: Add type checks to traceeval_data vals and keys

[Steven Rostedt]

From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

Now that the traceeval_data has a type, add checks to traceeval_insert()
and traceveal_query() as well as traceeval_stat() to make sure the keys
and vals match the types that were for the traceeval.

Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 src/histograms.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/src/histograms.c b/src/histograms.c
index 7c0eef6cf421..560046cc8d96 100644
--- a/src/histograms.c
+++ b/src/histograms.c
@@ -479,10 +479,16 @@ static int get_entry(struct traceeval *teval, const struct traceeval_data *keys,
 	struct hash_item *item;
 	unsigned key;
 	int check = 0;
+	int i;
 
 	if (!teval || !keys)
 		return -1;
 
+	for (i = 0; i < teval->nr_key_types; i++) {
+		if (keys[i].type != teval->key_types[i].type)
+			return -1;
+	}
+
 	key = make_hash(teval, keys, hist->bits);
 
 	for (iter = hash_iter_bucket(hist, key); (item = hash_iter_bucket_next(iter)); ) {
@@ -928,10 +934,16 @@ int traceeval_insert(struct traceeval *teval,
 {
 	struct entry *entry;
 	int check;
+	int i;
 
 	entry = NULL;
 	check = get_entry(teval, keys, &entry);
 
+	for (i = 0; i < teval->nr_val_types; i++) {
+		if (vals[i].type != teval->val_types[i].type)
+			return -1;
+	}
+
 	if (check == -1)
 		return check;
 
-- 
2.40.1

Re: [PATCH 6/9] libtraceveal: Add type checks to traceeval_data vals and keys

[Ross Zwisler]

On Thu, Aug 17, 2023 at 06:24:19PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> Now that the traceeval_data has a type, add checks to traceeval_insert()
> and traceveal_query() as well as traceeval_stat() to make sure the keys
> and vals match the types that were for the traceeval.
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
>  src/histograms.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/src/histograms.c b/src/histograms.c
> index 7c0eef6cf421..560046cc8d96 100644
> --- a/src/histograms.c
> +++ b/src/histograms.c
> @@ -479,10 +479,16 @@ static int get_entry(struct traceeval *teval, const struct traceeval_data *keys,
>  	struct hash_item *item;
>  	unsigned key;
>  	int check = 0;
> +	int i;
>  
>  	if (!teval || !keys)
>  		return -1;
>  
> +	for (i = 0; i < teval->nr_key_types; i++) {
> +		if (keys[i].type != teval->key_types[i].type)
> +			return -1;
> +	}
> +
>  	key = make_hash(teval, keys, hist->bits);
>  
>  	for (iter = hash_iter_bucket(hist, key); (item = hash_iter_bucket_next(iter)); ) {
> @@ -928,10 +934,16 @@ int traceeval_insert(struct traceeval *teval,
>  {
>  	struct entry *entry;
>  	int check;
> +	int i;
>  
>  	entry = NULL;
>  	check = get_entry(teval, keys, &entry);
>  
> +	for (i = 0; i < teval->nr_val_types; i++) {
> +		if (vals[i].type != teval->val_types[i].type)
> +			return -1;
> +	}

I think we should have a similar type check in the update path, so do all the
checks in a loop like this in update_entry(), or we can check each entry
individually in copy_traceeval_data().

That will make sure we don't update an entry with mismatched typed values.

> +
>  	if (check == -1)
>  		return check;
>  
> -- 
> 2.40.1
> 

[PATCH 7/9] libtraceeval: Add size checks to insert and query functions

[Steven Rostedt]

From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

Currently, there's nothing that checks the size of the keys/values being
passed into traceeval_query() and traceeval_insert(). Make functions that
take the size of those arrays:

   traceeval_query_size()
   traceeval_insert_size()

and convert the traceeval_query() and traceeval_insert() into macros that
calculate the size of the fields to pass in:

  #define traceeval_query(teval, keys, results) \
     traceeval_query_size(teval, sizeof(keys) / sizeof(keys[0]), results)

and this will allow the code to check to make sure the size matches what
is expected (note, that currently is not done, but can be added).

If the keys or vals is not an array, but instead a pointer, if one of the
macros is used, the compiler will complain with:

   warning: division sizeof (const struct traceeval_data *) / sizeof (const struct traceeval_data)
            does not compute the number of array elements [-Wsizeof-pointer-div]
  190 |                              sizeof(keys) / sizeof(keys[0]), results);

Then the user would need to call the "_size()" function directly
(as is done in the sample code, where the keys received by an iterator is
 used for a query)

Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 include/traceeval-hist.h | 18 +++++++++++++-----
 src/histograms.c         | 10 +++++-----
 2 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/include/traceeval-hist.h b/include/traceeval-hist.h
index 38d5dc878d3b..81fe805e691d 100644
--- a/include/traceeval-hist.h
+++ b/include/traceeval-hist.h
@@ -186,15 +186,23 @@ struct traceeval *traceeval_init(struct traceeval_type *keys,
 
 void traceeval_release(struct traceeval *teval);
 
-int traceeval_insert(struct traceeval *teval,
-		     const struct traceeval_data *keys,
-		     const struct traceeval_data *vals);
+int traceeval_insert_size(struct traceeval *teval,
+			  const struct traceeval_data *keys, size_t nr_keys,
+			  const struct traceeval_data *vals, size_t nr_vals);
+
+#define traceeval_insert(teval, keys, vals)				\
+	traceeval_insert_size(teval, keys, sizeof(keys) / sizeof(keys[0]), \
+			      vals, sizeof(vals) / sizeof(vals[0]))
 
 int traceeval_remove(struct traceeval *teval,
 		     const struct traceeval_data *keys);
 
-int traceeval_query(struct traceeval *teval, const struct traceeval_data *keys,
-		    const struct traceeval_data **results);
+int traceeval_query_size(struct traceeval *teval, const struct traceeval_data *keys,
+			 size_t nr_keys, const struct traceeval_data **results);
+
+#define traceeval_query(teval, keys, results)				\
+	traceeval_query_size(teval, keys,				\
+			     sizeof(keys) / sizeof(keys[0]), results);
 
 void traceeval_results_release(struct traceeval *teval,
 			       const struct traceeval_data *results);
diff --git a/src/histograms.c b/src/histograms.c
index 560046cc8d96..5e2e9200cbf6 100644
--- a/src/histograms.c
+++ b/src/histograms.c
@@ -675,8 +675,8 @@ fail:
  *
  * Returns 1 if found, 0 if not found, and -1 on error.
  */
-int traceeval_query(struct traceeval *teval, const struct traceeval_data *keys,
-		    const struct traceeval_data **results)
+int traceeval_query_size(struct traceeval *teval, const struct traceeval_data *keys,
+			 size_t nr_keys, const struct traceeval_data **results)
 {
 	struct entry *entry;
 	int check;
@@ -928,9 +928,9 @@ unsigned long long traceeval_stat_count(struct traceeval_stat *stat)
  *
  * Returns 0 on success, and -1 on error.
  */
-int traceeval_insert(struct traceeval *teval,
-		     const struct traceeval_data *keys,
-		     const struct traceeval_data *vals)
+int traceeval_insert_size(struct traceeval *teval,
+			  const struct traceeval_data *keys, size_t nr_keys,
+			  const struct traceeval_data *vals, size_t nr_vals)
 {
 	struct entry *entry;
 	int check;
-- 
2.40.1

Re: [PATCH 7/9] libtraceeval: Add size checks to insert and query functions

[Steven Rostedt]

On Thu, 17 Aug 2023 18:24:20 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> --- a/include/traceeval-hist.h
> +++ b/include/traceeval-hist.h
> @@ -186,15 +186,23 @@ struct traceeval *traceeval_init(struct traceeval_type *keys,
>  
>  void traceeval_release(struct traceeval *teval);
>  
> -int traceeval_insert(struct traceeval *teval,
> -		     const struct traceeval_data *keys,
> -		     const struct traceeval_data *vals);
> +int traceeval_insert_size(struct traceeval *teval,
> +			  const struct traceeval_data *keys, size_t nr_keys,
> +			  const struct traceeval_data *vals, size_t nr_vals);
> +
> +#define traceeval_insert(teval, keys, vals)				\
> +	traceeval_insert_size(teval, keys, sizeof(keys) / sizeof(keys[0]), \
> +			      vals, sizeof(vals) / sizeof(vals[0]))
>  
>  int traceeval_remove(struct traceeval *teval,
>  		     const struct traceeval_data *keys);
>  
> -int traceeval_query(struct traceeval *teval, const struct traceeval_data *keys,
> -		    const struct traceeval_data **results);
> +int traceeval_query_size(struct traceeval *teval, const struct traceeval_data *keys,
> +			 size_t nr_keys, const struct traceeval_data **results);
> +
> +#define traceeval_query(teval, keys, results)				\
> +	traceeval_query_size(teval, keys,				\
> +			     sizeof(keys) / sizeof(keys[0]), results);

That has an ';' at the end, and it causes failures for:

	if (traceeval_query(...) > 0)

(which I have in my trace-flames.c code).

I'll fix in v2.

-- Steve

[PATCH 8/9] libtraceeval: Add checks to traceeval_insert() and query()

[Steven Rostedt]

From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

Now that there are size functions for traceeval_insert() and
traceveal_query(), add checks to make sure that the size being passed in
is actually the size expected.

Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 src/histograms.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/histograms.c b/src/histograms.c
index 5e2e9200cbf6..973bf3ad279c 100644
--- a/src/histograms.c
+++ b/src/histograms.c
@@ -684,6 +684,9 @@ int traceeval_query_size(struct traceeval *teval, const struct traceeval_data *k
 	if (!teval || !keys || !results)
 		return -1;
 
+	if (nr_keys != teval->nr_key_types)
+		return -1;
+
 	/* find key and copy its corresponding value pair */
 	if ((check = get_entry(teval, keys, &entry)) < 1)
 		return check;
@@ -936,6 +939,9 @@ int traceeval_insert_size(struct traceeval *teval,
 	int check;
 	int i;
 
+	if (nr_keys != teval->nr_key_types || nr_vals != teval->nr_val_types)
+		return -1;
+
 	entry = NULL;
 	check = get_entry(teval, keys, &entry);
 
-- 
2.40.1

Re: [PATCH 8/9] libtraceeval: Add checks to traceeval_insert() and query()

[Ross Zwisler]

On Thu, Aug 17, 2023 at 06:24:21PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> Now that there are size functions for traceeval_insert() and
> traceveal_query(), add checks to make sure that the size being passed in
> is actually the size expected.

This looks fine, but I think it should be squashed with the previous commit so
we pass in the size and check it in the same patch.

Reviewed-by: Ross Zwisler <zwisler@google.com>

> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
>  src/histograms.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/src/histograms.c b/src/histograms.c
> index 5e2e9200cbf6..973bf3ad279c 100644
> --- a/src/histograms.c
> +++ b/src/histograms.c
> @@ -684,6 +684,9 @@ int traceeval_query_size(struct traceeval *teval, const struct traceeval_data *k
>  	if (!teval || !keys || !results)
>  		return -1;
>  
> +	if (nr_keys != teval->nr_key_types)
> +		return -1;
> +
>  	/* find key and copy its corresponding value pair */
>  	if ((check = get_entry(teval, keys, &entry)) < 1)
>  		return check;
> @@ -936,6 +939,9 @@ int traceeval_insert_size(struct traceeval *teval,
>  	int check;
>  	int i;
>  
> +	if (nr_keys != teval->nr_key_types || nr_vals != teval->nr_val_types)
> +		return -1;
> +
>  	entry = NULL;
>  	check = get_entry(teval, keys, &entry);
>  
> -- 
> 2.40.1
> 

[PATCH 9/9] libtraceeval: Only do stats on values marked with the STAT flag

[Steven Rostedt]

From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

Add TRACEEVAL_FL_STAT to perform stats on the value, otherwise ignore it.

Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 include/traceeval-hist.h | 1 +
 samples/task-eval.c      | 1 +
 src/histograms.c         | 2 +-
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/traceeval-hist.h b/include/traceeval-hist.h
index 81fe805e691d..34f01a51a334 100644
--- a/include/traceeval-hist.h
+++ b/include/traceeval-hist.h
@@ -36,6 +36,7 @@ enum traceeval_flags {
 	TRACEEVAL_FL_VALUE		= (1 << 1),
 	TRACEEVAL_FL_SIGNED		= (1 << 2),
 	TRACEEVAL_FL_TIMESTAMP		= (1 << 3),
+	TRACEEVAL_FL_STAT		= (1 << 3),
 };
 
 /*
diff --git a/samples/task-eval.c b/samples/task-eval.c
index ed3a1a95f097..0e3acb7644ef 100644
--- a/samples/task-eval.c
+++ b/samples/task-eval.c
@@ -147,6 +147,7 @@ static struct traceeval_type delta_vals[] = {
 	{
 		.type	= TRACEEVAL_TYPE_NUMBER_64,
 		.name	= "delta",
+		.flags = TRACEEVAL_FL_STAT,
 	},
 	{
 		.type	= TRACEEVAL_TYPE_NONE,
diff --git a/src/histograms.c b/src/histograms.c
index 973bf3ad279c..8f1cf4187713 100644
--- a/src/histograms.c
+++ b/src/histograms.c
@@ -571,7 +571,7 @@ static int copy_traceeval_data(struct traceeval_type *type,
 		return 0;
 	}
 
-	if (!stat)
+	if (!stat || !(type->flags & TRACEEVAL_FL_STAT))
 		return 0;
 
 	if (!stat->count++) {
-- 
2.40.1

Re: [PATCH 9/9] libtraceeval: Only do stats on values marked with the STAT flag

[Ross Zwisler]

On Thu, Aug 17, 2023 at 06:24:22PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> Add TRACEEVAL_FL_STAT to perform stats on the value, otherwise ignore it.
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
>  include/traceeval-hist.h | 1 +
>  samples/task-eval.c      | 1 +
>  src/histograms.c         | 2 +-
>  3 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/include/traceeval-hist.h b/include/traceeval-hist.h
> index 81fe805e691d..34f01a51a334 100644
> --- a/include/traceeval-hist.h
> +++ b/include/traceeval-hist.h
> @@ -36,6 +36,7 @@ enum traceeval_flags {
>  	TRACEEVAL_FL_VALUE		= (1 << 1),
>  	TRACEEVAL_FL_SIGNED		= (1 << 2),
>  	TRACEEVAL_FL_TIMESTAMP		= (1 << 3),
> +	TRACEEVAL_FL_STAT		= (1 << 3),

	TRACEEVAL_FL_STAT		= (1 << 4),

>  };
>  
>  /*
> diff --git a/samples/task-eval.c b/samples/task-eval.c
> index ed3a1a95f097..0e3acb7644ef 100644
> --- a/samples/task-eval.c
> +++ b/samples/task-eval.c
> @@ -147,6 +147,7 @@ static struct traceeval_type delta_vals[] = {
>  	{
>  		.type	= TRACEEVAL_TYPE_NUMBER_64,
>  		.name	= "delta",
> +		.flags = TRACEEVAL_FL_STAT,
>  	},
>  	{
>  		.type	= TRACEEVAL_TYPE_NONE,
> diff --git a/src/histograms.c b/src/histograms.c
> index 973bf3ad279c..8f1cf4187713 100644
> --- a/src/histograms.c
> +++ b/src/histograms.c
> @@ -571,7 +571,7 @@ static int copy_traceeval_data(struct traceeval_type *type,
>  		return 0;
>  	}
>  
> -	if (!stat)
> +	if (!stat || !(type->flags & TRACEEVAL_FL_STAT))

I think we actually want to check the TRACEEVAL_FL_STAT flag in is_stat_type()
and then use that check here.  This will help cover other cases where the type
means we can keep stats (values which are numbers) but we can bail if the flag
isn't set.

This will cover traceeval_stat() and traceeval_iterator_stat().

>  		return 0;
>  
>  	if (!stat->count++) {
> -- 
> 2.40.1
>