linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC][PATCH v2 0/5] perf config: Introduce perf_config_set class
@ 2016-03-14 12:16 Taeung Song
  2016-03-14 12:16 ` [PATCH v2 1/5] " Taeung Song
                   ` (4 more replies)
  0 siblings, 5 replies; 10+ messages in thread
From: Taeung Song @ 2016-03-14 12:16 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: linux-kernel, Jiri Olsa, Namhyung Kim, Ingo Molnar,
	Peter Zijlstra, Taeung Song

Hi, all :-)

We can use the config files (i.e user wide ~/.perfconfig
and system wide $(sysconfdir)/perfconfig)
to configure perf tools. perf-config help user
manage the config files, not manually look into or edit them.

Introduce new infrastructure code for config
management features of perf-config subcommand.

This pathset is for various purposes of configuration management
showing current configs, in the near future,
showing all configs with default value,
getting current configs from the config files
or writing configs that user type on the config files, etc.

IMHO, I want to add new effective funcationalities
for config management of perf-config based on this
infrastructure code.

Thanks,
Taeung

v2:
- remove perf_config_kind (user, system or both config files)
  and needless at this time, etc. (Namhyung)
- separate this patch as several patches (Namhyung)
- fix typing errors, etc.

Taeung Song (5):
  perf config: Introduce perf_config_set class
  perf config: Let show_config() work with perf_config_set
  perf config: Prepare all default configs
  perf config: Initialize perf_config_set with all default configs
  perf config: Add 'list-all' option to show all perf's configs

 tools/perf/Documentation/perf-config.txt |   6 +
 tools/perf/builtin-config.c              | 102 +++++++++++++++--
 tools/perf/util/config.c                 | 186 +++++++++++++++++++++++++++++++
 tools/perf/util/config.h                 | 112 +++++++++++++++++++
 4 files changed, 399 insertions(+), 7 deletions(-)
 create mode 100644 tools/perf/util/config.h

-- 
2.5.0

^ permalink raw reply	[flat|nested] 10+ messages in thread
* [PATCH v2 1/5] perf config: Introduce perf_config_set class
  2016-03-14 12:16 [RFC][PATCH v2 0/5] perf config: Introduce perf_config_set class Taeung Song
@ 2016-03-14 12:16 ` Taeung Song
  2016-03-17 12:31   ` Namhyung Kim
  2016-03-14 12:16 ` [PATCH v2 2/5] perf config: Let show_config() work with perf_config_set Taeung Song
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 10+ messages in thread
From: Taeung Song @ 2016-03-14 12:16 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: linux-kernel, Jiri Olsa, Namhyung Kim, Ingo Molnar,
	Peter Zijlstra, Taeung Song

This infrastructure code was designed for
upcoming features of perf-config.

That collect config key-value pairs from user and
system config files (i.e. user wide ~/.perfconfig
and system wide $(sysconfdir)/perfconfig)
to manage perf's configs.

Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
---
 tools/perf/builtin-config.c |   1 +
 tools/perf/util/config.c    | 123 ++++++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/config.h    |  21 ++++++++
 3 files changed, 145 insertions(+)
 create mode 100644 tools/perf/util/config.h

diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
index c42448e..412c725 100644
--- a/tools/perf/builtin-config.c
+++ b/tools/perf/builtin-config.c
@@ -12,6 +12,7 @@
 #include <subcmd/parse-options.h>
 #include "util/util.h"
 #include "util/debug.h"
+#include "util/config.h"
 
 static bool use_system_config, use_user_config;
 
diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
index 4e72763..b9660e4 100644
--- a/tools/perf/util/config.c
+++ b/tools/perf/util/config.c
@@ -13,6 +13,7 @@
 #include <subcmd/exec-cmd.h>
 #include "util/hist.h"  /* perf_hist_config */
 #include "util/llvm-utils.h"   /* perf_llvm_config */
+#include "config.h"
 
 #define MAXNAME (256)
 
@@ -506,6 +507,128 @@ out:
 	return ret;
 }
 
+static struct perf_config_item *find_config(struct list_head *config_list,
+				       const char *section,
+				       const char *name)
+{
+	struct perf_config_item *config;
+
+	list_for_each_entry(config, config_list, list) {
+		if (!strcmp(config->section, section) &&
+		    !strcmp(config->name, name))
+			return config;
+	}
+
+	return NULL;
+}
+
+static struct perf_config_item *add_config(struct list_head *config_list,
+					   const char *section,
+					   const char *name)
+{
+	struct perf_config_item *config = zalloc(sizeof(*config));
+
+	if (!config)
+		return NULL;
+
+	config->section = strdup(section);
+	if (!section)
+		goto out_err;
+
+	config->name = strdup(name);
+	if (!name) {
+		free((char *)config->section);
+		goto out_err;
+	}
+
+	list_add_tail(&config->list, config_list);
+	return config;
+
+out_err:
+	free(config);
+	pr_err("%s: strdup failed\n", __func__);
+	return NULL;
+}
+
+static int set_value(struct perf_config_item *config, const char *value)
+{
+	char *val = strdup(value);
+
+	if (!val)
+		return -1;
+	config->value = val;
+
+	return 0;
+}
+
+static int collect_config(const char *var, const char *value,
+			  void *configs)
+{
+	int ret = 0;
+	char *ptr, *key;
+	char *section, *name;
+	struct perf_config_item *config;
+	struct list_head *config_list = configs;
+
+	key = ptr = strdup(var);
+	if (!key) {
+		pr_err("%s: strdup failed\n", __func__);
+		return -1;
+	}
+
+	section = strsep(&ptr, ".");
+	name = ptr;
+	if (name == NULL || value == NULL) {
+		ret = -1;
+		goto out_free;
+	}
+
+	config = find_config(config_list, section, name);
+	if (!config) {
+		config = add_config(config_list, section, name);
+		if (!config) {
+			free(config->section);
+			free(config->name);
+			ret = -1;
+			goto out_free;
+		}
+	}
+
+	ret = set_value(config, value);
+
+out_free:
+	free(key);
+	return ret;
+}
+
+struct perf_config_set *perf_config_set__new(void)
+{
+	struct perf_config_set *perf_configs = zalloc(sizeof(*perf_configs));
+
+	if (!perf_configs)
+		return NULL;
+
+	INIT_LIST_HEAD(&perf_configs->config_list);
+	perf_config(collect_config, &perf_configs->config_list);
+
+	return perf_configs;
+}
+
+void perf_config_set__delete(struct perf_config_set *perf_configs)
+{
+	struct perf_config_item *pos, *item;
+
+	list_for_each_entry_safe(pos, item, &perf_configs->config_list, list) {
+		list_del(&pos->list);
+		free(pos->section);
+		free(pos->name);
+		free(pos->value);
+		free(pos);
+	}
+
+	free(perf_configs);
+}
+
 /*
  * Call this to report error for your variable that should not
  * get a boolean value (i.e. "[my] var" means "true").
diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
new file mode 100644
index 0000000..44c226f
--- /dev/null
+++ b/tools/perf/util/config.h
@@ -0,0 +1,21 @@
+#ifndef __PERF_CONFIG_H
+#define __PERF_CONFIG_H
+
+#include <stdbool.h>
+#include <linux/list.h>
+
+struct perf_config_item {
+	char *section;
+	char *name;
+	char *value;
+	struct list_head list;
+};
+
+struct perf_config_set {
+	struct list_head config_list;
+};
+
+struct perf_config_set *perf_config_set__new(void);
+void perf_config_set__delete(struct perf_config_set *perf_configs);
+
+#endif /* __PERF_CONFIG_H */
-- 
2.5.0

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [PATCH v2 2/5] perf config: Let show_config() work with perf_config_set
  2016-03-14 12:16 [RFC][PATCH v2 0/5] perf config: Introduce perf_config_set class Taeung Song
  2016-03-14 12:16 ` [PATCH v2 1/5] " Taeung Song
