| Message ID | 20241119134858.1862632-1-tglozar@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | rtla/timerlat: Fix histogram ALL for zero samples | expand |
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);
út 19. 11. 2024 v 16:22 odesílatel Steven Rostedt <rostedt@goodmis.org> napsal: > > There's a lot of duplicate code here. Can you please make a helper > function, that at least has: Good point, I thought it's not that bad when I was writing it, but together with the already implemented ifs, this pattern is now there 19 times. I will send a v2 once I'm back from vacation. Tomas
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);