linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Taeung Song <treeze.taeung@gmail.com>
To: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
Cc: linux-kernel@vger.kernel.org, Jiri Olsa <jolsa@kernel.org>,
	Namhyung Kim <namhyung@kernel.org>,
	Ingo Molnar <mingo@kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Masami Hiramatsu <mhiramat@kernel.org>,
	Jiri Olsa <jolsa@redhat.com>, Wang Nan <wangnan0@huawei.com>
Subject: Re: [PATCH v7 3/7] perf config: Add global variable 'config_set'
Date: Wed, 8 Jun 2016 21:38:17 +0900	[thread overview]
Message-ID: <575811B9.4060204@gmail.com> (raw)
In-Reply-To: <5756E4C4.5080306@gmail.com>

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 <namhyung@kernel.org>
>>> Cc: Jiri Olsa <jolsa@redhat.com>
>>> Cc: Masami Hiramatsu <mhiramat@kernel.org>
>>> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>>> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
>>> ---
>>>   tools/perf/builtin-config.c | 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

  reply	other threads:[~2016-06-08 12:38 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-06-07  9:26 [RFC][PATCH v7 0/9] perf config: Reimplement perf_config() using perf_config_set__inter() Taeung Song
2016-06-07  9:26 ` [BUGFIX][PATCH v7 1/7] perf config: If collect_config() is failed, finally free a config set after it is done Taeung Song
2016-06-08  8:44   ` [tip:perf/core] perf config: Constructor should free its allocated memory when failing tip-bot for Taeung Song
2016-06-07  9:26 ` [PATCH v7 2/7] perf config: Use new perf_config_set__init() to initialize config set Taeung Song
2016-06-08  8:45   ` [tip:perf/core] " tip-bot for Taeung Song
2016-06-07  9:26 ` [PATCH v7 3/7] perf config: Add global variable 'config_set' Taeung Song
2016-06-07 14:05   ` Arnaldo Carvalho de Melo
2016-06-07 15:14     ` Taeung Song
2016-06-08 12:38       ` Taeung Song [this message]
2016-06-07  9:26 ` [PATCH v7 4/7] perf config: Use zfree() instead of free() at perf_config_set__delete() Taeung Song
2016-06-07  9:26 ` [PATCH v7 5/7] perf config: Reimplement perf_config() using perf_config_set__iter() Taeung Song
2016-06-07  9:26 ` [PATCH v7 6/7] perf config: Reset the config set at only 'config' sub-command Taeung Song
2016-06-07  9:26 ` [PATCH v7 7/7] perf config: Reimplement show_config() using perf_config() Taeung Song

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=575811B9.4060204@gmail.com \
    --to=treeze.taeung@gmail.com \
    --cc=alexander.shishkin@linux.intel.com \
    --cc=arnaldo.melo@gmail.com \
    --cc=jolsa@kernel.org \
    --cc=jolsa@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mhiramat@kernel.org \
    --cc=mingo@kernel.org \
    --cc=namhyung@kernel.org \
    --cc=peterz@infradead.org \
    --cc=wangnan0@huawei.com \
    --subject='Re: [PATCH v7 3/7] perf config: Add global variable '\''config_set'\''' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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