Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933135AbcDTMpi (ORCPT ); Wed, 20 Apr 2016 08:45:38 -0400 Received: from mail-pf0-f195.google.com ([209.85.192.195]:34819 "EHLO mail-pf0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754819AbcDTMpK (ORCPT ); Wed, 20 Apr 2016 08:45:10 -0400 Date: Wed, 20 Apr 2016 21:44:38 +0900 From: Namhyung Kim To: Taeung Song Cc: Arnaldo Carvalho de Melo , linux-kernel@vger.kernel.org, Jiri Olsa , Ingo Molnar , Peter Zijlstra , Masami Hiramatsu Subject: Re: [PATCH v8 3/4] perf config: Prepare all default configs Message-ID: <20160420124438.GA29186@danjae.aot.lge.com> References: <1460620401-23430-1-git-send-email-treeze.taeung@gmail.com> <1460620401-23430-4-git-send-email-treeze.taeung@gmail.com> <20160414121916.GJ9056@kernel.org> <570FC86D.5060909@gmail.com> <5714F556.1000308@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <5714F556.1000308@gmail.com> User-Agent: Mutt/1.6.0 (2016-04-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 11667 Lines: 344 Hi Taeung, On Mon, Apr 18, 2016 at 11:55:18PM +0900, Taeung Song wrote: > Hi, Namhyung > > On 04/15/2016 01:42 AM, Taeung Song wrote: > > Hi, Arnaldo > > > > On 04/14/2016 09:19 PM, Arnaldo Carvalho de Melo wrote: > > > Em Thu, Apr 14, 2016 at 04:53:20PM +0900, Taeung Song escreveu: > > > > 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. > > > > > > > > Acked-by: Namhyung Kim > > > > Cc: Masami Hiramatsu > > > > Cc: Jiri Olsa > > > > Signed-off-by: Taeung Song > > > > --- > > > > tools/perf/util/config.c | 110 > > > > ++++++++++++++++++++++++++++++++++++++++++++++- > > > > tools/perf/util/config.h | 62 +++++++++++++++++++++++++- > > > > 2 files changed, 168 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c > > > > index dad7d82..5604392 100644 > > > > --- a/tools/perf/util/config.c > > > > +++ b/tools/perf/util/config.c > > > > @@ -15,6 +15,7 @@ > > > > #include "util/llvm-utils.h" /* perf_llvm_config */ > > > > #include "config.h" > > > > > > > > +#define MAX_CONFIGS 64 > > > > > > Do we have to add another arbitrary maximum? Where is it used? > > > > > > > IMHO, it is my idea. If you want to avoid using this arbitrary maxinum, > > I'd modify the code. > > > > MAX_CONFIGS is used in order to declare two-dimensional arrays > > 'default_config_items' > > as below. > > > > struct perf_config_item default_config_items[][MAX_CONFIGS] = { > > [CONFIG_COLORS] = { > > CONF_STR_VAR("top", "red, default"), > > CONF_STR_VAR("medium", "green, default"), > > ...(omitted)... > > > > IMHO, it is because of perf_config_set__init() in [PATCH v8 4/4]. > > If we don't use two-dimensional arrays, I think the code of > > perf_config_set__init() could be complicated. > > > > As above, I used MAX_CONFIGS because of two-dimensinal arrays > 'default_config_items'. > > What do you think about it ? I also agree that we'd better to avoid the arbitrary maximum. > > We don't need to add this arbitrary maximum ? > or would you mind, if I look for other way about > 'default_config_item' ? What about this? struct perf_config_item color_config_items[] = { CONF_STR_VAR("top", "red, default"), CONF_STR_VAR("medium", "green, default"), ... }; struct perf_config_item tui_config_items[] = { CONF_BOOL_VAR("report", true), CONF_BOOL_VAR("annotate", true), ... }; struct perf_config_item *default_config_items[] = { &color_config_items, &tui_config_items, ... }; This way we can access the config array by using constant index without the hard-coded maximum size IMHO. Thanks, Namhyung > > > > + [CONFIG_COLORS] = { > > > > + CONF_STR_VAR("top", "red, default"), > > > > + CONF_STR_VAR("medium", "green, default"), > > > > + CONF_STR_VAR("normal", "lightgray, default"), > > > > + CONF_STR_VAR("selected", "white, lightgray"), > > > > + CONF_STR_VAR("jump_arrows", "blue, default"), > > > > + CONF_STR_VAR("addr", "magenta, default"), > > > > + CONF_STR_VAR("root", "white, blue"), > > > > + CONF_END() > > > > + }, > > > > + [CONFIG_TUI] = { > > > > + CONF_BOOL_VAR("report", true), > > > > + CONF_BOOL_VAR("annotate", true), > > > > + CONF_BOOL_VAR("top", true), > > > > + CONF_END() > > > > + }, > > Thanks, > Taeung > > > > > #define MAXNAME (256) > > > > > > > > #define DEBUG_CACHE_DIR ".debug" > > > > @@ -29,6 +30,111 @@ 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" } > > > > +}; > > > > + > > > > +struct perf_config_item default_config_items[][MAX_CONFIGS] = { > > > > + [CONFIG_COLORS] = { > > > > + CONF_STR_VAR("top", "red, default"), > > > > + CONF_STR_VAR("medium", "green, default"), > > > > + CONF_STR_VAR("normal", "lightgray, default"), > > > > + CONF_STR_VAR("selected", "white, lightgray"), > > > > + CONF_STR_VAR("jump_arrows", "blue, default"), > > > > + CONF_STR_VAR("addr", "magenta, default"), > > > > + CONF_STR_VAR("root", "white, blue"), > > > > + CONF_END() > > > > + }, > > > > + [CONFIG_TUI] = { > > > > + CONF_BOOL_VAR("report", true), > > > > + CONF_BOOL_VAR("annotate", true), > > > > + CONF_BOOL_VAR("top", true), > > > > + CONF_END() > > > > + }, > > > > + [CONFIG_BUILDID] = { > > > > + CONF_STR_VAR("dir", "~/.debug"), > > > > + CONF_END() > > > > + }, > > > > + [CONFIG_ANNOTATE] = { > > > > + 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() > > > > + }, > > > > + [CONFIG_GTK] = { > > > > + CONF_BOOL_VAR("annotate", false), > > > > + CONF_BOOL_VAR("report", false), > > > > + CONF_BOOL_VAR("top", false), > > > > + CONF_END() > > > > + }, > > > > + [CONFIG_PAGER] = { > > > > + 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() > > > > + }, > > > > + [CONFIG_HELP] = { > > > > + CONF_STR_VAR("format", "man"), > > > > + CONF_INT_VAR("autocorrect", 0), > > > > + CONF_END() > > > > + }, > > > > + [CONFIG_HIST] = { > > > > + CONF_STR_VAR("percentage", "absolute"), > > > > + CONF_END() > > > > + }, > > > > + [CONFIG_UI] = { > > > > + CONF_BOOL_VAR("show-headers", true), > > > > + CONF_END() > > > > + }, > > > > + [CONFIG_CALL_GRAPH] = { > > > > + 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() > > > > + }, > > > > + [CONFIG_REPORT] = { > > > > + CONF_BOOL_VAR("group", true), > > > > + CONF_BOOL_VAR("children", true), > > > > + CONF_FLOAT_VAR("percent-limit", 0), > > > > + CONF_U64_VAR("queue-size", 0), > > > > + CONF_END() > > > > + }, > > > > + [CONFIG_TOP] = { > > > > + CONF_BOOL_VAR("children", true), > > > > + CONF_END() > > > > + }, > > > > + [CONFIG_MAN] = { > > > > + CONF_STR_VAR("viewer", "man"), > > > > + CONF_END() > > > > + }, > > > > + [CONFIG_KMEM] = { > > > > + CONF_STR_VAR("default", "slab"), > > > > + CONF_END() > > > > + } > > > > +}; > > > > + > > > > static int get_next_char(void) > > > > { > > > > int c; > > > > @@ -659,7 +765,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 +783,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..84dcc1d 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; > > > > struct list_head node; > > > > }; > > > > > > > > struct perf_config_section { > > > > - char *name; > > > > + const char *name; > > > > struct list_head items; > > > > struct list_head node; > > > > }; > > > > @@ -20,6 +40,44 @@ struct perf_config_set { > > > > struct list_head sections; > > > > }; > > > > > > > > +enum perf_config_secion_idx { > > > > + CONFIG_COLORS, > > > > + CONFIG_TUI, > > > > + CONFIG_BUILDID, > > > > + CONFIG_ANNOTATE, > > > > + CONFIG_GTK, > > > > + CONFIG_PAGER, > > > > + CONFIG_HELP, > > > > + CONFIG_HIST, > > > > + CONFIG_UI, > > > > + CONFIG_CALL_GRAPH, > > > > + CONFIG_REPORT, > > > > + CONFIG_TOP, > > > > + CONFIG_MAN, > > > > + CONFIG_KMEM, > > > > + CONFIG_END > > > > +}; > > > > + > > > > +#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); > > > > > > > > -- > > > > 2.5.0