Re: [RFC][PATCH] perf config: Introduce perf_config_set class

[Taeung Song]

On 03/12/2016 05:45 PM, Namhyung Kim wrote:
> Hi Taeung,
>
> On Sat, Mar 12, 2016 at 12:08:52AM +0900, Taeung Song wrote:
> > Hi, Namhyung
> >
> > On 03/11/2016 11:11 PM, Namhyung Kim wrote:
> > > Also I think it'd be better just keeping a single config value instead
> > > of 3 kinds.  Maybe you can read system-wide config first and overwrite
> > > them with user config (for the 'both' case).
> >
> > I know what you mean. I agonized about it.
> >
> > IMHO, I think that if keeping a single config value instead of 3 kinds and
> > perf-config has setting functionality when writing a changed config
> > on a specific config file, some problems can occur e.g.
>
> Do you plan to support 'set' and 'get' operation at the same time?
> IOW is it possible to do?
>
>    $ perf config --set aaa.bbb=xx --get ccc.ddd
>
> I don't think it's very useful.
>
> If we don't do it, I think we can simply read a single config file
> (default to user file) and re-write it for the 'set' operation.

I agree. I think that what you said is a simple way for 'set' operation.
But I have a plan about perf-config interface like 'sysctl'
(suggested by jiri)
http://marc.info/?l=linux-kernel&m=142842999926479&w=2

For examples,

       sysctl [options] [variable[=value]] [...]
       sysctl -p [file or regexp] [...]

# display current config
perf config

# display current config plus all keys with default values
perf config -a

# display key value:
perf config report.queue

# set key value:
perf config report.queue=100M

# remove key (not in sysctl)
perf config -r report.queue


If we do so,

perf-config support 'set' and 'get' operation at the same time e.g

sysctl:

    # sysctl vm.stat_interval vm.stat_interval=2 vm.user_reserve_kbytes
    vm.stat_interval = 1
    vm.stat_interval = 2
    vm.user_reserve_kbytes = 131072

perf-config:

    # perf config report.queue-size report.queue-size=100M top.children
    report.queue-size=1
    report.queue-size=104857600
    top.children=true

jiri, is it right ?
or the above situation wasn't what you mean ? (I understood so)

then, namhyung, is it better to use the simple way for 'set' and 'get' 
operation ?
(instead of 'sysctl' style)

> Or maybe we can add a field (like 'origin'?) in the perf_config_item
> struct to mark where it comes from.  And then it should write items
> matching 'origin' only.

I understood a field 'origin' e.g

struct perf_config_item {
(..omitted..)
    enum config_file {
        USER_CONFIG,
        SYSTEM_CONFIG
    } origin;
}

And if the two files have same variables,
user config file has a high priority. (as I understood)

IMHO, I think that even if we use 'origin',
some problem can occur when handling same variables e.g.

User wide:

    # cat ~/.perfconfig
    [report]
        queue-size = 1

System wide:

    # cat /usr/local/etc/perfconfig
    [report]
        queue-size = 2
    [top]
        children = false

If user or system config files has same variable as above,

    # perf config --system top.children=true

    # perf config --system --list
    top.children=false

the above problem can occur.

When rewriting items matching 'origin',
because a item for 'report.queue-size' has 'origin' marked
as user config file, 'report.queue-size' of system config file
can be removed.

And even though perf_config_item has a field 'origin'
to mark where it comes from,
if perf_config_item has only single value
we can't know old value of 'report.queue-size' for system config file as 
ever
because of overwriting a single value of perf_config_item
when collecting configs from the config files.

Did I misunderstand your intention using a field 'origin' ? :-\

Or
when the two files have same config variables,
are there two items each other (for system and user config file) ?

Thanks,
Taeung
> > (Because setting functionality I design is that overwrite
> > a specific config file by the perf config list)
> > (the perf config list : all perf configs from the config files)
> >
> > User wide:
> >
> >      # cat ~/.perfconfig
> >      [report]
> >          queue-size = 1
> >      [test]
> >          location = user
> >
> > System wide:
> >
> >      # cat /usr/local/etc/perfconfig
> >      [ui]
> >          show-headers = false
> >      [test]
> >          location = system
> >
> > And if perf-config has setting functionality,
> >
> >      # perf config --system top.children=false
> >
> > We hoped for:
> >
> >      # cat /usr/local/etc/perfconfig
> >      [ui]
> >          show-headers = false
> >      [test]
> >          location = system
> >      [top]
> >          children = false
> >
> > But actual result can be:
> >
> >      # cat /usr/local/etc/perfconfig
> >      [ui]
> >          show-headers = false
> >      [report]
> >          queue-size = 1
> >      [test]
> >          location = user
> >      [top]
> >          children = false
> >
> > We wouldn't want that system config file contain contents of
> > user config file.
> > The reason of this problem is that setting functionality I design
> > work with perf config list overwriting a specific config file
> > and if perf config list has only single value each config,
> > we don't exactly know old values of system config.
> >
> > Don't design setting functionality that overwrite by perf config list ?
> > (writing '# this file is auto-generated.' at the top of config file)
> >
> > Add a changed config into a specific config file by other way ? :-\
> >
> > Or
> > Not now, when add setting functionality into perf-config,
> > consider this problem ?
> >
> > Thanks,
> > Taeung