lkml.org 
[lkml]   [2021]   [Jul]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2] tracing: Add linear buckets to histogram logic
From
Date
Hi Steve,

Looks good to me, just a couple nits below.

Reviewed-by: Tom Zanussi <zanussi@kernel.org>

On Tue, 2021-07-06 at 15:43 -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
>
> 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@goodmis.org>
> ---
> 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


\
 
 \ /
  Last update: 2021-07-07 00:12    [W:0.028 / U:0.972 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site