On Thu, 10 Aug 2023 14:28:38 -0600
Ross Zwisler <zwisler@xxxxxxxxxx> wrote:
> On Tue, Aug 08, 2023 at 11:13:13PM -0400, Steven Rostedt wrote:
> > From: "Steven Rostedt (Google)" <rostedt@xxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxx>
> > ---
> > 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
|