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

Groups > linux.kernel > #1531133 > unrolled thread

[PATCH v9 1/7] perf config: Introduce default_config_section and default_config_item for default config key-value pairs

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

  [PATCH v9 1/7] perf config: Introduce default_config_section and default_config_item for default config key-value pairs Taeung Song <treeze.taeung@gmail.com> - 2016-11-28 10:00 +0100
    Re: [PATCH v9 1/7] perf config: Introduce default_config_section and  default_config_item for default config key-value pairs Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-11-29 17:10 +0100
      Re: [PATCH v9 1/7] perf config: Introduce default_config_section and  default_config_item for default config key-value pairs Taeung Song <treeze.taeung@gmail.com> - 2016-11-29 18:30 +0100

#1531133 — [PATCH v9 1/7] perf config: Introduce default_config_section and default_config_item for default config key-value pairs

When initializing default perf config values,
we currently use values of actual type(int, bool, char *, etc.).

For example,
If there isn't a user config value for 'annotate.use_offset'
config variable at ~/.perfconfig,
default value for it is 'true' bool type value in perf like below.

At ui/browsers/annoate.c

static struct annotate_browser_opt {
        bool hide_src_code,
             use_offset,
             jump_arrows,
             show_linenr,
             show_nr_jumps,
             show_total_period;
} annotate_browser__opts = {
        .use_offset      = true,
        .jump_arrows     = true,
};

But I suggest using 'struct default_config_section' and
'struct default_config_item' that can contain default config key-value pairs
in order to initialize default config values with them, in near future.

If we do, we could manage default perf config values at one spot (i.e. util/config.c)
with default config arrays and it could be easy and simple to modify
existing default config values or add default values for new config item.

Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Wang Nan <wangnan0@huawei.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: David Ahern <dsahern@gmail.com>
Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
---
 tools/perf/util/config.h | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
index 1a59a6b..434d71c 100644
--- a/tools/perf/util/config.h
+++ b/tools/perf/util/config.h
@@ -67,4 +67,33 @@ void perf_config__refresh(void);
 	perf_config_sections__for_each_entry(&set->sections, section)		\
 	perf_config_items__for_each_entry(&section->items, item)
 
+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 default_config_section {
+	const char *name;
+	const struct default_config_item *items;
+};
+
 #endif /* __PERF_CONFIG_H */
-- 
2.7.4

[toc] | [next] | [standalone]

#1532457 — Re: [PATCH v9 1/7] perf config: Introduce default_config_section and default_config_item for default config key-value pairs

Em Mon, Nov 28, 2016 at 05:51:12PM +0900, Taeung Song escreveu:
> When initializing default perf config values,
> we currently use values of actual type(int, bool, char *, etc.).
> 
> For example,
> If there isn't a user config value for 'annotate.use_offset'
> config variable at ~/.perfconfig,
> default value for it is 'true' bool type value in perf like below.
> 
> At ui/browsers/annoate.c
> 
> static struct annotate_browser_opt {
>         bool hide_src_code,
>              use_offset,
>              jump_arrows,
>              show_linenr,
>              show_nr_jumps,
>              show_total_period;
> } annotate_browser__opts = {
>         .use_offset      = true,
>         .jump_arrows     = true,
> };

And isn't this a good thing? I.e. its compact, we have just to set
values to things that are !0 (or !false), and it is close to the code
where it is used.
 
> But I suggest using 'struct default_config_section' and
> 'struct default_config_item' that can contain default config key-value pairs
> in order to initialize default config values with them, in near future.

 
> If we do, we could manage default perf config values at one spot (i.e. util/config.c)
> with default config arrays and it could be easy and simple to modify
> existing default config values or add default values for new config item.
> 
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Wang Nan <wangnan0@huawei.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: David Ahern <dsahern@gmail.com>
> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
> ---
>  tools/perf/util/config.h | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
> index 1a59a6b..434d71c 100644
> --- a/tools/perf/util/config.h
> +++ b/tools/perf/util/config.h
> @@ -67,4 +67,33 @@ void perf_config__refresh(void);
>  	perf_config_sections__for_each_entry(&set->sections, section)		\
>  	perf_config_items__for_each_entry(&section->items, item)
>  
> +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 default_config_section {
> +	const char *name;
> +	const struct default_config_item *items;
> +};
> +
>  #endif /* __PERF_CONFIG_H */
> -- 
> 2.7.4

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

#1532584 — Re: [PATCH v9 1/7] perf config: Introduce default_config_section and default_config_item for default config key-value pairs

Hi, Arnaldo :)

On 11/30/2016 01:05 AM, Arnaldo Carvalho de Melo wrote:
> Em Mon, Nov 28, 2016 at 05:51:12PM +0900, Taeung Song escreveu:
>> When initializing default perf config values,
>> we currently use values of actual type(int, bool, char *, etc.).
>>
>> For example,
>> If there isn't a user config value for 'annotate.use_offset'
>> config variable at ~/.perfconfig,
>> default value for it is 'true' bool type value in perf like below.
>>
>> At ui/browsers/annoate.c
>>
>> static struct annotate_browser_opt {
>>         bool hide_src_code,
>>              use_offset,
>>              jump_arrows,
>>              show_linenr,
>>              show_nr_jumps,
>>              show_total_period;
>> } annotate_browser__opts = {
>>         .use_offset      = true,
>>         .jump_arrows     = true,
>> };
>
> And isn't this a good thing? I.e. its compact, we have just to set
> values to things that are !0 (or !false), and it is close to the code
> where it is used.

I understood it. But I think we also need to manage all default config
values in only util/config.c.

The reason is that if we individually have default config arrays,
some difference can be made between actual config values and
values of default config arrays. (e.g. if we change default config
values or add new default config items and values)

So IMHO, I suggest that we use default config arrays instead of
actual type values even though its compact.

And if we have default config arrays, we can also show default values as 
below.

     [acme@jouet linux]$ perf config report.queue-size report.children
     report.children=false
     # report.queue-size=18446744073709551615 # Default, not set in 
~/.perfconfig
     [acme@jouet linux]$


Just individually make default config arrays in util/config.c ?

Thanks,
Taeung

>> But I suggest using 'struct default_config_section' and
>> 'struct default_config_item' that can contain default config key-value pairs
>> in order to initialize default config values with them, in near future.
>
>
>> If we do, we could manage default perf config values at one spot (i.e. util/config.c)
>> with default config arrays and it could be easy and simple to modify
>> existing default config values or add default values for new config item.
>>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Cc: Jiri Olsa <jolsa@redhat.com>
>> Cc: Wang Nan <wangnan0@huawei.com>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: Masami Hiramatsu <mhiramat@kernel.org>
>> Cc: David Ahern <dsahern@gmail.com>
>> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
>> ---
>>  tools/perf/util/config.h | 29 +++++++++++++++++++++++++++++
>>  1 file changed, 29 insertions(+)
>>
>> diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
>> index 1a59a6b..434d71c 100644
>> --- a/tools/perf/util/config.h
>> +++ b/tools/perf/util/config.h
>> @@ -67,4 +67,33 @@ void perf_config__refresh(void);
>>  	perf_config_sections__for_each_entry(&set->sections, section)		\
>>  	perf_config_items__for_each_entry(&section->items, item)
>>
>> +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 default_config_section {
>> +	const char *name;
>> +	const struct default_config_item *items;
>> +};
>> +
>>  #endif /* __PERF_CONFIG_H */
>> --
>> 2.7.4

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web