From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1425308AbcFHMi0 (ORCPT ); Wed, 8 Jun 2016 08:38:26 -0400 Received: from mail-pf0-f195.google.com ([209.85.192.195]:33849 "EHLO mail-pf0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1425261AbcFHMiX (ORCPT ); Wed, 8 Jun 2016 08:38:23 -0400 Subject: Re: [PATCH v7 3/7] perf config: Add global variable 'config_set' To: Arnaldo Carvalho de Melo References: <1465291577-20973-1-git-send-email-treeze.taeung@gmail.com> <1465291577-20973-4-git-send-email-treeze.taeung@gmail.com> <20160607140509.GA11589@kernel.org> <5756E4C4.5080306@gmail.com> Cc: linux-kernel@vger.kernel.org, Jiri Olsa , Namhyung Kim , Ingo Molnar , Peter Zijlstra , Alexander Shishkin , Masami Hiramatsu , Jiri Olsa , Wang Nan From: Taeung Song Message-ID: <575811B9.4060204@gmail.com> Date: Wed, 8 Jun 2016 21:38:17 +0900 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.8.0 MIME-Version: 1.0 In-Reply-To: <5756E4C4.5080306@gmail.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org My answer was too long.. To be short, many sub-commands use perf_config() so wherever new perf_config() is called, the config set that already contains all configs from config files should be shared. And when a sub-command is done, we need to free the config set at run_builtin() as below. So, IMHO we need to use 'config_set' as a global variable. Anyway I sent v8 recasting this patchset with other way. (But 'config_set' is still a global variable.) Please, reconsider it again. Thanks, Taeung On 06/08/2016 12:14 AM, Taeung Song wrote: > Good afternoon :) > > On 06/07/2016 11:05 PM, Arnaldo Carvalho de Melo wrote: >> Em Tue, Jun 07, 2016 at 06:26:13PM +0900, Taeung Song escreveu: >>> The config set is prepared by collecting >>> all configs from config files (i.e. user config >>> ~/.perfconfig and system config $(sysconfdir)/perfconfig) >>> so the config set contains all config key-value pairs. >>> >>> We need to use it as global variable to share it. >> >> We should generally avoid global variables, and in this case, from >> looking just at this patch, I'm not convinced we need to introduce more >> global variables, isn't this object instantiated and deleted at the >> cmd_config() function? So, can't we just pass it to any function needing >> to access it? > > The reason why I added global variable 'config_set' > that all config key-value pairs from config files > is because of upcoming perf_config_set__delete() and perf_config(). > > This variable can be used not only at cmd_config() > but also at new perf_config() and future perf_config_set__delete(). > > Wherever new perf_config() is called, this function check > whether the global variable 'config_set' is initialized or not. > And this function use the variable 'config_set' like below. > > (One of intended purposes of new perf_config() is > to avoid the repetitive work that read config files, every time > it is called and to reuse the global variable 'config_set' that > already contains all config info.) > > +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); > +} > > > Finally, after sub-command finished, called perf_config_set__delete() > use the global variable as argument to free it as below. > > > diff --git a/tools/perf/perf.c b/tools/perf/perf.c > index 15982ce..058d5dc 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(); > > > > Sure, we can avoid 'extern' for the global variable 'config_set'. > But IMHO, we may need to use a global variable 'config_set' > at only util/config.c at least. > (If perf_config_set__delete() don't take arguments) > > (But it seems natural that we should pair between destructor > and constructor as you said. > http://marc.info/?l=linux-kernel&m=146463657721848&w=2 ) > > And of course, there are other way e.g. > modifying arguments of perf_config(), using perf_config_set__new() > at the very beginning at main() or etc... > > Nevertheless, > if we haven't to use the global variable, > I'd find other way. > >> >> I applied the first two patches. >> > > Thank you! > > Taeung > >> >>> And in near future, the variable will be handled in perf_config() >>> and other functions at util/config.c >>> >>> Cc: Namhyung Kim >>> Cc: Jiri Olsa >>> Cc: Masami Hiramatsu >>> Cc: Alexander Shishkin >>> Signed-off-by: Taeung Song >>> --- >>> tools/perf/builtin-config.c | 9 ++++----- >>> tools/perf/util/config.c | 1 + >>> tools/perf/util/config.h | 2 ++ >>> 3 files changed, 7 insertions(+), 5 deletions(-) >>> >>> diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c >>> index fe1b77f..b3bc01a 100644 >>> --- a/tools/perf/builtin-config.c >>> +++ b/tools/perf/builtin-config.c >>> @@ -62,7 +62,6 @@ static int show_config(struct perf_config_set *set) >>> int cmd_config(int argc, const char **argv, const char *prefix >>> __maybe_unused) >>> { >>> int ret = 0; >>> - struct perf_config_set *set; >>> char *user_config = mkpath("%s/.perfconfig", getenv("HOME")); >>> >>> argc = parse_options(argc, argv, config_options, config_usage, >>> @@ -80,8 +79,8 @@ int cmd_config(int argc, const char **argv, const >>> char *prefix __maybe_unused) >>> else if (use_user_config) >>> config_exclusive_filename = user_config; >>> >>> - set = perf_config_set__new(); >>> - if (!set) { >>> + config_set = perf_config_set__new(); >>> + if (!config_set) { >>> ret = -1; >>> goto out_err; >>> } >>> @@ -92,7 +91,7 @@ int cmd_config(int argc, const char **argv, const >>> char *prefix __maybe_unused) >>> pr_err("Error: takes no arguments\n"); >>> parse_options_usage(config_usage, config_options, "l", 1); >>> } else { >>> - ret = show_config(set); >>> + ret = show_config(config_set); >>> if (ret < 0) { >>> const char * config_filename = >>> config_exclusive_filename; >>> if (!config_exclusive_filename) >>> @@ -106,7 +105,7 @@ int cmd_config(int argc, const char **argv, const >>> char *prefix __maybe_unused) >>> usage_with_options(config_usage, config_options); >>> } >>> >>> - perf_config_set__delete(set); >>> + perf_config_set__delete(config_set); >>> out_err: >>> return ret; >>> } >>> diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c >>> index 8749eca..02fc6d5 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) >>> { >>> diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h >>> index 22ec626..ea157a4 100644 >>> --- a/tools/perf/util/config.h >>> +++ b/tools/perf/util/config.h >>> @@ -20,6 +20,8 @@ struct perf_config_set { >>> struct list_head sections; >>> }; >>> >>> +extern struct perf_config_set *config_set; >>> + >>> struct perf_config_set *perf_config_set__new(void); >>> void perf_config_set__delete(struct perf_config_set *set); >>> >>> -- >>> 2.5.0