From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751893AbcEKKac (ORCPT ); Wed, 11 May 2016 06:30:32 -0400 Received: from mail-pa0-f66.google.com ([209.85.220.66]:32875 "EHLO mail-pa0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751271AbcEKKaa (ORCPT ); Wed, 11 May 2016 06:30:30 -0400 Subject: Re: [PATCH 1/4] perf config: Introduce default_config_item for all default config key-value pairs To: Arnaldo Carvalho de Melo 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> <5731CABC.20105@gmail.com> <20160510150539.GI13209@kernel.org> Cc: Jiri Olsa , Namhyung Kim , linux-kernel@vger.kernel.org, Ingo Molnar , Peter Zijlstra , Alexander Shishkin , Masami Hiramatsu From: Taeung Song Message-ID: <573309C1.306@gmail.com> Date: Wed, 11 May 2016 19:30:25 +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: <20160510150539.GI13209@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 On 05/11/2016 12:05 AM, Arnaldo Carvalho de Melo wrote: > Em Tue, May 10, 2016 at 08:49:16PM +0900, Taeung Song escreveu: >> 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 >> + > > No hard coded magical maximum values :-\ > I got it. I'll send v2 without the above change. :) Thanks, Taeung >> 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