[PATCH] powerpc/perf/hv-24x7: Dont create sysfs event files for dummy events

hv_24x7 performance monitoring unit creates list of supported events
from the event catalog obtained via HCALL. hv_24x7 catalog could also
contain invalid or dummy events (with names like FREE_  or CPM_FREE_ so
on). These events does not have any hardware counters backing them.
So patch adds a check to string compare the event names to filter
out them.

Signed-off-by: Kajol Jain <redacted>
---
 arch/powerpc/perf/hv-24x7.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index 6e7e820508df..c3252d8a7818 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c

@@ -894,6 +894,11 @@ static int create_events_from_catalog(struct attribute ***events_,
 
 		name = event_name(event, &nl);
 
+		if (strstr(name, "FREE_")) {
+			pr_info("invalid event %zu (%.*s)\n", event_idx, nl, name);
+			junk_events++;
+			continue;
+		}
 		if (event->event_group_record_len == 0) {
 			pr_devel("invalid event %zu (%.*s): group_record_len == 0, skipping\n",
 					event_idx, nl, name);

@@ -955,6 +960,9 @@ static int create_events_from_catalog(struct attribute ***events_,
 			continue;
 
 		name  = event_name(event, &nl);
+		if (strstr(name, "FREE_"))
+			continue;
+
 		nonce = event_uniq_add(&ev_uniq, name, nl, event->domain);
 		ct    = event_data_to_attrs(event_idx, events + event_attr_ct,
 					    event, nonce);

-- 
2.26.2

On 12/17/20 5:02 PM, Kajol Jain wrote:

hv_24x7 performance monitoring unit creates list of supported events
from the event catalog obtained via HCALL. hv_24x7 catalog could also
contain invalid or dummy events (with names like FREE_  or CPM_FREE_ so

Can you also include " RESERVED_NEST*" as part of the check.

# ls /sys/devices/hv_24x7/events | grep RESERVED
RESERVED_NEST1
RESERVED_NEST10
RESERVED_NEST11
RESERVED_NEST12
...


Maddy

On 12/17/20 5:10 PM, Madhavan Srinivasan wrote:

On 12/17/20 5:02 PM, Kajol Jain wrote:

quoted

hv_24x7 performance monitoring unit creates list of supported events
from the event catalog obtained via HCALL. hv_24x7 catalog could also
contain invalid or dummy events (with names like FREE_  or CPM_FREE_ so

Can you also include " RESERVED_NEST*" as part of the check.

Hi Maddy,
   Sure, I will add this check.

Thanks,
Kajol Jain

# ls /sys/devices/hv_24x7/events | grep RESERVED
RESERVED_NEST1
RESERVED_NEST10
RESERVED_NEST11
RESERVED_NEST12
...


Maddy

quoted

on). These events does not have any hardware counters backing them.
So patch adds a check to string compare the event names to filter
out them.

Signed-off-by: Kajol Jain <redacted>
---
  arch/powerpc/perf/hv-24x7.c | 8 ++++++++
  1 file changed, 8 insertions(+)

diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index 6e7e820508df..c3252d8a7818 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c

@@ -894,6 +894,11 @@ static int create_events_from_catalog(struct attribute ***events_,

          name = event_name(event, &nl);

+        if (strstr(name, "FREE_")) {
+            pr_info("invalid event %zu (%.*s)\n", event_idx, nl, name);
+            junk_events++;
+            continue;
+        }
          if (event->event_group_record_len == 0) {
              pr_devel("invalid event %zu (%.*s): group_record_len == 0, skipping\n",
                      event_idx, nl, name);

@@ -955,6 +960,9 @@ static int create_events_from_catalog(struct attribute ***events_,

              continue;

          name  = event_name(event, &nl);
+        if (strstr(name, "FREE_"))
+            continue;
+
          nonce = event_uniq_add(&ev_uniq, name, nl, event->domain);
          ct    = event_data_to_attrs(event_idx, events + event_attr_ct,
                          event, nonce);

Kajol Jain [off-list ref] writes:

I don't think we want a print for each event, just one at the end saying
"Dropped %d invalid events" would be preferable I think.

Would be nice if the string comparison was in a single place, ie. in a
helper function.

cheers

On 12/18/20 6:26 AM, Michael Ellerman wrote:

Kajol Jain [off-list ref] writes:

quoted

hv_24x7 performance monitoring unit creates list of supported events
from the event catalog obtained via HCALL. hv_24x7 catalog could also
contain invalid or dummy events (with names like FREE_  or CPM_FREE_ so
on). These events does not have any hardware counters backing them.
So patch adds a check to string compare the event names to filter
out them.

Signed-off-by: Kajol Jain <redacted>
---
 arch/powerpc/perf/hv-24x7.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index 6e7e820508df..c3252d8a7818 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c

@@ -894,6 +894,11 @@ static int create_events_from_catalog(struct attribute ***events_,
 
 		name = event_name(event, &nl);
 
+		if (strstr(name, "FREE_")) {
+			pr_info("invalid event %zu (%.*s)\n", event_idx, nl, name);
+			junk_events++;
+			continue;

I don't think we want a print for each event, just one at the end saying
"Dropped %d invalid events" would be preferable I think.

Hi Michael,
  Sure I will remove prints for each event. Having one print for number of dropped
events may not be useful. So I will drop that too.

quoted

+		}
 		if (event->event_group_record_len == 0) {
 			pr_devel("invalid event %zu (%.*s): group_record_len == 0, skipping\n",
 					event_idx, nl, name);

@@ -955,6 +960,9 @@ static int create_events_from_catalog(struct attribute ***events_,
 			continue;
 
 		name  = event_name(event, &nl);
+		if (strstr(name, "FREE_"))
+			continue;

Would be nice if the string comparison was in a single place, ie. in a
helper function.

Sure I will make that change.

Thanks,
Kajol Jain

cheers