[RFC][PATCH v8 0/5] perf config: Reimplement perf_config() using perf_config_set__inter()

[RFC][PATCH v8 0/5] perf config: Reimplement perf_config() using perf_config_set__inter()

[Taeung Song]

Hello, :)

This patchset is to reimplement perf_config() for efficient config management.

Everytime perf_config() is called, perf_config() always read config files.
(i.e. user config '~/.perfconfig' and system config '$(sysconfdir)/perfconfig')

But we need to use 'struct perf_config_set config_set' variable
that already contains all config key-value pairs
to avoid this repetitive work in perf_config().

In other words, if new perf_config() is called,
only first time 'config_set' is initialized
collecting all configs from config files and it work with perf_config_set__iter().

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.

IMHO, I think this patchset is needed because not only the repetitive work
should be avoided but also in near future, it would be smooth to manage perf configs.

If you give me any feedback, I'd apprecicated it. :)

Thanks,
Taeung

v8:
- handle the error about NULL at perf_config_set__delete()
- bring declarations about config from util/config.h to util/config.h
- reimplement show_config() using perf_config_set__iter() instead of perf_config()
- rebased onto perf-core-for-mingo-20160607
- applied ([PATCH v7 1/7] 25d8f48, [PATCH v7 2/7] 8beeb00)

v7:
- fill a missing crumb that assign NULL to 'set' variable in perf_config_set__new()
  (Arnaldo)
- two patches applied ([PATCH v6 1/9] 78f71c9, [PATCH v6 3/9] 7db91f2)

v6:
- add printing error message when perf_config_set__iter() is failed
- modify commit messages for bugfix 1~3 (PATCH 1/9 ~ 3/9)
  to help reviewers easily understand why them is needed

v5:
- solve the leak when perf_config_set__init() failed (Arnaldo)
  (to clear the problem it is needed to apply the bottom bugfix 1~3 patches)
- bugfix 1) fix the problem of abnormal terminaltion at perf_parse_file() called by perf_config()
- bugfix 2) if failed at collect_config(), finally free a config set
            after it is done instead of freeing the config set in the function
- bugfix 3) handle NULL pointer exception of 'set' at collect_config()

v4:
- Keep perf_config_set__delete() as it is (Arnaldo)
- Remove perf_config_set__check() (Arnaldo)
- Keep the existing code about the config set at cmd_config() (Arnaldo)

v3:
- add freeing config set after sub-command work at run_builtin() (Namhyung)
- remove needless code about the config set at cmd_config()
- add a patch about a global variable 'config_set'

v2:
- split a patch into several patches
- reimplement show_config() using new perf_config()
- modify perf_config_set__delete using global variable 'config_set'
- reset config set when only 'config' sub-commaned work
  because of options for config file location

Taeung Song (5):
  perf config: Handle the error about NULL at perf_config_set__delete()
  perf config: Bring declarations about config from util/cache.h to
    util/config.h
  perf config: Reimplement perf_config() using perf_config_set__iter()
  perf config: Use zfree() instead of free() at
    perf_config_set__delete()
  perf config: Reimplement show_config() using perf_config_set__iter()

 tools/perf/builtin-config.c | 35 ++++++----------
 tools/perf/perf.c           |  1 +
 tools/perf/util/cache.h     | 13 +-----
 tools/perf/util/config.c    | 98 ++++++++++++++++++++++-----------------------
 tools/perf/util/config.h    | 16 +++++++-
 5 files changed, 78 insertions(+), 85 deletions(-)

-- 
2.5.0

[BUGFIX][PATCH v8 1/5] perf config: Handle the error about NULL at perf_config_set__delete()

[Taeung Song]

perf_config_set__delete() purge and free the config set
that contains all config key-value pairs.
But if the config set (i.e. 'set' variable at the function)
is NULL, this is wrong so handle it.

Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Wang Nan <wangnan0@huawei.com>
Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
---
 tools/perf/util/config.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
index 8749eca..31e09a4 100644
--- a/tools/perf/util/config.c
+++ b/tools/perf/util/config.c
@@ -742,6 +742,9 @@ static void perf_config_set__purge(struct perf_config_set *set)
 
 void perf_config_set__delete(struct perf_config_set *set)
 {
+	if (set == NULL)
+		return;
+
 	perf_config_set__purge(set);
 	free(set);
 }
