From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753285AbcGLGD7 (ORCPT ); Tue, 12 Jul 2016 02:03:59 -0400 Received: from mail-pf0-f194.google.com ([209.85.192.194]:36483 "EHLO mail-pf0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751463AbcGLGD5 (ORCPT ); Tue, 12 Jul 2016 02:03:57 -0400 Subject: Re: [RFC PATCH v5 0/7] perf config: Introduce default config key-value pairs arrays To: Namhyung Kim References: <1467782423-29035-1-git-send-email-treeze.taeung@gmail.com> <57847013.5020300@gmail.com> <20160712053738.GA20077@danjae.aot.lge.com> Cc: Arnaldo Carvalho de Melo , linux-kernel@vger.kernel.org, Jiri Olsa , Ingo Molnar , Peter Zijlstra , Alexander Shishkin , Masami Hiramatsu , Wang Nan From: Taeung Song Message-ID: <57848848.1050003@gmail.com> Date: Tue, 12 Jul 2016 15:03:52 +0900 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.8.0 MIME-Version: 1.0 In-Reply-To: <20160712053738.GA20077@danjae.aot.lge.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 07/12/2016 02:37 PM, Namhyung Kim wrote: > Hi Taeung, > > On Tue, Jul 12, 2016 at 01:20:35PM +0900, Taeung Song wrote: >> Hi, Namhyung :) >> >> As you know, the patch work from v3 to v4 took too long time >> because of a patchset refactoring perf_config(). >> >> So I guess it is hard for you to recall this patchset for new default >> config arrays but could you a bit response two simple questions ? >> >> >> 1) I renamed 'fb_ground' to 'fore_back_colors' in struct ui_browser_colorset >> at ui/browser.c. >> >> Is it better than before ? > > Not sure. I don't think the renaming is necessary. If you do, I > prefer simpler name like 'colors' or 'color_pair'. > I got it! >> >> 2) I added not only 'struct default_config_item' but also >> 'struct default_config_section'. >> (In order to use const type and 'const struct default_config_item *items' >> instead of using 'struct list_head items') >> >> could you agree this way ? > > I'm ok with it. > Thanks, Taeung > > >> >> On 07/06/2016 02:20 PM, Taeung Song wrote: >>> Hello, :) >>> >>> When initializing default perf config values, >>> we currently use values of actual type(int, bool, char *, etc.). >>> But I suggest using default config key-value pairs arrays. >>> >>> For example, >>> If there isn't user config value at ~/.perfconfig for 'annotate.use_offset' config variable, >>> 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 if we use new config arrays that have all default config key-value pairs, >>> we could initialize default config values with them. >>> >>> 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. >>> >>> For example, >>> If we use new default config arrays and there isn't user config value for 'annoate.use_offset' >>> default value for it will be set as annotate_config_items[CONFIG_ANNOATE_USE_OFFSET].value >>> instead of actual boolean type value 'true'. >>> >>> IMHO, I think it would needed to use new default config arrays >>> to manage default perf config values more effectively. >>> And this pathset contains patchs for only 'colors' and 'annoate' section >>> because waiting for other opinions. >>> >>> If you review this patchset, I'd appreciate it :-) >>> >>> Thanks, >>> Taeung >>> >>> v5: >>> - rebased on current acme/perf/core >>> >>> v4: >>> - rename 'fb_ground' to 'fore_back_colors' (Namhyung) >>> - add struct default_config_section >>> - split first patch[PATCH 1/7] as two >>> - remove perf_default_config_init() at perf.c >>> - rebased on current acme/perf/core >>> >>> v3: >>> - remove default config arrays for the rest sections except 'colors' and 'annotate' >>> - use combined {fore, back}ground colors instead of each two color >>> - introduce perf_default_config_init() that call all default_*_config_init() >>> for each config section >>> >>> v2: >>> - rename 'ui_browser__config_gcolors' to 'ui_browser__config_colors' (Arnaldo) >>> - change 'ground colors' to '{back, fore}ground colors' (Arnaldo) >>> - use strtok + ltrim instead of strchr and while (isspace(*++bg)); (Arnaldo) >>> >>> Taeung Song (7): >>> perf config: Introduce default_config_section and default_config_item >>> for default config key-value pairs >>> perf config: Add macros assigning key-value pairs to >>> default_config_item >>> perf config: Add 'colors' section default configs arrrays >>> perf config: Use combined {fore,back}ground colors value instead of >>> each two color >>> perf config: Initialize ui_browser__colorsets with default config >>> items >>> perf config: Add 'annotate' section default configs arrrays >>> perf config: Initialize annotate_browser__opts with default config >>> items >>> >>> tools/perf/ui/browser.c | 64 +++++++++++++++++------------- >>> tools/perf/ui/browsers/annotate.c | 16 ++++++-- >>> tools/perf/util/config.c | 26 +++++++++++++ >>> tools/perf/util/config.h | 82 +++++++++++++++++++++++++++++++++++++++ >>> 4 files changed, 156 insertions(+), 32 deletions(-) >>>