Re: [PATCH v10 1/2] perf config: Prepare all default configs

[Taeung Song <treeze.taeung@gmail.com>]

Hi,

On 04/22/2016 07:21 PM, Taeung Song wrote:
> To precisely manage configs,
> prepare all default perf's configs that contain
> default section name, variable name, value
> and correct type, not string type.
>
> In the near future, this will be used when
> checking type of config variable or showing
> all configs with default values, etc.
>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
> ---
>   tools/perf/util/config.c | 152 ++++++++++++++++++++++++++++++++++++++++++++++-
>   tools/perf/util/config.h |  44 +++++++++++++-
>   2 files changed, 192 insertions(+), 4 deletions(-)
>
> diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
> index dad7d82..e09ecbb 100644
> --- a/tools/perf/util/config.c
> +++ b/tools/perf/util/config.c
> @@ -29,6 +29,154 @@ static int config_file_eof;
>
>   const char *config_exclusive_filename;
>
> +struct perf_config_section default_sections[] = {
> +	{ .name = "colors" },
> +	{ .name = "tui" },
> +	{ .name = "buildid" },
> +	{ .name = "annotate" },
> +	{ .name = "gtk" },
> +	{ .name = "pager" },
> +	{ .name = "help" },
> +	{ .name = "hist" },
> +	{ .name = "ui" },
> +	{ .name = "call-graph" },
> +	{ .name = "report" },
> +	{ .name = "top" },
> +	{ .name = "man" },
> +	{ .name = "kmem" },
> +	{ .name = "intel-pt" },
> +	{ .name = "convert" },
> +};
> +
> +struct perf_config_item colors_config_items[] = {
> +	CONF_STR_VAR("top", "red, default"),
> +	CONF_STR_VAR("medium", "green, default"),
> +	CONF_STR_VAR("normal", "default, default"),
> +	CONF_STR_VAR("selected", "black, yellow"),
> +	CONF_STR_VAR("jump_arrows", "blue, default"),
> +	CONF_STR_VAR("addr", "magenta, default"),
> +	CONF_STR_VAR("root", "white, blue"),
> +	CONF_END()
> +};
> +
> +struct perf_config_item tui_config_items[] = {
> +	CONF_BOOL_VAR("report", true),
> +	CONF_BOOL_VAR("annotate", true),
> +	CONF_BOOL_VAR("top", true),
> +	CONF_END()
> +};
> +
> +struct perf_config_item buildid_config_items[] = {
> +	CONF_STR_VAR("dir", "~/.debug"),
> +	CONF_END()
> +};
> +
> +struct perf_config_item annotate_config_items[] = {
> +	CONF_BOOL_VAR("hide_src_code", false),
> +	CONF_BOOL_VAR("use_offset", true),
> +	CONF_BOOL_VAR("jump_arrows", true),
> +	CONF_BOOL_VAR("show_nr_jumps", false),
> +	CONF_BOOL_VAR("show_linenr", false),
> +	CONF_BOOL_VAR("show_total_period", false),
> +	CONF_END()
> +};
> +
> +struct perf_config_item gtk_config_items[] = {
> +	CONF_BOOL_VAR("annotate", false),
> +	CONF_BOOL_VAR("report", false),
> +	CONF_BOOL_VAR("top", false),
> +	CONF_END()
> +};
> +
> +struct perf_config_item pager_config_items[] = {
> +	CONF_BOOL_VAR("cmd", true),
> +	CONF_BOOL_VAR("report", true),
> +	CONF_BOOL_VAR("annotate", true),
> +	CONF_BOOL_VAR("top", true),
> +	CONF_BOOL_VAR("diff", true),
> +	CONF_END()
> +};
> +
> +struct perf_config_item help_config_items[] = {
> +	CONF_STR_VAR("format", "man"),
> +	CONF_INT_VAR("autocorrect", 0),
> +	CONF_END()
> +};
> +
> +struct perf_config_item hist_config_items[] = {
> +	CONF_STR_VAR("percentage", "absolute"),
> +	CONF_END()
> +};
> +
> +struct perf_config_item ui_config_items[] = {
> +	CONF_BOOL_VAR("show-headers", true),
> +	CONF_END()
> +};
> +
> +struct perf_config_item call_graph_config_items[] = {
> +	CONF_STR_VAR("record-mode", "fp"),
> +	CONF_LONG_VAR("dump-size", 8192),
> +	CONF_STR_VAR("print-type", "graph"),
> +	CONF_STR_VAR("order", "callee"),
> +	CONF_STR_VAR("sort-key", "function"),
> +	CONF_DOUBLE_VAR("threshold", 0.5),
> +	CONF_LONG_VAR("print-limit", 0),
> +	CONF_END()
> +};
> +
> +struct perf_config_item report_config_items[] = {
> +	CONF_BOOL_VAR("group", true),
> +	CONF_BOOL_VAR("children", true),
> +	CONF_FLOAT_VAR("percent-limit", 0),
> +	CONF_U64_VAR("queue-size", 0),
> +	CONF_END()
> +};
> +
> +struct perf_config_item top_config_items[] = {
> +	CONF_BOOL_VAR("children", true),
> +	CONF_END()
> +};
> +
> +struct perf_config_item man_config_items[] = {
> +	CONF_STR_VAR("viewer", "man"),
> +	CONF_END()
> +};
> +
> +struct perf_config_item kmem_config_items[] = {
> +	CONF_STR_VAR("default", "slab"),
> +	CONF_END()
> +};
> +
> +struct perf_config_item intel_pt_config_items[] = {
> +	CONF_INT_VAR("cache-divisor", 64),
> +	CONF_BOOL_VAR("mispred-all", false),
> +	CONF_END()
> +};
> +
> +struct perf_config_item convert_config_items[] = {
> +	CONF_U64_VAR("queue-size", 0),
> +	CONF_END()
> +};
> +
> +struct perf_config_item *default_config_items[] = {
> +	colors_config_items,
> +	tui_config_items,
> +	buildid_config_items,
> +	annotate_config_items,
> +	gtk_config_items,
> +	pager_config_items,
> +	help_config_items,
> +	hist_config_items,
> +	ui_config_items,
> +	call_graph_config_items,
> +	report_config_items,
> +	top_config_items,
> +	man_config_items,
> +	kmem_config_items,
> +	intel_pt_config_items,
> +	convert_config_items,
> +};
> +
>   static int get_next_char(void)
>   {
>   	int c;
> @@ -659,7 +807,7 @@ struct perf_config_set *perf_config_set__new(void)
>
>   static void perf_config_item__delete(struct perf_config_item *item)
>   {
> -	zfree(&item->name);
> +	zfree((char **)&item->name);
>   	zfree(&item->value);
>   	free(item);
>   }
> @@ -677,7 +825,7 @@ static void perf_config_section__purge(struct perf_config_section *section)
>   static void perf_config_section__delete(struct perf_config_section *section)
>   {
>   	perf_config_section__purge(section);
> -	zfree(&section->name);
> +	zfree((char **)&section->name);
>   	free(section);
>   }
>
> diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
> index 22ec626..297b33e 100644
> --- a/tools/perf/util/config.h
> +++ b/tools/perf/util/config.h
> @@ -4,14 +4,34 @@
>   #include <stdbool.h>
>   #include <linux/list.h>
>
> +enum perf_config_type {
> +	CONFIG_TYPE_BOOL,
> +	CONFIG_TYPE_INT,
> +	CONFIG_TYPE_LONG,
> +	CONFIG_TYPE_U64,
> +	CONFIG_TYPE_FLOAT,
> +	CONFIG_TYPE_DOUBLE,
> +	CONFIG_TYPE_STRING
> +};
> +
>   struct perf_config_item {
> -	char *name;
> +	const char *name;
>   	char *value;
> +	union {
> +		bool b;
> +		int i;
> +		u32 l;
> +		u64 ll;
> +		float f;
> +		double d;
> +		const char *s;
> +	} default_value;
> +	enum perf_config_type type;

I have worries about mismatch between types of actual variables
which are related each config value
and types of 'default_value' in struct perf_config_item.

I rechecked all config default values looking into relevant source codes.

After the checking,
I thought that the reason why we use actual type in perf_config_item
is because of checking correct type of config values and
a possibility to replace raw values of actual variables with particular 
values of default_config_item[] e.g.


diff --git a/tools/perf/ui/browsers/annotate.c 
b/tools/perf/ui/browsers/annotate.c
index 4fc208e..a0d27ca 100644
--- a/tools/perf/ui/browsers/annotate.c
+++ b/tools/perf/ui/browsers/annotate.c
@@ -38,8 +38,8 @@ static struct annotate_browser_opt {
               show_nr_jumps,
               show_total_period;
  } annotate_browser__opts = {
-        .use_offset     = true,
-        .jump_arrows    = true,
+        .use_offset     = 
(bool)annotate_config_items[CONFIG_ANNOTATE_USE_OFFSET].default_value,
+        .jump_arrows    = 
(bool)annotate_config_items[CONFIG_ANNOTATE_JUMP_ARROWS].default_value,
  };


But I found actual variables related each config value can be
enum chain_mode, enum perf_call_graph_mode or etc.

and config 'hist.percentage' can be a string 'absolute' or 'relative'
but actual type of the variable(symbol_conf.filter_relative)
is bool type i.e.

     struct symbol_conf {
             unsigned short  priv_size;
             unsigned short  nr_events;
             bool            try_vmlinux_path,
                             force,
     ..(omitted)...
                             filter_relative,
     ...(omitted)...


Isn't the mismatch a problem ?

If we just use type casting or each function to convert type
(e.g. parse_filter_percentage(), etc.), the mismatch couldn't be
a problem ? or

Is it better to add each enum types into union type of struct 
perf_config_item ?

or is it better to fix default values according types of the actual 
variables e.g.


diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
index a19a15c..231a2bc 100644
--- a/tools/perf/util/config.c
+++ b/tools/perf/util/config.c
@@ -104,7 +104,7 @@ struct perf_config_item help_config_items[] = {
  };

  struct perf_config_item hist_config_items[] = {
-        CONF_STR_VAR("percentage", "absolute"),
+        CONF_STR_VAR("percentage", false),
          CONF_END()
  };


I want to know your opinions.
If anyone have other idea and tell me,
I'd appreciate it. :-)


Thanks,
Taeung

>   	struct list_head node;
>   };
>
>   struct perf_config_section {
> -	char *name;
> +	const char *name;
>   	struct list_head items;
>   	struct list_head node;
>   };
> @@ -20,6 +40,26 @@ struct perf_config_set {
>   	struct list_head sections;
>   };
>
> +#define CONF_VAR(_name, _field, _val, _type)				\
> +	{ .name = _name, .default_value._field = _val, .type = _type }
> +
> +#define CONF_BOOL_VAR(_name, _val)			\
> +	CONF_VAR(_name, b, _val, CONFIG_TYPE_BOOL)
> +#define CONF_INT_VAR(_name, _val)			\
> +	CONF_VAR(_name, i, _val, CONFIG_TYPE_INT)
> +#define CONF_LONG_VAR(_name, _val)			\
> +	CONF_VAR(_name, l, _val, CONFIG_TYPE_LONG)
> +#define CONF_U64_VAR(_name, _val)			\
> +	CONF_VAR(_name, ll, _val, CONFIG_TYPE_U64)
> +#define CONF_FLOAT_VAR(_name, _val)			\
> +	CONF_VAR(_name, f, _val, CONFIG_TYPE_FLOAT)
> +#define CONF_DOUBLE_VAR(_name, _val)			\
> +	CONF_VAR(_name, d, _val, CONFIG_TYPE_DOUBLE)
> +#define CONF_STR_VAR(_name, _val)			\
> +	CONF_VAR(_name, s, _val, CONFIG_TYPE_STRING)
> +#define CONF_END()					\
> +	{ .name = NULL }
> +
>   struct perf_config_set *perf_config_set__new(void);
>   void perf_config_set__delete(struct perf_config_set *set);
>
>