Messages in this thread Patch in this message | | | Subject | Re: [PATCH v8 3/5] perf config: Reimplement perf_config() using perf_config_set__iter() | From | Taeung Song <> | Date | Fri, 10 Jun 2016 19:58:46 +0900 |
| |
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 <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 | 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
| |