@ 2016-03-14 12:16 ` Taeung Song
  2016-03-14 12:16 ` [PATCH v2 3/5] perf config: Prepare all default configs Taeung Song
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 10+ messages in thread
From: Taeung Song @ 2016-03-14 12:16 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: linux-kernel, Jiri Olsa, Namhyung Kim, Ingo Molnar,
	Peter Zijlstra, Taeung Song

Current show_config() has a problem when user or
system config files have same config variables i.e.

    # cat ~/.perfconfig
        [top]
            children = false

    when $(sysconfdir) is /usr/local/etc
    # cat /usr/local/etc/perfconfig
        [top]
            children = true

Before:
    # perf config --user --list
    top.children=false

    # perf config --system --list
    top.children=true

    # perf config --list
    top.children=true
    top.children=false

Because perf_config() can call show_config()
each the config file (user and system).
So fix it.

After:
    # perf config --user --list
    top.children=false

    # perf config --system --list
    top.children=true

    # perf config --list
    top.children=false

Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
---
 tools/perf/builtin-config.c | 31 ++++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)

diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
index 412c725..2217772 100644
--- a/tools/perf/builtin-config.c
+++ b/tools/perf/builtin-config.c
@@ -33,13 +33,21 @@ static struct option config_options[] = {
 	OPT_END()
 };
 
-static int show_config(const char *key, const char *value,
-		       void *cb __maybe_unused)
+static int show_config(struct perf_config_set *perf_configs)
 {
-	if (value)
-		printf("%s=%s\n", key, value);
-	else
-		printf("%s\n", key);
+	struct perf_config_item *config;
+	struct list_head *config_list = &perf_configs->config_list;
+
+	if (list_empty(config_list))
+		return -1;
+
+	list_for_each_entry(config, config_list, list) {
+		char *value = config->value;
+
+		if (value)
+			printf("%s.%s=%s\n", config->section,
+			       config->name, value);
+	}
 
 	return 0;
 }
