* rtla osnoise hist: average duration is always zero
@ 2022-12-24 12:39 Andreas Ziegler
2022-12-24 21:17 ` Slade Watkins
2022-12-28 15:25 ` Daniel Bristot de Oliveira
0 siblings, 2 replies; 9+ messages in thread
From: Andreas Ziegler @ 2022-12-24 12:39 UTC (permalink / raw)
To: Daniel Bristot de Oliveira, Steven Rostedt
Cc: linux-trace-devel, linux-kernel
-- Observed in, but not limited to, Linux 6.1.1
Dear all,
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).
Rounding, instead of truncating, of the average value would be cool.
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);
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);
else
trace_seq_printf(trace->seq, " - ");
}
Kind regards,
Andreas
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: rtla osnoise hist: average duration is always zero
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
1 sibling, 1 reply; 9+ messages in thread
From: Slade Watkins @ 2022-12-24 21:17 UTC (permalink / raw)
To: Andreas Ziegler
Cc: Daniel Bristot de Oliveira, Steven Rostedt, linux-trace-devel,
linux-kernel, stable
On Sat, Dec 24, 2022 at 7:48 AM Andreas Ziegler <br015@umbiko.net> wrote:
>
> -- Observed in, but not limited to, Linux 6.1.1
Wait, "but not limited to"? What does that mean? Are there more
versions affected?
-- Slade
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: rtla osnoise hist: average duration is always zero
2022-12-24 21:17 ` Slade Watkins
@ 2022-12-26 11:50 ` Andreas Ziegler
0 siblings, 0 replies; 9+ messages in thread
From: Andreas Ziegler @ 2022-12-26 11:50 UTC (permalink / raw)
To: Slade Watkins
Cc: Daniel Bristot de Oliveira, Steven Rostedt, linux-trace-devel,
linux-kernel, stable
On 2022-12-24 21:17, Slade Watkins wrote:
> On Sat, Dec 24, 2022 at 7:48 AM Andreas Ziegler <br015@umbiko.net>
> wrote:
>>
>> -- Observed in, but not limited to, Linux 6.1.1
>
> Wait, "but not limited to"? What does that mean? Are there more
> versions affected?
This was meant to indicate that the bug is not a regression; it can be
found in every release since introduction in 5.17. Currently affected
are 6.1.y and 6.0.y kernel trees.
Regards,
Andreas
> -- Slade
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: rtla osnoise hist: average duration is always zero
2022-12-24 12:39 rtla osnoise hist: average duration is always zero Andreas Ziegler
2022-12-24 21:17 ` Slade Watkins
@ 2022-12-28 15:25 ` Daniel Bristot de Oliveira
2023-01-03 10:33 ` [PATCH 0/2 v2] rtla osnoise hist average calculation Andreas Ziegler
` (2 more replies)
1 sibling, 3 replies; 9+ messages in thread
From: Daniel Bristot de Oliveira @ 2022-12-28 15:25 UTC (permalink / raw)
To: Andreas Ziegler; +Cc: linux-trace-devel, linux-kernel, Steven Rostedt
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 0/2 v2] rtla osnoise hist average calculation
2022-12-28 15:25 ` Daniel Bristot de Oliveira
@ 2023-01-03 10:33 ` Andreas Ziegler
2023-01-03 10:33 ` [PATCH 1/2 v2] tools/tracing/rtla: osnoise_hist: use total duration for " Andreas Ziegler
2023-01-03 10:34 ` [PATCH 2/2 v2] tools/tracing/rtla: osnoise_hist: display average with two-digit precision Andreas Ziegler
2 siblings, 0 replies; 9+ messages in thread
From: Andreas Ziegler @ 2023-01-03 10:33 UTC (permalink / raw)
To: Daniel Bristot de Oliveira, Steven Rostedt
Cc: linux-trace-devel, linux-kernel, Andreas Ziegler
Version 2 of the proposed patch, with changes split in two separate commits,
as suggested by Daniel Bristot de Oliveira
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).
Truncating of the average value in final output suggests too optimistic
results; display floating point value instead.
Andreas Ziegler (2):
tools/tracing/rtla: osnoise_hist: use total duration for average
calculation
tools/tracing/rtla: osnoise_hist: display average with two-digit
precision
tools/tracing/rtla/src/osnoise_hist.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2 v2] tools/tracing/rtla: osnoise_hist: use total duration for average calculation
2022-12-28 15:25 ` Daniel Bristot de Oliveira
2023-01-03 10:33 ` [PATCH 0/2 v2] rtla osnoise hist average calculation Andreas Ziegler
@ 2023-01-03 10:33 ` 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
2 siblings, 1 reply; 9+ messages in thread
From: Andreas Ziegler @ 2023-01-03 10:33 UTC (permalink / raw)
To: Daniel Bristot de Oliveira, Steven Rostedt
Cc: linux-trace-devel, linux-kernel, Andreas Ziegler
Sampled durations must be weighted by observed quantity, to arrive at a correct
average duration value.
Perform calculation of total duration by summing (duration * count).
Fixes: 829a6c0b5698 ("rtla/osnoise: Add the hist mode")
Signed-off-by: Andreas Ziegler <br015@umbiko.net>
---
Changes v1 -> v2:
- add 'Fixes' line (Daniel)
tools/tracing/rtla/src/osnoise_hist.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/tools/tracing/rtla/src/osnoise_hist.c b/tools/tracing/rtla/src/osnoise_hist.c
index 5d7ea479ac89..fe34452fc4ec 100644
--- a/tools/tracing/rtla/src/osnoise_hist.c
+++ b/tools/tracing/rtla/src/osnoise_hist.c
@@ -121,6 +121,7 @@ static void osnoise_hist_update_multiple(struct osnoise_tool *tool, int cpu,
{
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 @@ static void osnoise_hist_update_multiple(struct osnoise_tool *tool, int cpu,
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);
update_max(&data->hist[cpu].max_sample, &duration);
if (bucket < entries)
--
2.34.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2 v2] tools/tracing/rtla: osnoise_hist: display average with two-digit precision
2022-12-28 15:25 ` Daniel Bristot de Oliveira
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-03 10:34 ` Andreas Ziegler
2023-01-12 14:33 ` Daniel Bristot de Oliveira
2 siblings, 1 reply; 9+ messages in thread
From: Andreas Ziegler @ 2023-01-03 10:34 UTC (permalink / raw)
To: Daniel Bristot de Oliveira, Steven Rostedt
Cc: linux-trace-devel, linux-kernel, Andreas Ziegler
Calculate average value in osnoise-hist summary with two-digit
precision to avoid displaying too optimitic results.
Signed-off-by: Andreas Ziegler <br015@umbiko.net>
---
Changes v1 -> v2:
- output with two-digit precision instead of rounding (Daniel)
tools/tracing/rtla/src/osnoise_hist.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/tracing/rtla/src/osnoise_hist.c b/tools/tracing/rtla/src/osnoise_hist.c
index fe34452fc4ec..13e1233690bb 100644
--- a/tools/tracing/rtla/src/osnoise_hist.c
+++ b/tools/tracing/rtla/src/osnoise_hist.c
@@ -335,8 +335,8 @@ osnoise_print_summary(struct osnoise_hist_params *params,
continue;
if (data->hist[cpu].count)
- trace_seq_printf(trace->seq, "%9llu ",
- data->hist[cpu].sum_sample / data->hist[cpu].count);
+ trace_seq_printf(trace->seq, "%9.2f ",
+ ((double) data->hist[cpu].sum_sample) / data->hist[cpu].count);
else
trace_seq_printf(trace->seq, " - ");
}
--
2.34.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2 v2] tools/tracing/rtla: osnoise_hist: use total duration for average calculation
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
0 siblings, 0 replies; 9+ messages in thread
From: Daniel Bristot de Oliveira @ 2023-01-12 14:32 UTC (permalink / raw)
To: Andreas Ziegler, Steven Rostedt; +Cc: linux-trace-devel, linux-kernel
On 1/3/23 11:33, Andreas Ziegler wrote:
> Sampled durations must be weighted by observed quantity, to arrive at a correct
> average duration value.
>
> Perform calculation of total duration by summing (duration * count).
>
> Fixes: 829a6c0b5698 ("rtla/osnoise: Add the hist mode")
> Signed-off-by: Andreas Ziegler <br015@umbiko.net>
Acked-by: Daniel Bristot de Oliveira <bristot@kernel.org>
-- Daniel
> ---
> Changes v1 -> v2:
> - add 'Fixes' line (Daniel)
>
> tools/tracing/rtla/src/osnoise_hist.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/tools/tracing/rtla/src/osnoise_hist.c b/tools/tracing/rtla/src/osnoise_hist.c
> index 5d7ea479ac89..fe34452fc4ec 100644
> --- a/tools/tracing/rtla/src/osnoise_hist.c
> +++ b/tools/tracing/rtla/src/osnoise_hist.c
> @@ -121,6 +121,7 @@ static void osnoise_hist_update_multiple(struct osnoise_tool *tool, int cpu,
> {
> 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 @@ static void osnoise_hist_update_multiple(struct osnoise_tool *tool, int cpu,
> 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);
> update_max(&data->hist[cpu].max_sample, &duration);
>
> if (bucket < entries)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2 v2] tools/tracing/rtla: osnoise_hist: display average with two-digit precision
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
0 siblings, 0 replies; 9+ messages in thread
From: Daniel Bristot de Oliveira @ 2023-01-12 14:33 UTC (permalink / raw)
To: Andreas Ziegler, Steven Rostedt; +Cc: linux-trace-devel, linux-kernel
On 1/3/23 11:34, Andreas Ziegler wrote:
> Calculate average value in osnoise-hist summary with two-digit
> precision to avoid displaying too optimitic results.
>
> Signed-off-by: Andreas Ziegler <br015@umbiko.net>
Acked-by: Daniel Bristot de Oliveira <bristot@kernel.org>
-- Daniel
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-01-12 14:45 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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
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).