Re: [PATCH v6 4/4] perf config: Initialize perf_config_set with all default configs

From: Taeung Song
Date: Thu Apr 07 2016 - 06:36:07 EST

Next message: Florian Weimer: "Re: [RFC PATCH v6 1/5] Thread-local ABI system call: cache CPU number of running thread"
Previous message: Julien Grall: "Re: [PATCH v10 00/17] Add ACPI support for Xen Dom0 on ARM64"
In reply to: Taeung Song: "Re: [PATCH v6 4/4] perf config: Initialize perf_config_set with all default configs"
Next in thread: Taeung Song: "[PATCH v6 1/4] perf config: Introduce perf_config_set class"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi, Masami

Because of my unclear response,
I resend this email after submitting v7 this patchset. :)

On 04/05/2016 08:23 PM, Masami Hiramatsu wrote:

On Tue, 5 Apr 2016 15:06:47 +0900
Taeung Song <treeze.taeung@xxxxxxxxx> wrote:

Hi, Masami

On 04/04/2016 10:28 PM, Masami Hiramatsu wrote:

On Mon, 4 Apr 2016 18:17:07 +0900
Taeung Song <treeze.taeung@xxxxxxxxx> wrote:

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

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

Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
Signed-off-by: Taeung Song <treeze.taeung@xxxxxxxxx>
---
tools/perf/builtin-config.c | 11 +++++++----
tools/perf/util/config.c | 33 ++++++++++++++++++++++++++++++---
tools/perf/util/config.h | 7 +++++++
3 files changed, 44 insertions(+), 7 deletions(-)

diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
index 1133224..66a269e 100644
--- a/tools/perf/builtin-config.c
+++ b/tools/perf/builtin-config.c
@@ -35,23 +35,26 @@ static struct option config_options[] = {

static int show_config(struct perf_config_set *set)
{
+ bool has_value = false;
struct perf_config_section *section;
struct perf_config_item *item;
struct list_head *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)
+ if (value) {
printf("%s.%s=%s\n", section->name,
item->name, value);
+ has_value = true;
+ }
}
}

+ if (!has_value)
+ return -1;

Could this path be really executed? It seems that the set is
always initialized with default values. This means not empty.
If it is for asserting bugs, we also have to check set != NULL
beforehand.

I got it.
But before calling show_config(set), the set is checked as below

... (omitted) ...
set = perf_config_set__new();
if (!set) {
ret = -1;
goto out_err;
}

