Re: [PATCH v8 3/4] perf config: Prepare all default configs

From: Namhyung Kim
Date: Wed Apr 20 2016 - 08:45:41 EST

Next message: Robin Murphy: "Re: [PATCH v7 01/10] iommu: Add DOMAIN_ATTR_MSI_MAPPING attribute"
Previous message: Laszlo Ersek: "Re: [PATCH] lib: Always NUL terminate ucs2_as_utf8"
In reply to: Taeung Song: "Re: [PATCH v8 3/4] perf config: Prepare all default configs"
Next in thread: Arnaldo Carvalho de Melo: "Re: [PATCH v8 3/4] perf config: Prepare all default configs"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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 <namhyung@xxxxxxxxxx>
> > > > Cc: Masami Hiramatsu <mhiramat@xxxxxxxxxx>
> > > > Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
> > > > Signed-off-by: Taeung Song <treeze.taeung@xxxxxxxxx>
> > > > ---
> > > > 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(&section->name);
> > > > + zfree((char **)&section->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 <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 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

Next message: Robin Murphy: "Re: [PATCH v7 01/10] iommu: Add DOMAIN_ATTR_MSI_MAPPING attribute"
Previous message: Laszlo Ersek: "Re: [PATCH] lib: Always NUL terminate ucs2_as_utf8"
In reply to: Taeung Song: "Re: [PATCH v8 3/4] perf config: Prepare all default configs"
Next in thread: Arnaldo Carvalho de Melo: "Re: [PATCH v8 3/4] perf config: Prepare all default configs"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]