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

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

[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;
                }
        }

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 <zhong@linux.vnet.ibm.com>
---
 arch/powerpc/perf/hv-24x7.c | 37 ++++++++++++++++++++++++++++---------
 1 file changed, 28 insertions(+), 9 deletions(-)

diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index 9445a82..1e433f7 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c
@@ -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;

Re: [RFC, powerpc] perf/hv-24x7 set the attr group to NULL if events failed to be initialized

[Michael Ellerman]

On Sun, 2015-15-02 at 09:42:57 UTC, Li Zhong wrote:
> 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.

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

Re: [RFC, powerpc] perf/hv-24x7 set the attr group to NULL if events failed to be initialized

[Sukadev Bhattiprolu]

Michael Ellerman [mpe@ellerman.id.au] 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

Re: [RFC, powerpc] perf/hv-24x7 set the attr group to NULL if events failed to be initialized

[Michael Ellerman]

On Wed, 2015-03-25 at 11:41 -0700, Sukadev Bhattiprolu wrote:
> Michael Ellerman [mpe@ellerman.id.au] 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


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

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

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

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 <zhong@linux.vnet.ibm.com>
--- arch/powerpc/perf/hv-24x7.c | 37 +++++++++++++++++++++++++++++--------
 1 file changed, 29 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index 9445a82..5efc276 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c
@@ -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;

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

[Michael Ellerman]

On Fri, 2015-04-10 at 16:42 +0800, Li Zhong wrote:
> 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.
 
> 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

[PATCH v3 powerpc] perf/hv-24x7 fail 24x7 initcall if create_events_from_catalog() fails

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

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

Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
---
 arch/powerpc/perf/hv-24x7.c | 37 +++++++++++++++++++++++++++++--------
 1 file changed, 29 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index 9445a82..ead1420 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c
@@ -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;