| Message ID | 20201217113230.1069882-1-kjain@linux.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | powerpc/perf/hv-24x7: Dont create sysfs event files for dummy events | expand |
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). 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 <kjain@linux.ibm.com> > --- > 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);
On 12/17/20 5:10 PM, Madhavan Srinivasan wrote: > > 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. 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 > > >> 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 <kjain@linux.ibm.com> >> --- >> 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 <kjain@linux.ibm.com> writes: > 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 <kjain@linux.ibm.com> > --- > 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. > + } > 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. cheers
On 12/18/20 6:26 AM, Michael Ellerman wrote: > Kajol Jain <kjain@linux.ibm.com> writes: >> 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 <kjain@linux.ibm.com> >> --- >> 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. > > >> + } >> 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 >
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);
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 <kjain@linux.ibm.com> --- arch/powerpc/perf/hv-24x7.c | 8 ++++++++ 1 file changed, 8 insertions(+)