-- 
2.5.0

[PATCH v8 2/5] perf config: Bring declarations about config from util/cache.h to util/config.h

[Taeung Song]

Lately util/config.h has been added
but util/cache.h has declarations of functions
and extern variable for config features.

To manage codes about configuration at one spot, move them
to util/config.h and util/cache.h include util/config.h

Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Wang Nan <wangnan0@huawei.com>
Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
---
 tools/perf/util/cache.h  | 13 +------------
 tools/perf/util/config.h | 12 ++++++++++++
 2 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/tools/perf/util/cache.h b/tools/perf/util/cache.h
index 0d814bb..cd8cd4d 100644
--- a/tools/perf/util/cache.h
+++ b/tools/perf/util/cache.h
@@ -7,6 +7,7 @@
 #include <subcmd/pager.h>
 #include "../perf.h"
 #include "../ui/ui.h"
+#include "config.h"
 
 #include <linux/string.h>
 
@@ -23,18 +24,6 @@
 #define PERF_TRACEFS_ENVIRONMENT "PERF_TRACEFS_DIR"
 #define PERF_PAGER_ENVIRONMENT "PERF_PAGER"
 
-extern const char *config_exclusive_filename;
-
-typedef int (*config_fn_t)(const char *, const char *, void *);
-int perf_default_config(const char *, const char *, void *);
-int perf_config(config_fn_t fn, void *);
-int perf_config_int(const char *, const char *);
-u64 perf_config_u64(const char *, const char *);
-int perf_config_bool(const char *, const char *);
-int config_error_nonbool(const char *);
-const char *perf_config_dirname(const char *, const char *);
-const char *perf_etc_perfconfig(void);
-
 char *alias_lookup(const char *alias);
 int split_cmdline(char *cmdline, const char ***argv);
 
diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
index 22ec626..35ccddb 100644
--- a/tools/perf/util/config.h
+++ b/tools/perf/util/config.h
@@ -20,6 +20,18 @@ struct perf_config_set {
 	struct list_head sections;
 };
 
+extern const char *config_exclusive_filename;
+
+typedef int (*config_fn_t)(const char *, const char *, void *);
+int perf_default_config(const char *, const char *, void *);
+int perf_config(config_fn_t fn, void *);
+int perf_config_int(const char *, const char *);
+u64 perf_config_u64(const char *, const char *);
+int perf_config_bool(const char *, const char *);
+int config_error_nonbool(const char *);
+const char *perf_config_dirname(const char *, const char *);
+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);
 
-- 
2.5.0

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

[Taeung Song]

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')

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
+++ 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

[PATCH v8 4/5] perf config: Use zfree() instead of free() at perf_config_set__delete()

[Taeung Song]

perf_config_set__delete() delete allocated the config set
but the global variable 'config_set' is used all around.

So purge and zfree by an address of the global variable
, i.e. 'struct perf_config_set **' type
instead of using local variable 'set' of which type
is 'struct perf_config_set *'.

Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Wang Nan <wangnan0@huawei.com>
Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
---
 tools/perf/builtin-config.c |  2 +-
 tools/perf/perf.c           |  2 +-
 tools/perf/util/config.c    | 10 +++++-----
 tools/perf/util/config.h    |  2 +-
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
index cfd1036..c07f744 100644
--- a/tools/perf/builtin-config.c
+++ b/tools/perf/builtin-config.c
@@ -110,7 +110,7 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused)
 		usage_with_options(config_usage, config_options);
 	}
 
-	perf_config_set__delete(set);
+	perf_config_set__delete(&set);
 out_err:
 	return ret;
 }
diff --git a/tools/perf/perf.c b/tools/perf/perf.c
index fe2ab7c..058d5dc 100644
--- a/tools/perf/perf.c
+++ b/tools/perf/perf.c
@@ -391,7 +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);
+	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 72db134..23fb8e4 100644
--- a/tools/perf/util/config.c
+++ b/tools/perf/util/config.c
@@ -654,7 +654,7 @@ struct perf_config_set *perf_config_set__new(void)
 	if (set) {
 		INIT_LIST_HEAD(&set->sections);
 		if (perf_config_set__init(set) < 0) {
-			perf_config_set__delete(set);
+			perf_config_set__delete(&set);
 			set = NULL;
 		}
 	}