switch (actions) {
case ACTION_LIST:
if (argc) {
pr_err("Error: takes no arguments\n");
parse_options_usage(config_usage, config_options, "l", 1);
} else {
ret = show_config(set);

...(omitted)...

I thought that the set can never be NULL in show_config().
Is it wrong ?
(I'm afraid that there are things I misunderstood.)

Hmm, my main point was has_value is always true because
perf_config_set__init() initializes the set with default value.
If so, has_value can be removed.

I tested multiple cases (e.g after removing ~/.perfconfig
or with noting configured config files(but there are config files).

But has_value isn't always true because config_item has both 'value'
and 'default_value' and perf_config_set__init() don't handle 'value'.

Even though perf_config_set__init() initializes the set with default values,
the function don't use 'value' and handle 'default_value' variable
of each config_item.

(The reason that config_item has both 'value' and 'default' is
for upcoming features (e.g. --list-all, getting, setting, --remove, etc.))

And I used has_value to know whether config files have config information or not in show_config().

> But even though, if has_value

is intended to detect unexpected bugs, also introducing (set != NULL)
check is more natural.

I added it at '[PATCH v7 2/4] perf config: Let show_config() work with perf_config_set'.

Thanks,
Taeung

diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
index f937124..1855560 100644
--- a/tools/perf/util/config.c
+++ b/tools/perf/util/config.c
@@ -663,6 +663,7 @@ static struct perf_config_section *add_section(struct list_head *sections,
if (!section)
return NULL;

+ section->is_custom = true;
INIT_LIST_HEAD(&section->items);
section->name = strdup(section_name);
if (!section->name) {
@@ -683,6 +684,7 @@ static struct perf_config_item *add_config_item(struct perf_config_section *sect
if (!item)
return NULL;

+ item->is_custom = true;
item->name = strdup(name);
if (!item->name) {
pr_debug("%s: strdup failed\n", __func__);
@@ -751,12 +753,35 @@ out_free:
return -1;
}

+static struct perf_config_set *perf_config_set__init(struct perf_config_set *set)
+{
+ int i, j;
+ struct perf_config_section *section;
+ struct perf_config_item *items;
+ struct list_head *sections = &set->sections;
+
+ INIT_LIST_HEAD(&set->sections);
+
+ for (i = 0; i != CONFIG_END; i++) {
+ section = &default_sections[i];
+ INIT_LIST_HEAD(&section->items);
+
+ items = default_config_items[i];
+ for (j = 0; items[j].name != NULL; j++)
+ list_add_tail(&items[j].node, &section->items);
+
+ list_add_tail(&section->node, sections);
+ }
+
+ return set;
+}
+
struct perf_config_set *perf_config_set__new(void)
{
struct perf_config_set *set = zalloc(sizeof(*set));

if (set) {
- INIT_LIST_HEAD(&set->sections);
+ perf_config_set__init(set);
perf_config(collect_config, set);
}

@@ -776,7 +801,8 @@ static void perf_config_section__purge(struct perf_config_section *section)

list_for_each_entry_safe(item, tmp, &section->items, node) {
list_del_init(&item->node);
- perf_config_item__delete(item);
+ if (item->is_custom)
+ perf_config_item__delete(item);
}
}

@@ -793,7 +819,8 @@ static void perf_config_set__purge(struct perf_config_set *set)

list_for_each_entry_safe(section, tmp, &set->sections, node) {
list_del_init(&section->node);
- perf_config_section__delete(section);
+ if (section->is_custom)
+ perf_config_section__delete(section);

Here, unless perf_config_section__delete() is called, the items
under the section is never be checked.

It is my stupid mistake..

However, if a user changes
existing item value, its section->is_custom is false, but the
item itself is_custom be true.
This means that such custom item values may not be freed.

And there is one more what I missed.
I didn't consider that when existing item's 'value' is set by set_value(),
it can not be freed. So fix it as below.

static void perf_config_item__delete(struct perf_config_item *item)
{
- zfree((char **)&item->name);
zfree(&item->value);
- free(item);
+ if (item->is_allocated) {
+ zfree((char **)&item->name);
+ free(item);
+ }
}

static void perf_config_section__purge(struct perf_config_section
*section)
{
struct perf_config_item *item, *tmp;

list_for_each_entry_safe(item, tmp, &section->items, node) {
list_del_init(&item->node);
- if (item->is_custom)
- perf_config_item__delete(item);
+ perf_config_item__delete(item);
}
}

Good catch! it also should be done :)

So, the section->is_custom flag should be checked in *__delete
method as below,
static void perf_config_section__delete(struct perf_config_section *section)
{
perf_config_section__purge(section);
if (section->is_custom) {
zfree(&section->name);
free(section);
}
}

Or, you can just call perf_config_section__purge(section) in
perf_config_set__purge().

I'll modify this code by the former that you said like below.

static void perf_config_section__delete(struct perf_config_section
*section)
{
perf_config_section__purge(section);
- zfree((char **)&section->name);
- free(section);
+ if (section->is_allocated) {
+ zfree((char **)&section->name);
+ free(section);
+ }
}

Looks good :)

static void perf_config_set__purge(struct perf_config_set *set)
@@ -819,8 +822,7 @@ static void perf_config_set__purge(struct
perf_config_set *set)

list_for_each_entry_safe(section, tmp, &set->sections, node) {
list_del_init(&section->node);
- if (section->is_custom)
- perf_config_section__delete(section);
+ perf_config_section__delete(section);
}
}

Also, I like ->allocated instead of ->is_custom.

What you said is right.
Because the variable means whether a config section or item is allocated,
not customized by a user.
I'll rename it.

Thank you!

Thanks,
Taeung

}
}

diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
index 84dcc1d..1df96b7 100644
--- a/tools/perf/util/config.h
+++ b/tools/perf/util/config.h
@@ -14,6 +14,11 @@ enum perf_config_type {
CONFIG_TYPE_STRING
};

+/**
+ * struct perf_config_item - element of perf's configs
+ *
+ * @is_custom: unknown or new config other than default config
+ */
struct perf_config_item {
const char *name;
char *value;
@@ -27,11 +32,13 @@ struct perf_config_item {
const char *s;
} default_value;
enum perf_config_type type;
+ bool is_custom;
struct list_head node;
};

struct perf_config_section {
const char *name;
+ bool is_custom;
struct list_head items;
struct list_head node;
};
--
2.5.0

Next message: Florian Weimer: "Re: [RFC PATCH v6 1/5] Thread-local ABI system call: cache CPU number of running thread"
Previous message: Julien Grall: "Re: [PATCH v10 00/17] Add ACPI support for Xen Dom0 on ARM64"
In reply to: Taeung Song: "Re: [PATCH v6 4/4] perf config: Initialize perf_config_set with all default configs"
Next in thread: Taeung Song: "[PATCH v6 1/4] perf config: Introduce perf_config_set class"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]