| Message ID | 20230803225413.40697-2-stevie.6strings@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | histograms: bug fixes and convention compliance | expand |
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. -- 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__ */
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__ */ >
On Fri, 4 Aug 2023 13:41:59 -0400 Stevie Alvarez <stevie.6strings@gmail.com> wrote: > > > +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? They do matter. Word bounders are important, but the compiler will just make "holes" if needed. For example, let's say on 64 bit, everything above is 64 bits but the type. I would have created a "hole". But because the type is more important than the id, I kept it at the top. struct traceeval_type { char *name; // offset 0 enum traceeval_data_type type; // offset 8 [ compiler adds 4 byte "hole" or "padding" ] size_t flags; // offset 16 size_t id; // offset 24 traceeval_dyn_release_fn dyn_release; // offset 32 traceeval_dyn_cmp_fn dyn_cmp; // offset 40 }; If a cache line is 32 bytes (most is usually 128, but let's say on an older architecture) I don't care if the the dyn_release and dyn_cmp are in the same cache line as name. -- Steve
On Fri, Aug 04, 2023 at 01:57:18PM -0400, Steven Rostedt wrote: > On Fri, 4 Aug 2023 13:41:59 -0400 > Stevie Alvarez <stevie.6strings@gmail.com> wrote: > > > > > +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? > > They do matter. Word bounders are important, but the compiler will just > make "holes" if needed. For example, let's say on 64 bit, everything above > is 64 bits but the type. I would have created a "hole". But because the > type is more important than the id, I kept it at the top. > > struct traceeval_type { > char *name; // offset 0 > enum traceeval_data_type type; // offset 8 > > [ compiler adds 4 byte "hole" or "padding" ] > > size_t flags; // offset 16 > size_t id; // offset 24 > traceeval_dyn_release_fn dyn_release; // offset 32 > traceeval_dyn_cmp_fn dyn_cmp; // offset 40 > }; > > If a cache line is 32 bytes (most is usually 128, but let's say on an older > architecture) I don't care if the the dyn_release and dyn_cmp are in the > same cache line as name. > > -- Steve This makes sense, I appreciate the explination! I was treating size_t as an int in my head by accident.... oops! -- Stevie
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, + TRACEEVAL_TYPE_DYNAMIC +}; + +/* Statistics specification flags */ +enum traceeval_flags { + TRACEEVAL_FL_SIGNED = (1 << 0), + TRACEEVAL_FL_TIMESTAMP = (1 << 1), + TRACEEVAL_FL_STATS = (1 << 2) +}; + +/* + * 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; +}; + +/* 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; +}; + +/* 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__ */