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

[kajoljain <kjain@linux.ibm.com>]

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
>