Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758143AbcK2RYG (ORCPT ); Tue, 29 Nov 2016 12:24:06 -0500 Received: from mail-pg0-f66.google.com ([74.125.83.66]:34361 "EHLO mail-pg0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933481AbcK2RWt (ORCPT ); Tue, 29 Nov 2016 12:22:49 -0500 Subject: Re: [PATCH v9 1/7] perf config: Introduce default_config_section and default_config_item for default config key-value pairs To: Arnaldo Carvalho de Melo References: <1480323078-15623-1-git-send-email-treeze.taeung@gmail.com> <1480323078-15623-2-git-send-email-treeze.taeung@gmail.com> <20161129160538.GB23423@kernel.org> Cc: linux-kernel@vger.kernel.org, Jiri Olsa , Namhyung Kim , Ingo Molnar , Peter Zijlstra , Wang Nan , Jiri Olsa , Ingo Molnar , Masami Hiramatsu , David Ahern From: Taeung Song Message-ID: <9adee2dc-542c-bcfd-19f6-eee05dcb280d@gmail.com> Date: Wed, 30 Nov 2016 02:21:56 +0900 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <20161129160538.GB23423@kernel.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3675 Lines: 117 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 >> Cc: Jiri Olsa >> Cc: Wang Nan >> Cc: Peter Zijlstra >> Cc: Ingo Molnar >> Cc: Masami Hiramatsu >> Cc: David Ahern >> Signed-off-by: Taeung Song >> --- >> 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(§ion->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