Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753847AbcDVLdg (ORCPT ); Fri, 22 Apr 2016 07:33:36 -0400 Received: from mail-pa0-f66.google.com ([209.85.220.66]:36634 "EHLO mail-pa0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751735AbcDVLdd (ORCPT ); Fri, 22 Apr 2016 07:33:33 -0400 Subject: Re: [PATCH v10 1/2] perf config: Prepare all default configs References: <1461320502-18604-1-git-send-email-treeze.taeung@gmail.com> <1461320502-18604-2-git-send-email-treeze.taeung@gmail.com> Cc: linux-kernel@vger.kernel.org, Ingo Molnar , Peter Zijlstra , Alexander Shishkin , Masami Hiramatsu To: Arnaldo Carvalho de Melo , Jiri Olsa , Namhyung Kim From: Taeung Song Message-ID: <571A0C07.6090608@gmail.com> Date: Fri, 22 Apr 2016 20:33:27 +0900 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <1461320502-18604-2-git-send-email-treeze.taeung@gmail.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9925 Lines: 351 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 > Cc: Masami Hiramatsu > Cc: Jiri Olsa > Signed-off-by: Taeung Song > --- > 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(§ion->name); > + zfree((char **)§ion->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 > #include > > +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); > >