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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).