Re: [PATCH] tools/perf: Add check to tool pmu tests to ensure if the event is valid

[Athira Rajeev <atrajeev@linux.ibm.com>]

> On 13 Feb 2025, at 9:04 AM, Namhyung Kim <namhyung@kernel.org> wrote:
> 
> On Thu, Feb 13, 2025 at 12:24:38AM +0530, Athira Rajeev wrote:
>> "Tool PMU" tests fails on powerpc as below:
>> 
>>   12.1: Parsing without PMU name:
>>   --- start ---
>>   test child forked, pid 48492
>>   Using CPUID 0x00800200
>>   Attempt to add: tool/duration_time/
>>   ..after resolving event: tool/config=0x1/
>>   duration_time -> tool/duration_time/
>>   Attempt to add: tool/user_time/
>>   ..after resolving event: tool/config=0x2/
>>   user_time -> tool/user_time/
>>   Attempt to add: tool/system_time/
>>   ..after resolving event: tool/config=0x3/
>>   system_time -> tool/system_time/
>>   Attempt to add: tool/has_pmem/
>>   ..after resolving event: tool/config=0x4/
>>   has_pmem -> tool/has_pmem/
>>   Attempt to add: tool/num_cores/
>>   ..after resolving event: tool/config=0x5/
>>   num_cores -> tool/num_cores/
>>   Attempt to add: tool/num_cpus/
>>   ..after resolving event: tool/config=0x6/
>>   num_cpus -> tool/num_cpus/
>>   Attempt to add: tool/num_cpus_online/
>>   ..after resolving event: tool/config=0x7/
>>   num_cpus_online -> tool/num_cpus_online/
>>   Attempt to add: tool/num_dies/
>>   ..after resolving event: tool/config=0x8/
>>   num_dies -> tool/num_dies/
>>   Attempt to add: tool/num_packages/
>>   ..after resolving event: tool/config=0x9/
>>   num_packages -> tool/num_packages/
>> 
>>   ---- unexpected signal (11) ----
>>   12.1: Parsing without PMU name                                      : FAILED!
>> 
>> Same fail is observed for "Parsing with PMU name" as well.
>> 
>> The testcase loops through events in tool_pmu__for_each_event()
>> and access event name using "tool_pmu__event_to_str()".
>> Here tool_pmu__event_to_str returns null for "slots" event
>> and "system_tsc_freq" event. These two events are only applicable
>> for arm64 and x86 respectively. So the function tool_pmu__event_to_str()
>> skips for unsupported events and returns null. This null value is
>> causing testcase fail.
>> 
>> To address this in "Tool PMU" testcase, add a helper function
>> tool_pmu__all_event_to_str() which returns the name for all
>> events mapping to the tool_pmu_event index including the
>> skipped ones. So that even if its a skipped event, the
>> helper function helps to resolve the tool_pmu_event index to
>> its mapping event name. Update the testcase to check for null event
>> names before proceeding the test.
>> 
>> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> 
> Please take a look at:
> https://lore.kernel.org/r/20250212163859.1489916-1-james.clark@linaro.org
> 
> Thanks,
> Namhyung

Hi,

Sure thanks for the fix James

Thomas,
Thanks for testing this patch.  But James already fixed this with a different patch and it is part of perf-tools-next
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/commit/?h=perf-tools-next&id=615ec00b06f78912c370b372426190768402a5b9

Please test with latest perf-tools-next 

Thanks
Athira

> 
>> ---
>> tools/perf/tests/tool_pmu.c | 12 ++++++++++++
>> tools/perf/util/tool_pmu.c  | 17 +++++++++++++++++
>> tools/perf/util/tool_pmu.h  |  1 +
>> 3 files changed, 30 insertions(+)
>> 
>> diff --git a/tools/perf/tests/tool_pmu.c b/tools/perf/tests/tool_pmu.c
>> index 187942b749b7..e468e5fb3c73 100644
>> --- a/tools/perf/tests/tool_pmu.c
>> +++ b/tools/perf/tests/tool_pmu.c
>> @@ -19,6 +19,18 @@ static int do_test(enum tool_pmu_event ev, bool with_pmu)
>> return TEST_FAIL;
>> }
>> 
>> + /*
>> +  * if tool_pmu__event_to_str returns NULL, Check if the event is
>> +  * valid for the platform.
>> +  * Example:
>> +  * slots event is only on arm64.
>> +  * system_tsc_freq event is only on x86.
>> +  */
>> + if (!tool_pmu__event_to_str(ev) && tool_pmu__skip_event(tool_pmu__all_event_to_str(ev))) {
>> + ret = TEST_OK;
>> + goto out;
>> + }
>> +
>> if (with_pmu)
>> snprintf(str, sizeof(str), "tool/%s/", tool_pmu__event_to_str(ev));
>> else
>> diff --git a/tools/perf/util/tool_pmu.c b/tools/perf/util/tool_pmu.c
>> index 3a68debe7143..572422797f6e 100644
>> --- a/tools/perf/util/tool_pmu.c
>> +++ b/tools/perf/util/tool_pmu.c
>> @@ -60,6 +60,15 @@ int tool_pmu__num_skip_events(void)
>> return num;
>> }
>> 
>> +/*
>> + * tool_pmu__event_to_str returns only supported event names.
>> + * For events which are supposed to be skipped in the platform,
>> + * return NULL
>> + *
>> + * tool_pmu__all_event_to_str returns the name for all
>> + * events mapping to the tool_pmu_event index including the
>> + * skipped ones.
>> + */
>> const char *tool_pmu__event_to_str(enum tool_pmu_event ev)
>> {
>> if ((ev > TOOL_PMU__EVENT_NONE && ev < TOOL_PMU__EVENT_MAX) &&
>> @@ -69,6 +78,14 @@ const char *tool_pmu__event_to_str(enum tool_pmu_event ev)
>> return NULL;
>> }
>> 
>> +const char *tool_pmu__all_event_to_str(enum tool_pmu_event ev)
>> +{
>> + if (ev > TOOL_PMU__EVENT_NONE && ev < TOOL_PMU__EVENT_MAX)
>> + return tool_pmu__event_names[ev];
>> +
>> + return NULL;
>> +}
>> +
>> enum tool_pmu_event tool_pmu__str_to_event(const char *str)
>> {
>> int i;
>> diff --git a/tools/perf/util/tool_pmu.h b/tools/perf/util/tool_pmu.h
>> index a60184859080..da76808ae71e 100644
>> --- a/tools/perf/util/tool_pmu.h
>> +++ b/tools/perf/util/tool_pmu.h
>> @@ -30,6 +30,7 @@ enum tool_pmu_event {
>> for ((ev) = TOOL_PMU__EVENT_DURATION_TIME; (ev) < TOOL_PMU__EVENT_MAX; ev++)
>> 
>> const char *tool_pmu__event_to_str(enum tool_pmu_event ev);
>> +const char *tool_pmu__all_event_to_str(enum tool_pmu_event ev);
>> enum tool_pmu_event tool_pmu__str_to_event(const char *str);
>> bool tool_pmu__skip_event(const char *name);
>> int tool_pmu__num_skip_events(void);
>> -- 
>> 2.43.5