Re: [PATCH] libtraceeval: Add traceeval_stat_max/min_timestamp() API

[Steven Rostedt <rostedt@goodmis.org>]

On Wed, 11 Oct 2023 16:21:00 -0600
Ross Zwisler <zwisler@google.com> wrote:

> On Thu, Oct 05, 2023 at 06:14:48PM -0400, Steven Rostedt wrote:
> > From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> > 
> > If a value field is flagged as a TIMESTAMP, and another field is flagged
> > as a STAT, have the statistics for the STAT field automatically keep track
> > of when the max and min happened (what the TIMESTAMP field was at those
> > instances).  
> 
> I'm confused as to why we have TRACEEVAL_FL_TIMESTAMP and a separate data
> entry with that flag, instead of only storing the timestamp in the entry
> metadata, as we do with the other STATs?

Actually, I may remove this part entirely, as it's pretty much superseded
by the TRACEEVAL_TYPE_DELTA, which I moved to, and haven't needed the
separate val marked for the timestamp.

> 
> >From the description above where we have one TIMESTAMP and one STAT, I would  
> expect to see structures defined like this
> (from [PATCH v2] libtraceeval: Add wake-lat sample code):
> 
> +struct traceeval_type sched_vals[] = {
> +       {
> +               .name           = "timestamp",
> +               .flags          = TRACEEVAL_FL_TIMESTAMP,
> +               .type           = TRACEEVAL_TYPE_NUMBER_64,
> +       },
> +       {
> +               .name           = "delta",
> +               .flags          = TRACEEVAL_FL_STAT,
> +               .type           = TRACEEVAL_TYPE_NUMBER_64,
> +       }
> 
> where the timestamp is sync'd with the STAT min and max, right?

The new type is done like:

	{
		.name		= "delta",
		.type		= TRACEEVAL_TYPE_DELTA,
	}

Which will automatically be marked as STAT flag, and is set with:

	TRACEEVAL_SET_DELTA(vals, delta, timestamp);

Is that what you are thinking about?

-- Steve



> 
> But we also have this:
> 
> +struct traceeval_type wakeup_vals[] = {
> +       {
> +               .name           = "timestamp",
> +               .flags          = TRACEEVAL_FL_TIMESTAMP,
> +               .type           = TRACEEVAL_TYPE_NUMBER_64,
> +       }
> +};
> 
> in a structure that doesn't have a corresponding STAT field?  This is also
> confusing to me because we need to keep 2 values (min and max TS), but this
> only holds one?
> 
> The timestamp is already stored independently in the stat metadata in
> struct traceeval_stat:
> 
> >  struct traceeval_stat {
> >  	unsigned long long	max;
> > +	unsigned long long	max_ts;
> >  	unsigned long long	min;
> > +	unsigned long long	min_ts;
> >  	unsigned long long	total;
> >  	unsigned long long	std;
> >  	size_t			count;  
> 
> which I think is enough?
> 
> It seems like instead we really want our two flags to be:
> 
> TRACEEVAL_FL_STAT
> and
> TRACEEVAL_FL_STAT_TS
> 
> where the first just keeps the normal stats (min, max, average, std. dev, etc)
> and the second does all that plus timestamps for min and max?
> 
> This would also allow you to keep min and max stats independently for multiple
> entries in a single structure, i.e.:
> 
> struct traceeval_type task_vals[] = {
>        {
>                .name           = "wake_delay",
>                .flags          = TRACEEVAL_FL_STAT_TS,
>                .type           = TRACEEVAL_TYPE_NUMBER_64,
>        },
>        {
>                .name           = "runtime",
>                .flags          = TRACEEVAL_FL_STAT_TS,
>                .type           = TRACEEVAL_TYPE_NUMBER_64,
>        }
> };
> 
> which I don't think is possible under the current scheme?
> 
> What am I missing?