@@ -737,13 +737,13 @@ static void perf_config_set__purge(struct perf_config_set *set)
 	}
 }
 
-void perf_config_set__delete(struct perf_config_set *set)
+void perf_config_set__delete(struct perf_config_set **set)
 {
-	if (set == NULL)
+	if (*set == NULL)
 		return;
 
-	perf_config_set__purge(set);
-	free(set);
+	perf_config_set__purge(*set);
+	zfree(set);
 }
 
 /*
diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
index 7cc4fea..fafba86 100644
--- a/tools/perf/util/config.h
+++ b/tools/perf/util/config.h
@@ -34,6 +34,6 @@ const char *perf_config_dirname(const char *, const char *);
 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_set__delete(struct perf_config_set **set);
 
 #endif /* __PERF_CONFIG_H */
-- 
2.5.0

[PATCH v8 5/5] perf config: Reimplement show_config() using perf_config_set__iter()

[Taeung Song]

Old show_config() directly use config set so
there are many duplicated code with perf_config_set__iter().

So reimplement show_config() using perf_config() that use
perf_config_set__iter() with config set that already
contains all configs.

Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Wang Nan <wangnan0@huawei.com>
Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
---
 tools/perf/builtin-config.c | 29 +++++++----------------------
 tools/perf/util/config.c    |  2 +-
 tools/perf/util/config.h    |  1 +
 3 files changed, 9 insertions(+), 23 deletions(-)

diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
index c07f744..9c654f9 100644
--- a/tools/perf/builtin-config.c
+++ b/tools/perf/builtin-config.c
@@ -33,28 +33,13 @@ static struct option config_options[] = {
 	OPT_END()
 };
 
-static int show_config(struct perf_config_set *set)
+static int show_config(const char *key, const char *value,
+		       void *cb __maybe_unused)
 {
-	struct perf_config_section *section;
-	struct perf_config_item *item;
-	struct list_head *sections;
-
-	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)
-				printf("%s.%s=%s\n", section->name,
-				       item->name, value);
-		}
-	}
+	if (value)
+		printf("%s=%s\n", key, value);
+	else
+		printf("%s\n", key);
 
 	return 0;
 }
@@ -96,7 +81,7 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused)
 			pr_err("Error: takes no arguments\n");
 			parse_options_usage(config_usage, config_options, "l", 1);
 		} else {
-			ret = show_config(set);
+			ret = perf_config_set__iter(set, show_config, NULL);
 			if (ret < 0) {
 				const char * config_filename = config_exclusive_filename;
 				if (!config_exclusive_filename)
diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
index 23fb8e4..4775d18 100644
--- a/tools/perf/util/config.c
+++ b/tools/perf/util/config.c
@@ -662,7 +662,7 @@ 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)
+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;
diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
index fafba86..3526bb7 100644
--- a/tools/perf/util/config.h
+++ b/tools/perf/util/config.h
@@ -35,5 +35,6 @@ 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);
+int perf_config_set__iter(struct perf_config_set *set, config_fn_t fn, void *data);
 
 #endif /* __PERF_CONFIG_H */
-- 
2.5.0

Re: [PATCH v8 2/5] perf config: Bring declarations about config from util/cache.h to util/config.h

[Arnaldo Carvalho de Melo]

Em Wed, Jun 08, 2016 at 09:36:50PM +0900, Taeung Song escreveu:
> Lately util/config.h has been added
> but util/cache.h has declarations of functions
> and extern variable for config features.
> 
> To manage codes about configuration at one spot, move them
> to util/config.h and util/cache.h include util/config.h
> 
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Wang Nan <wangnan0@huawei.com>
> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
> ---
>  tools/perf/util/cache.h  | 13 +------------
>  tools/perf/util/config.h | 12 ++++++++++++
>  2 files changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/perf/util/cache.h b/tools/perf/util/cache.h
> index 0d814bb..cd8cd4d 100644
> --- a/tools/perf/util/cache.h
> +++ b/tools/perf/util/cache.h
> @@ -7,6 +7,7 @@
>  #include <subcmd/pager.h>
>  #include "../perf.h"
>  #include "../ui/ui.h"
> +#include "config.h"

Why have you added that? Are those config functions used in cache.h?

- Arnaldo

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

[Arnaldo Carvalho de Melo]

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'.

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.

