Messages in this thread | | | Date | Tue, 10 May 2016 12:05:39 -0300 | From | Arnaldo Carvalho de Melo <> | Subject | Re: [PATCH 1/4] perf config: Introduce default_config_item for all default config key-value pairs |
| |
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 <linux/list.h> > > 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 :-\
> 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 <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 | 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 <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 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
| |