On Fri, Aug 04, 2023 at 09:50:58AM -0400, Steven Rostedt wrote:
> On Thu, 3 Aug 2023 18:53:59 -0400
> Stevie Alvarez <stevie.6strings@xxxxxxxxx> wrote:
>
> > From: "Stevie Alvarez (Google)" <stevie.6strings@xxxxxxxxx>
> >
> > Initial header file for libtraceeval's histogram API. The interface
> > provides a simple way of aggregating trace data and reading through said
> > data.
> >
> > Signed-off-by: Stevie Alvarez (Google) <stevie.6strings@xxxxxxxxx>
> > ---
> > include/traceeval-hist.h | 128 +++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 128 insertions(+)
> > create mode 100644 include/traceeval-hist.h
> >
> > diff --git a/include/traceeval-hist.h b/include/traceeval-hist.h
> > new file mode 100644
> > index 0000000..4664974
> > --- /dev/null
> > +++ b/include/traceeval-hist.h
> > @@ -0,0 +1,128 @@
> > +/* SPDX-License-Identifier: MIT */
> > +/*
> > + * libtraceeval histogram interface.
> > + *
> > + * Copyright (C) 2023 Google Inc, Steven Rostedt <rostedt@xxxxxxxxxxx>
> > + * Copyright (C) 2023 Google Inc, Stevie Alvarez <stevie.6strings@xxxxxxxxx>
> > + */
> > +#ifndef __LIBTRACEEVAL_HIST_H__
> > +#define __LIBTRACEEVAL_HIST_H__
> > +
> > +#include <stdlib.h>
> > +#include <stddef.h>
> > +#include <stdbool.h>
> > +
> > +/* Data definition interfaces */
> > +
> > +/* Field name/descriptor for number of hits */
> > +#define TRACEEVAL_VAL_HITS ((const char *)(-1UL))
> > +
> > +/* Data type distinguishers */
> > +enum traceeval_data_type {
> > + TRACEEVAL_TYPE_NONE,
> > + TRACEEVAL_TYPE_STRING,
> > + TRACEEVAL_TYPE_NUMBER,
> > + TRACEEVAL_TYPE_NUMBER_64,
> > + TRACEEVAL_TYPE_NUMBER_32,
> > + TRACEEVAL_TYPE_NUMBER_16,
> > + TRACEEVAL_TYPE_NUMBER_8,
>
> Hmm, I'm thinking it would be nice to rearrange this a bit to:
>
> enum traceeval_data_type {
> TRACEEVAL_TYPE_NONE,
> TRACEEVAL_TYPE_NUMBER_8,
> TRACEEVAL_TYPE_NUMBER_16,
> TRACEEVAL_TYPE_NUMBER_32,
> TRACEEVAL_TYPE_NUMBER_64,
> TRACEEVAL_TYPE_NUMBER,
> TRACEEVAL_TYPE_STRING,
>
> That way NUMBER_8 will be 1, NUMBER_16 is 2, NUMBER_32 is 3 and NUMBER_64
> is 4. If we ever wanted to do a trick, we can get the byte size from:
>
> 2^(1 * (enum - 1))
>
> 2^(1 * (NUMBER_8 - 1)) = 2^(1 * 0) = 1
> 2^(1 * (NUMBER_16 - 1)) = 2^(1 * 1) = 2
> 2^(1 * (NUMBER_32 - 1)) = 2^(1 * 2) = 4
> 2^(1 * (NUMBER_64 - 1)) = 2^(1 * 3) = 8
>
> I love hacks!
>
>
> > + TRACEEVAL_TYPE_DYNAMIC
> > +};
> > +
> > +/* Statistics specification flags */
> > +enum traceeval_flags {
> > + TRACEEVAL_FL_SIGNED = (1 << 0),
> > + TRACEEVAL_FL_TIMESTAMP = (1 << 1),
> > + TRACEEVAL_FL_STATS = (1 << 2)
>
> We may as well remove STATS until we decided to use it.
>
> Maybe even get rid of this enum until we decide to use it.
>
> Remember, once it's exposed, it is API.
>
> > +};
> > +
> > +/*
> > + * Trace data entry for a traceeval histogram
> > + * Constitutes keys and values.
> > + */
> > +union traceeval_data {
> > + char *string;
> > + struct traceeval_dynamic *dyn_data;
> > + unsigned long long number_64;
> > + unsigned long number;
> > + unsigned int number_32;
> > + unsigned short number_16;
> > + unsigned char number_8;
> > +};
> > +
> > +/*
> > + * struct traceeval_dynamic - Storage for dynamic traceeval_types
> > + * @size: The size of the dynamic type
> > + * @data: The pointer to the data of the dynamic type
> > + */
> > +struct traceeval_dynamic {
> > + void *data;
> > + size_t size;
> > +};
> > +
>
> As I replied to the cover letter, you need to add:
>
> struct traceeval_type;
>
> to get rid of the warning.
>
> > +/* struct traceeval_dynamic release function signature */
> > +typedef void (*traceeval_dyn_release_fn)(struct traceeval_dynamic *,
> > + struct traceeval_type *);
> > +
> > +/* struct traceeval_dynamic compare function signature */
> > +typedef int (*traceeval_dyn_cmp_fn)(struct traceeval_dynamic *,
> > + struct traceeval_dynamic *, struct traceeval_type *);
> > +
> > +/*
> > + * struct traceeval_type - Describes the type of a traceevent_data instance
> > + * @type: The enum type that describes the traceeval_data
> > + * @name: The string name of the traceeval_data
> > + * @flags: flags to describe the traceeval_data
> > + * @id: User specified identifier
> > + * @dyn_release: For dynamic types called on release (ignored for other types)
> > + * @dyn_cmp: A way to compare dynamic types (ignored for other types)
> > + *
> > + * The traceeval_type structure defines expectations for a corresponding
> > + * traceeval_data instance for a traceeval histogram instance. Used to
> > + * describe both keys and values.
> > + *
> > + * The @id field is an optional value in case the user has multiple struct
> > + * traceeval_type instances with @type fields set to TRACEEVAL_TYPE_DYNAMIC,
> > + * which each relate to distinct user defined struct traceeval_dynamic
> > + * 'sub-types'.
> > + *
> > + * For flexibility, @dyn_cmp() and @dyn_release() take a struct
> > + * traceeval_type instance. This allows the user to distinguish between
> > + * different sub-types of struct traceeval_dynamic within a single
> > + * callback function by examining the @id field. This is not a required
> > + * approach, merely one that is accommodated.
> > + *
> > + * @dyn_cmp() is used to compare two struct traceeval_dynamic instances when a
> > + * corresponding struct traceeval_type is reached with its type field set to
> > + * TRACEEVAL_TYPE_DYNAMIC. It should return 0 on equality, 1 if the first
> > + * argument is greater than the second, -1 for the other way around, and -2 on
> > + * error.
> > + *
> > + * dyn_release() is used during traceeval_release() to release a union
> > + * traceeval_data's struct traceeval_dynamic field when the corresponding
> > + * traceeval_type type is set to TRACEEVAL_TYPE_DYNAMIC.
> > + */
> > +struct traceeval_type {
> > + char *name;
> > + traceeval_dyn_release_fn dyn_release;
> > + traceeval_dyn_cmp_fn dyn_cmp;
> > + enum traceeval_data_type type;
> > + size_t flags;
> > + size_t id;
>
> Let's reorder this a little. Normally function pointers come at the end of
> a structure. That's more of a guideline than a rule, but let's have it here.
>
> struct traceeval_type {
> char *name;
> enum traceeval_data_type type;
> size_t flags;
> size_t id;
> traceeval_dyn_release_fn dyn_release;
> traceeval_dyn_cmp_fn dyn_cmp;
> };
>
> Especially since dynamic types are going to be rare, we don't want it in
> the hot cache.
Does the order of the fields in a struct definition not matter? I
thought word-boundaries applied to struct definitions? Or does the
compiler take care of this?
-- Stevie
>
> -- Steve
>
>
> > +};
> > +
> > +/* 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;
> > +};
> > +
> > +/* Iterator over aggregated data */
> > +struct traceeval_iterator;
> > +
> > +struct traceeval;
> > +
> > +#endif /* __LIBTRACEEVAL_HIST_H__ */
>
|