Messages in this thread | | | Subject | Re: [PATCH v10 2/3] perf config: Reimplement perf_config() introducing new perf_config_init() and perf_config_finish() | From | Taeung Song <> | Date | Thu, 23 Jun 2016 21:41:43 +0900 |
| |
Hi, Arnaldo :)
On 06/23/2016 08:56 PM, Arnaldo Carvalho de Melo wrote: > Em Thu, Jun 23, 2016 at 05:55:18PM +0900, Taeung Song escreveu: >> Many sub-commands use perf_config() but >> everytime perf_config() is called, perf_config() always read config files. >> (i.e. user config '~/.perfconfig' and system config '$(sysconfdir)/perfconfig') >> >> But it is better to use the config set that already contains all config >> key-value pairs to avoid this repetitive work reading the config files >> in perf_config(). (the config set mean a static variable 'config_set') >> >> In other words, if new perf_config_init() is called, >> only first time 'config_set' is initialized collecting all configs from the config files. >> And then we could use new perf_config() like old perf_config(). >> When a sub-command finished, free the config set by perf_config_finish() at run_builtin(). >> >> If we do, 'config_set' can be reused wherever perf_config() is called >> and a feature of old perf_config() is the same as new perf_config() work >> without the repetitive work that read the config files. >> >> In summary, in order to use features about configuration, >> we can call the functions at perf.c and other source files as below. >> >> # initialize a config set >> perf_config_init() >> >> # configure actual variables from a config set >> perf_config() >> >> # eliminate allocated config set >> perf_config_finish() >> >> # destroy existing config set and initialize a new config set. >> perf_config_refresh() >> >> Cc: Namhyung Kim <namhyung@kernel.org> >> Cc: Jiri Olsa <jolsa@redhat.com> >> Cc: Wang Nan <wangnan0@huawei.com> >> Cc: Peter Zijlstra <peterz@infradead.org> >> Cc: Ingo Molnar <mingo@redhat.com> >> Cc: Masami Hiramatsu <mhiramat@kernel.org> >> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> >> Signed-off-by: Taeung Song <treeze.taeung@gmail.com> >> --- >> tools/perf/builtin-config.c | 4 ++ >> tools/perf/perf.c | 2 + >> tools/perf/util/config.c | 92 +++++++++++++++++++++++---------------------- >> tools/perf/util/config.h | 29 ++++++++++++++ >> 4 files changed, 82 insertions(+), 45 deletions(-) >> >> diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c >> index fe1b77f..cfd1036 100644 >> --- a/tools/perf/builtin-config.c >> +++ b/tools/perf/builtin-config.c >> @@ -80,6 +80,10 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) >> else if (use_user_config) >> config_exclusive_filename = user_config; >> >> + /* >> + * At only 'config' sub-command, individually use the config set >> + * because of reinitializing with options config file location. >> + */ >> set = perf_config_set__new(); >> if (!set) { >> ret = -1; >> diff --git a/tools/perf/perf.c b/tools/perf/perf.c >> index 66772da..280967e 100644 >> --- a/tools/perf/perf.c >> +++ b/tools/perf/perf.c >> @@ -355,6 +355,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv) >> >> perf_env__set_cmdline(&perf_env, argc, argv); >> status = p->fn(argc, argv, prefix); >> + perf_config_finish(); >> exit_browser(status); >> perf_env__exit(&perf_env); >> bpf__clear(); >> @@ -522,6 +523,7 @@ int main(int argc, const char **argv) >> >> srandom(time(NULL)); >> >> + perf_config_init(); >> perf_config(perf_default_config, NULL); >> set_buildid_dir(NULL); >> >> diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c >> index d15c592..a16f95d 100644 >> --- a/tools/perf/util/config.c >> +++ b/tools/perf/util/config.c >> @@ -26,6 +26,7 @@ static FILE *config_file; >> static const char *config_file_name; >> static int config_linenr; >> static int config_file_eof; >> +static struct perf_config_set *config_set; >> >> const char *config_exclusive_filename; >> >> @@ -478,51 +479,6 @@ static int perf_config_global(void) >> return !perf_env_bool("PERF_CONFIG_NOGLOBAL", 0); >> } >> >> -int perf_config(config_fn_t fn, void *data) >> -{ >> - int ret = -1; >> - const char *home = NULL; >> - >> - /* Setting $PERF_CONFIG makes perf read _only_ the given config file. */ >> - if (config_exclusive_filename) >> - return perf_config_from_file(fn, config_exclusive_filename, data); >> - if (perf_config_system() && !access(perf_etc_perfconfig(), R_OK)) { >> - if (perf_config_from_file(fn, perf_etc_perfconfig(), data) < 0) >> - goto out; >> - } >> - >> - home = getenv("HOME"); >> - if (perf_config_global() && home) { >> - char *user_config = strdup(mkpath("%s/.perfconfig", home)); >> - struct stat st; >> - >> - if (user_config == NULL) { >> - warning("Not enough memory to process %s/.perfconfig, " >> - "ignoring it.", home); >> - goto out; >> - } >> - >> - if (stat(user_config, &st) < 0) >> - goto out_free; >> - >> - if (st.st_uid && (st.st_uid != geteuid())) { >> - warning("File %s not owned by current user or root, " >> - "ignoring it.", user_config); >> - goto out_free; >> - } >> - >> - if (!st.st_size) >> - goto out_free; >> - >> - ret = perf_config_from_file(fn, user_config, data); >> - >> -out_free: >> - free(user_config); >> - } >> -out: >> - return ret; >> -} >> - >> static struct perf_config_section *find_section(struct list_head *sections, >> const char *section_name) >> { >> @@ -706,6 +662,52 @@ struct perf_config_set *perf_config_set__new(void) >> return set; >> } >> >> +int perf_config(config_fn_t fn, void *data) >> +{ >> + int ret = 0; >> + char key[BUFSIZ]; >> + struct perf_config_section *section; >> + struct perf_config_item *item; >> + >> + if (config_set == NULL) >> + return -1; >> + >> + config_set__for_each(config_set, section, item) { >> + char *value = item->value; >> + >> + if (value) { >> + scnprintf(key, sizeof(key), "%s.%s", >> + section->name, item->name); >> + ret = fn(key, value, data); >> + if (ret < 0) { >> + pr_err("Error: wrong config key-value pair %s=%s\n", >> + key, value); >> + break; >> + } >> + } >> + } >> + >> + return ret; >> +} >> + >> +void perf_config_init(void) >> +{ >> + if (config_set == NULL) >> + config_set = perf_config_set__new(); >> +} >> + >> +void perf_config_finish(void) >> +{ >> + perf_config_set__delete(config_set); >> + config_set = NULL; >> +} >> + >> +void perf_config_refresh(void) >> +{ >> + perf_config_finish(); >> + perf_config_init(); >> +} >> + >> static void perf_config_item__delete(struct perf_config_item *item) >> { >> zfree(&item->name); >> diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h >> index 155a441..746c619 100644 >> --- a/tools/perf/util/config.h >> +++ b/tools/perf/util/config.h >> @@ -33,5 +33,34 @@ const char *perf_etc_perfconfig(void); >> >> struct perf_config_set *perf_config_set__new(void); >> void perf_config_set__delete(struct perf_config_set *set); >> +void perf_config_init(void); >> +void perf_config_finish(void); >> +void perf_config_refresh(void); > > Please use double _ to separate the subsystem/class from its methods, > i.e.: > > void perf_config__init(void); > void perf_config__finish(void); > void perf_config__refresh(void);
I got it :)
>> + >> +/** >> + * config_sections__for_each - iterate thru all the sections >> + * @list: list_head instance to iterate >> + * @section: struct perf_config_section iterator >> + */ >> +#define config_sections__for_each(list, section) \ >> + list_for_each_entry(section, list, node) > > These macros operate on perf_config_sections, so please name it > accordingly, i.e.: > perf_config_sections__for_each() > >> + >> +/** >> + * config_items__for_each - iterate thru all the items >> + * @list: list_head instance to iterate >> + * @item: struct perf_config_item iterator >> + */ >> +#define config_items__for_each(list, item) \ >> + list_for_each_entry(item, list, node) >> + > > Ditto > >> +/** >> + * config_set__for_each - iterate thru all the config section-item pairs >> + * @set: evlist instance to iterate >> + * @section: struct perf_config_section iterator >> + * @item: struct perf_config_item iterator >> + */ >> +#define config_set__for_each(set, section, item) \ >> + config_sections__for_each(&set->sections, section) \ >> + config_items__for_each(§ion->items, item) > > Ditto. >
Granted.
I'll send v11 soon with this change!
Thanks, Taeung
| |