diff mbox series

[v2,09/17] libtraceeval histogram: Label and check keys and values

Message ID 20230811053940.1408424-10-rostedt@goodmis.org (mailing list archive)
State Superseded
Headers show
Series libtraceeval histogram: Updates | expand

Commit Message

Steven Rostedt Aug. 11, 2023, 5:39 a.m. UTC 
From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

When initializing the traceeval descriptor, mark each key and value as
their type via the flags (keys get TRACEEVAL_FL_KEY and values get
TRACEEVAL_FL_VALUE) as well as adding the index of that key/value type of
the key/value data it represents. This will be used by the iterators for
sorting. The iterator will point to the key/value type and use that
information to know which key/value data to sort with.

The keys and values passed in will also be updated to have their flags
match the proper key/value type as well as the index. This will be useful
for traceeval_stat() that will take the type as a parameter, and use it to
figure out fast where the data is that is to be looked up.

All pointer and dynamic types in keys must have both a cmp and hash
function defined, otherwise they cannot be indexed or hashed. Add a check
to make sure all key types of pointer and dynamic have those functions.

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

Comments

Ross Zwisler Aug. 15, 2023, 7:48 p.m. UTC | #1 
On Fri, Aug 11, 2023 at 01:39:32AM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> When initializing the traceeval descriptor, mark each key and value as
> their type via the flags (keys get TRACEEVAL_FL_KEY and values get
> TRACEEVAL_FL_VALUE) as well as adding the index of that key/value type of
> the key/value data it represents. This will be used by the iterators for
> sorting. The iterator will point to the key/value type and use that
> information to know which key/value data to sort with.
> 
> The keys and values passed in will also be updated to have their flags
> match the proper key/value type as well as the index. This will be useful
> for traceeval_stat() that will take the type as a parameter, and use it to
> figure out fast where the data is that is to be looked up.
> 
> All pointer and dynamic types in keys must have both a cmp and hash
> function defined, otherwise they cannot be indexed or hashed. Add a check
> to make sure all key types of pointer and dynamic have those functions.
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
>  include/traceeval-hist.h | 11 +++++---
>  src/histograms.c         | 60 ++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 64 insertions(+), 7 deletions(-)
> 
> diff --git a/include/traceeval-hist.h b/include/traceeval-hist.h
> index 4baed4237787..1c02f3039809 100644
> --- a/include/traceeval-hist.h
> +++ b/include/traceeval-hist.h
<>
> @@ -248,6 +292,16 @@ struct traceeval *traceeval_init(const struct traceeval_type *keys,
>  		goto fail;
>  	}
>  
> +	ret = check_keys(keys);
> +	if (ret < 0)
> +		goto fail;
> +
> +	if (vals) {
> +		ret = check_vals(vals);
> +		if (ret < 0)
> +			goto fail;

These two goto statements should both be to fail_release so we clean up
'teval', else we could do this check above the alloc.

> +	}
> +
>  	/* alloc key types */
>  	teval->nr_key_types = type_alloc(keys, &teval->key_types);
>  	if (teval->nr_key_types <= 0) {
> -- 
> 2.40.1
>
Steven Rostedt Aug. 15, 2023, 8:24 p.m. UTC | #2 
On Tue, 15 Aug 2023 13:48:09 -0600
Ross Zwisler <zwisler@google.com> wrote:

> On Fri, Aug 11, 2023 at 01:39:32AM -0400, Steven Rostedt wrote:
> > From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> > 
> > When initializing the traceeval descriptor, mark each key and value as
> > their type via the flags (keys get TRACEEVAL_FL_KEY and values get
> > TRACEEVAL_FL_VALUE) as well as adding the index of that key/value type of
> > the key/value data it represents. This will be used by the iterators for
> > sorting. The iterator will point to the key/value type and use that
> > information to know which key/value data to sort with.
> > 
> > The keys and values passed in will also be updated to have their flags
> > match the proper key/value type as well as the index. This will be useful
> > for traceeval_stat() that will take the type as a parameter, and use it to
> > figure out fast where the data is that is to be looked up.
> > 
> > All pointer and dynamic types in keys must have both a cmp and hash
> > function defined, otherwise they cannot be indexed or hashed. Add a check
> > to make sure all key types of pointer and dynamic have those functions.
> > 
> > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> > ---
> >  include/traceeval-hist.h | 11 +++++---
> >  src/histograms.c         | 60 ++++++++++++++++++++++++++++++++++++++--
> >  2 files changed, 64 insertions(+), 7 deletions(-)
> > 
> > diff --git a/include/traceeval-hist.h b/include/traceeval-hist.h
> > index 4baed4237787..1c02f3039809 100644
> > --- a/include/traceeval-hist.h
> > +++ b/include/traceeval-hist.h  
> <>
> > @@ -248,6 +292,16 @@ struct traceeval *traceeval_init(const struct traceeval_type *keys,
> >  		goto fail;
> >  	}
> >  
> > +	ret = check_keys(keys);
> > +	if (ret < 0)
> > +		goto fail;
> > +
> > +	if (vals) {
> > +		ret = check_vals(vals);
> > +		if (ret < 0)
> > +			goto fail;  
> 
> These two goto statements should both be to fail_release so we clean up
> 'teval', else we could do this check above the alloc.

Nice catch. Will fix.

-- Steve

> 
> > +	}
> > +
> >  	/* alloc key types */
> >  	teval->nr_key_types = type_alloc(keys, &teval->key_types);
> >  	if (teval->nr_key_types <= 0) {
> > -- 
> > 2.40.1
> >
diff mbox series

Patch

diff --git a/include/traceeval-hist.h b/include/traceeval-hist.h
index 4baed4237787..1c02f3039809 100644
--- a/include/traceeval-hist.h
+++ b/include/traceeval-hist.h
@@ -32,8 +32,10 @@  enum traceeval_data_type {
 
 /* Statistics specification flags */
 enum traceeval_flags {
-	TRACEEVAL_FL_SIGNED		= (1 << 0),
-	TRACEEVAL_FL_TIMESTAMP		= (1 << 1),
+	TRACEEVAL_FL_KEY		= (1 << 0),
+	TRACEEVAL_FL_VALUE		= (1 << 1),
+	TRACEEVAL_FL_SIGNED		= (1 << 2),
+	TRACEEVAL_FL_TIMESTAMP		= (1 << 3),
 };
 
 /*
@@ -120,6 +122,7 @@  struct traceeval_type {
 	char				*name;
 	enum traceeval_data_type	type;
 	size_t				flags;
+	size_t				index;
 	size_t				id;
 	traceeval_data_release_fn	release;
 	traceeval_data_cmp_fn		cmp;
@@ -142,8 +145,8 @@  struct traceeval;
 
 /* Histogram interfaces */
 
-struct traceeval *traceeval_init(const struct traceeval_type *keys,
-				 const struct traceeval_type *vals);
+struct traceeval *traceeval_init(struct traceeval_type *keys,
+				 struct traceeval_type *vals);
 
 void traceeval_release(struct traceeval *teval);
 
diff --git a/src/histograms.c b/src/histograms.c
index 574bb115ffc0..a1a2daaa2e9b 100644
--- a/src/histograms.c
+++ b/src/histograms.c
@@ -203,6 +203,44 @@  fail:
 	return -1;
 }
 
+static int check_keys(struct traceeval_type *keys)
+{
+	for (int i = 0; keys[i].type != TRACEEVAL_TYPE_NONE; i++) {
+		/* Define this as a key */
+		keys[i].flags |= TRACEEVAL_FL_KEY;
+		keys[i].flags &= ~TRACEEVAL_FL_VALUE;
+
+		keys[i].index = i;
+
+		switch (keys[i].type) {
+		case TRACEEVAL_TYPE_POINTER:
+		case TRACEEVAL_TYPE_DYNAMIC:
+			/*
+			 * Key pointers and dynamic types must have a
+			 * cmp and hash function
+			 */
+			if (!keys[i].cmp || !keys[i].hash)
+				return -1;
+			break;
+		default:
+			break;
+		}
+	}
+	return 0;
+}
+
+static int check_vals(struct traceeval_type *vals)
+{
+	for (int i = 0; vals[i].type != TRACEEVAL_TYPE_NONE; i++) {
+		/* Define this as a value */
+		vals[i].flags |= TRACEEVAL_FL_VALUE;
+		vals[i].flags &= ~TRACEEVAL_FL_KEY;
+
+		vals[i].index = i;
+	}
+	return 0;
+}
+
 /*
  * traceeval_init - create a traceeval descriptor
  * @keys: Defines the keys to differentiate traceeval entries
@@ -213,7 +251,12 @@  fail:
  * the "histogram". Note, both the @keys and @vals array must end with:
  * { .type = TRACEEVAL_TYPE_NONE }.
  *
- * The @keys and @vals passed in are copied for internal use.
+ * The @keys and @vals passed in are copied for internal use, but they are
+ * still modified to add the flags to denote their type (key or value) as
+ * well as the index into the keys or vals array respectively. This is
+ * to help speed up other operations that may need to know the index of
+ * the given type, and remove the burden from the user to make sure they
+ * are added.
  *
  * For any member of @keys or @vals that isn't of type TRACEEVAL_TYPE_NONE,
  * the name field must be a null-terminated string. Members of type
@@ -227,11 +270,12 @@  fail:
  *
  * Returns the descriptor on success, or NULL on error.
  */
-struct traceeval *traceeval_init(const struct traceeval_type *keys,
-				 const struct traceeval_type *vals)
+struct traceeval *traceeval_init(struct traceeval_type *keys,
+				 struct traceeval_type *vals)
 {
 	struct traceeval *teval;
 	char *err_msg;
+	int ret;
 
 	if (!keys)
 		return NULL;
@@ -248,6 +292,16 @@  struct traceeval *traceeval_init(const struct traceeval_type *keys,
 		goto fail;
 	}
 
+	ret = check_keys(keys);
+	if (ret < 0)
+		goto fail;
+
+	if (vals) {
+		ret = check_vals(vals);
+		if (ret < 0)
+			goto fail;
+	}
+
 	/* alloc key types */
 	teval->nr_key_types = type_alloc(keys, &teval->key_types);
 	if (teval->nr_key_types <= 0) {