On Wed, 11 Oct 2023 16:21:00 -0600
Ross Zwisler <zwisler@xxxxxxxxxx> wrote:
> On Thu, Oct 05, 2023 at 06:14:48PM -0400, Steven Rostedt wrote:
> > From: "Steven Rostedt (Google)" <rostedt@xxxxxxxxxxx>
> >
> > 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?
|