Re: [PATCH v2 1/5] histograms: Initial histograms interface

[Stevie Alvarez <stevie.6strings@gmail.com>]

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@gmail.com> wrote:
> 
> > From: "Stevie Alvarez (Google)" <stevie.6strings@gmail.com>
> > 
> > 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@gmail.com>
> > ---
> >  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@goodmis.org>
> > + * Copyright (C) 2023 Google Inc, Stevie Alvarez <stevie.6strings@gmail.com>
> > + */
> > +#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__ */
>