Re: [PATCH v8 3/5] perf config: Reimplement perf_config() using perf_config_set__iter()

From: Taeung Song
Date: Sun Jun 12 2016 - 04:57:53 EST

Hi, Arnaldo

What do you think about setting all actual config variables
before a sub-command start at perf.c ?

(in order to free the config set that contains all configs
by perf_config_set__delete() after using it was finished where it was needed)

i.e. initialize all actual config variables at perf.c
instead of at each source file as below.
(before running a particular sub-command at run_builitin())

util/alias.c
22: perf_config(alias_lookup_cb, NULL);

util/intel-pt.c
330: perf_config(intel_pt_config_div, &d);
1993:static int intel_pt_perf_config(const char *var, const char *value, void *data)
2047: perf_config(intel_pt_perf_config, pt);

util/data-convert-bt.c
1302: perf_config(convert__config, &c);

util/help-unknown-cmd.c
62: perf_config(perf_unknown_cmd_config, NULL);

builtin-help.c
454: perf_config(perf_help_config, &help_format);

perf.c
94: perf_config(pager_command_config, &c);
117: perf_config(browser_command_config, &c);
561: perf_config(perf_default_config, NULL);

builtin-record.c
1432: perf_config(perf_record_config, rec);

ui/browsers/annotate.c
1163: perf_config(annotate__config, NULL);

ui/browser.c
743: perf_config(ui_browser__color_config, NULL);

builtin-report.c
829: perf_config(report__config, &report);

builtin-kmem.c
1899: perf_config(kmem_config, NULL);

builtin-top.c
1231: perf_config(perf_top_config, &top);


If we do, we could set all actual config variables
before a sub-command work. And the config set that is made
by perf_config_set__new() would be reused
when we initialize all actual config variables
and then if using is is finished, we could free the config set.


Thanks,
Taeung


On 06/10/2016 07:58 PM, Taeung Song wrote:


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@xxxxxxxxxx>
Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
Cc: Wang Nan <wangnan0@xxxxxxxxxx>
Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
Cc: Ingo Molnar <mingo@xxxxxxxxxx>
Cc: Masami Hiramatsu <mhiramat@xxxxxxxxxx>
Cc: Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>
Signed-off-by: Taeung Song <treeze.taeung@xxxxxxxxx>
---
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, &section->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