On Mon, Oct 02, 2023 at 01:53:04PM -0600, Ross Zwisler wrote:
> On Wed, Sep 27, 2023 at 08:33:09AM -0400, Steven Rostedt wrote:
> > From: "Steven Rostedt (Google)" <rostedt@xxxxxxxxxxx>
> >
> > 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.
> >
> > Reviewed-by: Ross Zwisler <zwisler@xxxxxxxxxx>
> > Signed-off-by: Steven Rostedt (Google) <rostedt@xxxxxxxxxxx>
> > ---
> > include/traceeval-hist.h | 90 ++++++++++++++++---------
> > samples/task-eval.c | 139 ++++++++++++++++-----------------------
> > src/eval-local.h | 4 +-
> > src/histograms.c | 70 ++++++++++----------
> > 4 files changed, 153 insertions(+), 150 deletions(-)
> >
> > diff --git a/include/traceeval-hist.h b/include/traceeval-hist.h
> > index e619d52ea0d0..78cfac5982ee 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,11 +215,11 @@ 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);
> > void traceeval_iterator_results_release(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..e62bfddafdd3 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,17 +779,13 @@ 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++) {
> > - keys[1].number = i;
> > + TRACEEVAL_SET_NUMBER_64(keys[1], i);
>
> I think this should be
> + TRACEEVAL_SET_NUMBER(keys[1], i);
>
> to match the
> + DEFINE_TRACEEVAL_NUMBER( RUNNING ),
>
> a little up in this function.
Ah, it looks like you fix this in the next patch.
|