From: Daniel Bristot de Oliveira <bristot@kernel.org>
To: Andreas Ziegler <br015@umbiko.net>
Cc: linux-trace-devel@vger.kernel.org, linux-kernel@vger.kernel.org,
Steven Rostedt <rostedt@goodmis.org>
Subject: Re: rtla osnoise hist: average duration is always zero
Date: Wed, 28 Dec 2022 16:25:08 +0100 [thread overview]
Message-ID: <f47e877c-c95f-e3e6-b96f-89b0ca582878@kernel.org> (raw)
In-Reply-To: <d7bb31547e9bbf6684801a7bbd857810@umbiko.net>
Hi Andreas,
On 12/24/22 13:39, Andreas Ziegler wrote:
> -- Observed in, but not limited to, Linux 6.1.1
Since original commit... The best way to report this is adding
a Fixes: tag. For example:
Fixes: 829a6c0b5698 ("rtla/osnoise: Add the hist mode")
> rtla osnoise hist always outputs '0' as average duration value. Example:
>
> # rtla osnoise hist -P F:1 -c 0-1 -r 900000 -d 1M -b 1 -E 5000 -T 1
> # RTLA osnoise histogram
> # Time unit is microseconds (us)
> # Duration: 0 00:01:00
> ...
> count: 5629 1364
> min: 1 1
> avg: 0 0
> max: 2955 56
>
> This is due to sum_sample in osnoise_hist_update_multiple() being calculated as the sum (duration), not as sum (duration * count).
Yeah, that is correct. It works on timerlat hist because timerlat hist collects
each trace event. osnoise hist uses in-kernel histograms, so we need to multiply
the value with the count. This is a leftover from the development phase, as I started
using tracing and then moved to histograms (which is better).
> Rounding, instead of truncating, of the average value would be cool.
I thought: the values were already rounded up, so it might be rounding too much.
But we are in user space. It is just easier to add a two digits precision
to the value, no?
> The following patch would solve the issue described above:
>
>
> Sampled duration must be weighted by observed quantity, to arrive at a
> correct average duration value.
>
> Fix calculation of total duration by summing (duration * count).
> Introduce rounding for calculation of final value.
>
> Signed-off-by: Andreas Ziegler <br015@umbiko.net>
>
> --- a/tools/tracing/rtla/src/osnoise_hist.c
> +++ b/tools/tracing/rtla/src/osnoise_hist.c
> @@ -121,6 +121,7 @@
> {
> struct osnoise_hist_params *params = tool->params;
> struct osnoise_hist_data *data = tool->data;
> + unsigned long long total_duration;
> int entries = data->entries;
> int bucket;
> int *hist;
> @@ -131,10 +132,12 @@
> if (data->bucket_size)
> bucket = duration / data->bucket_size;
>
> + total_duration = duration * count;
> +
> hist = data->hist[cpu].samples;
> data->hist[cpu].count += count;
> update_min(&data->hist[cpu].min_sample, &duration);
> - update_sum(&data->hist[cpu].sum_sample, &duration);
> + update_sum(&data->hist[cpu].sum_sample, &total_duration);
How about re-seding a patch with the code above, adding the:
Fixes: 829a6c0b5698 ("rtla/osnoise: Add the hist mode")
and...
> update_max(&data->hist[cpu].max_sample, &duration);
>
> if (bucket < entries)
> @@ -333,7 +336,7 @@
>
> if (data->hist[cpu].count)
> trace_seq_printf(trace->seq, "%9llu ",
> - data->hist[cpu].sum_sample / data->hist[cpu].count);
> + (data->hist[cpu].sum_sample + data->hist[cpu].count / 2) / data->hist[cpu].count);
another patch with this part, adding two digits precision?
> else
> trace_seq_printf(trace->seq, " - ");
> }
>
Thanks!
-- Daniel
> Kind regards,
> Andreas
next prev parent reply other threads:[~2022-12-28 15:25 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-24 12:39 rtla osnoise hist: average duration is always zero Andreas Ziegler
2022-12-24 21:17 ` Slade Watkins
2022-12-26 11:50 ` Andreas Ziegler
2022-12-28 15:25 ` Daniel Bristot de Oliveira [this message]
2023-01-03 10:33 ` [PATCH 0/2 v2] rtla osnoise hist average calculation Andreas Ziegler
2023-01-03 10:33 ` [PATCH 1/2 v2] tools/tracing/rtla: osnoise_hist: use total duration for " Andreas Ziegler
2023-01-12 14:32 ` Daniel Bristot de Oliveira
2023-01-03 10:34 ` [PATCH 2/2 v2] tools/tracing/rtla: osnoise_hist: display average with two-digit precision Andreas Ziegler
2023-01-12 14:33 ` Daniel Bristot de Oliveira
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=f47e877c-c95f-e3e6-b96f-89b0ca582878@kernel.org \
--to=bristot@kernel.org \
--cc=br015@umbiko.net \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-trace-devel@vger.kernel.org \
--cc=rostedt@goodmis.org \
/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 NNTP newsgroup(s).