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

[Taeung Song <treeze.taeung@gmail.com>]

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