Skip to contentSkip to conversation

perf/hv-24x7 set the attr group to NULL if events failed to be initialized

RFC PATCH In review Li Zhong 3 participants 7 messages Feb 15, 2015 – Apr 13, 2015 PowerPC

lore
Share & tools
lore

Li Zhong

author
#0 lore raw

RFC PATCH 1/1 perf/hv-24x7 set the attr group to NULL if events failed to be initialized

sysfs_create_groups() creates groups one by one in the attr_groups array before a NULL entry is encountered. But if an error is seen, it stops and removes all the groups already created:

        for (i = 0; groups[i]; i++) {
                error = sysfs_create_group(kobj, groups[i]);
                if (error) {
                        while (--i >= 0)
                                sysfs_remove_group(kobj, groups[i]);
                        break;
                }
        }

And for the three event groups of 24x7, if it is not supported, according to the above logic, it causes format and interface group to be removed because of the error.

This patch moves the three events groups to the end of the attr groups, and if create_events_from_catalog() fails to set their attributes, we set them to NULL in attr_groups.

Signed-off-by Li Zhong <[email protected]>
---
 arch/powerpc/perf/hv-24x7.c | 37 ++++++++++++++++++++++++++++---------
 1 file changed, 28 insertions(+), 9 deletions(-)
arch/powerpc/perf/hv-24x7.c +28 -9
index 9445a82..1e433f7 100644
@@ -637,7 +637,7 @@ static ssize_t catalog_event_len_validate(struct hv_24x7_event_data *event,
 
 #define MAX_4K (SIZE_MAX / 4096)
 
-static void create_events_from_catalog(struct attribute ***events_,
+static int create_events_from_catalog(struct attribute ***events_,
 		struct attribute ***event_descs_,
 		struct attribute ***event_long_descs_)
 {

@@ -843,7 +843,7 @@ static void create_events_from_catalog(struct attribute ***events_,
 	*events_ = events;
 	*event_descs_ = event_descs;
 	*event_long_descs_ = event_long_descs;
-	return;
+	return 0;
 
 e_event_descs:
 	kfree(event_descs);

@@ -857,6 +857,7 @@ e_out:
 	*events_ = NULL;
 	*event_descs_ = NULL;
 	*event_long_descs_ = NULL;
+	return -1;
 }
 
 static ssize_t catalog_read(struct file *filp, struct kobject *kobj,

@@ -967,13 +968,25 @@ static struct attribute_group if_group = {
 	.attrs = if_attrs,
 };
 
+enum GROUP_INDEX {
+	FORMAT,
+	IF,
+	EVENT,
+	EVENT_DESC,
+	EVENT_LONG_DESC,
+	MAX
+};
+
+/*
+ * keep the attr group whose attr is set in init (may fail)
+ * at the end
+ */
 static const struct attribute_group *attr_groups[] = {
-	&format_group,
-	&event_group,
-	&event_desc_group,
-	&event_long_desc_group,
-	&if_group,
-	NULL,
+	[FORMAT] = &format_group,
+	[IF] = &if_group,
+	[EVENT] = &event_group,
+	[EVENT_DESC] = &event_desc_group,
+	[EVENT_LONG_DESC] = &event_long_desc_group
 };
 
 DEFINE_PER_CPU(char, hv_24x7_reqb[4096]) __aligned(4096);

@@ -1219,10 +1232,16 @@ static int hv_24x7_init(void)
 	/* sampling not supported */
 	h_24x7_pmu.capabilities |= PERF_PMU_CAP_NO_INTERRUPT;
 
-	create_events_from_catalog(&event_group.attrs,
+	r = create_events_from_catalog(&event_group.attrs,
 				   &event_desc_group.attrs,
 				   &event_long_desc_group.attrs);
 
+	if (r) {
+		h_24x7_pmu.attr_groups[EVENT] =
+		h_24x7_pmu.attr_groups[EVENT_DESC] =
+		h_24x7_pmu.attr_groups[EVENT_LONG_DESC] = 0;
+	}
+
 	r = perf_pmu_register(&h_24x7_pmu, h_24x7_pmu.name, -1);
 	if (r)
 		return r;
reviewer
#1 lore raw
Quoted 12 lines · Li Zhong

sysfs_create_groups() creates groups one by one in the attr_groups array before a NULL entry is encountered. But if an error is seen, it stops and removes all the groups already created:

        for (i = 0; groups[i]; i++) {
                error = sysfs_create_group(kobj, groups[i]);
                if (error) {
                        while (--i >= 0)
                                sysfs_remove_group(kobj, groups[i]);
                        break;
                }
        }
Quoted 7 lines

And for the three event groups of 24x7, if it is not supported, according to the above logic, it causes format and interface group to be removed because of the error.

This patch moves the three events groups to the end of the attr groups, and if create_events_from_catalog() fails to set their attributes, we set them to NULL in attr_groups.

But why are we continuing at all if create_events_from_catalog() fails?

Shouldn't that just be a fatal error and we bail?

cheers

#2 lore raw

Michael Ellerman [[email protected]] wrote:

| On Sun, 2015-15-02 at 09:42:57 UTC, Li Zhong wrote:
| > This patch moves the three events groups to the end of the attr groups,
| > and if create_events_from_catalog() fails to set their attributes, we
| > set them to NULL in attr_groups.
| 
| But why are we continuing at all if create_events_from_catalog() fails?
| 
| Shouldn't that just be a fatal error and we bail?

Well, even if create_events_from_catalog() fails, we can continue to use the 24x7 events, rather clumsily, as long as the catalog is readable. i.e. parse /sys/bus/event_source/devices/hv_24x7/interface/catalog to find event offset and run:

	perf stat -C 0 -e hv_24x7/domain=2,offset=8,core=0/ workload

Suka

reviewer
#3 lore raw
Quoted 10 lines · Sukadev Bhattiprolu

Michael Ellerman [[email protected]] wrote:

| On Sun, 2015-15-02 at 09:42:57 UTC, Li Zhong wrote:
| > This patch moves the three events groups to the end of the attr groups,
| > and if create_events_from_catalog() fails to set their attributes, we
| > set them to NULL in attr_groups.
| 
| But why are we continuing at all if create_events_from_catalog() fails?
| 
| Shouldn't that just be a fatal error and we bail?
Quoted 6 lines

Well, even if create_events_from_catalog() fails, we can continue to use the 24x7 events, rather clumsily, as long as the catalog is readable. i.e. parse /sys/bus/event_source/devices/hv_24x7/interface/catalog to find event offset and run:

	perf stat -C 0 -e hv_24x7/domain=2,offset=8,core=0/ workload

Yeah I guess, but is that really useful? And is it a case we want to support?

It seems to me if create_events_from_catalog() fails then we either have:
- a kernel bug
- some sort of hypervisor misconfiguration
- ENOMEM (in which case the system's probably dead anyway)

So in all cases trying to continue on seems fairly pointless to me.

cheers

author
#4 lore raw

RFC v2 1/1 perf/hv-24x7 fail 24x7 initcall if create_events_from_catalog() fails

As Michael pointed out, create_events_from_catalog() fails when we either have:
- a kernel bug
- some sort of hypervisor misconfiguration
- ENOMEM

In all the above cases, we can also fail 24x7 initcall.

In the patch below, I used ENODEV for hypervisor errors, not sure whether some other error values is more proper here.

Signed-off-by Li Zhong <[email protected]>

--- arch/powerpc/perf/hv-24x7.c | 37 +++++++++++++++++++++++++++++--------

 1 file changed, 29 insertions(+), 8 deletions(-)
arch/powerpc/perf/hv-24x7.c +29 -8
index 9445a82..5efc276 100644
@@ -637,7 +637,7 @@ static ssize_t catalog_event_len_validate(struct hv_24x7_event_data *event,
 
 #define MAX_4K (SIZE_MAX / 4096)
 
-static void create_events_from_catalog(struct attribute ***events_,
+static int create_events_from_catalog(struct attribute ***events_,
 		struct attribute ***event_descs_,
 		struct attribute ***event_long_descs_)
 {

@@ -655,19 +655,25 @@ static void create_events_from_catalog(struct attribute ***events_,
 	void *event_data, *end;
 	struct hv_24x7_event_data *event;
 	struct rb_root ev_uniq = RB_ROOT;
+	int ret = 0;
 
-	if (!page)
+	if (!page) {
+		ret = -ENOMEM;
 		goto e_out;
+	}
 
 	hret = h_get_24x7_catalog_page(page, 0, 0);
-	if (hret)
+	if (hret) {
+		ret = -ENODEV;
 		goto e_free;
+	}
 
 	catalog_version_num = be64_to_cpu(page_0->version);
 	catalog_page_len = be32_to_cpu(page_0->length);
 
 	if (MAX_4K < catalog_page_len) {
 		pr_err("invalid page count: %zu\n", catalog_page_len);
+		ret = -ENODEV;
 		goto e_free;
 	}
 

@@ -686,6 +692,7 @@ static void create_events_from_catalog(struct attribute ***events_,
 			|| (MAX_4K - event_data_offs < event_data_len)) {
 		pr_err("invalid event data offs %zu and/or len %zu\n",
 				event_data_offs, event_data_len);
+		ret = -ENODEV;
 		goto e_free;
 	}
 

@@ -694,12 +701,14 @@ static void create_events_from_catalog(struct attribute ***events_,
 				event_data_offs,
 				event_data_offs + event_data_len,
 				catalog_page_len);
+		ret = -ENODEV;
 		goto e_free;
 	}
 
 	if (SIZE_MAX / MAX_EVENTS_PER_EVENT_DATA - 1 < event_entry_count) {
 		pr_err("event_entry_count %zu is invalid\n",
 				event_entry_count);
+		ret = -ENODEV;
 		goto e_free;
 	}
 

@@ -712,6 +721,7 @@ static void create_events_from_catalog(struct attribute ***events_,
 	event_data = vmalloc(event_data_bytes);
 	if (!event_data) {
 		pr_err("could not allocate event data\n");
+		ret = -ENOMEM;
 		goto e_free;
 	}
 

@@ -731,6 +741,7 @@ static void create_events_from_catalog(struct attribute ***events_,
 		if (hret) {
 			pr_err("failed to get event data in page %zu\n",
 					i + event_data_offs);
+			ret = -ENODEV;
 			goto e_event_data;
 		}
 	}

@@ -778,18 +789,24 @@ static void create_events_from_catalog(struct attribute ***events_,
 				event_idx_last, event_entry_count, junk_events);
 
 	events = kmalloc_array(attr_max + 1, sizeof(*events), GFP_KERNEL);
-	if (!events)
+	if (!events) {
+		ret = -ENOMEM;
 		goto e_event_data;
+	}
 
 	event_descs = kmalloc_array(event_idx + 1, sizeof(*event_descs),
 				GFP_KERNEL);
-	if (!event_descs)
+	if (!event_descs) {
+		ret = -ENOMEM;
 		goto e_event_attrs;
+	}
 
 	event_long_descs = kmalloc_array(event_idx + 1,
 			sizeof(*event_long_descs), GFP_KERNEL);
-	if (!event_long_descs)
+	if (!event_long_descs) {
+		ret = -ENOMEM;
 		goto e_event_descs;
+	}
 
 	/* Iterate over the catalog filling in the attribute vector */
 	for (junk_events = 0, event_attr_ct = 0, desc_ct = 0, long_desc_ct = 0,

@@ -843,7 +860,7 @@ static void create_events_from_catalog(struct attribute ***events_,
 	*events_ = events;
 	*event_descs_ = event_descs;
 	*event_long_descs_ = event_long_descs;
-	return;
+	return 0;
 
 e_event_descs:
 	kfree(event_descs);

@@ -857,6 +874,7 @@ e_out:
 	*events_ = NULL;
 	*event_descs_ = NULL;
 	*event_long_descs_ = NULL;
+	return ret;
 }
 
 static ssize_t catalog_read(struct file *filp, struct kobject *kobj,

@@ -1219,10 +1237,13 @@ static int hv_24x7_init(void)
 	/* sampling not supported */
 	h_24x7_pmu.capabilities |= PERF_PMU_CAP_NO_INTERRUPT;
 
-	create_events_from_catalog(&event_group.attrs,
+	r = create_events_from_catalog(&event_group.attrs,
 				   &event_desc_group.attrs,
 				   &event_long_desc_group.attrs);
 
+	if (r)
+		return r;
+
 	r = perf_pmu_register(&h_24x7_pmu, h_24x7_pmu.name, -1);
 	if (r)
 		return r;
reviewer
#5 lore raw

Re: [RFC v2 powerpc] perf/hv-24x7 fail 24x7 initcall if create_events_from_catalog() fails

Quoted 7 lines · Li Zhong

As Michael pointed out, create_events_from_catalog() fails when we either have:
- a kernel bug
- some sort of hypervisor misconfiguration
- ENOMEM

In all the above cases, we can also fail 24x7 initcall.

Thanks.

Quoted 2 lines

In the patch below, I used ENODEV for hypervisor errors, not sure whether some other error values is more proper here.

ENODEV is a special case, if an initcall returns ENODEV no error is reported to dmesg.

In this case I think we would like an error to be reported, so EIO would be better I think.

cheers

author
#6 lore raw

PATCH v3 1/1 perf/hv-24x7 fail 24x7 initcall if create_events_from_catalog() fails

As Michael pointed out, create_events_from_catalog() fails when we either have:
- a kernel bug
- some sort of hypervisor misconfiguration
- ENOMEM

In all the above cases, we can also fail 24x7 initcall.

For hypervisor errors, EIO is used so there is something reported in dmesg.

Signed-off-by Li Zhong <[email protected]>
---
 arch/powerpc/perf/hv-24x7.c | 37 +++++++++++++++++++++++++++++--------
 1 file changed, 29 insertions(+), 8 deletions(-)
arch/powerpc/perf/hv-24x7.c +29 -8
index 9445a82..ead1420 100644
@@ -637,7 +637,7 @@ static ssize_t catalog_event_len_validate(struct hv_24x7_event_data *event,
 
 #define MAX_4K (SIZE_MAX / 4096)
 
-static void create_events_from_catalog(struct attribute ***events_,
+static int create_events_from_catalog(struct attribute ***events_,
 		struct attribute ***event_descs_,
 		struct attribute ***event_long_descs_)
 {

@@ -655,19 +655,25 @@ static void create_events_from_catalog(struct attribute ***events_,
 	void *event_data, *end;
 	struct hv_24x7_event_data *event;
 	struct rb_root ev_uniq = RB_ROOT;
+	int ret = 0;
 
-	if (!page)
+	if (!page) {
+		ret = -ENOMEM;
 		goto e_out;
+	}
 
 	hret = h_get_24x7_catalog_page(page, 0, 0);
-	if (hret)
+	if (hret) {
+		ret = -EIO;
 		goto e_free;
+	}
 
 	catalog_version_num = be64_to_cpu(page_0->version);
 	catalog_page_len = be32_to_cpu(page_0->length);
 
 	if (MAX_4K < catalog_page_len) {
 		pr_err("invalid page count: %zu\n", catalog_page_len);
+		ret = -EIO;
 		goto e_free;
 	}
 

@@ -686,6 +692,7 @@ static void create_events_from_catalog(struct attribute ***events_,
 			|| (MAX_4K - event_data_offs < event_data_len)) {
 		pr_err("invalid event data offs %zu and/or len %zu\n",
 				event_data_offs, event_data_len);
+		ret = -EIO;
 		goto e_free;
 	}
 

@@ -694,12 +701,14 @@ static void create_events_from_catalog(struct attribute ***events_,
 				event_data_offs,
 				event_data_offs + event_data_len,
 				catalog_page_len);
+		ret = -EIO;
 		goto e_free;
 	}
 
 	if (SIZE_MAX / MAX_EVENTS_PER_EVENT_DATA - 1 < event_entry_count) {
 		pr_err("event_entry_count %zu is invalid\n",
 				event_entry_count);
+		ret = -EIO;
 		goto e_free;
 	}
 

@@ -712,6 +721,7 @@ static void create_events_from_catalog(struct attribute ***events_,
 	event_data = vmalloc(event_data_bytes);
 	if (!event_data) {
 		pr_err("could not allocate event data\n");
+		ret = -ENOMEM;
 		goto e_free;
 	}
 

@@ -731,6 +741,7 @@ static void create_events_from_catalog(struct attribute ***events_,
 		if (hret) {
 			pr_err("failed to get event data in page %zu\n",
 					i + event_data_offs);
+			ret = -EIO;
 			goto e_event_data;
 		}
 	}

@@ -778,18 +789,24 @@ static void create_events_from_catalog(struct attribute ***events_,
 				event_idx_last, event_entry_count, junk_events);
 
 	events = kmalloc_array(attr_max + 1, sizeof(*events), GFP_KERNEL);
-	if (!events)
+	if (!events) {
+		ret = -ENOMEM;
 		goto e_event_data;
+	}
 
 	event_descs = kmalloc_array(event_idx + 1, sizeof(*event_descs),
 				GFP_KERNEL);
-	if (!event_descs)
+	if (!event_descs) {
+		ret = -ENOMEM;
 		goto e_event_attrs;
+	}
 
 	event_long_descs = kmalloc_array(event_idx + 1,
 			sizeof(*event_long_descs), GFP_KERNEL);
-	if (!event_long_descs)
+	if (!event_long_descs) {
+		ret = -ENOMEM;
 		goto e_event_descs;
+	}
 
 	/* Iterate over the catalog filling in the attribute vector */
 	for (junk_events = 0, event_attr_ct = 0, desc_ct = 0, long_desc_ct = 0,

@@ -843,7 +860,7 @@ static void create_events_from_catalog(struct attribute ***events_,
 	*events_ = events;
 	*event_descs_ = event_descs;
 	*event_long_descs_ = event_long_descs;
-	return;
+	return 0;
 
 e_event_descs:
 	kfree(event_descs);

@@ -857,6 +874,7 @@ e_out:
 	*events_ = NULL;
 	*event_descs_ = NULL;
 	*event_long_descs_ = NULL;
+	return ret;
 }
 
 static ssize_t catalog_read(struct file *filp, struct kobject *kobj,

@@ -1219,10 +1237,13 @@ static int hv_24x7_init(void)
 	/* sampling not supported */
 	h_24x7_pmu.capabilities |= PERF_PMU_CAP_NO_INTERRUPT;
 
-	create_events_from_catalog(&event_group.attrs,
+	r = create_events_from_catalog(&event_group.attrs,
 				   &event_desc_group.attrs,
 				   &event_long_desc_group.attrs);
 
+	if (r)
+		return r;
+
 	r = perf_pmu_register(&h_24x7_pmu, h_24x7_pmu.name, -1);
 	if (r)
 		return r;