Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1381786 > unrolled thread

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

Started byTaeung Song <treeze.taeung@gmail.com>
First post2016-04-18 17:00 +0200
Last post2016-04-20 17:20 +0200
Articles 5 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v8 3/4] perf config: Prepare all default configs Taeung Song <treeze.taeung@gmail.com> - 2016-04-18 17:00 +0200
    Re: [PATCH v8 3/4] perf config: Prepare all default configs Namhyung Kim <namhyung@kernel.org> - 2016-04-20 14:50 +0200
      Re: [PATCH v8 3/4] perf config: Prepare all default configs Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-04-20 15:30 +0200
        Re: [PATCH v8 3/4] perf config: Prepare all default configs Taeung Song <treeze.taeung@gmail.com> - 2016-04-20 17:30 +0200
      Re: [PATCH v8 3/4] perf config: Prepare all default configs Taeung Song <treeze.taeung@gmail.com> - 2016-04-20 17:20 +0200

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

FromTaeung Song <treeze.taeung@gmail.com>
Date2016-04-18 17:00 +0200
SubjectRe: [PATCH v8 3/4] perf config: Prepare all default configs
Message-ID<rpiOC-7ii-29@gated-at.bofh.it>
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@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 | 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 ?

We don't need to add this arbitrary maximum ?
or would you mind, if I look for other way about 'default_config_item' ?

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

[toc] | [next] | [standalone]

#1383347

FromNamhyung Kim <namhyung@kernel.org>
Date2016-04-20 14:50 +0200
Message-ID<rpZJU-7YC-27@gated-at.bofh.it>
In reply to#1381786
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@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 | 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

[toc] | [prev] | [next] | [standalone]

#1383384

FromArnaldo Carvalho de Melo <acme@kernel.org>
Date2016-04-20 15:30 +0200
Message-ID<rq0mC-5D-13@gated-at.bofh.it>
In reply to#1383347
Em Wed, Apr 20, 2016 at 09:44:38PM +0900, Namhyung Kim escreveu:
> On Mon, Apr 18, 2016 at 11:55:18PM +0900, Taeung Song wrote:
> > On 04/15/2016 01:42 AM, Taeung Song wrote:
> > > 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:
> > > > > +++ b/tools/perf/util/config.c
> > > > > @@ -15,6 +15,7 @@
> > > > > +#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 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?

Yeah, I guess this should work, no? At least this is how it is done
elsewhere, see:

tools/perf/builtin-bench.c, 'struct collection' has a benchmarks array,
that in turn is organized as Namhyung suggests.

Then you either use ARRAY_SIZE() somewhere to get the number of entries
or use a sentinel, i.e. use NULL for the last entry.

- Arnaldo
 
> 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

[toc] | [prev] | [next] | [standalone]

#1383495

FromTaeung Song <treeze.taeung@gmail.com>
Date2016-04-20 17:30 +0200
Message-ID<rq2eK-1vN-17@gated-at.bofh.it>
In reply to#1383384
Hi, Arnaldo :-)

On 04/20/2016 10:22 PM, Arnaldo Carvalho de Melo wrote:
> Em Wed, Apr 20, 2016 at 09:44:38PM +0900, Namhyung Kim escreveu:
>> On Mon, Apr 18, 2016 at 11:55:18PM +0900, Taeung Song wrote:
>>> On 04/15/2016 01:42 AM, Taeung Song wrote:
>>>> 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:
>>>>>> +++ b/tools/perf/util/config.c
>>>>>> @@ -15,6 +15,7 @@
>>>>>> +#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 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?
>
> Yeah, I guess this should work, no? At least this is how it is done
> elsewhere, see:
>
> tools/perf/builtin-bench.c, 'struct collection' has a benchmarks array,
> that in turn is organized as Namhyung suggests.

I got it!

> Then you either use ARRAY_SIZE() somewhere to get the number of entries
> or use a sentinel, i.e. use NULL for the last entry.
>

I think that it would better to use ARRAY_SIZE() than to use NULL.
It makes no odds but it seem to be clean, IMHO. :)

After changing this patchset, I'll send v9 !

Thanks,
Taeung

>
>> 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

[toc] | [prev] | [next] | [standalone]

#1383489

FromTaeung Song <treeze.taeung@gmail.com>
Date2016-04-20 17:20 +0200
Message-ID<rq254-1rF-19@gated-at.bofh.it>
In reply to#1383347
Hi, Namhyung

On 04/20/2016 09:44 PM, Namhyung Kim wrote:
> 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@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 | 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.
>

OK, I got it.
I was hesitating a bit but I'll change this patch by this way you told. :-)

Thanks,
Taeung

>
>
>>>>> +    [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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web