@@ -47,6 +55,7 @@ static int show_config(const char *key, const char *value,
 int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused)
 {
 	int ret = 0;
+	struct perf_config_set *perf_configs;
 	char *user_config = mkpath("%s/.perfconfig", getenv("HOME"));
 
 	argc = parse_options(argc, argv, config_options, config_usage,
@@ -64,13 +73,19 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused)
 	else if (use_user_config)
 		config_exclusive_filename = user_config;
 
+	perf_configs = perf_config_set__new();
+	if (!perf_configs) {
+		ret = -1;
+		goto out_err;
+	}
+
 	switch (actions) {
 	case ACTION_LIST:
 		if (argc) {
 			pr_err("Error: takes no arguments\n");
 			parse_options_usage(config_usage, config_options, "l", 1);
 		} else {
-			ret = perf_config(show_config, NULL);
+			ret = show_config(perf_configs);
 			if (ret < 0) {
 				const char * config_filename = config_exclusive_filename;
 				if (!config_exclusive_filename)
@@ -84,5 +99,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(perf_configs);
+out_err:
 	return ret;
 }
-- 
2.5.0

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [PATCH v2 3/5] perf config: Prepare all default configs
  2016-03-14 12:16 [RFC][PATCH v2 0/5] perf config: Introduce perf_config_set class Taeung Song
  2016-03-14 12:16 ` [PATCH v2 1/5] " Taeung Song
  2016-03-14 12:16 ` [PATCH v2 2/5] perf config: Let show_config() work with perf_config_set Taeung Song
@ 2016-03-14 12:16 ` Taeung Song
  2016-03-14 12:16 ` [PATCH v2 4/5] perf config: Initialize perf_config_set with " Taeung Song
  2016-03-14 12:16 ` [PATCH v2 5/5] perf config: Add 'list-all' option to show all perf's configs Taeung Song
  4 siblings, 0 replies; 10+ messages in thread
From: Taeung Song @ 2016-03-14 12:16 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: linux-kernel, Jiri Olsa, Namhyung Kim, Ingo Molnar,
	Peter Zijlstra, Taeung Song

To precisely manage configs,
prepare all default perf's configs that contain
default section name, variable name, value
and correct type, not string type.

In the near future, this will be used when
checking type of config variable or showing
all configs with default values, etc.

Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
---
 tools/perf/util/config.c | 54 ++++++++++++++++++++++++++---
 tools/perf/util/config.h | 89 ++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 137 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
index b9660e4..0b9ba51 100644
--- a/tools/perf/util/config.c
+++ b/tools/perf/util/config.c
@@ -29,6 +29,52 @@ static int config_file_eof;
 
 const char *config_exclusive_filename;
 
+struct perf_config_item default_configs[] = {
+	CONF_STR_VAR(COLORS_TOP, "colors", "top", "red, default"),
+	CONF_STR_VAR(COLORS_MEDIUM, "colors", "medium", "green, default"),
+	CONF_STR_VAR(COLORS_NORMAL, "colors", "normal", "lightgray, default"),
+	CONF_STR_VAR(COLORS_SELECTED, "colors", "selected", "white, lightgray"),
+	CONF_STR_VAR(COLORS_JUMP_ARROWS, "colors", "jump_arrows", "blue, default"),
+	CONF_STR_VAR(COLORS_ADDR, "colors", "addr", "magenta, default"),
+	CONF_STR_VAR(COLORS_ROOT, "colors", "root", "white, blue"),
+	CONF_BOOL_VAR(TUI_REPORT, "tui", "report", true),
+	CONF_BOOL_VAR(TUI_ANNOTATE, "tui", "annotate", true),
+	CONF_BOOL_VAR(TUI_TOP, "tui", "top", true),
+	CONF_STR_VAR(BUILDID_DIR, "buildid", "dir", "~/.debug"),
+	CONF_BOOL_VAR(ANNOTATE_HIDE_SRC_CODE, "annotate", "hide_src_code", false),
+	CONF_BOOL_VAR(ANNOTATE_USE_OFFSET, "annotate", "use_offset", true),
+	CONF_BOOL_VAR(ANNOTATE_JUMP_ARROWS, "annotate", "jump_arrows", true),
+	CONF_BOOL_VAR(ANNOTATE_SHOW_NR_JUMPS, "annotate", "show_nr_jumps", false),
+	CONF_BOOL_VAR(ANNOTATE_SHOW_LINENR, "annotate", "show_linenr", false),
+	CONF_BOOL_VAR(ANNOTATE_SHOW_TOTAL_PERIOD, "annotate", "show_total_period", false),
+	CONF_BOOL_VAR(GTK_ANNOTATE, "gtk", "annotate", false),
+	CONF_BOOL_VAR(GTK_REPORT, "gtk", "report", false),
+	CONF_BOOL_VAR(GTK_TOP, "gtk", "top", false),
+	CONF_BOOL_VAR(PAGER_CMD, "pager", "cmd", true),
+	CONF_BOOL_VAR(PAGER_REPORT, "pager", "report", true),
+	CONF_BOOL_VAR(PAGER_ANNOTATE, "pager", "annotate", true),
+	CONF_BOOL_VAR(PAGER_TOP, "pager", "top", true),
+	CONF_BOOL_VAR(PAGER_DIFF, "pager", "diff", true),
+	CONF_STR_VAR(HELP_FORMAT, "help", "format", "man"),
+	CONF_INT_VAR(HELP_AUTOCORRECT, "help", "autocorrect", 0),
+	CONF_STR_VAR(HIST_PERCENTAGE, "hist", "percentage", "absolute"),
+	CONF_BOOL_VAR(UI_SHOW_HEADERS, "ui", "show-headers", true),
+	CONF_STR_VAR(CALL_GRAPH_RECORD_MODE, "call-graph", "record-mode", "fp"),
+	CONF_LONG_VAR(CALL_GRAPH_DUMP_SIZE, "call-graph", "dump-size", 8192),
+	CONF_STR_VAR(CALL_GRAPH_PRINT_TYPE, "call-graph", "print-type", "graph"),
+	CONF_STR_VAR(CALL_GRAPH_ORDER, "call-graph", "order", "callee"),
+	CONF_STR_VAR(CALL_GRAPH_SORT_KEY, "call-graph", "sort-key", "function"),
+	CONF_DOUBLE_VAR(CALL_GRAPH_THRESHOLD, "call-graph", "threshold", 0.5),
+	CONF_LONG_VAR(CALL_GRAPH_PRINT_LIMIT, "call-graph", "print-limit", 0),
+	CONF_BOOL_VAR(REPORT_GROUP, "report", "group", true),
+	CONF_BOOL_VAR(REPORT_CHILDREN, "report", "children", true),
+	CONF_FLOAT_VAR(REPORT_PERCENT_LIMIT, "report", "percent-limit", 0),
+	CONF_U64_VAR(REPORT_QUEUE_SIZE, "report", "queue-size", 0),
+	CONF_BOOL_VAR(TOP_CHILDREN, "top", "children", true),
+	CONF_STR_VAR(MAN_VIEWER, "man", "viewer", "man"),
+	CONF_STR_VAR(KMEM_DEFAULT, "kmem", "default", "slab"),
+};
+
 static int get_next_char(void)
 {
 	int c;
@@ -587,8 +633,8 @@ static int collect_config(const char *var, const char *value,
 	if (!config) {
 		config = add_config(config_list, section, name);
 		if (!config) {
-			free(config->section);
-			free(config->name);
+			free((char *)config->section);
+			free((char *)config->name);
 			ret = -1;
 			goto out_free;
 		}
@@ -620,8 +666,8 @@ void perf_config_set__delete(struct perf_config_set *perf_configs)
 
 	list_for_each_entry_safe(pos, item, &perf_configs->config_list, list) {
 		list_del(&pos->list);
-		free(pos->section);
-		free(pos->name);
+		free((char *)pos->section);
+		free((char *)pos->name);
 		free(pos->value);
 		free(pos);
 	}
diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
index 44c226f..3c30576 100644
--- a/tools/perf/util/config.h
+++ b/tools/perf/util/config.h
@@ -4,10 +4,30 @@
 #include <stdbool.h>
 #include <linux/list.h>
 
+enum perf_config_type {
+	CONFIG_TYPE_BOOL,
+	CONFIG_TYPE_INT,
+	CONFIG_TYPE_LONG,
+	CONFIG_TYPE_U64,
+	CONFIG_TYPE_FLOAT,
+	CONFIG_TYPE_DOUBLE,
+	CONFIG_TYPE_STRING
+};
+
 struct perf_config_item {
-	char *section;
-	char *name;
+	const char *section;
+	const char *name;
 	char *value;
+	union {
+		bool b;
+		int i;
+		u32 l;
+		u64 ll;
+		float f;
+		double d;
+		const char *s;
+	} default_value;
+	enum perf_config_type type;
 	struct list_head list;
 };
 
@@ -15,6 +35,71 @@ struct perf_config_set {
 	struct list_head config_list;
 };
 
+enum perf_config_idx {
+	CONFIG_COLORS_TOP,
+	CONFIG_COLORS_MEDIUM,
+	CONFIG_COLORS_NORMAL,
+	CONFIG_COLORS_SELECTED,
+	CONFIG_COLORS_JUMP_ARROWS,
+	CONFIG_COLORS_ADDR,
+	CONFIG_COLORS_ROOT,
+	CONFIG_TUI_REPORT,
+	CONFIG_TUI_ANNOTATE,
+	CONFIG_TUI_TOP,
+	CONFIG_BUILDID_DIR,
+	CONFIG_ANNOTATE_HIDE_SRC_CODE,
+	CONFIG_ANNOTATE_USE_OFFSET,
+	CONFIG_ANNOTATE_JUMP_ARROWS,
+	CONFIG_ANNOTATE_SHOW_NR_JUMPS,
+	CONFIG_ANNOTATE_SHOW_LINENR,
+	CONFIG_ANNOTATE_SHOW_TOTAL_PERIOD,
+	CONFIG_GTK_ANNOTATE,
+	CONFIG_GTK_REPORT,
+	CONFIG_GTK_TOP,
+	CONFIG_PAGER_CMD,
+	CONFIG_PAGER_REPORT,
+	CONFIG_PAGER_ANNOTATE,
+	CONFIG_PAGER_TOP,
+	CONFIG_PAGER_DIFF,
+	CONFIG_HELP_FORMAT,
+	CONFIG_HELP_AUTOCORRECT,
+	CONFIG_HIST_PERCENTAGE,
+	CONFIG_UI_SHOW_HEADERS,
+	CONFIG_CALL_GRAPH_RECORD_MODE,
+	CONFIG_CALL_GRAPH_DUMP_SIZE,
+	CONFIG_CALL_GRAPH_PRINT_TYPE,
+	CONFIG_CALL_GRAPH_ORDER,
+	CONFIG_CALL_GRAPH_SORT_KEY,
+	CONFIG_CALL_GRAPH_THRESHOLD,
+	CONFIG_CALL_GRAPH_PRINT_LIMIT,
+	CONFIG_REPORT_GROUP,
+	CONFIG_REPORT_CHILDREN,
+	CONFIG_REPORT_PERCENT_LIMIT,
+	CONFIG_REPORT_QUEUE_SIZE,
+	CONFIG_TOP_CHILDREN,
+	CONFIG_MAN_VIEWER,
+	CONFIG_KMEM_DEFAULT,
+	CONFIG_END
+};
+
+#define CONF_VAR(_sec, _name, _field, _val, _type)			\
+	{ .section = _sec, .name = _name, .default_value._field = _val, .type = _type }
+
+#define CONF_BOOL_VAR(_idx, _sec, _name, _val)				\
+	[CONFIG_##_idx] = CONF_VAR(_sec, _name, b, _val, CONFIG_TYPE_BOOL)
+#define CONF_INT_VAR(_idx, _sec, _name, _val)				\
+	[CONFIG_##_idx] = CONF_VAR(_sec, _name, i, _val, CONFIG_TYPE_INT)
+#define CONF_LONG_VAR(_idx, _sec, _name, _val)				\
+	[CONFIG_##_idx] = CONF_VAR(_sec, _name, l, _val, CONFIG_TYPE_LONG)
+#define CONF_U64_VAR(_idx, _sec, _name, _val)				\
+	[CONFIG_##_idx] = CONF_VAR(_sec, _name, ll, _val, CONFIG_TYPE_U64)
+#define CONF_FLOAT_VAR(_idx, _sec, _name, _val)				\
+	[CONFIG_##_idx] = CONF_VAR(_sec, _name, f, _val, CONFIG_TYPE_FLOAT)
+#define CONF_DOUBLE_VAR(_idx, _sec, _name, _val)			\
+	[CONFIG_##_idx] = CONF_VAR(_sec, _name, d, _val, CONFIG_TYPE_DOUBLE)
+#define CONF_STR_VAR(_idx, _sec, _name, _val)				\
+	[CONFIG_##_idx] = CONF_VAR(_sec, _name, s, _val, CONFIG_TYPE_STRING)
+
 struct perf_config_set *perf_config_set__new(void);
 void perf_config_set__delete(struct perf_config_set *perf_configs);
 
-- 
2.5.0

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [PATCH v2 4/5] perf config: Initialize perf_config_set with all default configs
  2016-03-14 12:16 [RFC][PATCH v2 0/5] perf config: Introduce perf_config_set class Taeung Song
                   ` (2 preceding siblings ...)
  2016-03-14 12:16 ` [PATCH v2 3/5] perf config: Prepare all default configs Taeung Song
@ 2016-03-14 12:16 ` Taeung Song
  2016-03-14 12:16 ` [PATCH v2 5/5] perf config: Add 'list-all' option to show all perf's configs Taeung Song
  4 siblings, 0 replies; 10+ messages in thread
From: Taeung Song @ 2016-03-14 12:16 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: linux-kernel, Jiri Olsa, Namhyung Kim, Ingo Molnar,
	Peter Zijlstra, Taeung Song

To avoid duplicated config variables and
use perf_config_set classifying between standard
perf config variables and unknown or new config
variables other than them, initialize perf_config_set
with all default configs.

And this will be needed when showing all configs with
default value or checking correct type of a config
variable in the near future.

Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
---
 tools/perf/builtin-config.c | 11 +++++++----
 tools/perf/util/config.c    | 27 ++++++++++++++++++++++-----
 tools/perf/util/config.h    |  6 ++++++
 3 files changed, 35 insertions(+), 9 deletions(-)

diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
index 2217772..1bc1121 100644
--- a/tools/perf/builtin-config.c
+++ b/tools/perf/builtin-config.c
@@ -35,20 +35,23 @@ static struct option config_options[] = {
 
 static int show_config(struct perf_config_set *perf_configs)
 {
+	bool has_value = false;
 	struct perf_config_item *config;
 	struct list_head *config_list = &perf_configs->config_list;
 
-	if (list_empty(config_list))
-		return -1;
-
 	list_for_each_entry(config, config_list, list) {
 		char *value = config->value;
 
-		if (value)
+		if (value) {
 			printf("%s.%s=%s\n", config->section,
 			       config->name, value);
+			has_value = true;
+		}
 	}
 
+	if (!has_value)
+		return -1;
+
 	return 0;
 }
 
diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
index 0b9ba51..5289063 100644
--- a/tools/perf/util/config.c
+++ b/tools/perf/util/config.c
@@ -647,6 +647,21 @@ out_free:
 	return ret;
 }
 
+static struct perf_config_set *perf_config_set__init(struct perf_config_set *perf_configs)
+{
+	int i;
+	struct list_head *head = &perf_configs->config_list;
+
+	INIT_LIST_HEAD(&perf_configs->config_list);
+
+	for (i = 0; i != CONFIG_END; i++) {
+		struct perf_config_item *config = &default_configs[i];
+
+		list_add_tail(&config->list, head);
+	}
+	return perf_configs;
+}
+
 struct perf_config_set *perf_config_set__new(void)
 {
 	struct perf_config_set *perf_configs = zalloc(sizeof(*perf_configs));
@@ -654,7 +669,7 @@ struct perf_config_set *perf_config_set__new(void)
 	if (!perf_configs)
 		return NULL;
 
-	INIT_LIST_HEAD(&perf_configs->config_list);
+	perf_config_set__init(perf_configs);
 	perf_config(collect_config, &perf_configs->config_list);
 
 	return perf_configs;
@@ -666,10 +681,12 @@ void perf_config_set__delete(struct perf_config_set *perf_configs)
 
 	list_for_each_entry_safe(pos, item, &perf_configs->config_list, list) {
 		list_del(&pos->list);
-		free((char *)pos->section);
-		free((char *)pos->name);
-		free(pos->value);
-		free(pos);
+		if (pos->is_custom) {
+			free((char *)pos->section);
+			free((char *)pos->name);
+			free(pos->value);
+			free(pos);
+		}
 	}
 
 	free(perf_configs);
diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
index 3c30576..df47420 100644
--- a/tools/perf/util/config.h
+++ b/tools/perf/util/config.h
@@ -14,6 +14,11 @@ enum perf_config_type {
 	CONFIG_TYPE_STRING
 };
 
+/**
+ * struct perf_config_item - element of perf's configs
+ *
+ * @is_custom: unknown or new config other than default config
+ */
 struct perf_config_item {
 	const char *section;
 	const char *name;
@@ -28,6 +33,7 @@ struct perf_config_item {
 		const char *s;
 	} default_value;
 	enum perf_config_type type;
+	bool is_custom;
 	struct list_head list;
 };
 
-- 
2.5.0

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [PATCH v2 5/5] perf config: Add 'list-all' option to show all perf's configs
  2016-03-14 12:16 [RFC][PATCH v2 0/5] perf config: Introduce perf_config_set class Taeung Song
                   ` (3 preceding siblings ...)
  2016-03-14 12:16 ` [PATCH v2 4/5] perf config: Initialize perf_config_set with " Taeung Song
@ 2016-03-14 12:16 ` Taeung Song
  4 siblings, 0 replies; 10+ messages in thread
From: Taeung Song @ 2016-03-14 12:16 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: linux-kernel, Jiri Olsa, Namhyung Kim, Ingo Molnar,
	Peter Zijlstra, Taeung Song

That show all possible config variables with
default values. The syntax examples are like below.

    perf config [<file-option>] [options]

    display all perf config with default values.
    # perf config -a | --list-all

But configs from user or system
config file have a high priority e.g.

    Default of report.children is true
    # cat ~/.perfconfig
    [report]
        children=false

    # perf config --list-all
    ..(omitted)..
    call-graph.threshold=0.500000
    call-graph.print-limit=0
    report.group=true
    report.children=false
    report.percent-limit=0.000000
    report.queue-size=0
    ..(omitted)..

Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
---
 tools/perf/Documentation/perf-config.txt |  6 +++
 tools/perf/builtin-config.c              | 69 +++++++++++++++++++++++++++++++-
 2 files changed, 74 insertions(+), 1 deletion(-)

diff --git a/tools/perf/Documentation/perf-config.txt b/tools/perf/Documentation/perf-config.txt
index 15949e2..d9fb8c3 100644
--- a/tools/perf/Documentation/perf-config.txt
+++ b/tools/perf/Documentation/perf-config.txt
@@ -9,6 +9,8 @@ SYNOPSIS
 --------
 [verse]
 'perf config' [<file-option>] -l | --list
+or
+'perf config' [<file-option>] -a | --list-all
 
 DESCRIPTION
 -----------
@@ -29,6 +31,10 @@ OPTIONS
 	For writing and reading options: write to system-wide
 	'$(sysconfdir)/perfconfig' or read it.
 
+-a::
+--list-all::
+	Show current and all possible config variables with default values.
+
 CONFIGURATION FILE
 ------------------
 
diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
index 1bc1121..7ceae55 100644
--- a/tools/perf/builtin-config.c
+++ b/tools/perf/builtin-config.c
@@ -22,17 +22,75 @@ static const char * const config_usage[] = {
 };
 
 enum actions {
-	ACTION_LIST = 1
+	ACTION_LIST = 1,
+	ACTION_LIST_ALL
 } actions;
 
 static struct option config_options[] = {
 	OPT_SET_UINT('l', "list", &actions,
 		     "show current config variables", ACTION_LIST),
+	OPT_SET_UINT('a', "list-all", &actions,
+		     "show current and all possible config"
+		     " variables with default values", ACTION_LIST_ALL),
 	OPT_BOOLEAN(0, "system", &use_system_config, "use system config file"),
 	OPT_BOOLEAN(0, "user", &use_user_config, "use user config file"),
 	OPT_END()
 };
 
+#define DEFAULT_VAL(_field) config->default_value._field
+
+static char *get_default_value(struct perf_config_item *config)
+{
+	int ret = 0;
+	char *value;
+
+	if (config->is_custom)
+		return NULL;
+
+	if (config->type == CONFIG_TYPE_BOOL)
+		ret = asprintf(&value, "%s",
+			       DEFAULT_VAL(b) ? "true" : "false");
+	else if (config->type == CONFIG_TYPE_INT)
+		ret = asprintf(&value, "%d", DEFAULT_VAL(i));
+	else if (config->type == CONFIG_TYPE_LONG)
+		ret = asprintf(&value, "%u", DEFAULT_VAL(l));
+	else if (config->type == CONFIG_TYPE_U64)
+		ret = asprintf(&value, "%"PRId64, DEFAULT_VAL(ll));
+	else if (config->type == CONFIG_TYPE_FLOAT)
+		ret = asprintf(&value, "%f", DEFAULT_VAL(f));
+	else if (config->type == CONFIG_TYPE_DOUBLE)
+		ret = asprintf(&value, "%f", DEFAULT_VAL(d));
+	else
+		ret = asprintf(&value, "%s", DEFAULT_VAL(s));
+
+	if (ret < 0)
+		return NULL;
+
+	return value;
+}
+
+static int show_all_config(struct perf_config_set *perf_configs)
+{
+	char *value, *default_value;
+	struct perf_config_item *config;
+	struct list_head *config_list = &perf_configs->config_list;
+
+	list_for_each_entry(config, config_list, list) {
+		value = config->value;
+		if (!value)
+			value = default_value = get_default_value(config);
+
+		if (value)
+			printf("%s.%s=%s\n", config->section,
+			       config->name, value);
+
+		free(default_value);
+		default_value = NULL;
+	}
+
+	return 0;
+}
+
 static int show_config(struct perf_config_set *perf_configs)
 {
 	bool has_value = false;
@@ -61,6 +119,9 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused)
 	struct perf_config_set *perf_configs;
 	char *user_config = mkpath("%s/.perfconfig", getenv("HOME"));
 
+	set_option_flag(config_options, 'l', "list", PARSE_OPT_EXCLUSIVE);
+	set_option_flag(config_options, 'a', "list-all", PARSE_OPT_EXCLUSIVE);
+
 	argc = parse_options(argc, argv, config_options, config_usage,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
 
@@ -83,6 +144,12 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused)
 	}
 
 	switch (actions) {
+	case ACTION_LIST_ALL:
+		if (argc)
+			parse_options_usage(config_usage, config_options, "a", 1);
+		else
+			ret = show_all_config(perf_configs);
+		break;
 	case ACTION_LIST:
 		if (argc) {
 			pr_err("Error: takes no arguments\n");
-- 
2.5.0

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/5] perf config: Introduce perf_config_set class
  2016-03-14 12:16 ` [PATCH v2 1/5] " Taeung Song
@ 2016-03-17 12:31   ` Namhyung Kim
  2016-03-17 14:10     ` Taeung Song
  0 siblings, 1 reply; 10+ messages in thread
From: Namhyung Kim @ 2016-03-17 12:31 UTC (permalink / raw)
  To: Taeung Song
  Cc: Arnaldo Carvalho de Melo, linux-kernel, Jiri Olsa, Ingo Molnar,
	Peter Zijlstra

Hi Taeung,

On Mon, Mar 14, 2016 at 09:16:05PM +0900, Taeung Song wrote:
> This infrastructure code was designed for
> upcoming features of perf-config.
> 
> That collect config key-value pairs from user and
> system config files (i.e. user wide ~/.perfconfig
> and system wide $(sysconfdir)/perfconfig)
> to manage perf's configs.
> 
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
> ---
>  tools/perf/builtin-config.c |   1 +
>  tools/perf/util/config.c    | 123 ++++++++++++++++++++++++++++++++++++++++++++
>  tools/perf/util/config.h    |  21 ++++++++
>  3 files changed, 145 insertions(+)
>  create mode 100644 tools/perf/util/config.h
> 
> diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
> index c42448e..412c725 100644
> --- a/tools/perf/builtin-config.c
> +++ b/tools/perf/builtin-config.c
> @@ -12,6 +12,7 @@
>  #include <subcmd/parse-options.h>
>  #include "util/util.h"
>  #include "util/debug.h"
> +#include "util/config.h"
>  
>  static bool use_system_config, use_user_config;
>  
> diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
> index 4e72763..b9660e4 100644
> --- a/tools/perf/util/config.c
> +++ b/tools/perf/util/config.c
> @@ -13,6 +13,7 @@
>  #include <subcmd/exec-cmd.h>
>  #include "util/hist.h"  /* perf_hist_config */
>  #include "util/llvm-utils.h"   /* perf_llvm_config */
> +#include "config.h"
>  
>  #define MAXNAME (256)
>  
> @@ -506,6 +507,128 @@ out:
>  	return ret;
>  }
>  
> +static struct perf_config_item *find_config(struct list_head *config_list,
> +				       const char *section,
> +				       const char *name)
> +{
> +	struct perf_config_item *config;
> +
> +	list_for_each_entry(config, config_list, list) {
> +		if (!strcmp(config->section, section) &&
> +		    !strcmp(config->name, name))
> +			return config;
> +	}

Hmm.. why do you remove the section list?


> +
> +	return NULL;
> +}
> +
> +static struct perf_config_item *add_config(struct list_head *config_list,
> +					   const char *section,
> +					   const char *name)
> +{
> +	struct perf_config_item *config = zalloc(sizeof(*config));
> +
> +	if (!config)
> +		return NULL;
> +
> +	config->section = strdup(section);
> +	if (!section)
> +		goto out_err;
> +
> +	config->name = strdup(name);
> +	if (!name) {
> +		free((char *)config->section);
> +		goto out_err;
> +	}
> +
> +	list_add_tail(&config->list, config_list);
> +	return config;
> +
> +out_err:
> +	free(config);
> +	pr_err("%s: strdup failed\n", __func__);
> +	return NULL;
> +}
> +
> +static int set_value(struct perf_config_item *config, const char *value)
> +{
> +	char *val = strdup(value);
> +
> +	if (!val)
> +		return -1;
> +	config->value = val;

It seems to overwrite old value..

Thanks,
Namhyung


> +
> +	return 0;
> +}
> +
> +static int collect_config(const char *var, const char *value,
> +			  void *configs)
> +{
> +	int ret = 0;
> +	char *ptr, *key;
> +	char *section, *name;
> +	struct perf_config_item *config;
> +	struct list_head *config_list = configs;
> +
> +	key = ptr = strdup(var);
> +	if (!key) {
> +		pr_err("%s: strdup failed\n", __func__);
> +		return -1;
> +	}
> +
> +	section = strsep(&ptr, ".");
> +	name = ptr;
> +	if (name == NULL || value == NULL) {
> +		ret = -1;
> +		goto out_free;
> +	}
> +
> +	config = find_config(config_list, section, name);
> +	if (!config) {
> +		config = add_config(config_list, section, name);
> +		if (!config) {
> +			free(config->section);
> +			free(config->name);
> +			ret = -1;
> +			goto out_free;
> +		}
> +	}
> +
> +	ret = set_value(config, value);
> +
> +out_free:
> +	free(key);
> +	return ret;
> +}
> +
> +struct perf_config_set *perf_config_set__new(void)
> +{
> +	struct perf_config_set *perf_configs = zalloc(sizeof(*perf_configs));
> +
> +	if (!perf_configs)
> +		return NULL;
> +
> +	INIT_LIST_HEAD(&perf_configs->config_list);
> +	perf_config(collect_config, &perf_configs->config_list);
> +
> +	return perf_configs;
> +}
> +
> +void perf_config_set__delete(struct perf_config_set *perf_configs)
> +{
> +	struct perf_config_item *pos, *item;
> +
> +	list_for_each_entry_safe(pos, item, &perf_configs->config_list, list) {
> +		list_del(&pos->list);
> +		free(pos->section);
> +		free(pos->name);
> +		free(pos->value);
> +		free(pos);
> +	}
> +
> +	free(perf_configs);
> +}
> +
>  /*
>   * Call this to report error for your variable that should not
>   * get a boolean value (i.e. "[my] var" means "true").
> diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
> new file mode 100644
> index 0000000..44c226f
> --- /dev/null
> +++ b/tools/perf/util/config.h
> @@ -0,0 +1,21 @@
> +#ifndef __PERF_CONFIG_H
> +#define __PERF_CONFIG_H
> +
> +#include <stdbool.h>
> +#include <linux/list.h>
> +
> +struct perf_config_item {
> +	char *section;
> +	char *name;
> +	char *value;
> +	struct list_head list;
> +};
> +
> +struct perf_config_set {
> +	struct list_head config_list;
> +};
> +
> +struct perf_config_set *perf_config_set__new(void);
> +void perf_config_set__delete(struct perf_config_set *perf_configs);
> +
> +#endif /* __PERF_CONFIG_H */
> -- 
> 2.5.0
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/5] perf config: Introduce perf_config_set class
  2016-03-17 12:31   ` Namhyung Kim
@ 2016-03-17 14:10     ` Taeung Song
  2016-03-17 23:27       ` Namhyung Kim
  0 siblings, 1 reply; 10+ messages in thread
From: Taeung Song @ 2016-03-17 14:10 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Arnaldo Carvalho de Melo, linux-kernel, Jiri Olsa, Ingo Molnar,
	Peter Zijlstra

Hi, Namhyung

On 03/17/2016 09:31 PM, Namhyung Kim wrote:
> Hi Taeung,
>
> On Mon, Mar 14, 2016 at 09:16:05PM +0900, Taeung Song wrote:
>> This infrastructure code was designed for
>> upcoming features of perf-config.
>>
>> That collect config key-value pairs from user and
>> system config files (i.e. user wide ~/.perfconfig
>> and system wide $(sysconfdir)/perfconfig)
>> to manage perf's configs.
>>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Cc: Jiri Olsa <jolsa@kernel.org>
>> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
>> ---
>>   tools/perf/builtin-config.c |   1 +
>>   tools/perf/util/config.c    | 123 ++++++++++++++++++++++++++++++++++++++++++++
>>   tools/perf/util/config.h    |  21 ++++++++
>>   3 files changed, 145 insertions(+)
>>   create mode 100644 tools/perf/util/config.h
>>
>> diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
>> index c42448e..412c725 100644
>> --- a/tools/perf/builtin-config.c
>> +++ b/tools/perf/builtin-config.c
>> @@ -12,6 +12,7 @@
>>   #include <subcmd/parse-options.h>
>>   #include "util/util.h"
>>   #include "util/debug.h"
>> +#include "util/config.h"
>>
>>   static bool use_system_config, use_user_config;
>>
>> diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
>> index 4e72763..b9660e4 100644
>> --- a/tools/perf/util/config.c
>> +++ b/tools/perf/util/config.c
>> @@ -13,6 +13,7 @@
>>   #include <subcmd/exec-cmd.h>
>>   #include "util/hist.h"  /* perf_hist_config */
>>   #include "util/llvm-utils.h"   /* perf_llvm_config */
>> +#include "config.h"
>>
>>   #define MAXNAME (256)
>>
>> @@ -506,6 +507,128 @@ out:
>>   	return ret;
>>   }
>>
>> +static struct perf_config_item *find_config(struct list_head *config_list,
>> +				       const char *section,
>> +				       const char *name)
>> +{
>> +	struct perf_config_item *config;
>> +
>> +	list_for_each_entry(config, config_list, list) {
>> +		if (!strcmp(config->section, section) &&
>> +		    !strcmp(config->name, name))
>> +			return config;
>> +	}
>
> Hmm.. why do you remove the section list?
>

IMHO, there are several reasons

     1) To use only one list (default config, custom config(user/system))

1-1) I used two list that were 'list_head sections'
and 'config_item default_configs[]'. So if checking
type of config variable, two for-loop must be needed
for each list. Because two structure was different i.e.

'sections' list mean config_section list
that each section contain config_element list.
(there wasn't telling about correct type of 'value' instead of 
string(char *))

     struct config_element {
             char *name;
             char *value;
             struct list_head list;
     };

     struct config_section {
             char *name;
             struct list_head element_head;
             struct list_head list;
     };

'struct config_item default_configs[]' mean all default configs.

     struct config_item {
             const char *section;
             const char *name;
             union {
                 bool b;
                 int i;
                 u32 l;
                 u64 ll;
                 float f;
                 double d;
                 const char *s;
             } value;
             enum config_type type;
             const char *desc;
     };


IMHO, I think this is a bit complex
and I want to simplify the perf's config list on perf-config.

     2) A small number of perf's configs

