* [RFC][PATCH] perf config: Introduce perf_config_set class
@ 2016-03-10 14:04 Taeung Song
2016-03-11 14:11 ` Namhyung Kim
0 siblings, 1 reply; 5+ messages in thread
From: Taeung Song @ 2016-03-10 14:04 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: linux-kernel, Jiri Olsa, Namhyung Kim, Ingo Molnar,
Peter Zijlstra, Taeung Song
In order to variously handle config key-value pairs,
prepare all default configs and collect
config key-value pairs from user and
system config files (i.e user wide ~/.perfconfig
and system wide $(sysconfdir)/perfconfig)
The various purposes are like
showing current configs,
in the near future, showing all configs with
default value, getting current configs or
setting configs that user type, etc.
There aren't additional functionalities by appearances
excluding handling the error by file status.
(e.g checking permission, whether it is empty or etc)
and when the two config files have each other's
values of same key 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
After:
# perf config --user --list
top.children=false
# perf config --system --list
top.children=true
# perf config --list
top.children=false
But this patch is designed with considering
not only current functionality but also upcoming features.
And send [TEST REPORT] email follwing this patch
to save time for review. (kinds of test: BASIC, EXCEP HANDLING)
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 | 61 +++++++---
tools/perf/util/cache.h | 2 -
tools/perf/util/config.c | 268 +++++++++++++++++++++++++++++++++++++++++---
tools/perf/util/config.h | 138 +++++++++++++++++++++++
4 files changed, 433 insertions(+), 36 deletions(-)
create mode 100644 tools/perf/util/config.h
diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
index c42448e..d79f4d9 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;
@@ -32,21 +33,40 @@ 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;
+ enum perf_config_kind pos = perf_configs->pos;
+ bool *file_usable = perf_configs->file_usable;
+ struct list_head *config_list = &perf_configs->config_list;
+
+ if (pos == BOTH) {
+ if (!file_usable[USER] && !file_usable[SYSTEM])
+ return -1;
+ } else if (!file_usable[pos])
+ return -1;
+
+ list_for_each_entry(config, config_list, list) {
+ char *value = config->value[pos];
+
+ if (value)
+ printf("%s.%s=%s\n", config->section,
+ config->name, value);
+ }
return 0;
}
int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused)
{
+ /*
+ * The perf_configs that contains not only default configs
+ * but also key-value pairs from config files.
+ * (i.e user wide ~/.perfconfig and system wide $(sysconfdir)/perfconfig)
+ * This is designed to be used by several functions that handle config set.
+ */
+ struct perf_config_set *perf_configs;
int ret = 0;
- char *user_config = mkpath("%s/.perfconfig", getenv("HOME"));
argc = parse_options(argc, argv, config_options, config_usage,
PARSE_OPT_STOP_AT_NON_OPTION);
@@ -58,10 +78,17 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused)
return -1;
}
- if (use_system_config)
- config_exclusive_filename = perf_etc_perfconfig();
- else if (use_user_config)
- config_exclusive_filename = user_config;
+ perf_configs = perf_config_set__new();
+ if (!perf_configs)
+ return -1;
+
+ if (use_user_config)
+ perf_configs->pos = USER;
+ else if (use_system_config)
+ perf_configs->pos = SYSTEM;
+ else
+ perf_configs->pos = BOTH;
+
switch (actions) {
case ACTION_LIST:
@@ -69,19 +96,17 @@ 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 = perf_config(show_config, NULL);
- if (ret < 0) {
- const char * config_filename = config_exclusive_filename;
- if (!config_exclusive_filename)
- config_filename = user_config;
+ ret = show_config(perf_configs);
+ if (ret < 0)
pr_err("Nothing configured, "
- "please check your %s \n", config_filename);
- }
+ "please check your %s \n",
+ perf_configs->file_path[perf_configs->pos]);
}
break;
default:
usage_with_options(config_usage, config_options);
}
+ perf_config_set__delete(perf_configs);
return ret;
}
diff --git a/tools/perf/util/cache.h b/tools/perf/util/cache.h
index 3ca453f..ba657ad 100644
--- a/tools/perf/util/cache.h
+++ b/tools/perf/util/cache.h
@@ -23,8 +23,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 *);
extern int perf_default_config(const char *, const char *, void *);
extern int perf_config(config_fn_t fn, void *);
diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
index 4e72763..f827932 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)
@@ -26,7 +27,53 @@ static const char *config_file_name;
static int config_linenr;
static int config_file_eof;
-const char *config_exclusive_filename;
+static 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)
{
@@ -458,21 +505,10 @@ static int perf_config_global(void)
return !perf_env_bool("PERF_CONFIG_NOGLOBAL", 0);
}
-int perf_config(config_fn_t fn, void *data)
+static char *perf_user_perfconfig(void)
{
- int ret = 0, found = 0;
- const char *home = NULL;
+ const char *home = getenv("HOME");
- /* 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)) {
- ret += perf_config_from_file(fn, perf_etc_perfconfig(),
- data);
- found += 1;
- }
-
- home = getenv("HOME");
if (perf_config_global() && home) {
char *user_config = strdup(mkpath("%s/.perfconfig", home));
struct stat st;
@@ -495,17 +531,217 @@ int perf_config(config_fn_t fn, void *data)
if (!st.st_size)
goto out_free;
- ret += perf_config_from_file(fn, user_config, data);
- found += 1;
+ return user_config;
+
out_free:
free(user_config);
}
out:
+ return NULL;
+}
+
+int perf_config(config_fn_t fn, void *data)
+{
+ int ret = 0, found = 0;
+ char *user_config;
+
+ /* 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)) {
+ ret += perf_config_from_file(fn, perf_etc_perfconfig(),
+ data);
+ found += 1;
+ }
+
+ user_config = perf_user_perfconfig();
+ if (user_config) {
+ ret += perf_config_from_file(fn, user_config, data);
+ found += 1;
+ free(user_config);
+ }
+
if (found == 0)
return -1;
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->is_custom = true;
+ 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, enum perf_config_kind pos,
+ const char *value)
+{
+ char *val = strdup(value);
+
+ if (!val)
+ return -1;
+
+ config->value[BOTH] = val;
+ config->value[pos] = val;
+ return 0;
+}
+
+static int collect_current_config(const char *var, const char *value,
+ void *perf_config_set)
+{
+ int ret = 0;
+ char *ptr, *key;
+ char *section, *name;
+ struct perf_config_item *config;
+ struct perf_config_set *perf_configs = perf_config_set;
+ struct list_head *config_list = &perf_configs->config_list;
+
+ 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_err;
+ }
+
+ config = find_config(config_list, section, name);
+ if (!config) {
+ config = add_config(config_list, section, name);
+ if (!config) {
+ free((char *)config->section);
+ free((char *)config->name);
+ goto out_err;
+ }
+ }
+ ret = set_value(config, perf_configs->pos, value);
+
+out_err:
+ free(key);
+ 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)
+{
+ int ret;
+ struct perf_config_set *perf_configs = zalloc(sizeof(*perf_configs));
+ char *user_config = perf_user_perfconfig();
+ char *system_config = strdup(perf_etc_perfconfig());
+
+ if (!system_config)
+ goto out_err;
+
+ perf_config_set__init(perf_configs);
+
+ if (!access(system_config, R_OK)) {
+ perf_configs->pos = SYSTEM;
+ ret = perf_config_from_file(collect_current_config, system_config,
+ perf_configs);
+ if (ret == 0)
+ perf_configs->file_usable[SYSTEM] = true;
+ }
+
+ if (user_config) {
+ perf_configs->pos = USER;
+ ret = perf_config_from_file(collect_current_config, user_config,
+ perf_configs);
+ if (ret == 0)
+ perf_configs->file_usable[USER] = true;
+ } else {
+ user_config = strdup(mkpath("%s/.perfconfig", getenv("HOME")));
+ if (!user_config)
+ goto out_err;
+ }
+
+ /* user config file has order of priority */
+ perf_configs->file_path[BOTH] = user_config;
+ perf_configs->file_path[USER] = user_config;
+ perf_configs->file_path[SYSTEM] = system_config;
+
+ return perf_configs;
+
+out_err:
+ pr_err("%s: strdup failed\n", __func__);
+ free(perf_configs);
+ return NULL;
+}
+
+void perf_config_set__delete(struct perf_config_set *perf_configs)
+{
+ struct perf_config_item *pos, *item;
+
+ free(perf_configs->file_path[USER]);
+ free(perf_configs->file_path[SYSTEM]);
+
+ list_for_each_entry_safe(pos, item, &perf_configs->config_list, list) {
+ list_del(&pos->list);
+ if (pos->is_custom) {
+ free((char *)pos->section);
+ free((char *)pos->name);
+ }
+ free(pos->value[USER]);
+ free(pos->value[SYSTEM]);
+ }
+
+ 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..2523221
--- /dev/null
+++ b/tools/perf/util/config.h
@@ -0,0 +1,138 @@
+#ifndef __PERF_CONFIG_H
+#define __PERF_CONFIG_H
+
+#include <stdbool.h>
+#include <linux/list.h>
+
+/**
+ * enum perf_config_kind - three kinds of config information
+ *
+ * @USER: user wide ~/.perfconfig
+ * @SYSTEM: systeom wide $(sysconfdir)/perfconfig
+ * @BOTH: both the config files
+ * but user config file has order of priority.
+ */
+#define CONFIG_KIND 3
+enum perf_config_kind {
+ BOTH,
+ USER,
+ SYSTEM,
+};
+
+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 - element of perf's configs
+ *
+ * @value: array that has values for each kind (USER/SYSTEM/BOTH)
+ * @is_custom: if the user or system config files contain this
+ */
+struct perf_config_item {
+ const char *section;
+ const char *name;
+ char *value[CONFIG_KIND];
+ union {
+ bool b;
+ int i;
+ u32 l;
+ u64 ll;
+ float f;
+ double d;
+ const char *s;
+ } default_value;
+ enum perf_config_type type;
+ bool is_custom;
+ struct list_head list;
+};
+
+/**
+ * struct perf_config_set - perf's config set from the config files
+ *
+ * @file_path: array that has paths of config files
+ * @pos: current major config file
+ * @config_list: perf_config_item list head
+ */
+struct perf_config_set {
+ enum perf_config_kind pos;
+ char *file_path[CONFIG_KIND];
+ bool file_usable[CONFIG_KIND];
+ 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);
+
+#endif /* __PERF_CONFIG_H */
--
2.5.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [RFC][PATCH] perf config: Introduce perf_config_set class
2016-03-10 14:04 [RFC][PATCH] perf config: Introduce perf_config_set class Taeung Song
@ 2016-03-11 14:11 ` Namhyung Kim
2016-03-11 15:08 ` Taeung Song
0 siblings, 1 reply; 5+ messages in thread
From: Namhyung Kim @ 2016-03-11 14:11 UTC (permalink / raw)
To: Taeung Song
Cc: Arnaldo Carvalho de Melo, linux-kernel, Jiri Olsa, Ingo Molnar,
Peter Zijlstra
Hi Taeung,
On Thu, Mar 10, 2016 at 11:04:27PM +0900, Taeung Song wrote:
> In order to variously handle config key-value pairs,
> prepare all default configs and collect
> config key-value pairs from user and
> system config files (i.e user wide ~/.perfconfig
> and system wide $(sysconfdir)/perfconfig)
>
> The various purposes are like
> showing current configs,
> in the near future, showing all configs with
> default value, getting current configs or
> setting configs that user type, etc.
>
> There aren't additional functionalities by appearances
> excluding handling the error by file status.
> (e.g checking permission, whether it is empty or etc)
> and when the two config files have each other's
> values of same key 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
>
> After:
> # perf config --user --list
> top.children=false
>
> # perf config --system --list
> top.children=true
>
> # perf config --list
> top.children=false
>
> But this patch is designed with considering
> not only current functionality but also upcoming features.
>
> And send [TEST REPORT] email follwing this patch
> to save time for review. (kinds of test: BASIC, EXCEP HANDLING)
>
> 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 | 61 +++++++---
> tools/perf/util/cache.h | 2 -
> tools/perf/util/config.c | 268 +++++++++++++++++++++++++++++++++++++++++---
> tools/perf/util/config.h | 138 +++++++++++++++++++++++
> 4 files changed, 433 insertions(+), 36 deletions(-)
> create mode 100644 tools/perf/util/config.h
This is a pretty big change. I recommend you to split the patch into
pieces. I guess it can consist of 3 patches at least.
1. implement basic perf_config_item/set operartion
2. add actual config items using the above ops
3. handle system/user (or both) case
Also I think it'd be better just keeping a single config value instead
of 3 kinds. Maybe you can read system-wide config first and overwrite
them with user config (for the 'both' case).
Thanks,
Namhyung
>
> diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
> index c42448e..d79f4d9 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;
>
> @@ -32,21 +33,40 @@ 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;
> + enum perf_config_kind pos = perf_configs->pos;
> + bool *file_usable = perf_configs->file_usable;
> + struct list_head *config_list = &perf_configs->config_list;
> +
> + if (pos == BOTH) {
> + if (!file_usable[USER] && !file_usable[SYSTEM])
> + return -1;
> + } else if (!file_usable[pos])
> + return -1;
> +
> + list_for_each_entry(config, config_list, list) {
> + char *value = config->value[pos];
> +
> + if (value)
> + printf("%s.%s=%s\n", config->section,
> + config->name, value);
> + }
>
> return 0;
> }
>
> int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused)
> {
> + /*
> + * The perf_configs that contains not only default configs
> + * but also key-value pairs from config files.
> + * (i.e user wide ~/.perfconfig and system wide $(sysconfdir)/perfconfig)
> + * This is designed to be used by several functions that handle config set.
> + */
> + struct perf_config_set *perf_configs;
> int ret = 0;
> - char *user_config = mkpath("%s/.perfconfig", getenv("HOME"));
>
> argc = parse_options(argc, argv, config_options, config_usage,
> PARSE_OPT_STOP_AT_NON_OPTION);
> @@ -58,10 +78,17 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused)
> return -1;
> }
>
> - if (use_system_config)
> - config_exclusive_filename = perf_etc_perfconfig();
> - else if (use_user_config)
> - config_exclusive_filename = user_config;
> + perf_configs = perf_config_set__new();
> + if (!perf_configs)
> + return -1;
> +
> + if (use_user_config)
> + perf_configs->pos = USER;
> + else if (use_system_config)
> + perf_configs->pos = SYSTEM;
> + else
> + perf_configs->pos = BOTH;
> +
>
> switch (actions) {
> case ACTION_LIST:
> @@ -69,19 +96,17 @@ 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 = perf_config(show_config, NULL);
> - if (ret < 0) {
> - const char * config_filename = config_exclusive_filename;
> - if (!config_exclusive_filename)
> - config_filename = user_config;
> + ret = show_config(perf_configs);
> + if (ret < 0)
> pr_err("Nothing configured, "
> - "please check your %s \n", config_filename);
> - }
> + "please check your %s \n",
> + perf_configs->file_path[perf_configs->pos]);
> }
> break;
> default:
> usage_with_options(config_usage, config_options);
> }
>
> + perf_config_set__delete(perf_configs);
> return ret;
> }
> diff --git a/tools/perf/util/cache.h b/tools/perf/util/cache.h
> index 3ca453f..ba657ad 100644
> --- a/tools/perf/util/cache.h
> +++ b/tools/perf/util/cache.h
> @@ -23,8 +23,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 *);
> extern int perf_default_config(const char *, const char *, void *);
> extern int perf_config(config_fn_t fn, void *);
> diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
> index 4e72763..f827932 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)
>
> @@ -26,7 +27,53 @@ static const char *config_file_name;
> static int config_linenr;
> static int config_file_eof;
>
> -const char *config_exclusive_filename;
> +static 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)
> {
> @@ -458,21 +505,10 @@ static int perf_config_global(void)
> return !perf_env_bool("PERF_CONFIG_NOGLOBAL", 0);
> }
>
> -int perf_config(config_fn_t fn, void *data)
> +static char *perf_user_perfconfig(void)
> {
> - int ret = 0, found = 0;
> - const char *home = NULL;
> + const char *home = getenv("HOME");
>
> - /* 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)) {
> - ret += perf_config_from_file(fn, perf_etc_perfconfig(),
> - data);
> - found += 1;
> - }
> -
> - home = getenv("HOME");
> if (perf_config_global() && home) {
> char *user_config = strdup(mkpath("%s/.perfconfig", home));
> struct stat st;
> @@ -495,17 +531,217 @@ int perf_config(config_fn_t fn, void *data)
> if (!st.st_size)
> goto out_free;
>
> - ret += perf_config_from_file(fn, user_config, data);
> - found += 1;
> + return user_config;
> +
> out_free:
> free(user_config);
> }
> out:
> + return NULL;
> +}
> +
> +int perf_config(config_fn_t fn, void *data)
> +{
> + int ret = 0, found = 0;
> + char *user_config;
> +
> + /* 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)) {
> + ret += perf_config_from_file(fn, perf_etc_perfconfig(),
> + data);
> + found += 1;
> + }
> +
> + user_config = perf_user_perfconfig();
> + if (user_config) {
> + ret += perf_config_from_file(fn, user_config, data);
> + found += 1;
> + free(user_config);
> + }
> +
> if (found == 0)
> return -1;
> 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->is_custom = true;
> + 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, enum perf_config_kind pos,
> + const char *value)
> +{
> + char *val = strdup(value);
> +
> + if (!val)
> + return -1;
> +
> + config->value[BOTH] = val;
> + config->value[pos] = val;
> + return 0;
> +}
> +
> +static int collect_current_config(const char *var, const char *value,
> + void *perf_config_set)
> +{
> + int ret = 0;
> + char *ptr, *key;
> + char *section, *name;
> + struct perf_config_item *config;
> + struct perf_config_set *perf_configs = perf_config_set;
> + struct list_head *config_list = &perf_configs->config_list;
> +
> + 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_err;
> + }
> +
> + config = find_config(config_list, section, name);
> + if (!config) {
> + config = add_config(config_list, section, name);
> + if (!config) {
> + free((char *)config->section);
> + free((char *)config->name);
> + goto out_err;
> + }
> + }
> + ret = set_value(config, perf_configs->pos, value);
> +
> +out_err:
> + free(key);
> + 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)
> +{
> + int ret;
> + struct perf_config_set *perf_configs = zalloc(sizeof(*perf_configs));
> + char *user_config = perf_user_perfconfig();
> + char *system_config = strdup(perf_etc_perfconfig());
> +
> + if (!system_config)
> + goto out_err;
> +
> + perf_config_set__init(perf_configs);
> +
> + if (!access(system_config, R_OK)) {
> + perf_configs->pos = SYSTEM;
> + ret = perf_config_from_file(collect_current_config, system_config,
> + perf_configs);
> + if (ret == 0)
> + perf_configs->file_usable[SYSTEM] = true;
> + }
> +
> + if (user_config) {
> + perf_configs->pos = USER;
> + ret = perf_config_from_file(collect_current_config, user_config,
> + perf_configs);
> + if (ret == 0)
> + perf_configs->file_usable[USER] = true;
> + } else {
> + user_config = strdup(mkpath("%s/.perfconfig", getenv("HOME")));
> + if (!user_config)
> + goto out_err;
> + }
> +
> + /* user config file has order of priority */
> + perf_configs->file_path[BOTH] = user_config;
> + perf_configs->file_path[USER] = user_config;
> + perf_configs->file_path[SYSTEM] = system_config;
> +
> + return perf_configs;
> +
> +out_err:
> + pr_err("%s: strdup failed\n", __func__);
> + free(perf_configs);
> + return NULL;
> +}
> +
> +void perf_config_set__delete(struct perf_config_set *perf_configs)
> +{
> + struct perf_config_item *pos, *item;
> +
> + free(perf_configs->file_path[USER]);
> + free(perf_configs->file_path[SYSTEM]);
> +
> + list_for_each_entry_safe(pos, item, &perf_configs->config_list, list) {
> + list_del(&pos->list);
> + if (pos->is_custom) {
> + free((char *)pos->section);
> + free((char *)pos->name);
> + }
> + free(pos->value[USER]);
> + free(pos->value[SYSTEM]);
> + }
> +
> + 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..2523221
> --- /dev/null
> +++ b/tools/perf/util/config.h
> @@ -0,0 +1,138 @@
> +#ifndef __PERF_CONFIG_H
> +#define __PERF_CONFIG_H
> +
> +#include <stdbool.h>
> +#include <linux/list.h>
> +
> +/**
> + * enum perf_config_kind - three kinds of config information
> + *
> + * @USER: user wide ~/.perfconfig
> + * @SYSTEM: systeom wide $(sysconfdir)/perfconfig
> + * @BOTH: both the config files
> + * but user config file has order of priority.
> + */
> +#define CONFIG_KIND 3
> +enum perf_config_kind {
> + BOTH,
> + USER,
> + SYSTEM,
> +};
> +
> +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 - element of perf's configs
> + *
> + * @value: array that has values for each kind (USER/SYSTEM/BOTH)
> + * @is_custom: if the user or system config files contain this
> + */
> +struct perf_config_item {
> + const char *section;
> + const char *name;
> + char *value[CONFIG_KIND];
> + union {
> + bool b;
> + int i;
> + u32 l;
> + u64 ll;
> + float f;
> + double d;
> + const char *s;
> + } default_value;
> + enum perf_config_type type;
> + bool is_custom;
> + struct list_head list;
> +};
> +
> +/**
> + * struct perf_config_set - perf's config set from the config files
> + *
> + * @file_path: array that has paths of config files
> + * @pos: current major config file
> + * @config_list: perf_config_item list head
> + */
> +struct perf_config_set {
> + enum perf_config_kind pos;
> + char *file_path[CONFIG_KIND];
> + bool file_usable[CONFIG_KIND];
> + 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);
> +
> +#endif /* __PERF_CONFIG_H */
> --
> 2.5.0
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC][PATCH] perf config: Introduce perf_config_set class
2016-03-11 14:11 ` Namhyung Kim
@ 2016-03-11 15:08 ` Taeung Song
2016-03-12 8:45 ` Namhyung Kim
0 siblings, 1 reply; 5+ messages in thread
From: Taeung Song @ 2016-03-11 15:08 UTC (permalink / raw)
To: Namhyung Kim
Cc: Arnaldo Carvalho de Melo, linux-kernel, Jiri Olsa, Ingo Molnar,
Peter Zijlstra
Hi, Namhyung
On 03/11/2016 11:11 PM, Namhyung Kim wrote:
> Hi Taeung,
>
> On Thu, Mar 10, 2016 at 11:04:27PM +0900, Taeung Song wrote:
>> In order to variously handle config key-value pairs,
>> prepare all default configs and collect
>> config key-value pairs from user and
>> system config files (i.e user wide ~/.perfconfig
>> and system wide $(sysconfdir)/perfconfig)
>>
>> The various purposes are like
>> showing current configs,
>> in the near future, showing all configs with
>> default value, getting current configs or
>> setting configs that user type, etc.
>>
>> There aren't additional functionalities by appearances
>> excluding handling the error by file status.
>> (e.g checking permission, whether it is empty or etc)
>> and when the two config files have each other's
>> values of same key 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
>>
>> After:
>> # perf config --user --list
>> top.children=false
>>
>> # perf config --system --list
>> top.children=true
>>
>> # perf config --list
>> top.children=false
>>
>> But this patch is designed with considering
>> not only current functionality but also upcoming features.
>>
>> And send [TEST REPORT] email follwing this patch
>> to save time for review. (kinds of test: BASIC, EXCEP HANDLING)
>>
>> 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 | 61 +++++++---
>> tools/perf/util/cache.h | 2 -
>> tools/perf/util/config.c | 268 +++++++++++++++++++++++++++++++++++++++++---
>> tools/perf/util/config.h | 138 +++++++++++++++++++++++
>> 4 files changed, 433 insertions(+), 36 deletions(-)
>> create mode 100644 tools/perf/util/config.h
>
> This is a pretty big change. I recommend you to split the patch into
> pieces. I guess it can consist of 3 patches at least.
>
> 1. implement basic perf_config_item/set operartion
> 2. add actual config items using the above ops
> 3. handle system/user (or both) case
I got it. I'll split this patch.
> Also I think it'd be better just keeping a single config value instead
> of 3 kinds. Maybe you can read system-wide config first and overwrite
> them with user config (for the 'both' case).
>
I know what you mean. I agonized about it.
IMHO, I think that if keeping a single config value instead of 3 kinds
and perf-config has setting functionality when writing a changed config
on a specific config file, some problems can occur e.g.
(Because setting functionality I design is that overwrite
a specific config file by the perf config list)
(the perf config list : all perf configs from the config files)
User wide:
# cat ~/.perfconfig
[report]
queue-size = 1
[test]
location = user
System wide:
# cat /usr/local/etc/perfconfig
[ui]
show-headers = false
[test]
location = system
And if perf-config has setting functionality,
# perf config --system top.children=false
We hoped for:
# cat /usr/local/etc/perfconfig
[ui]
show-headers = false
[test]
location = system
[top]
children = false
But actual result can be:
# cat /usr/local/etc/perfconfig
[ui]
show-headers = false
[report]
queue-size = 1
[test]
location = user
[top]
children = false
We wouldn't want that system config file contain contents of
user config file.
The reason of this problem is that setting functionality I design
work with perf config list overwriting a specific config file
and if perf config list has only single value each config,
we don't exactly know old values of system config.
Don't design setting functionality that overwrite by perf config list ?
(writing '# this file is auto-generated.' at the top of config file)
Add a changed config into a specific config file by other way ? :-\
Or
Not now, when add setting functionality into perf-config,
consider this problem ?
Thanks,
Taeung
>
>
>>
>> diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
>> index c42448e..d79f4d9 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;
>>
>> @@ -32,21 +33,40 @@ 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;
>> + enum perf_config_kind pos = perf_configs->pos;
>> + bool *file_usable = perf_configs->file_usable;
>> + struct list_head *config_list = &perf_configs->config_list;
>> +
>> + if (pos == BOTH) {
>> + if (!file_usable[USER] && !file_usable[SYSTEM])
>> + return -1;
>> + } else if (!file_usable[pos])
>> + return -1;
>> +
>> + list_for_each_entry(config, config_list, list) {
>> + char *value = config->value[pos];
>> +
>> + if (value)
>> + printf("%s.%s=%s\n", config->section,
>> + config->name, value);
>> + }
>>
>> return 0;
>> }
>>
>> int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused)
>> {
>> + /*
>> + * The perf_configs that contains not only default configs
>> + * but also key-value pairs from config files.
>> + * (i.e user wide ~/.perfconfig and system wide $(sysconfdir)/perfconfig)
>> + * This is designed to be used by several functions that handle config set.
>> + */
>> + struct perf_config_set *perf_configs;
>> int ret = 0;
>> - char *user_config = mkpath("%s/.perfconfig", getenv("HOME"));
>>
>> argc = parse_options(argc, argv, config_options, config_usage,
>> PARSE_OPT_STOP_AT_NON_OPTION);
>> @@ -58,10 +78,17 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused)
>> return -1;
>> }
>>
>> - if (use_system_config)
>> - config_exclusive_filename = perf_etc_perfconfig();
>> - else if (use_user_config)
>> - config_exclusive_filename = user_config;
>> + perf_configs = perf_config_set__new();
>> + if (!perf_configs)
>> + return -1;
>> +
>> + if (use_user_config)
>> + perf_configs->pos = USER;
>> + else if (use_system_config)
>> + perf_configs->pos = SYSTEM;
>> + else
>> + perf_configs->pos = BOTH;
>> +
>>
>> switch (actions) {
>> case ACTION_LIST:
>> @@ -69,19 +96,17 @@ 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 = perf_config(show_config, NULL);
>> - if (ret < 0) {
>> - const char * config_filename = config_exclusive_filename;
>> - if (!config_exclusive_filename)
>> - config_filename = user_config;
>> + ret = show_config(perf_configs);
>> + if (ret < 0)
>> pr_err("Nothing configured, "
>> - "please check your %s \n", config_filename);
>> - }
>> + "please check your %s \n",
>> + perf_configs->file_path[perf_configs->pos]);
>> }
>> break;
>> default:
>> usage_with_options(config_usage, config_options);
>> }
>>
>> + perf_config_set__delete(perf_configs);
>> return ret;
>> }
>> diff --git a/tools/perf/util/cache.h b/tools/perf/util/cache.h
>> index 3ca453f..ba657ad 100644
>> --- a/tools/perf/util/cache.h
>> +++ b/tools/perf/util/cache.h
>> @@ -23,8 +23,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 *);
>> extern int perf_default_config(const char *, const char *, void *);
>> extern int perf_config(config_fn_t fn, void *);
>> diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
>> index 4e72763..f827932 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)
>>
>> @@ -26,7 +27,53 @@ static const char *config_file_name;
>> static int config_linenr;
>> static int config_file_eof;
>>
>> -const char *config_exclusive_filename;
>> +static 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)
>> {
>> @@ -458,21 +505,10 @@ static int perf_config_global(void)
>> return !perf_env_bool("PERF_CONFIG_NOGLOBAL", 0);
>> }
>>
>> -int perf_config(config_fn_t fn, void *data)
>> +static char *perf_user_perfconfig(void)
>> {
>> - int ret = 0, found = 0;
>> - const char *home = NULL;
>> + const char *home = getenv("HOME");
>>
>> - /* 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)) {
>> - ret += perf_config_from_file(fn, perf_etc_perfconfig(),
>> - data);
>> - found += 1;
>> - }
>> -
>> - home = getenv("HOME");
>> if (perf_config_global() && home) {
>> char *user_config = strdup(mkpath("%s/.perfconfig", home));
>> struct stat st;
>> @@ -495,17 +531,217 @@ int perf_config(config_fn_t fn, void *data)
>> if (!st.st_size)
>> goto out_free;
>>
>> - ret += perf_config_from_file(fn, user_config, data);
>> - found += 1;
>> + return user_config;
>> +
>> out_free:
>> free(user_config);
>> }
>> out:
>> + return NULL;
>> +}
>> +
>> +int perf_config(config_fn_t fn, void *data)
>> +{
>> + int ret = 0, found = 0;
>> + char *user_config;
>> +
>> + /* 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)) {
>> + ret += perf_config_from_file(fn, perf_etc_perfconfig(),
>> + data);
>> + found += 1;
>> + }
>> +
>> + user_config = perf_user_perfconfig();
>> + if (user_config) {
>> + ret += perf_config_from_file(fn, user_config, data);
>> + found += 1;
>> + free(user_config);
>> + }
>> +
>> if (found == 0)
>> return -1;
>> 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->is_custom = true;
>> + 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, enum perf_config_kind pos,
>> + const char *value)
>> +{
>> + char *val = strdup(value);
>> +
>> + if (!val)
>> + return -1;
>> +
>> + config->value[BOTH] = val;
>> + config->value[pos] = val;
>> + return 0;
>> +}
>> +
>> +static int collect_current_config(const char *var, const char *value,
>> + void *perf_config_set)
>> +{
>> + int ret = 0;
>> + char *ptr, *key;
>> + char *section, *name;
>> + struct perf_config_item *config;
>> + struct perf_config_set *perf_configs = perf_config_set;
>> + struct list_head *config_list = &perf_configs->config_list;
>> +
>> + 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_err;
>> + }
>> +
>> + config = find_config(config_list, section, name);
>> + if (!config) {
>> + config = add_config(config_list, section, name);
>> + if (!config) {
>> + free((char *)config->section);
>> + free((char *)config->name);
>> + goto out_err;
>> + }
>> + }
>> + ret = set_value(config, perf_configs->pos, value);
>> +
>> +out_err:
>> + free(key);
>> + 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)
>> +{
>> + int ret;
>> + struct perf_config_set *perf_configs = zalloc(sizeof(*perf_configs));
>> + char *user_config = perf_user_perfconfig();
>> + char *system_config = strdup(perf_etc_perfconfig());
>> +
>> + if (!system_config)
>> + goto out_err;
>> +
>> + perf_config_set__init(perf_configs);
>> +
>> + if (!access(system_config, R_OK)) {
>> + perf_configs->pos = SYSTEM;
>> + ret = perf_config_from_file(collect_current_config, system_config,
>> + perf_configs);
>> + if (ret == 0)
>> + perf_configs->file_usable[SYSTEM] = true;
>> + }
>> +
>> + if (user_config) {
>> + perf_configs->pos = USER;
>> + ret = perf_config_from_file(collect_current_config, user_config,
>> + perf_configs);
>> + if (ret == 0)
>> + perf_configs->file_usable[USER] = true;
>> + } else {
>> + user_config = strdup(mkpath("%s/.perfconfig", getenv("HOME")));
>> + if (!user_config)
>> + goto out_err;
>> + }
>> +
>> + /* user config file has order of priority */
>> + perf_configs->file_path[BOTH] = user_config;
>> + perf_configs->file_path[USER] = user_config;
>> + perf_configs->file_path[SYSTEM] = system_config;
>> +
>> + return perf_configs;
>> +
>> +out_err:
>> + pr_err("%s: strdup failed\n", __func__);
>> + free(perf_configs);
>> + return NULL;
>> +}
>> +
>> +void perf_config_set__delete(struct perf_config_set *perf_configs)
>> +{
>> + struct perf_config_item *pos, *item;
>> +
>> + free(perf_configs->file_path[USER]);
>> + free(perf_configs->file_path[SYSTEM]);
>> +
>> + list_for_each_entry_safe(pos, item, &perf_configs->config_list, list) {
>> + list_del(&pos->list);
>> + if (pos->is_custom) {
>> + free((char *)pos->section);
>> + free((char *)pos->name);
>> + }
>> + free(pos->value[USER]);
>> + free(pos->value[SYSTEM]);
>> + }
>> +
>> + 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..2523221
>> --- /dev/null
>> +++ b/tools/perf/util/config.h
>> @@ -0,0 +1,138 @@
>> +#ifndef __PERF_CONFIG_H
>> +#define __PERF_CONFIG_H
>> +
>> +#include <stdbool.h>
>> +#include <linux/list.h>
>> +
>> +/**
>> + * enum perf_config_kind - three kinds of config information
>> + *
>> + * @USER: user wide ~/.perfconfig
>> + * @SYSTEM: systeom wide $(sysconfdir)/perfconfig
>> + * @BOTH: both the config files
>> + * but user config file has order of priority.
>> + */
>> +#define CONFIG_KIND 3
>> +enum perf_config_kind {
>> + BOTH,
>> + USER,
>> + SYSTEM,
>> +};
>> +
>> +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 - element of perf's configs
>> + *
>> + * @value: array that has values for each kind (USER/SYSTEM/BOTH)
>> + * @is_custom: if the user or system config files contain this
>> + */
>> +struct perf_config_item {
>> + const char *section;
>> + const char *name;
>> + char *value[CONFIG_KIND];
>> + union {
>> + bool b;
>> + int i;
>> + u32 l;
>> + u64 ll;
>> + float f;
>> + double d;
>> + const char *s;
>> + } default_value;
>> + enum perf_config_type type;
>> + bool is_custom;
>> + struct list_head list;
>> +};
>> +
>> +/**
>> + * struct perf_config_set - perf's config set from the config files
>> + *
>> + * @file_path: array that has paths of config files
>> + * @pos: current major config file
>> + * @config_list: perf_config_item list head
>> + */
>> +struct perf_config_set {
>> + enum perf_config_kind pos;
>> + char *file_path[CONFIG_KIND];
>> + bool file_usable[CONFIG_KIND];
>> + 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);
>> +
>> +#endif /* __PERF_CONFIG_H */
>> --
>> 2.5.0
>>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC][PATCH] perf config: Introduce perf_config_set class
2016-03-11 15:08 ` Taeung Song
@ 2016-03-12 8:45 ` Namhyung Kim
2016-03-12 10:28 ` Taeung Song
0 siblings, 1 reply; 5+ messages in thread
From: Namhyung Kim @ 2016-03-12 8:45 UTC (permalink / raw)
To: Taeung Song
Cc: Arnaldo Carvalho de Melo, linux-kernel, Jiri Olsa, Ingo Molnar,
Peter Zijlstra
Hi Taeung,
On Sat, Mar 12, 2016 at 12:08:52AM +0900, Taeung Song wrote:
> Hi, Namhyung
>
> On 03/11/2016 11:11 PM, Namhyung Kim wrote:
> >Also I think it'd be better just keeping a single config value instead
> >of 3 kinds. Maybe you can read system-wide config first and overwrite
> >them with user config (for the 'both' case).
> >
>
> I know what you mean. I agonized about it.
>
> IMHO, I think that if keeping a single config value instead of 3 kinds and
> perf-config has setting functionality when writing a changed config
> on a specific config file, some problems can occur e.g.
Do you plan to support 'set' and 'get' operation at the same time?
IOW is it possible to do?
$ perf config --set aaa.bbb=xx --get ccc.ddd
I don't think it's very useful.
If we don't do it, I think we can simply read a single config file
(default to user file) and re-write it for the 'set' operation.
Or maybe we can add a field (like 'origin'?) in the perf_config_item
struct to mark where it comes from. And then it should write items
matching 'origin' only.
Thanks,
Namhyung
>
> (Because setting functionality I design is that overwrite
> a specific config file by the perf config list)
> (the perf config list : all perf configs from the config files)
>
> User wide:
>
> # cat ~/.perfconfig
> [report]
> queue-size = 1
> [test]
> location = user
>
> System wide:
>
> # cat /usr/local/etc/perfconfig
> [ui]
> show-headers = false
> [test]
> location = system
>
> And if perf-config has setting functionality,
>
> # perf config --system top.children=false
>
> We hoped for:
>
> # cat /usr/local/etc/perfconfig
> [ui]
> show-headers = false
> [test]
> location = system
> [top]
> children = false
>
> But actual result can be:
>
> # cat /usr/local/etc/perfconfig
> [ui]
> show-headers = false
> [report]
> queue-size = 1
> [test]
> location = user
> [top]
> children = false
>
> We wouldn't want that system config file contain contents of
> user config file.
> The reason of this problem is that setting functionality I design
> work with perf config list overwriting a specific config file
> and if perf config list has only single value each config,
> we don't exactly know old values of system config.
>
> Don't design setting functionality that overwrite by perf config list ?
> (writing '# this file is auto-generated.' at the top of config file)
>
> Add a changed config into a specific config file by other way ? :-\
>
> Or
> Not now, when add setting functionality into perf-config,
> consider this problem ?
>
> Thanks,
> Taeung
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC][PATCH] perf config: Introduce perf_config_set class
2016-03-12 8:45 ` Namhyung Kim
@ 2016-03-12 10:28 ` Taeung Song
0 siblings, 0 replies; 5+ messages in thread
From: Taeung Song @ 2016-03-12 10:28 UTC (permalink / raw)
To: Namhyung Kim, Jiri Olsa
Cc: Arnaldo Carvalho de Melo, linux-kernel, Peter Zijlstra, Ingo Molnar
On 03/12/2016 05:45 PM, Namhyung Kim wrote:
> Hi Taeung,
>
> On Sat, Mar 12, 2016 at 12:08:52AM +0900, Taeung Song wrote:
>> Hi, Namhyung
>>
>> On 03/11/2016 11:11 PM, Namhyung Kim wrote:
>>> Also I think it'd be better just keeping a single config value instead
>>> of 3 kinds. Maybe you can read system-wide config first and overwrite
>>> them with user config (for the 'both' case).
>>>
>>
>> I know what you mean. I agonized about it.
>>
>> IMHO, I think that if keeping a single config value instead of 3 kinds and
>> perf-config has setting functionality when writing a changed config
>> on a specific config file, some problems can occur e.g.
>
> Do you plan to support 'set' and 'get' operation at the same time?
> IOW is it possible to do?
>
> $ perf config --set aaa.bbb=xx --get ccc.ddd
>
> I don't think it's very useful.
>
> If we don't do it, I think we can simply read a single config file
> (default to user file) and re-write it for the 'set' operation.
I agree. I think that what you said is a simple way for 'set' operation.
But I have a plan about perf-config interface like 'sysctl'
(suggested by jiri)
http://marc.info/?l=linux-kernel&m=142842999926479&w=2
For examples,
sysctl [options] [variable[=value]] [...]
sysctl -p [file or regexp] [...]
# display current config
perf config
# display current config plus all keys with default values
perf config -a
# display key value:
perf config report.queue
# set key value:
perf config report.queue=100M
# remove key (not in sysctl)
perf config -r report.queue
If we do so,
perf-config support 'set' and 'get' operation at the same time e.g
sysctl:
# sysctl vm.stat_interval vm.stat_interval=2 vm.user_reserve_kbytes
vm.stat_interval = 1
vm.stat_interval = 2
vm.user_reserve_kbytes = 131072
perf-config:
# perf config report.queue-size report.queue-size=100M top.children
report.queue-size=1
report.queue-size=104857600
top.children=true
jiri, is it right ?
or the above situation wasn't what you mean ? (I understood so)
then, namhyung, is it better to use the simple way for 'set' and 'get'
operation ?
(instead of 'sysctl' style)
> Or maybe we can add a field (like 'origin'?) in the perf_config_item
> struct to mark where it comes from. And then it should write items
> matching 'origin' only.
>
I understood a field 'origin' e.g
struct perf_config_item {
(..omitted..)
enum config_file {
USER_CONFIG,
SYSTEM_CONFIG
} origin;
}
And if the two files have same variables,
user config file has a high priority. (as I understood)
IMHO, I think that even if we use 'origin',
some problem can occur when handling same variables e.g.
User wide:
# cat ~/.perfconfig
[report]
queue-size = 1
System wide:
# cat /usr/local/etc/perfconfig
[report]
queue-size = 2
[top]
children = false
If user or system config files has same variable as above,
# perf config --system top.children=true
# perf config --system --list
top.children=false
the above problem can occur.
When rewriting items matching 'origin',
because a item for 'report.queue-size' has 'origin' marked
as user config file, 'report.queue-size' of system config file
can be removed.
And even though perf_config_item has a field 'origin'
to mark where it comes from,
if perf_config_item has only single value
we can't know old value of 'report.queue-size' for system config file as
ever
because of overwriting a single value of perf_config_item
when collecting configs from the config files.
Did I misunderstand your intention using a field 'origin' ? :-\
Or
when the two files have same config variables,
are there two items each other (for system and user config file) ?
Thanks,
Taeung
>
>
>>
>> (Because setting functionality I design is that overwrite
>> a specific config file by the perf config list)
>> (the perf config list : all perf configs from the config files)
>>
>> User wide:
>>
>> # cat ~/.perfconfig
>> [report]
>> queue-size = 1
>> [test]
>> location = user
>>
>> System wide:
>>
>> # cat /usr/local/etc/perfconfig
>> [ui]
>> show-headers = false
>> [test]
>> location = system
>>
>> And if perf-config has setting functionality,
>>
>> # perf config --system top.children=false
>>
>> We hoped for:
>>
>> # cat /usr/local/etc/perfconfig
>> [ui]
>> show-headers = false
>> [test]
>> location = system
>> [top]
>> children = false
>>
>> But actual result can be:
>>
>> # cat /usr/local/etc/perfconfig
>> [ui]
>> show-headers = false
>> [report]
>> queue-size = 1
>> [test]
>> location = user
>> [top]
>> children = false
>>
>> We wouldn't want that system config file contain contents of
>> user config file.
>> The reason of this problem is that setting functionality I design
>> work with perf config list overwriting a specific config file
>> and if perf config list has only single value each config,
>> we don't exactly know old values of system config.
>>
>> Don't design setting functionality that overwrite by perf config list ?
>> (writing '# this file is auto-generated.' at the top of config file)
>>
>> Add a changed config into a specific config file by other way ? :-\
>>
>> Or
>> Not now, when add setting functionality into perf-config,
>> consider this problem ?
>>
>> Thanks,
>> Taeung
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-03-12 10:29 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-10 14:04 [RFC][PATCH] perf config: Introduce perf_config_set class Taeung Song
2016-03-11 14:11 ` Namhyung Kim
2016-03-11 15:08 ` Taeung Song
2016-03-12 8:45 ` Namhyung Kim
2016-03-12 10:28 ` 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).