From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932382AbcFJK6y (ORCPT ); Fri, 10 Jun 2016 06:58:54 -0400 Received: from mail-pf0-f196.google.com ([209.85.192.196]:34959 "EHLO mail-pf0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751919AbcFJK6v (ORCPT ); Fri, 10 Jun 2016 06:58:51 -0400 Subject: Re: [PATCH v8 3/5] perf config: Reimplement perf_config() using perf_config_set__iter() To: Arnaldo Carvalho de Melo References: <1465389413-8936-1-git-send-email-treeze.taeung@gmail.com> <1465389413-8936-4-git-send-email-treeze.taeung@gmail.com> <20160609133426.GJ11589@kernel.org> Cc: linux-kernel@vger.kernel.org, Jiri Olsa , Namhyung Kim , Ingo Molnar , Peter Zijlstra , Alexander Shishkin , Masami Hiramatsu , Wang Nan , Jiri Olsa , Ingo Molnar From: Taeung Song Message-ID: <575A9D66.1060004@gmail.com> Date: Fri, 10 Jun 2016 19:58:46 +0900 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.8.0 MIME-Version: 1.0 In-Reply-To: <20160609133426.GJ11589@kernel.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 06/09/2016 10:34 PM, Arnaldo Carvalho de Melo wrote: > Em Wed, Jun 08, 2016 at 09:36:51PM +0900, Taeung Song escreveu: >> Many sub-commands use perf_config() so >> everytime perf_config() is called, perf_config() always read config files. >> (i.e. user config '~/.perfconfig' and system config '$(sysconfdir)/perfconfig') > > And this is not always a bad thing, think about being in 'mutt' and > adding an entry to ~/.mail_aliases, then going to compose a message, > would be good that the just added entry to ~/.mail_aliases be considered > when adding recipients to your messages, right? > > In the same fashion, 'perf report', 'perf annotate', 'perf top' are long > running utilities that have operations that could get changes to config > files without having to restart them, i.e. do annotation changes in your > ~/.perfconfig and then see them reflected next time you hit 'AŽ to > annotate a function in 'perf top' or 'perf report'. > Currently, this suggestion isn't already contained among features of perf. right? I understood that you mean while perf process is already running if user change a config file, the changed config can be reflected in 'perf top', 'perf report' or etc without restarting perf process like mutt. Is it right ? > So, I think that if tools want ammortize the cost of reading config > files, they should create an instance of the relevant object > (perf_config_set?) use it and then delete it, but not keep one around > for a long time. > I got it. How about the two ideas that I have in mind ? 1) If we eliminate the config set object(perf_config_set) after using it because we don't need to keep it until perf process is done, we could bring many codes using perf_config() at one spot of perf.c ? (because it is hard to decide a point of time we destroy the config set.) For example, (This code isn't executable) diff --git a/tools/perf/perf.c b/tools/perf/perf.c index 15982ce..6a56985 100644 --- a/tools/perf/perf.c +++ b/tools/perf/perf.c @@ -77,6 +77,23 @@ struct pager_config { int val; }; +static void perf_default_config_init(void) +{ + default_colors_config_init(); + default_annoate_config_init(); + default_report_config_init(); + ... +} + +static void perf_config_init(void) +{ + perf_default_config_init(); + colors_config_init(); + annotate_config_init(); + report_config_init(); + ... +} + static int pager_command_config(const char *var, const char *value, void *data) { struct pager_config *c = data; @@ -558,7 +575,7 @@ int main(int argc, const char **argv) srandom(time(NULL)); - perf_config(perf_default_config, NULL); + perf_config_init(); set_buildid_dir(NULL); /* get debugfs/tracefs mount point from /proc/mounts */ diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c index af68a9d..4b827c2 100644 --- a/tools/perf/ui/browser.c +++ b/tools/perf/ui/browser.c @@ -740,8 +740,6 @@ void ui_browser__init(void) { int i = 0; - perf_config(ui_browser__color_config, NULL); - while (ui_browser__colorsets[i].name) { struct ui_browser_colorset *c = &ui_browser__colorsets[i++]; sltt_set_color(c->colorset, c->name, c->fg, c->bg); ... (omitted) Many sub-commands call perf_config() at builtin-report.c, ui/browser.c and etc. So I think it is ambiguous to decide where perf_config_set__delete() will be called instead of at run_builtin(). (If the config set's life cycle is the same as perf's , I would call perf_config_set__delete() at run_builtin() because a sub-command is done) 2) If having the config set for perf's life, we would add configuration features to TUI like TUI configuring options for linux kernel compiling (e.g. make menuconfig) ? Of course, we don't need to have the same function as make menuconfig but IMHO, we could add similar way to menuconfig while perf is running. How do you feel about these ? Thanks, :) Taeung > >> But we need 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 global variable 'config_set') >> >> In other words, if new perf_config() is called, >> only first time 'config_set' is initialized collecting all configs >> from the config files and it work with perf_config_set__iter(). >> And free the config set after a sub-command work at run_builtin(). >> >> If we do, 'config_set' can be reused wherever using perf_config() >> and a feature of old perf_config() is the same as new perf_config() work >> without the repetitive work that read the config files. >> >> Cc: Namhyung Kim >> Cc: Jiri Olsa >> Cc: Wang Nan >> Cc: Peter Zijlstra >> Cc: Ingo Molnar >> Cc: Masami Hiramatsu >> Cc: Alexander Shishkin >> Signed-off-by: Taeung Song >> --- >> tools/perf/builtin-config.c | 4 +++ >> tools/perf/perf.c | 1 + >> tools/perf/util/config.c | 87 ++++++++++++++++++++++----------------------- >> tools/perf/util/config.h | 1 + >> 4 files changed, 48 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 15982ce..fe2ab7c 100644 >> --- a/tools/perf/perf.c> - Arnaldo >> +++ b/tools/perf/perf.c >> @@ -391,6 +391,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_set__delete(config_set); >> exit_browser(status); >> perf_env__exit(&perf_env); >> bpf__clear(); >> diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c >> index 31e09a4..72db134 100644 >> --- a/tools/perf/util/config.c >> +++ b/tools/perf/util/config.c >> @@ -28,6 +28,7 @@ static int config_linenr; >> static int config_file_eof; >> >> const char *config_exclusive_filename; >> +struct perf_config_set *config_set; >> >> static int get_next_char(void) >> { >> @@ -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,47 @@ struct perf_config_set *perf_config_set__new(void) >> return set; >> } >> >> +static int perf_config_set__iter(struct perf_config_set *set, config_fn_t fn, void *data) >> +{ >> + struct perf_config_section *section; >> + struct perf_config_item *item; >> + struct list_head *sections; >> + char key[BUFSIZ]; >> + >> + if (set == NULL) >> + return -1; >> + >> + sections = &set->sections; >> + if (list_empty(sections)) >> + return -1; >> + >> + list_for_each_entry(section, sections, node) { >> + list_for_each_entry(item, §ion->items, node) { >> + char *value = item->value; >> + >> + if (value) { >> + scnprintf(key, sizeof(key), "%s.%s", >> + section->name, item->name); >> + if (fn(key, value, data) < 0) { >> + pr_err("Error: wrong config key-value pair %s=%s\n", >> + key, value); >> + return -1; >> + } >> + } >> + } >> + } >> + >> + return 0; >> +} >> + >> +int perf_config(config_fn_t fn, void *data) >> +{ >> + if (config_set == NULL) >> + config_set = perf_config_set__new(); >> + >> + return perf_config_set__iter(config_set, fn, data); >> +} >> + >> 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 35ccddb..7cc4fea 100644 >> --- a/tools/perf/util/config.h >> +++ b/tools/perf/util/config.h >> @@ -21,6 +21,7 @@ struct perf_config_set { >> }; >> >> extern const char *config_exclusive_filename; >> +extern struct perf_config_set *config_set; >> >> typedef int (*config_fn_t)(const char *, const char *, void *); >> int perf_default_config(const char *, const char *, void *); >> -- >> 2.5.0