I think perf's configs aren't too many so I think
two structure for section and element aren't needed.

     3) A object for a config variable need to have enough info for itself

This is a bit similar to 1) reason.
If using only 'struct config_item' for the config list,
it can contain section name, name, values(default, user config,
system config, both config), correct type, etc.

If we do, we needn't to find detail for a config variable at other 
objects e.g.
When we find correct type of a config variable,
we needn't to do for-loop for default_configs[] in order to know the type.


I think this is better than old two structure.

>> +
>> +	return NULL;
>> +}
>> +
>> +static struct perf_config_item *add_config(struct list_head *config_list,
>> +					   const char *section,
>> +					   const char *name)
>> +{
>> +	struct perf_config_item *config = zalloc(sizeof(*config));
>> +
>> +	if (!config)
>> +		return NULL;
>> +
>> +	config->section = strdup(section);
>> +	if (!section)
>> +		goto out_err;
>> +
>> +	config->name = strdup(name);
>> +	if (!name) {
>> +		free((char *)config->section);
>> +		goto out_err;
>> +	}
>> +
>> +	list_add_tail(&config->list, config_list);
>> +	return config;
>> +
>> +out_err:
>> +	free(config);
>> +	pr_err("%s: strdup failed\n", __func__);
>> +	return NULL;
>> +}
>> +
>> +static int set_value(struct perf_config_item *config, const char *value)
>> +{
>> +	char *val = strdup(value);
>> +
>> +	if (!val)
>> +		return -1;
>> +	config->value = val;
>
> It seems to overwrite old value..
>

Yes, I know it.
If don't using '--user' or '--system',
there isn't exclusive config file path
then have to read both config files.

But because user config file has a high order of priority,
if two config file has same variable, old value(for system config)
must be overwrote by new value(for user config).


Thanks,
Taeung

>
>
>> +
>> +	return 0;
>> +}
>> +
>> +static int collect_config(const char *var, const char *value,
>> +			  void *configs)
>> +{
>> +	int ret = 0;
>> +	char *ptr, *key;
>> +	char *section, *name;
>> +	struct perf_config_item *config;
>> +	struct list_head *config_list = configs;
>> +
>> +	key = ptr = strdup(var);
>> +	if (!key) {
>> +		pr_err("%s: strdup failed\n", __func__);
>> +		return -1;
>> +	}
>> +
>> +	section = strsep(&ptr, ".");
>> +	name = ptr;
>> +	if (name == NULL || value == NULL) {
>> +		ret = -1;
>> +		goto out_free;
>> +	}
>> +
>> +	config = find_config(config_list, section, name);
>> +	if (!config) {
>> +		config = add_config(config_list, section, name);
>> +		if (!config) {
>> +			free(config->section);
>> +			free(config->name);
>> +			ret = -1;
>> +			goto out_free;
>> +		}
>> +	}
>> +
>> +	ret = set_value(config, value);
>> +
>> +out_free:
>> +	free(key);
>> +	return ret;
>> +}
>> +
>> +struct perf_config_set *perf_config_set__new(void)
>> +{
>> +	struct perf_config_set *perf_configs = zalloc(sizeof(*perf_configs));
>> +
>> +	if (!perf_configs)
>> +		return NULL;
>> +
>> +	INIT_LIST_HEAD(&perf_configs->config_list);
>> +	perf_config(collect_config, &perf_configs->config_list);
>> +
>> +	return perf_configs;
>> +}
>> +
>> +void perf_config_set__delete(struct perf_config_set *perf_configs)
>> +{
>> +	struct perf_config_item *pos, *item;
>> +
>> +	list_for_each_entry_safe(pos, item, &perf_configs->config_list, list) {
>> +		list_del(&pos->list);
>> +		free(pos->section);
>> +		free(pos->name);
>> +		free(pos->value);
>> +		free(pos);
>> +	}
>> +
>> +	free(perf_configs);
>> +}
>> +
>>   /*
>>    * Call this to report error for your variable that should not
>>    * get a boolean value (i.e. "[my] var" means "true").
>> diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
>> new file mode 100644
>> index 0000000..44c226f
>> --- /dev/null
>> +++ b/tools/perf/util/config.h
>> @@ -0,0 +1,21 @@
>> +#ifndef __PERF_CONFIG_H
>> +#define __PERF_CONFIG_H
>> +
>> +#include <stdbool.h>
>> +#include <linux/list.h>
>> +
>> +struct perf_config_item {
>> +	char *section;
>> +	char *name;
>> +	char *value;
>> +	struct list_head list;
>> +};
>> +
>> +struct perf_config_set {
>> +	struct list_head config_list;
>> +};
>> +
>> +struct perf_config_set *perf_config_set__new(void);
>> +void perf_config_set__delete(struct perf_config_set *perf_configs);
>> +
>> +#endif /* __PERF_CONFIG_H */
>> --
>> 2.5.0
>>

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/5] perf config: Introduce perf_config_set class
  2016-03-17 14:10     ` Taeung Song
@ 2016-03-17 23:27       ` Namhyung Kim
  2016-03-18  0:52         ` Taeung Song
  0 siblings, 1 reply; 10+ messages in thread
From: Namhyung Kim @ 2016-03-17 23:27 UTC (permalink / raw)
  To: Taeung Song
  Cc: Arnaldo Carvalho de Melo, linux-kernel, Jiri Olsa, Ingo Molnar,
	Peter Zijlstra

On Thu, Mar 17, 2016 at 11:10:12PM +0900, Taeung Song wrote:
> Hi, Namhyung
> 
> On 03/17/2016 09:31 PM, Namhyung Kim wrote:
> >Hi Taeung,
> >
> >On Mon, Mar 14, 2016 at 09:16:05PM +0900, Taeung Song wrote:
> >>This infrastructure code was designed for
> >>upcoming features of perf-config.
> >>
> >>That collect config key-value pairs from user and
> >>system config files (i.e. user wide ~/.perfconfig
> >>and system wide $(sysconfdir)/perfconfig)
> >>to manage perf's configs.
> >>
> >>Cc: Namhyung Kim <namhyung@kernel.org>
> >>Cc: Jiri Olsa <jolsa@kernel.org>
> >>Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
> >>---
> >>  tools/perf/builtin-config.c |   1 +
> >>  tools/perf/util/config.c    | 123 ++++++++++++++++++++++++++++++++++++++++++++
> >>  tools/perf/util/config.h    |  21 ++++++++
> >>  3 files changed, 145 insertions(+)
> >>  create mode 100644 tools/perf/util/config.h
> >>
> >>diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
> >>index c42448e..412c725 100644
> >>--- a/tools/perf/builtin-config.c
> >>+++ b/tools/perf/builtin-config.c
> >>@@ -12,6 +12,7 @@
> >>  #include <subcmd/parse-options.h>
> >>  #include "util/util.h"
> >>  #include "util/debug.h"
> >>+#include "util/config.h"
> >>
> >>  static bool use_system_config, use_user_config;
> >>
> >>diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
> >>index 4e72763..b9660e4 100644
> >>--- a/tools/perf/util/config.c
> >>+++ b/tools/perf/util/config.c
> >>@@ -13,6 +13,7 @@
> >>  #include <subcmd/exec-cmd.h>
> >>  #include "util/hist.h"  /* perf_hist_config */
> >>  #include "util/llvm-utils.h"   /* perf_llvm_config */
> >>+#include "config.h"
> >>
> >>  #define MAXNAME (256)
> >>
> >>@@ -506,6 +507,128 @@ out:
> >>  	return ret;
> >>  }
> >>
> >>+static struct perf_config_item *find_config(struct list_head *config_list,
> >>+				       const char *section,
> >>+				       const char *name)
> >>+{
> >>+	struct perf_config_item *config;
> >>+
> >>+	list_for_each_entry(config, config_list, list) {
> >>+		if (!strcmp(config->section, section) &&
> >>+		    !strcmp(config->name, name))
> >>+			return config;
> >>+	}
> >
> >Hmm.. why do you remove the section list?
> >
> 
> IMHO, there are several reasons
> 
>     1) To use only one list (default config, custom config(user/system))
> 
> 1-1) I used two list that were 'list_head sections'
> and 'config_item default_configs[]'. So if checking
> type of config variable, two for-loop must be needed
> for each list. Because two structure was different i.e.
> 
> 'sections' list mean config_section list
> that each section contain config_element list.
> (there wasn't telling about correct type of 'value' instead of string(char
> *))
> 
>     struct config_element {
>             char *name;
>             char *value;
>             struct list_head list;
>     };
> 
>     struct config_section {
>             char *name;
>             struct list_head element_head;
>             struct list_head list;
>     };
> 
> 'struct config_item default_configs[]' mean all default configs.
> 
>     struct config_item {
>             const char *section;
>             const char *name;
>             union {
>                 bool b;
>                 int i;
>                 u32 l;
>                 u64 ll;
>                 float f;
>                 double d;
>                 const char *s;
>             } value;
>             enum config_type type;
>             const char *desc;
>     };
> 
> 
> IMHO, I think this is a bit complex
> and I want to simplify the perf's config list on perf-config.
> 
>     2) A small number of perf's configs
> 
> I think perf's configs aren't too many so I think
> two structure for section and element aren't needed.

OK.


> 
>     3) A object for a config variable need to have enough info for itself
> 
> This is a bit similar to 1) reason.
> If using only 'struct config_item' for the config list,
> it can contain section name, name, values(default, user config,
> system config, both config), correct type, etc.
> 
> If we do, we needn't to find detail for a config variable at other objects
> e.g.
> When we find correct type of a config variable,
> we needn't to do for-loop for default_configs[] in order to know the
> type.

I'm not sure I understand you correctly, but I think this is not
related to the two-level structure.


> 
> 
> I think this is better than old two structure.
> 
> >>+
> >>+	return NULL;
> >>+}
> >>+
> >>+static struct perf_config_item *add_config(struct list_head *config_list,
> >>+					   const char *section,
> >>+					   const char *name)
> >>+{
> >>+	struct perf_config_item *config = zalloc(sizeof(*config));
> >>+
> >>+	if (!config)
> >>+		return NULL;
> >>+
> >>+	config->section = strdup(section);
> >>+	if (!section)
> >>+		goto out_err;
> >>+
> >>+	config->name = strdup(name);
> >>+	if (!name) {
> >>+		free((char *)config->section);
> >>+		goto out_err;
> >>+	}
> >>+
> >>+	list_add_tail(&config->list, config_list);
> >>+	return config;
> >>+
> >>+out_err:
> >>+	free(config);
> >>+	pr_err("%s: strdup failed\n", __func__);
> >>+	return NULL;
> >>+}
> >>+
> >>+static int set_value(struct perf_config_item *config, const char *value)
> >>+{
> >>+	char *val = strdup(value);
> >>+
> >>+	if (!val)
> >>+		return -1;
> >>+	config->value = val;
> >
> >It seems to overwrite old value..
> >
> 
> Yes, I know it.
> If don't using '--user' or '--system',
> there isn't exclusive config file path
> then have to read both config files.
> 
> But because user config file has a high order of priority,
> if two config file has same variable, old value(for system config)
> must be overwrote by new value(for user config).

But shouldn't it free the old value before overwriting?

Thanks,
Namhyung


> 
> >
> >
> >>+
> >>+	return 0;
> >>+}

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/5] perf config: Introduce perf_config_set class
  2016-03-17 23:27       ` Namhyung Kim