- Arnaldo
 
> 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
> +++ 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

Re: [PATCH v8 4/5] perf config: Use zfree() instead of free() at perf_config_set__delete()

[Arnaldo Carvalho de Melo]

Em Wed, Jun 08, 2016 at 09:36:52PM +0900, Taeung Song escreveu:
> perf_config_set__delete() delete allocated the config set
> but the global variable 'config_set' is used all around.
 
> So purge and zfree by an address of the global variable
> , i.e. 'struct perf_config_set **' type
> instead of using local variable 'set' of which type
> is 'struct perf_config_set *'.

> -void perf_config_set__delete(struct perf_config_set *set)
> +void perf_config_set__delete(struct perf_config_set **set)
>  {
> -	if (set == NULL)
> +	if (*set == NULL)
>  		return;
>  
> -	perf_config_set__purge(set);
> -	free(set);
> +	perf_config_set__purge(*set);
> +	zfree(set);
>  }

Nope, don't change conventions like taht, a delete method should not
receive a pointer to the pointer to be deleted, no odd cases, please.

If you really think this is interesting, please introduce zdelete(),
i.e.:

void perf_config_set__zdelete(struct perf_config_set **set)
{
	if (!set)
		return;

	perf_config_set__delete(*set);
	*set = NULL;
}

- Arnaldo

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

[Arnaldo Carvalho de Melo]

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')
> 
> 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
> +++ 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);
> +}
> +

Try not using those iter + callback things, use something like this
instead:

	perf_config_set__for_each(pos, config_set)
		fn(pos, data)

I.e. use the list.h model. At some point this perf_config() function
should probably die, I guess.

- Arnaldo

Re: [PATCH v8 2/5] perf config: Bring declarations about config from util/cache.h to util/config.h

[Taeung Song]

Hi, Arnaldo :)

Thank you for your review.

On 06/09/2016 10:29 PM, Arnaldo Carvalho de Melo wrote:
> Em Wed, Jun 08, 2016 at 09:36:50PM +0900, Taeung Song escreveu:
>> Lately util/config.h has been added
>> but util/cache.h has declarations of functions
>> and extern variable for config features.
>>
>> To manage codes about configuration at one spot, move them
>> to util/config.h and util/cache.h include util/config.h
>>
>> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>> Cc: Jiri Olsa <jolsa@redhat.com>
>> Cc: Masami Hiramatsu <mhiramat@kernel.org>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Cc: Wang Nan <wangnan0@huawei.com>
>> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
>> ---
>>   tools/perf/util/cache.h  | 13 +------------
>>   tools/perf/util/config.h | 12 ++++++++++++
>>   2 files changed, 13 insertions(+), 12 deletions(-)
>>
>> diff --git a/tools/perf/util/cache.h b/tools/perf/util/cache.h
>> index 0d814bb..cd8cd4d 100644
>> --- a/tools/perf/util/cache.h
>> +++ b/tools/perf/util/cache.h
>> @@ -7,6 +7,7 @@
>>   #include <subcmd/pager.h>
>>   #include "../perf.h"
>>   #include "../ui/ui.h"
>> +#include "config.h"
>
> Why have you added that? Are those config functions used in cache.h?
>

Yes, it does. Many source files include cache.h
e.g. builtin-annoate.c, util/color.c, builtin-report.c and etc.
And They can use perf_config() function including this header file.

So, If I totally eliminate not only declarations about config
but also #include "util/config.h" at util/cache.h,
we should add '#include "util/config.h"' to each source file that
need perf_config() overall.

But IMHO, if cache.h include config.h,
we don't need to find source files that use perf_config().

Thanks,
Taeung

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

[Taeung Song]

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, &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

Re: [PATCH v8 4/5] perf config: Use zfree() instead of free() at perf_config_set__delete()

[Taeung Song]

