Re: [PATCH v13 09/29] tracing: Add 'hist' event trigger command

[Steven Rostedt <rostedt@goodmis.org>]

On Thu, 10 Dec 2015 12:50:51 -0600
Tom Zanussi <tom.zanussi@linux.intel.com> wrote:

> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> new file mode 100644
> index 0000000..213aa79
> --- /dev/null
> +++ b/kernel/trace/trace_events_hist.c
> @@ -0,0 +1,837 @@
> +/*
> + * trace_events_hist - trace event hist triggers
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * Copyright (C) 2015 Tom Zanussi <tom.zanussi@linux.intel.com>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kallsyms.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +#include <linux/stacktrace.h>
> +
> +#include "tracing_map.h"
> +#include "trace.h"
> +
> +struct hist_field;
> +
> +typedef u64 (*hist_field_fn_t) (struct hist_field *field, void *event);
> +
> +struct hist_field {
> +	struct ftrace_event_field	*field;
> +	unsigned long			flags;
> +	hist_field_fn_t			fn;
> +	unsigned int			size;
> +};
> +
> +static u64 hist_field_counter(struct hist_field *field, void *event)
> +{
> +	return 1;
> +}
> +
> +static u64 hist_field_string(struct hist_field *hist_field, void *event)
> +{
> +	char *addr = (char *)(event + hist_field->field->offset);
> +
> +	return (u64)(unsigned long)addr;
> +}
> +
> +#define DEFINE_HIST_FIELD_FN(type)					\
> +static u64 hist_field_##type(struct hist_field *hist_field, void *event)\
> +{									\
> +	type *addr = (type *)(event + hist_field->field->offset);	\
> +									\
> +	return (u64)*addr;						\
> +}
> +
> +DEFINE_HIST_FIELD_FN(s64);
> +DEFINE_HIST_FIELD_FN(u64);
> +DEFINE_HIST_FIELD_FN(s32);
> +DEFINE_HIST_FIELD_FN(u32);
> +DEFINE_HIST_FIELD_FN(s16);
> +DEFINE_HIST_FIELD_FN(u16);
> +DEFINE_HIST_FIELD_FN(s8);
> +DEFINE_HIST_FIELD_FN(u8);
> +
> +#define for_each_hist_field(i, hist_data)	\
> +	for (i = 0; i < hist_data->n_fields; i++)
> +
> +#define for_each_hist_val_field(i, hist_data)	\
> +	for (i = 0; i < hist_data->n_vals; i++)
> +
> +#define for_each_hist_key_field(i, hist_data)	\
> +	for (i = hist_data->n_vals; i < hist_data->n_fields; i++)

The above should have parenthesis around the macro variables.

e.g.

 (hist_data)->n_fields

> +
> +#define HITCOUNT_IDX		0
> +#define HIST_KEY_MAX		1
> +#define HIST_KEY_SIZE_MAX	MAX_FILTER_STR_VAL
> +
> +enum hist_field_flags {
> +	HIST_FIELD_HITCOUNT	= 1,
> +	HIST_FIELD_KEY		= 2,
> +	HIST_FIELD_STRING	= 4,

Note, please add "_FL" to the enums to denote they are flags (used for
bitmask).

e.g. HIST_FIELD_FL_HITCOUNT


> +};
> +
> +struct hist_trigger_attrs {
> +	char		*keys_str;
> +	unsigned int	map_bits;
> +};
> +
> +struct hist_trigger_data {
> +	struct hist_field               *fields[TRACING_MAP_FIELDS_MAX];

Hmm I'm confused, TRACING_MAP_FIELDS_MAX == 4

But below it we index into fields with n_fields. Which look to me can
be much bigger than 4.

Or is there some correlation between n_vals, n_keys and 4?

> +	unsigned int			n_vals;
> +	unsigned int			n_keys;
> +	unsigned int			n_fields;
> +	unsigned int			key_size;
> +	struct tracing_map_sort_key	sort_keys[TRACING_MAP_SORT_KEYS_MAX];
> +	unsigned int			n_sort_keys;
> +	struct trace_event_file		*event_file;
> +	struct hist_trigger_attrs	*attrs;
> +	struct tracing_map		*map;
> +};
> +
> +static hist_field_fn_t select_value_fn(int field_size, int field_is_signed)
> +{
> +	hist_field_fn_t fn = NULL;
> +
> +	switch (field_size) {
> +	case 8:
> +		if (field_is_signed)
> +			fn = hist_field_s64;
> +		else
> +			fn = hist_field_u64;
> +		break;
> +	case 4:
> +		if (field_is_signed)
> +			fn = hist_field_s32;
> +		else
> +			fn = hist_field_u32;
> +		break;
> +	case 2:
> +		if (field_is_signed)
> +			fn = hist_field_s16;
> +		else
> +			fn = hist_field_u16;
> +		break;
> +	case 1:
> +		if (field_is_signed)
> +			fn = hist_field_s8;
> +		else
> +			fn = hist_field_u8;
> +		break;
> +	}
> +
> +	return fn;
> +}
> +
> +static int parse_map_size(char *str)
> +{
> +	unsigned long size, map_bits;
> +	int ret;
> +
> +	strsep(&str, "=");
> +	if (!str) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	ret = kstrtoul(str, 0, &size);
> +	if (ret)
> +		goto out;
> +
> +	map_bits = ilog2(roundup_pow_of_two(size));
> +	if (map_bits < TRACING_MAP_BITS_MIN ||
> +	    map_bits > TRACING_MAP_BITS_MAX)
> +		ret = -EINVAL;
> +	else
> +		ret = map_bits;
> + out:
> +	return ret;
> +}
> +
> +static void destroy_hist_trigger_attrs(struct hist_trigger_attrs *attrs)
> +{
> +	if (!attrs)
> +		return;
> +
> +	kfree(attrs->keys_str);
> +	kfree(attrs);
> +}
> +
> +static struct hist_trigger_attrs *parse_hist_trigger_attrs(char *trigger_str)
> +{
> +	struct hist_trigger_attrs *attrs;
> +	int ret = 0;
> +
> +	attrs = kzalloc(sizeof(*attrs), GFP_KERNEL);
> +	if (!attrs)
> +		return ERR_PTR(-ENOMEM);
> +
> +	while (trigger_str) {
> +		char *str = strsep(&trigger_str, ":");
> +
> +		if (!strncmp(str, "keys", strlen("keys")) ||
> +		    !strncmp(str, "key", strlen("key")))

nit, I prefer "strncmp(x,y,n) == 0" as it shows an equals, and, at
least to me, easier to quickly understand.

But not so nit,  if !strncmp(str, "keys", strlen("keys")) then
 !strcmp(str, "key", strlen("key")) is true!

It has the same result as:

 if (!strncmp(str, "key", strlen("key")))

That is equivalent to if (x > 12 || x > 10) where
if (x > 10) will do.

Is that suppose to be a full match or partial match?

> +			attrs->keys_str = kstrdup(str, GFP_KERNEL);
> +		else if (!strncmp(str, "size", strlen("size"))) {

Is it OK to allow "sizeUgaBooga=10"?

Perhaps the compare should be against "size=" ?

Same for the keys above.

> +			int map_bits = parse_map_size(str);
> +
> +			if (map_bits < 0) {
> +				ret = map_bits;
> +				goto free;
> +			}
> +			attrs->map_bits = map_bits;
> +		} else {
> +			ret = -EINVAL;
> +			goto free;
> +		}
> +	}
> +
> +	if (!attrs->keys_str) {
> +		ret = -EINVAL;
> +		goto free;
> +	}
> +
> +	return attrs;
> + free:
> +	destroy_hist_trigger_attrs(attrs);
> +
> +	return ERR_PTR(ret);
> +}
> +

I stopped here. I'll review the rest later.

-- Steve