@ 2016-03-18  0:52         ` Taeung Song
  0 siblings, 0 replies; 10+ messages in thread
From: Taeung Song @ 2016-03-18  0:52 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Arnaldo Carvalho de Melo, linux-kernel, Jiri Olsa, Ingo Molnar,
	Peter Zijlstra



On 03/18/2016 08:27 AM, Namhyung Kim wrote:
> On Thu, Mar 17, 2016 at 11:10:12PM +0900, Taeung Song wrote:
>> Hi, Namhyung
>>
>> On 03/17/2016 09:31 PM, Namhyung Kim wrote:
>>> Hi Taeung,
>>>
>>> On Mon, Mar 14, 2016 at 09:16:05PM +0900, Taeung Song wrote:
>>>> This infrastructure code was designed for
>>>> upcoming features of perf-config.
>>>>
>>>> That collect config key-value pairs from user and
>>>> system config files (i.e. user wide ~/.perfconfig
>>>> and system wide $(sysconfdir)/perfconfig)
>>>> to manage perf's configs.
>>>>
>>>> Cc: Namhyung Kim <namhyung@kernel.org>
>>>> Cc: Jiri Olsa <jolsa@kernel.org>
>>>> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
>>>> ---
>>>>   tools/perf/builtin-config.c |   1 +
>>>>   tools/perf/util/config.c    | 123 ++++++++++++++++++++++++++++++++++++++++++++
>>>>   tools/perf/util/config.h    |  21 ++++++++
>>>>   3 files changed, 145 insertions(+)
>>>>   create mode 100644 tools/perf/util/config.h
>>>>
>>>> diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
>>>> index c42448e..412c725 100644
>>>> --- a/tools/perf/builtin-config.c
>>>> +++ b/tools/perf/builtin-config.c
>>>> @@ -12,6 +12,7 @@
>>>>   #include <subcmd/parse-options.h>
>>>>   #include "util/util.h"
>>>>   #include "util/debug.h"
>>>> +#include "util/config.h"
>>>>
>>>>   static bool use_system_config, use_user_config;
>>>>
>>>> diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
>>>> index 4e72763..b9660e4 100644
>>>> --- a/tools/perf/util/config.c
>>>> +++ b/tools/perf/util/config.c
>>>> @@ -13,6 +13,7 @@
>>>>   #include <subcmd/exec-cmd.h>
>>>>   #include "util/hist.h"  /* perf_hist_config */
>>>>   #include "util/llvm-utils.h"   /* perf_llvm_config */
>>>> +#include "config.h"
>>>>
>>>>   #define MAXNAME (256)
>>>>
>>>> @@ -506,6 +507,128 @@ out:
>>>>   	return ret;
>>>>   }
>>>>
>>>> +static struct perf_config_item *find_config(struct list_head *config_list,
>>>> +				       const char *section,
>>>> +				       const char *name)
>>>> +{
>>>> +	struct perf_config_item *config;
>>>> +
>>>> +	list_for_each_entry(config, config_list, list) {
>>>> +		if (!strcmp(config->section, section) &&
>>>> +		    !strcmp(config->name, name))
>>>> +			return config;
>>>> +	}
>>>
>>> Hmm.. why do you remove the section list?
>>>
>>
>> IMHO, there are several reasons
>>
>>      1) To use only one list (default config, custom config(user/system))
>>
>> 1-1) I used two list that were 'list_head sections'
>> and 'config_item default_configs[]'. So if checking
>> type of config variable, two for-loop must be needed
>> for each list. Because two structure was different i.e.
>>
>> 'sections' list mean config_section list
>> that each section contain config_element list.
>> (there wasn't telling about correct type of 'value' instead of string(char
>> *))
>>
>>      struct config_element {
>>              char *name;
>>              char *value;
>>              struct list_head list;
>>      };
>>
>>      struct config_section {
>>              char *name;
>>              struct list_head element_head;
>>              struct list_head list;
>>      };
>>
>> 'struct config_item default_configs[]' mean all default configs.
>>
>>      struct config_item {
>>              const char *section;
>>              const char *name;
>>              union {
>>                  bool b;
>>                  int i;
>>                  u32 l;
>>                  u64 ll;
>>                  float f;
>>                  double d;
>>                  const char *s;
>>              } value;
>>              enum config_type type;
>>              const char *desc;
>>      };
>>
>>
>> IMHO, I think this is a bit complex
>> and I want to simplify the perf's config list on perf-config.
>>
>>      2) A small number of perf's configs
>>
>> I think perf's configs aren't too many so I think
>> two structure for section and element aren't needed.
>
> OK.
>
>
>>
>>      3) A object for a config variable need to have enough info for itself
>>
>> This is a bit similar to 1) reason.
>> If using only 'struct config_item' for the config list,
>> it can contain section name, name, values(default, user config,
>> system config, both config), correct type, etc.
>>
>> If we do, we needn't to find detail for a config variable at other objects
>> e.g.
>> When we find correct type of a config variable,
>> we needn't to do for-loop for default_configs[] in order to know the
>> type.
>
> I'm not sure I understand you correctly, but I think this is not
> related to the two-level structure.

What you said is right.
I just thought using two lists (list_head sections, default_configs[])
was complex..
At this patchset, I only focused on simplifying the config list on 
perf-config..

As you said, it hasn't problems to use the two-level structure i.e.

     struct config_element
     struct config_section (that has multiple elements)

So, what about this structures ?
(you may already think about it, but..)

     struct config_element {
             char *name;
             char *value; /*from the two config file (user/system)*/
             union {
                 bool b;
                 int i;
                 u32 l;
                 u64 ll;
                 float f;
                 double d;
                 const char *s;
             } default_value; /* perf's default config value */
             enum config_type type;
             struct list_head list;
     };

     struct config_section {
             char *name;
             struct list_head element_head;
             struct list_head list;
     };

Because I want to use only one list (default_configs + sections)
for more concise code than old code.

>
>>
>>
>> I think this is better than old two structure.
>>
>>>> +
>>>> +	return NULL;
>>>> +}
>>>> +
>>>> +static struct perf_config_item *add_config(struct list_head *config_list,
>>>> +					   const char *section,
>>>> +					   const char *name)
>>>> +{
>>>> +	struct perf_config_item *config = zalloc(sizeof(*config));
>>>> +
>>>> +	if (!config)
>>>> +		return NULL;
>>>> +
>>>> +	config->section = strdup(section);
>>>> +	if (!section)
>>>> +		goto out_err;
>>>> +
>>>> +	config->name = strdup(name);
>>>> +	if (!name) {
>>>> +		free((char *)config->section);
>>>> +		goto out_err;
>>>> +	}
>>>> +
>>>> +	list_add_tail(&config->list, config_list);
>>>> +	return config;
>>>> +
>>>> +out_err:
>>>> +	free(config);
>>>> +	pr_err("%s: strdup failed\n", __func__);
>>>> +	return NULL;
>>>> +}
>>>> +
>>>> +static int set_value(struct perf_config_item *config, const char *value)
>>>> +{
>>>> +	char *val = strdup(value);
>>>> +
>>>> +	if (!val)
>>>> +		return -1;
>>>> +	config->value = val;
>>>
>>> It seems to overwrite old value..
>>>
>>
>> Yes, I know it.
>> If don't using '--user' or '--system',
>> there isn't exclusive config file path
>> then have to read both config files.
>>
>> But because user config file has a high order of priority,
>> if two config file has same variable, old value(for system config)
>> must be overwrote by new value(for user config).
>
> But shouldn't it free the old value before overwriting?
>

Sorry, I missed free() out.
I'll fix it.

Thanks,
Taeung

>
>>
>>>
>>>
>>>> +
>>>> +	return 0;
>>>> +}

^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-03-18  0:52 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-14 12:16 [RFC][PATCH v2 0/5] perf config: Introduce perf_config_set class Taeung Song
2016-03-14 12:16 ` [PATCH v2 1/5] " Taeung Song
2016-03-17 12:31   ` Namhyung Kim
2016-03-17 14:10     ` Taeung Song
2016-03-17 23:27       ` Namhyung Kim
2016-03-18  0:52         ` Taeung Song
2016-03-14 12:16 ` [PATCH v2 2/5] perf config: Let show_config() work with perf_config_set Taeung Song
2016-03-14 12:16 ` [PATCH v2 3/5] perf config: Prepare all default configs Taeung Song
2016-03-14 12:16 ` [PATCH v2 4/5] perf config: Initialize perf_config_set with " Taeung Song
2016-03-14 12:16 ` [PATCH v2 5/5] perf config: Add 'list-all' option to show all perf's configs Taeung Song

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).