On 06/09/2016 10:37 PM, Arnaldo Carvalho de Melo wrote:
> Em Wed, Jun 08, 2016 at 09:36:52PM +0900, Taeung Song escreveu:
>> perf_config_set__delete() delete allocated the config set
>> but the global variable 'config_set' is used all around.
>
>> So purge and zfree by an address of the global variable
>> , i.e. 'struct perf_config_set **' type
>> instead of using local variable 'set' of which type
>> is 'struct perf_config_set *'.
>
>> -void perf_config_set__delete(struct perf_config_set *set)
>> +void perf_config_set__delete(struct perf_config_set **set)
>>   {
>> -	if (set == NULL)
>> +	if (*set == NULL)
>>   		return;
>>
>> -	perf_config_set__purge(set);
>> -	free(set);
>> +	perf_config_set__purge(*set);
>> +	zfree(set);
>>   }
>
> Nope, don't change conventions like taht, a delete method should not
> receive a pointer to the pointer to be deleted, no odd cases, please.
>
> If you really think this is interesting, please introduce zdelete(),
> i.e.:
>
> void perf_config_set__zdelete(struct perf_config_set **set)
> {
> 	if (!set)
> 		return;
>
> 	perf_config_set__delete(*set);
> 	*set = NULL;
> }
>

I understood!

If we don't use the config set as a global variable,
it seems that we wouldn't need to change perf_config_set__delete() to 
perf_config_set__zdelete() as this patch.

Thanks,
Taeung

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

[Taeung Song]

On 06/09/2016 10:41 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')
>>
>> 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
>> +++ 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);
>> +}
>> +
>
> Try not using those iter + callback things, use something like this
> instead:
>
> 	perf_config_set__for_each(pos, config_set)
> 		fn(pos, data)
>
> I.e. use the list.h model. At some point this perf_config() function
> should probably die, I guess.
>


Granted!!
Will change perf_config_set__iter() to perf_config_set__for_each() as 
you said!!

Thanks,
Taeung

Re: [PATCH v8 2/5] perf config: Bring declarations about config from util/cache.h to util/config.h

[Arnaldo Carvalho de Melo]

Em Fri, Jun 10, 2016 at 03:20:43PM +0900, Taeung Song escreveu:
> On 06/09/2016 10:29 PM, Arnaldo Carvalho de Melo wrote:
> > > +++ b/tools/perf/util/cache.h
> > > @@ -7,6 +7,7 @@
> > >   #include <subcmd/pager.h>
> > > +#include "config.h"

> > Why have you added that? Are those config functions used in cache.h?
 
> Yes, it does. Many source files include cache.h
> e.g. builtin-annoate.c, util/color.c, builtin-report.c and etc.
> And They can use perf_config() function including this header file.
 
> So, If I totally eliminate not only declarations about config
> but also #include "util/config.h" at util/cache.h,
> we should add '#include "util/config.h"' to each source file that
> need perf_config() overall.

Sure, that is how we should do it. We should not include cache.h just to
get what is in config.h, we should instead include config.h.

This way when we do a change to cache.h we will not be rebuilding all
those files that depend on it just to get config.h.

What you're doing, removing from cache.h things that shouldn't be there
in the first place is good, among other things, because of that.

- Arnaldo

Re: [PATCH v8 2/5] perf config: Bring declarations about config from util/cache.h to util/config.h

[Taeung Song]

Good evening :)

On 06/11/2016 04:06 AM, Arnaldo Carvalho de Melo wrote:
> Em Fri, Jun 10, 2016 at 03:20:43PM +0900, Taeung Song escreveu:
>> On 06/09/2016 10:29 PM, Arnaldo Carvalho de Melo wrote:
>>>> +++ b/tools/perf/util/cache.h
>>>> @@ -7,6 +7,7 @@
>>>>    #include <subcmd/pager.h>
>>>> +#include "config.h"
>
>>> Why have you added that? Are those config functions used in cache.h?
>
>> Yes, it does. Many source files include cache.h
>> e.g. builtin-annoate.c, util/color.c, builtin-report.c and etc.
>> And They can use perf_config() function including this header file.
>
>> So, If I totally eliminate not only declarations about config
>> but also #include "util/config.h" at util/cache.h,
>> we should add '#include "util/config.h"' to each source file that
>> need perf_config() overall.
>
> Sure, that is how we should do it. We should not include cache.h just to
> get what is in config.h, we should instead include config.h.
>
> This way when we do a change to cache.h we will not be rebuilding all
> those files that depend on it just to get config.h.
>
> What you're doing, removing from cache.h things that shouldn't be there
> in the first place is good, among other things, because of that.
>

Granted!
I've also experienced the situation all those files which include cache.h
are rebuilt after I changed cache.h.
It also seems a problem as you mention.

