From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933119AbcGLFig (ORCPT ); Tue, 12 Jul 2016 01:38:36 -0400 Received: from mail-pa0-f67.google.com ([209.85.220.67]:36345 "EHLO mail-pa0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750743AbcGLFif (ORCPT ); Tue, 12 Jul 2016 01:38:35 -0400 Date: Tue, 12 Jul 2016 14:37:38 +0900 From: Namhyung Kim To: Taeung Song Cc: Arnaldo Carvalho de Melo , linux-kernel@vger.kernel.org, Jiri Olsa , Ingo Molnar , Peter Zijlstra , Alexander Shishkin , Masami Hiramatsu , Wang Nan Subject: Re: [RFC PATCH v5 0/7] perf config: Introduce default config key-value pairs arrays Message-ID: <20160712053738.GA20077@danjae.aot.lge.com> References: <1467782423-29035-1-git-send-email-treeze.taeung@gmail.com> <57847013.5020300@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <57847013.5020300@gmail.com> User-Agent: Mutt/1.6.0 (2016-04-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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'. > > 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, Namhyung > > 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(-) > >