From: Ross Zwisler <zwisler@google.com>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: linux-trace-devel@vger.kernel.org,
Stevie Alvarez <stevie.6strings@gmail.com>
Subject: Re: [PATCH v2 06/11] libtraceeval histogram: Add type to traceeval_data and make it a structure
Date: Mon, 2 Oct 2023 13:53:04 -0600 [thread overview]
Message-ID: <20231002195304.GC1532181@google.com> (raw)
In-Reply-To: <20230927123314.989589-7-rostedt@goodmis.org>
On Wed, Sep 27, 2023 at 08:33:09AM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
>
> 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@google.com>
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
> 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.
next prev parent reply other threads:[~2023-10-02 19:53 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-27 12:33 [PATCH v2 00/11] libtraceeval: Even more updates! Steven Rostedt
2023-09-27 12:33 ` [PATCH v2 01/11] libtraceeval: Add check for updates to know to recreate iter array Steven Rostedt
2023-09-27 12:33 ` [PATCH v2 02/11] libtraceeval: Add traceeval_iterator_query() Steven Rostedt
2023-09-27 12:33 ` [PATCH v2 03/11] libtraceeval: Add traceeval_iterator_results_release() Steven Rostedt
2023-10-02 19:23 ` Ross Zwisler
2023-09-27 12:33 ` [PATCH v2 04/11] libtraceeval: Add traceeval_iterator_stat() Steven Rostedt
2023-09-27 12:33 ` [PATCH v2 05/11] libtraceeval: Add traceeval_iterator_remove() Steven Rostedt
2023-10-02 19:45 ` Ross Zwisler
2023-09-27 12:33 ` [PATCH v2 06/11] libtraceeval histogram: Add type to traceeval_data and make it a structure Steven Rostedt
2023-10-02 19:53 ` Ross Zwisler [this message]
2023-10-02 19:54 ` Ross Zwisler
2023-10-03 13:22 ` Steven Rostedt
2023-10-02 20:16 ` Steven Rostedt
2023-09-27 12:33 ` [PATCH v2 07/11] libtraceveal: Add type checks to traceeval_data vals and keys Steven Rostedt
2023-10-02 19:59 ` Ross Zwisler
2023-09-27 12:33 ` [PATCH v2 08/11] libtraceeval: Add size checks to insert and query functions Steven Rostedt
2023-09-27 12:33 ` [PATCH v2 09/11] libtraceeval: Make traceeval_remove() check size of keys array Steven Rostedt
2023-10-02 20:03 ` Ross Zwisler
2023-09-27 12:33 ` [PATCH v2 10/11] libtraceeval: Make traceeval_stat() " Steven Rostedt
2023-10-02 20:07 ` Ross Zwisler
2023-09-27 12:33 ` [PATCH v2 11/11] libtraceeval: Only do stats on values marked with the STAT flag Steven Rostedt
2023-10-02 20:15 ` Ross Zwisler
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20231002195304.GC1532181@google.com \
--to=zwisler@google.com \
--cc=linux-trace-devel@vger.kernel.org \
--cc=rostedt@goodmis.org \
--cc=stevie.6strings@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.