Re: [PATCH] rtla/timerlat: Fix histogram ALL for zero samples

[Steven Rostedt <rostedt@goodmis.org>]

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);