So, I'll send this patch that reflect what you said with v9.

Have a nice weekend :-D

Thanks,
Taeung

Re: [PATCH v8 2/5] perf config: Bring declarations about config from util/cache.h to util/config.h

[Taeung Song]

Hi,

I have a question about header files.

I'm cleaning up source files that used cache.h
after moving codes about config from cache.h to config.h.

But I found there are header files that are repeatedly declared over all.

For example, builtin-report.c include util/sort.h,
perf.h, util/util.h, util/cache.h and etc.
However, util/sort.h also have #include "cache.h"
and cache.h even include util.h and perf.h.

Isn't this a problem (but this is minor) ?

Of course, this patch don't need to contain codes
to fix this above problem.

Should we fix this problem ?
(If we do, I'd individually send patches for this problem.)


Thanks,
Taeung

On 06/11/2016 09:59 AM, Taeung Song wrote:
> Good evening :)
>
> On 06/11/2016 04:06 AM, Arnaldo Carvalho de Melo wrote:
>> Em Fri, Jun 10, 2016 at 03:20:43PM +0900, Taeung Song escreveu:
>>> On 06/09/2016 10:29 PM, Arnaldo Carvalho de Melo wrote:
>>>>> +++ b/tools/perf/util/cache.h
>>>>> @@ -7,6 +7,7 @@
>>>>>    #include <subcmd/pager.h>
>>>>> +#include "config.h"
>>
>>>> Why have you added that? Are those config functions used in cache.h?
>>
>>> Yes, it does. Many source files include cache.h
>>> e.g. builtin-annoate.c, util/color.c, builtin-report.c and etc.
>>> And They can use perf_config() function including this header file.
>>
>>> So, If I totally eliminate not only declarations about config
>>> but also #include "util/config.h" at util/cache.h,
>>> we should add '#include "util/config.h"' to each source file that
>>> need perf_config() overall.
>>
>> Sure, that is how we should do it. We should not include cache.h just to
>> get what is in config.h, we should instead include config.h.
>>
>> This way when we do a change to cache.h we will not be rebuilding all
>> those files that depend on it just to get config.h.
>>
>> What you're doing, removing from cache.h things that shouldn't be there
>> in the first place is good, among other things, because of that.
>>
>
> Granted!
> I've also experienced the situation all those files which include cache.h
> are rebuilt after I changed cache.h.
> It also seems a problem as you mention.
>
> So, I'll send this patch that reflect what you said with v9.
>
> Have a nice weekend :-D
>
> Thanks,
> Taeung

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

[Taeung Song]

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@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, &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

[tip:perf/core] perf config: Handle NULL at perf_config_set__delete()

[tip-bot for Taeung Song]

Commit-ID:  826424cc919529d6d234af12c6ba975b63528a74
Gitweb:     http://git.kernel.org/tip/826424cc919529d6d234af12c6ba975b63528a74
Author:     Taeung Song <treeze.taeung@gmail.com>
AuthorDate: Wed, 8 Jun 2016 21:36:49 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 14 Jun 2016 09:29:53 -0300

perf config: Handle NULL at perf_config_set__delete()

perf_config_set__delete() purge and free the config set that contains
all config key-value pairs.  But if the config set (i.e. 'set' variable
at the function) is NULL, this is wrong so handle it.

Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Wang Nan <wangnan0@huawei.com>
Link: http://lkml.kernel.org/r/1465389413-8936-2-git-send-email-treeze.taeung@gmail.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/config.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
index 8749eca..31e09a4 100644
--- a/tools/perf/util/config.c
+++ b/tools/perf/util/config.c
@@ -742,6 +742,9 @@ static void perf_config_set__purge(struct perf_config_set *set)
 
 void perf_config_set__delete(struct perf_config_set *set)
 {
+	if (set == NULL)
+		return;
+
 	perf_config_set__purge(set);
 	free(set);
 }

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

[Taeung Song]

Hi, Arnaldo :-D

I also discussed this patchset with Namhyung at offline gathering.
I sent v9 patchset that contains what Namhyung and you advice me to do.
If having your spare time, please review the v9 patchset.
If you do, I'd appreciate it :-)

Thanks,
Taeung

On 06/12/2016 05:57 PM, Taeung Song wrote:
> 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@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, &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