From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751758AbcEJLtY (ORCPT ); Tue, 10 May 2016 07:49:24 -0400 Received: from mail-pf0-f196.google.com ([209.85.192.196]:34978 "EHLO mail-pf0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751623AbcEJLtV (ORCPT ); Tue, 10 May 2016 07:49:21 -0400 Subject: Re: [PATCH 1/4] perf config: Introduce default_config_item for all default config key-value pairs To: Arnaldo Carvalho de Melo , Jiri Olsa , Namhyung Kim References: <1462794109-14652-1-git-send-email-treeze.taeung@gmail.com> <1462794109-14652-2-git-send-email-treeze.taeung@gmail.com> <20160509171746.GB13209@kernel.org> Cc: linux-kernel@vger.kernel.org, Ingo Molnar , Peter Zijlstra , Alexander Shishkin , Masami Hiramatsu From: Taeung Song Message-ID: <5731CABC.20105@gmail.com> Date: Tue, 10 May 2016 20:49:16 +0900 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.7.2 MIME-Version: 1.0 In-Reply-To: <20160509171746.GB13209@kernel.org> 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 Hi, Arnaldo, Namhyung and jirka :) On 05/10/2016 02:17 AM, Arnaldo Carvalho de Melo wrote: > Em Mon, May 09, 2016 at 08:41:46PM +0900, Taeung Song escreveu: >> We currently use values of actual type(int, bool, char *, etc.) >> when initializing default perf config values. >> >> But I suggest using new config arrays(default_config_items) >> that have all default perf config key-value pairs. >> >> Because if we do, we can manage default perf config values at one spot (like util/config.c) >> and it can be easy and simple to modify default config values or add new configs. >> >> In the near future, this could be used when >> showing all configs with default values, >> initializing default values of each actual config variable >> with the default_config_item arrays and etc. > > looks ok Thank you for your review !! :-) We can use this patch as it is, but what about this change ? diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h index 84414af..8e143fb 100644 --- a/tools/perf/util/config.h +++ b/tools/perf/util/config.h @@ -5,6 +5,9 @@ #include enum perf_config_type { + /* special type */ + CONFIG_END, + CONFIG_TYPE_BOOL, CONFIG_TYPE_INT, CONFIG_TYPE_LONG, @@ -14,8 +17,10 @@ enum perf_config_type { CONFIG_TYPE_STRING }; +#define MAX_CONFIG_NAME 100 + struct default_config_item { - const char *name; + const char name[MAX_CONFIG_NAME]; union { bool b; int i; @@ -62,7 +67,7 @@ struct perf_config_set { #define CONF_STR_VAR(_name, _val) \ CONF_VAR(_name, s, _val, CONFIG_TYPE_STRING) #define CONF_END() \ - { .name = NULL } + { .type = CONFIG_END } Because if we do, we can use name of default_config_item as a constant to replace values of actual type(int, char *, etc) with colors_config_items[].name more easy e.g. (Contrastively if we use 'name' member variable as 'const char *' type, we can't use it as a constant like below. It is a limitation of C language.) diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c index 385d0de..207c62d 100644 --- a/tools/perf/ui/browser.c +++ b/tools/perf/ui/browser.c @@ -508,31 +508,31 @@ static struct ui_browser_colorset { } ui_browser__colorsets[] = { { .colorset = HE_COLORSET_TOP, - .name = "top", + .name = colors_config_items[CONFIG_COLORS_TOP].name, }, { .colorset = HE_COLORSET_MEDIUM, - .name = "medium", + .name = colors_config_items[CONFIG_COLORS_MEDIUM].name, }, { .colorset = HE_COLORSET_NORMAL, - .name = "normal", + .name = colors_config_items[CONFIG_COLORS_NORMAL].name, }, { .colorset = HE_COLORSET_SELECTED, - .name = "selected", + .name = colors_config_items[CONFIG_COLORS_SELECTED].name, }, { .colorset = HE_COLORSET_JUMP_ARROWS, - .name = "jump_arrows", + .name = colors_config_items[CONFIG_COLORS_JUMP_ARROWS].name, }, { .colorset = HE_COLORSET_ADDR, - .name = "addr", + .name = colors_config_items[CONFIG_COLORS_ADDR].name, }, { .colorset = HE_COLORSET_ROOT, - .name = "root", + .name = colors_config_items[CONFIG_COLORS_ROOT].name, }, { .name = NULL, If you give me your opinion or other way, I'd appreciate it. :) Thanks, Taeung > >> Cc: Namhyung Kim >> Cc: Masami Hiramatsu >> Cc: Jiri Olsa >> Signed-off-by: Taeung Song >> --- >> tools/perf/util/config.c | 150 ++++++++++++++++++++++++++++++++++++++++++++++- >> tools/perf/util/config.h | 46 ++++++++++++++- >> 2 files changed, 194 insertions(+), 2 deletions(-) >> >> diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c >> index dad7d82..3a72ed7 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; >> >> +const 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" }, >> +}; >> + >> +const struct default_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() >> +}; >> + >> +const struct default_config_item tui_config_items[] = { >> + CONF_BOOL_VAR("report", true), >> + CONF_BOOL_VAR("annotate", true), >> + CONF_BOOL_VAR("top", true), >> + CONF_END() >> +}; >> + >> +const struct default_config_item buildid_config_items[] = { >> + CONF_STR_VAR("dir", "~/.debug"), >> + CONF_END() >> +}; >> + >> +const struct default_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() >> +}; >> + >> +const struct default_config_item gtk_config_items[] = { >> + CONF_BOOL_VAR("annotate", false), >> + CONF_BOOL_VAR("report", false), >> + CONF_BOOL_VAR("top", false), >> + CONF_END() >> +}; >> + >> +const struct default_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() >> +}; >> + >> +const struct default_config_item help_config_items[] = { >> + CONF_STR_VAR("format", "man"), >> + CONF_INT_VAR("autocorrect", 0), >> + CONF_END() >> +}; >> + >> +const struct default_config_item hist_config_items[] = { >> + CONF_STR_VAR("percentage", "absolute"), >> + CONF_END() >> +}; >> + >> +const struct default_config_item ui_config_items[] = { >> + CONF_BOOL_VAR("show-headers", true), >> + CONF_END() >> +}; >> + >> +const struct default_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() >> +}; >> + >> +const struct default_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() >> +}; >> + >> +const struct default_config_item top_config_items[] = { >> + CONF_BOOL_VAR("children", true), >> + CONF_END() >> +}; >> + >> +const struct default_config_item man_config_items[] = { >> + CONF_STR_VAR("viewer", "man"), >> + CONF_END() >> +}; >> + >> +const struct default_config_item kmem_config_items[] = { >> + CONF_STR_VAR("default", "slab"), >> + CONF_END() >> +}; >> + >> +const struct default_config_item intel_pt_config_items[] = { >> + CONF_INT_VAR("cache-divisor", 64), >> + CONF_BOOL_VAR("mispred-all", false), >> + CONF_END() >> +}; >> + >> +const struct default_config_item convert_config_items[] = { >> + CONF_U64_VAR("queue-size", 0), >> + CONF_END() >> +}; >> + >> +const struct default_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; >> @@ -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..84414af 100644 >> --- a/tools/perf/util/config.h >> +++ b/tools/perf/util/config.h >> @@ -4,6 +4,30 @@ >> #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 default_config_item { >> + const char *name; >> + union { >> + bool b; >> + int i; >> + u32 l; >> + u64 ll; >> + float f; >> + double d; >> + const char *s; >> + } value; >> + enum perf_config_type type; >> +}; >> + >> struct perf_config_item { >> char *name; >> char *value; >> @@ -11,7 +35,7 @@ struct perf_config_item { >> }; >> >> struct perf_config_section { >> - char *name; >> + const char *name; >> struct list_head items; >> struct list_head node; >> }; >> @@ -20,6 +44,26 @@ struct perf_config_set { >> struct list_head sections; >> }; >> >> +#define CONF_VAR(_name, _field, _val, _type) \ >> + { .name = _name, .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); >> >> -- >> 2.5.0