* [PATCH] rtla/timerlat: Fix histogram ALL for zero samples
@ 2024-11-19 13:48 tglozar
2024-11-19 15:22 ` Steven Rostedt
0 siblings, 1 reply; 3+ messages in thread
From: tglozar @ 2024-11-19 13:48 UTC (permalink / raw)
To: rostedt; +Cc: linux-trace-kernel, linux-kernel, jkacur, Tomas Glozar
From: Tomas Glozar <tglozar@redhat.com>
rtla timerlat hist currently computers the minimum, maximum and average
latency even in cases when there are zero samples. This leads to
nonsensical values being calculated for maximum and minimum, and to
divide by zero for average.
A similar bug is fixed by 01b05fc0e5f3 ("rtla/timerlat: Fix histogram
report when a cpu count is 0") but the bug still remains for printing
the sum over all CPUs in timerlat_print_stats_all.
The issue can be reproduced with this command:
$ rtla timerlat hist -U -k -d 1s
Index
over:
count:
min:
avg:
max:
Floating point exception (core dumped)
(There are always no samples with -U unless the user workload is
created; the -k is to work around a bug with -U.)
Fix the bug by omitting max/min/avg when sample count is zero,
displaying a dash instead, just like we already do for the individual
CPUs.
Cc: stable@vger.kernel.org
Fixes: 1462501c7a8 ("rtla/timerlat: Add a summary for hist mode")
Signed-off-by: Tomas Glozar <tglozar@redhat.com>
---
tools/tracing/rtla/src/timerlat_hist.c | 93 ++++++++++++++++++--------
1 file changed, 66 insertions(+), 27 deletions(-)
diff --git a/tools/tracing/rtla/src/timerlat_hist.c b/tools/tracing/rtla/src/timerlat_hist.c
index a3907c390d67..5cbbe3e98c1d 100644
--- a/tools/tracing/rtla/src/timerlat_hist.c
+++ b/tools/tracing/rtla/src/timerlat_hist.c
@@ -504,51 +504,90 @@ timerlat_print_stats_all(struct timerlat_hist_params *params,
if (!params->no_index)
trace_seq_printf(trace->seq, "min: ");
- if (!params->no_irq)
- trace_seq_printf(trace->seq, "%9llu ",
- sum.min_irq);
+ if (!params->no_irq) {
+ if (sum.irq_count != 0)
+ trace_seq_printf(trace->seq, "%9llu ",
+ sum.min_irq);
+ else
+ trace_seq_printf(trace->seq, "%9c ", '-');
+ }
- if (!params->no_thread)
- trace_seq_printf(trace->seq, "%9llu ",
- sum.min_thread);
+ if (!params->no_thread) {
+ if (sum.thread_count != 0)
+ trace_seq_printf(trace->seq, "%9llu ",
+ sum.min_thread);
+ else
+ trace_seq_printf(trace->seq, "%9c ", '-');
+ }
- if (params->user_hist)
- trace_seq_printf(trace->seq, "%9llu ",
- sum.min_user);
+ if (params->user_hist) {
+ if (sum.user_count != 0) {
+ trace_seq_printf(trace->seq, "%9llu ",
+ sum.min_user);
+ } else {
+ trace_seq_printf(trace->seq, "%9c ", '-');
+ }
+ }
trace_seq_printf(trace->seq, "\n");
if (!params->no_index)
trace_seq_printf(trace->seq, "avg: ");
- if (!params->no_irq)
- trace_seq_printf(trace->seq, "%9llu ",
- sum.sum_irq / sum.irq_count);
+ if (!params->no_irq) {
+ if (sum.irq_count != 0)
+ trace_seq_printf(trace->seq, "%9llu ",
+ sum.sum_irq / sum.irq_count);
+ else
+ trace_seq_printf(trace->seq, "%9c ", '-');
+ }
- if (!params->no_thread)
- trace_seq_printf(trace->seq, "%9llu ",
- sum.sum_thread / sum.thread_count);
+ if (!params->no_thread) {
+ if (sum.thread_count != 0)
+ trace_seq_printf(trace->seq, "%9llu ",
+ sum.sum_thread / sum.thread_count);
+ else
+ trace_seq_printf(trace->seq, "%9c ", '-');
+ }
- if (params->user_hist)
- trace_seq_printf(trace->seq, "%9llu ",
- sum.sum_user / sum.user_count);
+ if (params->user_hist) {
+ if (sum.user_count != 0) {
+ trace_seq_printf(trace->seq, "%9llu ",
+ sum.sum_user / sum.user_count);
+ } else {
+ trace_seq_printf(trace->seq, "%9c ", '-');
+ }
+ }
trace_seq_printf(trace->seq, "\n");
if (!params->no_index)
trace_seq_printf(trace->seq, "max: ");
- if (!params->no_irq)
- trace_seq_printf(trace->seq, "%9llu ",
- sum.max_irq);
+ if (!params->no_irq) {
+ if (sum.irq_count != 0)
+ trace_seq_printf(trace->seq, "%9llu ",
+ sum.max_irq);
+ else
+ trace_seq_printf(trace->seq, "%9c ", '-');
+ }
- if (!params->no_thread)
- trace_seq_printf(trace->seq, "%9llu ",
- sum.max_thread);
+ if (!params->no_thread) {
+ if (sum.thread_count != 0)
+ trace_seq_printf(trace->seq, "%9llu ",
+ sum.max_thread);
+ else
+ trace_seq_printf(trace->seq, "%9c ", '-');
+ }
- if (params->user_hist)
- trace_seq_printf(trace->seq, "%9llu ",
- sum.max_user);
+ if (params->user_hist) {
+ if (sum.user_count != 0) {
+ trace_seq_printf(trace->seq, "%9llu ",
+ sum.max_user);
+ } else {
+ trace_seq_printf(trace->seq, "%9c ", '-');
+ }
+ }
trace_seq_printf(trace->seq, "\n");
trace_seq_do_printf(trace->seq);
--
2.47.0
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] rtla/timerlat: Fix histogram ALL for zero samples
2024-11-19 13:48 [PATCH] rtla/timerlat: Fix histogram ALL for zero samples tglozar
@ 2024-11-19 15:22 ` Steven Rostedt
2024-11-20 5:04 ` Tomas Glozar
0 siblings, 1 reply; 3+ messages in thread
From: Steven Rostedt @ 2024-11-19 15:22 UTC (permalink / raw)
To: tglozar; +Cc: linux-trace-kernel, linux-kernel, jkacur
On Tue, 19 Nov 2024 14:48:57 +0100
tglozar@redhat.com wrote:
> From: Tomas Glozar <tglozar@redhat.com>
>
> rtla timerlat hist currently computers the minimum, maximum and average
> latency even in cases when there are zero samples. This leads to
> nonsensical values being calculated for maximum and minimum, and to
> divide by zero for average.
>
> A similar bug is fixed by 01b05fc0e5f3 ("rtla/timerlat: Fix histogram
> report when a cpu count is 0") but the bug still remains for printing
> the sum over all CPUs in timerlat_print_stats_all.
>
> The issue can be reproduced with this command:
>
> $ rtla timerlat hist -U -k -d 1s
> Index
> over:
> count:
> min:
> avg:
> max:
> Floating point exception (core dumped)
>
> (There are always no samples with -U unless the user workload is
> created; the -k is to work around a bug with -U.)
>
> Fix the bug by omitting max/min/avg when sample count is zero,
> displaying a dash instead, just like we already do for the individual
> CPUs.
>
> Cc: stable@vger.kernel.org
> Fixes: 1462501c7a8 ("rtla/timerlat: Add a summary for hist mode")
> Signed-off-by: Tomas Glozar <tglozar@redhat.com>
> ---
> tools/tracing/rtla/src/timerlat_hist.c | 93 ++++++++++++++++++--------
> 1 file changed, 66 insertions(+), 27 deletions(-)
>
> diff --git a/tools/tracing/rtla/src/timerlat_hist.c b/tools/tracing/rtla/src/timerlat_hist.c
> index a3907c390d67..5cbbe3e98c1d 100644
> --- a/tools/tracing/rtla/src/timerlat_hist.c
> +++ b/tools/tracing/rtla/src/timerlat_hist.c
> @@ -504,51 +504,90 @@ timerlat_print_stats_all(struct timerlat_hist_params *params,
> if (!params->no_index)
> trace_seq_printf(trace->seq, "min: ");
>
> - if (!params->no_irq)
> - trace_seq_printf(trace->seq, "%9llu ",
> - sum.min_irq);
> + if (!params->no_irq) {
> + if (sum.irq_count != 0)
> + trace_seq_printf(trace->seq, "%9llu ",
> + sum.min_irq);
> + else
> + trace_seq_printf(trace->seq, "%9c ", '-');
> + }
>
> - if (!params->no_thread)
> - trace_seq_printf(trace->seq, "%9llu ",
> - sum.min_thread);
> + if (!params->no_thread) {
> + if (sum.thread_count != 0)
> + trace_seq_printf(trace->seq, "%9llu ",
> + sum.min_thread);
> + else
> + trace_seq_printf(trace->seq, "%9c ", '-');
> + }
>
> - if (params->user_hist)
> - trace_seq_printf(trace->seq, "%9llu ",
> - sum.min_user);
> + if (params->user_hist) {
> + if (sum.user_count != 0) {
> + trace_seq_printf(trace->seq, "%9llu ",
> + sum.min_user);
> + } else {
> + trace_seq_printf(trace->seq, "%9c ", '-');
> + }
> + }
>
> trace_seq_printf(trace->seq, "\n");
>
> if (!params->no_index)
> trace_seq_printf(trace->seq, "avg: ");
>
> - if (!params->no_irq)
> - trace_seq_printf(trace->seq, "%9llu ",
> - sum.sum_irq / sum.irq_count);
> + if (!params->no_irq) {
> + if (sum.irq_count != 0)
> + trace_seq_printf(trace->seq, "%9llu ",
> + sum.sum_irq / sum.irq_count);
> + else
> + trace_seq_printf(trace->seq, "%9c ", '-');
> + }
>
> - if (!params->no_thread)
> - trace_seq_printf(trace->seq, "%9llu ",
> - sum.sum_thread / sum.thread_count);
> + if (!params->no_thread) {
> + if (sum.thread_count != 0)
> + trace_seq_printf(trace->seq, "%9llu ",
> + sum.sum_thread / sum.thread_count);
> + else
> + trace_seq_printf(trace->seq, "%9c ", '-');
> + }
>
> - if (params->user_hist)
> - trace_seq_printf(trace->seq, "%9llu ",
> - sum.sum_user / sum.user_count);
> + if (params->user_hist) {
> + if (sum.user_count != 0) {
> + trace_seq_printf(trace->seq, "%9llu ",
> + sum.sum_user / sum.user_count);
> + } else {
> + trace_seq_printf(trace->seq, "%9c ", '-');
> + }
> + }
>
> trace_seq_printf(trace->seq, "\n");
>
> if (!params->no_index)
> trace_seq_printf(trace->seq, "max: ");
>
> - if (!params->no_irq)
> - trace_seq_printf(trace->seq, "%9llu ",
> - sum.max_irq);
> + if (!params->no_irq) {
> + if (sum.irq_count != 0)
> + trace_seq_printf(trace->seq, "%9llu ",
> + sum.max_irq);
> + else
> + trace_seq_printf(trace->seq, "%9c ", '-');
> + }
>
> - if (!params->no_thread)
> - trace_seq_printf(trace->seq, "%9llu ",
> - sum.max_thread);
> + if (!params->no_thread) {
> + if (sum.thread_count != 0)
> + trace_seq_printf(trace->seq, "%9llu ",
> + sum.max_thread);
> + else
> + trace_seq_printf(trace->seq, "%9c ", '-');
> + }
>
> - if (params->user_hist)
> - trace_seq_printf(trace->seq, "%9llu ",
> - sum.max_user);
> + if (params->user_hist) {
> + if (sum.user_count != 0) {
> + trace_seq_printf(trace->seq, "%9llu ",
> + sum.max_user);
> + } else {
> + trace_seq_printf(trace->seq, "%9c ", '-');
> + }
> + }
There's a lot of duplicate code here. Can you please make a helper
function, that at least has:
static void show_count(struct trace_seq *seq, int count, unsigned long long val)
{
if (count)
trace_seq_printf(seq, "%9llu ", val);
else
trace_seq_printf(seq, "%9c ", '-');
}
Then the above could be just:
if (params->user_hist)
show_count(trace->seq, sum.user_count, sum.max_user);
Thanks,
-- Steve
>
> trace_seq_printf(trace->seq, "\n");
> trace_seq_do_printf(trace->seq);
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-11-20 5:04 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-11-19 13:48 [PATCH] rtla/timerlat: Fix histogram ALL for zero samples tglozar
2024-11-19 15:22 ` Steven Rostedt
2024-11-20 5:04 ` Tomas Glozar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox