From: Steven Rostedt <rostedt@goodmis.org>
To: Ross Zwisler <zwisler@google.com>
Cc: linux-trace-devel@vger.kernel.org,
Stevie Alvarez <stevie.6strings@gmail.com>
Subject: Re: [PATCH 6/6] libtraceeval samples: Update task-eval to use the histogram logic
Date: Fri, 11 Aug 2023 01:30:12 -0400 [thread overview]
Message-ID: <20230811013012.2528a864@gandalf.local.home> (raw)
In-Reply-To: <20230810202838.GB1961992@google.com>
On Thu, 10 Aug 2023 14:28:38 -0600
Ross Zwisler <zwisler@google.com> wrote:
> On Tue, Aug 08, 2023 at 11:13:13PM -0400, Steven Rostedt wrote:
> > From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> >
> > Convert task-eval over to the new API.
> >
> > Note, it still requires some functions for displaying the output, but
> > those were added just as stubs to get an idea on how to use it.
> >
> > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> > ---
> > include/traceeval-hist.h | 1 +
> > samples/Makefile | 2 +-
> > samples/task-eval.c | 683 ++++++++++++++++++++++-----------------
> > 3 files changed, 388 insertions(+), 298 deletions(-)
> >
> > diff --git a/include/traceeval-hist.h b/include/traceeval-hist.h
> > index b3427c662d99..c363444f7fae 100644
> > --- a/include/traceeval-hist.h
> > +++ b/include/traceeval-hist.h
> <>
> > @@ -111,28 +192,45 @@ static void update_process(struct task_data *tdata, const char *comm,
> > enum sched_state state, enum command cmd,
> > unsigned long long ts)
>
> update_process(), update_cpu() and update_thread() are all pretty much the
> same, and I think you could combine them if you wanted. Just create a generic
> 'update' that takes keys & vals & a pair of 'struct traceval' pointers, one
> for start data and one for delta data. That also relies on the fact that the
> TS always comes first in 'vals'. Up to you if that's cleaner or not.
I did actually create a struct teval_pair because I was getting confused ;-)
But I do agree, some of this can be consolidated a bit more. Right now,
this is just a bare minimum switch from the old logic to the new so that I
can make sure they produce the same output.
>
> > {
> > - struct traceeval_key keys[] = {
> > + union traceeval_data keys[] = {
> > {
> > - .type = TRACEEVAL_TYPE_STRING,
> > - .string = comm,
> > + .cstring = comm,
> > },
> > {
> > - .type = TRACEEVAL_TYPE_NUMBER,
> > - .number = state,
> > + .number = state,
> > + },
> > + };
> > + union traceeval_data vals[] = {
> > + {
> > + .number_64 = ts,
> > + },
> > + {
> > + .number = (long)NULL, /* data */
> > }
> > };
> > + union traceeval_data *results;
> > + unsigned long long delta;
> > int ret;
> >
> > switch (cmd) {
> > case START:
> > - ret = traceeval_n_start(tdata->teval_processes, keys, ts);
> > + ret = traceeval_insert(tdata->teval_processes_start, keys, vals);
> > if (ret < 0)
> > pdie("Could not start process");
> > return;
> > case STOP:
> > - ret = traceeval_n_stop(tdata->teval_processes, keys, ts);
> > + ret = traceeval_query(tdata->teval_processes_start, keys, &results);
> > + if (ret < 0)
> > + return;
> > +
> > + delta = ts - results[0].number_64;
> > + results[0].number_64 = delta;
> > +
> > + ret = traceeval_insert(tdata->teval_processes, keys, results);
> > if (ret < 0)
> > pdie("Could not stop process");
> > +
> > + traceeval_results_release(tdata->teval_processes_start, results);
> > return;
> > }
> > }
> <>
> > @@ -283,32 +421,15 @@ static struct tep_format_field *get_field(struct tep_event *event, const char *n
> >
> > static void init_process_data(struct process_data *pdata)
> > {
> > - struct traceeval_key_info cpu_info[] = {
> > - {
> > - .type = TRACEEVAL_TYPE_NUMBER,
> > - .name = "CPU",
> > - },
> > - {
> > - .type = TRACEEVAL_TYPE_NUMBER,
> > - .name = "Schedule state",
> > - }
> > - };
> > - struct traceeval_key_info thread_info[] = {
> > - {
> > - .type = TRACEEVAL_TYPE_NUMBER,
> > - .name = "TID",
> > - },
> > - {
> > - .type = TRACEEVAL_TYPE_NUMBER,
> > - .name = "Schedule state",
> > - }
> > - };
> >
> > - pdata->teval_cpus = traceeval_2_alloc("CPUs", cpu_info);
> > + pdata->teval_cpus = traceeval_init(cpu_keys, timestamp_vals);
> > + if (!pdata->teval_cpus)
> > + pdie("Creating trace eval");
> > + pdata->teval_cpus = traceeval_init(cpu_keys, timestamp_vals);
> > if (!pdata->teval_cpus)
> > pdie("Creating trace eval");
>
> We're initializing pdata->teval_cpus twice - I'm guessing one of these wants to
> be pdata->teval_cpus_start?
Good catch. I also caught it when I started working back on it.
>
> >
> > - pdata->teval_threads = traceeval_2_alloc("Threads", thread_info);
> > + pdata->teval_threads = traceeval_init(thread_keys, timestamp_vals);
> > if (!pdata->teval_threads)
> > pdie("Creating trace eval");
>
> Missing init of pdata->teval_threads_start?
Yep.
new patches in a bit. (now I can go to bed!)
-- Steve
prev parent reply other threads:[~2023-08-11 5:30 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-09 3:13 [PATCH 0/6] libtraceeval histogram: Updates Steven Rostedt
2023-08-09 3:13 ` [PATCH 1/6] libtraceeval: Add sample task-eval program Steven Rostedt
2023-08-09 3:13 ` [PATCH 2/6] libtraceeval hist: Add pointer and const string types Steven Rostedt
2023-08-09 3:13 ` [PATCH 3/6] libtraceeval histogram: Have cmp and release functions be generic Steven Rostedt
2023-08-09 3:13 ` [PATCH 4/6] libtraceeval histograms: Add traceeval struct to compare function Steven Rostedt
2023-08-09 3:13 ` [PATCH 5/6] libtraceeval histogram: Fix the return value of traceeval_query() Steven Rostedt
2023-08-10 17:47 ` Ross Zwisler
2023-08-11 5:25 ` Steven Rostedt
2023-08-09 3:13 ` [PATCH 6/6] libtraceeval samples: Update task-eval to use the histogram logic Steven Rostedt
2023-08-09 3:19 ` Steven Rostedt
2023-08-10 20:28 ` Ross Zwisler
2023-08-11 5:30 ` Steven Rostedt [this message]
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=20230811013012.2528a864@gandalf.local.home \
--to=rostedt@goodmis.org \
--cc=linux-trace-devel@vger.kernel.org \
--cc=stevie.6strings@gmail.com \
--cc=zwisler@google.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 read-only IMAP folder(s) and NNTP newsgroup(s).