Re: [PATCH v2] tracing: Add linear buckets to histogram logic

[Tom Zanussi]

Hi Steve,

Looks good to me, just a couple nits below.

Reviewed-by: Tom Zanussi <zanussi@xxxxxxxxxx>

On Tue, 2021-07-06 at 15:43 -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (VMware)" <rostedt@xxxxxxxxxxx>
> 
> There's been several times I wished the histogram logic had a
> "grouping"
> feature for the buckets. Currently, each bucket has a size of one.
> That
> is, if you trace the amount of requested allocations, each allocation
> is
> its own bucket, even if you are interested in what allocates 100
> bytes or
> less, 100 to 200, 200 to 300, etc.
> 
> Also, without grouping, it fills up the allocated histogram buckets
> quickly. If you are tracking latency, and don't care if something is
> 200
> microseconds off, or 201 microseconds off, but want to track them by
> say
> 10 microseconds each. This can not currently be done.
> 
> There is a log2 but that grouping get's too big too fast for a lot of
> cases.
> 
> Introduce a "buckets=SIZE" command to each field where it will record
> in a
> rounded number. For example:
> 
>  ># echo 'hist:keys=bytes_req.buckets=100:sort=bytes_req' >
> events/kmem/kmalloc/trigger
>  ># cat events/kmem/kmalloc/hist
>  # event histogram
>  #
>  # trigger info:
>  hist:keys=bytes_req.buckets=100:vals=hitcount:sort=bytes_req.buckets
> =100:size=2048
>  [active]
>  #
> 
>  { bytes_req: ~ 0-99 } hitcount:       3149
>  { bytes_req: ~ 100-199 } hitcount:       1468
>  { bytes_req: ~ 200-299 } hitcount:         39
>  { bytes_req: ~ 300-399 } hitcount:        306
>  { bytes_req: ~ 400-499 } hitcount:        364
>  { bytes_req: ~ 500-599 } hitcount:         32
>  { bytes_req: ~ 600-699 } hitcount:         69
>  { bytes_req: ~ 700-799 } hitcount:         37
>  { bytes_req: ~ 1200-1299 } hitcount:         16
>  { bytes_req: ~ 1400-1499 } hitcount:         30
>  { bytes_req: ~ 2000-2099 } hitcount:          6
>  { bytes_req: ~ 4000-4099 } hitcount:       2168
>  { bytes_req: ~ 5000-5099 } hitcount:          6
> 
>  Totals:
>      Hits: 7690
>      Entries: 13
>      Dropped: 0
> 
> Signed-off-by: Steven Rostedt (VMware) <rostedt@xxxxxxxxxxx>
> ---
> Changes since v1:
> 
>  - Used modifier notation of ".buckets=SIZE" instead of hyphen
> (Namhyung Kim)
>  - Incorporated it to be more like the ".log2" modifier (Tom Zanussi)
>  - Used "~" notation like the log2 modifier.
> 
>  kernel/trace/trace_events_hist.c | 65 ++++++++++++++++++++++++++++
> ----
>  1 file changed, 58 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/trace/trace_events_hist.c
> b/kernel/trace/trace_events_hist.c
> index ba03b7d84fc2..607d0fb291ea 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -120,6 +120,7 @@ struct hist_field {
>  	unsigned int			size;
>  	unsigned int			offset;
>  	unsigned int                    is_signed;
> +	unsigned long			grouping;

Just wondering if it would be more consistent to name this 'buckets' or
even 'bucket_size'.

>  	const char			*type;
>  	struct hist_field		*operands[HIST_FIELD_OPERANDS_MAX];
>  	struct hist_trigger_data	*hist_data;
> @@ -218,6 +219,27 @@ static u64 hist_field_log2(struct hist_field
> *hist_field,
>  	return (u64) ilog2(roundup_pow_of_two(val));
>  }
>  
> 
> 

[snip]

> @@ -4657,6 +4701,11 @@ static void hist_trigger_print_key(struct
> seq_file *m,
>  		} else if (key_field->flags & HIST_FIELD_FL_LOG2) {
>  			seq_printf(m, "%s: ~ 2^%-2llu", field_name,
>  				   *(u64 *)(key + key_field->offset));
> +		} else if (key_field->flags & HIST_FIELD_FL_BUCKET) {
> +			unsigned long grouping = key_field->grouping;
> +			uval = *(u64 *)(key + key_field->offset);
> +			seq_printf(m, "%s: ~ %llu-%llu", field_name,
> +				   uval, uval + grouping -1);

Need a space before 1 i.e. 'grouping - 1'?

Thanks,

Tom