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

[Arnaldo Carvalho de Melo]

Em Wed, Jun 08, 2016 at 09:36:51PM +0900, Taeung Song escreveu:
> Many sub-commands use perf_config() so
> everytime perf_config() is called, perf_config() always read config files.
> (i.e. user config '~/.perfconfig' and system config '$(sysconfdir)/perfconfig')

And this is not always a bad thing, think about being in 'mutt' and
adding an entry to ~/.mail_aliases, then going to compose a message,
would be good that the just added entry to ~/.mail_aliases be considered
when adding recipients to your messages, right?

In the same fashion, 'perf report', 'perf annotate', 'perf top' are long
running utilities that have operations that could get changes to config
files without having to restart them, i.e. do annotation changes in your
~/.perfconfig and then see them reflected next time you hit 'A´ to
annotate a function in 'perf top' or 'perf report'.

So, I think that if tools want ammortize the cost of reading config
files, they should create an instance of the relevant object
(perf_config_set?) use it and then delete it, but not keep one around
for a long time.

- Arnaldo
 
> But we need to use the config set that already contains all config
> key-value pairs to avoid this repetitive work reading the config files
> in perf_config(). (the config set mean a global variable 'config_set')
> 
> In other words, if new perf_config() is called,
> only first time 'config_set' is initialized collecting all configs
> from the config files and it work with perf_config_set__iter().
> And free the config set after a sub-command work at run_builtin().
> 
> If we do, 'config_set' can be reused wherever using perf_config()
> and a feature of old perf_config() is the same as new perf_config() work
> without the repetitive work that read the config files.
> 
> Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
> Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
> Cc: Wang Nan <wangnan0@xxxxxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: Masami Hiramatsu <mhiramat@xxxxxxxxxx>
> Cc: Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>
> Signed-off-by: Taeung Song <treeze.taeung@xxxxxxxxx>
> ---
>  tools/perf/builtin-config.c |  4 +++
>  tools/perf/perf.c           |  1 +
>  tools/perf/util/config.c    | 87 ++++++++++++++++++++++-----------------------
>  tools/perf/util/config.h    |  1 +
>  4 files changed, 48 insertions(+), 45 deletions(-)
> 
> diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
> index fe1b77f..cfd1036 100644
> --- a/tools/perf/builtin-config.c
> +++ b/tools/perf/builtin-config.c
> @@ -80,6 +80,10 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused)
>  	else if (use_user_config)
>  		config_exclusive_filename = user_config;
>  
> +	/*
> +	 * At only 'config' sub-command, individually use the config set
> +	 * because of reinitializing with options config file location.
> +	 */
>  	set = perf_config_set__new();
>  	if (!set) {
>  		ret = -1;
> diff --git a/tools/perf/perf.c b/tools/perf/perf.c
> index 15982ce..fe2ab7c 100644
> --- a/tools/perf/perf.c
> +++ b/tools/perf/perf.c
> @@ -391,6 +391,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
>  
>  	perf_env__set_cmdline(&perf_env, argc, argv);
>  	status = p->fn(argc, argv, prefix);
> +	perf_config_set__delete(config_set);
>  	exit_browser(status);
>  	perf_env__exit(&perf_env);
>  	bpf__clear();
> diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
> index 31e09a4..72db134 100644
> --- a/tools/perf/util/config.c
> +++ b/tools/perf/util/config.c
> @@ -28,6 +28,7 @@ static int config_linenr;
>  static int config_file_eof;
>  
>  const char *config_exclusive_filename;
> +struct perf_config_set *config_set;
>  
>  static int get_next_char(void)
>  {
> @@ -478,51 +479,6 @@ static int perf_config_global(void)
>  	return !perf_env_bool("PERF_CONFIG_NOGLOBAL", 0);
>  }
>  
> -int perf_config(config_fn_t fn, void *data)
> -{
> -	int ret = -1;
> -	const char *home = NULL;
> -
> -	/* Setting $PERF_CONFIG makes perf read _only_ the given config file. */
> -	if (config_exclusive_filename)
> -		return perf_config_from_file(fn, config_exclusive_filename, data);
> -	if (perf_config_system() && !access(perf_etc_perfconfig(), R_OK)) {
> -		if (perf_config_from_file(fn, perf_etc_perfconfig(), data) < 0)
> -			goto out;
> -	}
> -
> -	home = getenv("HOME");
> -	if (perf_config_global() && home) {
> -		char *user_config = strdup(mkpath("%s/.perfconfig", home));
> -		struct stat st;
> -
> -		if (user_config == NULL) {
> -			warning("Not enough memory to process %s/.perfconfig, "
> -				"ignoring it.", home);
> -			goto out;
> -		}
> -
> -		if (stat(user_config, &st) < 0)
> -			goto out_free;
> -
> -		if (st.st_uid && (st.st_uid != geteuid())) {
> -			warning("File %s not owned by current user or root, "
> -				"ignoring it.", user_config);
> -			goto out_free;
> -		}
> -
> -		if (!st.st_size)
> -			goto out_free;
> -
> -		ret = perf_config_from_file(fn, user_config, data);
> -
> -out_free:
> -		free(user_config);
> -	}
> -out:
> -	return ret;
> -}
> -
>  static struct perf_config_section *find_section(struct list_head *sections,
>  						const char *section_name)
>  {
> @@ -706,6 +662,47 @@ struct perf_config_set *perf_config_set__new(void)
>  	return set;
>  }
>  
> +static int perf_config_set__iter(struct perf_config_set *set, config_fn_t fn, void *data)
> +{
> +	struct perf_config_section *section;
> +	struct perf_config_item *item;
> +	struct list_head *sections;
> +	char key[BUFSIZ];
> +
> +	if (set == NULL)
> +		return -1;
> +
> +	sections = &set->sections;
> +	if (list_empty(sections))
> +		return -1;
> +
> +	list_for_each_entry(section, sections, node) {
> +		list_for_each_entry(item, &section->items, node) {
> +			char *value = item->value;
> +
> +			if (value) {
> +				scnprintf(key, sizeof(key), "%s.%s",
> +					  section->name, item->name);
> +				if (fn(key, value, data) < 0) {
> +					pr_err("Error: wrong config key-value pair %s=%s\n",
> +					       key, value);
> +					return -1;
> +				}
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +int perf_config(config_fn_t fn, void *data)
> +{
> +	if (config_set == NULL)
> +		config_set = perf_config_set__new();
> +
> +	return perf_config_set__iter(config_set, fn, data);
> +}
> +
>  static void perf_config_item__delete(struct perf_config_item *item)
>  {
>  	zfree(&item->name);
> diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
> index 35ccddb..7cc4fea 100644
> --- a/tools/perf/util/config.h
> +++ b/tools/perf/util/config.h
> @@ -21,6 +21,7 @@ struct perf_config_set {
>  };
>  
>  extern const char *config_exclusive_filename;
> +extern struct perf_config_set *config_set;
>  
>  typedef int (*config_fn_t)(const char *, const char *, void *);
>  int perf_default_config(const char *, const char *, void *);
> -- 
> 2.5.0