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

Groups > linux.kernel > #1441062 > unrolled thread

Re: [RFC PATCH v5 0/7] perf config: Introduce default config key-value pairs arrays

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: [RFC PATCH v5 0/7] perf config: Introduce default config  key-value pairs arrays Taeung Song <treeze.taeung@gmail.com> - 2016-07-12 06:30 +0200
    Re: [RFC PATCH v5 0/7] perf config: Introduce default config  key-value pairs arrays Namhyung Kim <namhyung@kernel.org> - 2016-07-12 07:40 +0200
      Re: [RFC PATCH v5 0/7] perf config: Introduce default config  key-value pairs arrays Taeung Song <treeze.taeung@gmail.com> - 2016-07-12 08:10 +0200

#1441062 — Re: [RFC PATCH v5 0/7] perf config: Introduce default config key-value pairs arrays

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 ?

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 ?


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

[toc] | [next] | [standalone]

#1441073

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

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

#1441079

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web