Re: [PATCH 6/6] libtraceeval samples: Update task-eval to use the histogram logic

[Ross Zwisler <zwisler@google.com>]

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.

>  {
> -	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?

>  
> -	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?

>  }