On Tue, 19 Nov 2024 14:48:57 +0100
tglo...@redhat.com wrote:
> From: Tomas Glozar <tglo...@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: sta...@vger.kernel.org
> Fixes: 1462501c7a8 ("rtla/timerlat: Add a summary for hist mode")
> Signed-off-by: Tomas Glozar <tglo...@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);
