Re: [PATCH] perf mem: Fix perf mem error on hybrid

[Liang, Kan]

On 2023-11-29 1:24 a.m., Ian Rogers wrote:
> On Tue, Nov 28, 2023 at 12:39 PM <kan.liang@xxxxxxxxxxxxxxx> wrote:
>>
>> From: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
>>
>> The below error can be triggered on a hybrid machine.
>>
>>  $ perf mem record -t load sleep 1
>>  event syntax error: 'breakpoint/mem-loads,ldlat=30/P'
>>                                 \___ Bad event or PMU
>>
>>  Unable to find PMU or event on a PMU of 'breakpoint'
>>
>> In the perf_mem_events__record_args(), the current perf never checks the
>> availability of a mem event on a given PMU. All the PMUs will be added
>> to the perf mem event list. Perf errors out for the unsupported PMU.
>>
>> Extend perf_mem_event__supported() and take a PMU into account. Check
>> the mem event for each PMU before adding it to the perf mem event list.
>>
>> Optimize the perf_mem_events__init() a little bit. The function is to
>> check whether the mem events are supported in the system. It doesn't
>> need to scan all PMUs. Just return with the first supported PMU is good
>> enough.
>>
>> Fixes: 5752c20f3787 ("perf mem: Scan all PMUs instead of just core ones")
>> Reported-by: Ammy Yi <ammy.yi@xxxxxxxxx>
>> Tested-by: Ammy Yi <ammy.yi@xxxxxxxxx>
>> Signed-off-by: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
>> ---
>>  tools/perf/util/mem-events.c | 25 ++++++++++++++-----------
>>  1 file changed, 14 insertions(+), 11 deletions(-)
>>
>> diff --git a/tools/perf/util/mem-events.c b/tools/perf/util/mem-events.c
>> index 954b235e12e5..3a2e3687878c 100644
>> --- a/tools/perf/util/mem-events.c
>> +++ b/tools/perf/util/mem-events.c
>> @@ -100,11 +100,14 @@ int perf_mem_events__parse(const char *str)
>>         return -1;
>>  }
>>
>> -static bool perf_mem_event__supported(const char *mnt, char *sysfs_name)
>> +static bool perf_mem_event__supported(const char *mnt, struct perf_pmu *pmu,
>> +                                     struct perf_mem_event *e)
>>  {
>> +       char sysfs_name[100];
>>         char path[PATH_MAX];
>>         struct stat st;
>>
>> +       scnprintf(sysfs_name, sizeof(sysfs_name), e->sysfs_name, pmu->name);
> 
> Not sure if this is right. Looking at sysfs_name values:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/mem-events.c?h=perf-tools-next#n23
> "cpu/events/mem-loads" and "cpu/events/mem-stores", so won't pmu->name
> never be used?
> Is there a missed change to change the cpu to %s?

There is a X86 specific perf_mem_events__ptr(), which uses the
"%s/mem-loads,ldlat=%u/P" and "%s/events/mem-loads" for Intel platforms.
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/arch/x86/util/mem-events.c?h=perf-tools-next#n20
The pmu->name is used especially for the hybrid platforms.

Thanks,
Kan
> 
> Thanks,
> Ian
> 
>>         scnprintf(path, PATH_MAX, "%s/devices/%s", mnt, sysfs_name);
>>         return !stat(path, &st);
>>  }
>> @@ -120,7 +123,6 @@ int perf_mem_events__init(void)
>>
>>         for (j = 0; j < PERF_MEM_EVENTS__MAX; j++) {
>>                 struct perf_mem_event *e = perf_mem_events__ptr(j);
>> -               char sysfs_name[100];
>>                 struct perf_pmu *pmu = NULL;
>>
>>                 /*
>> @@ -136,12 +138,12 @@ int perf_mem_events__init(void)
>>                  * of core PMU.
>>                  */
>>                 while ((pmu = perf_pmus__scan(pmu)) != NULL) {
>> -                       scnprintf(sysfs_name, sizeof(sysfs_name), e->sysfs_name, pmu->name);
>> -                       e->supported |= perf_mem_event__supported(mnt, sysfs_name);
>> +                       e->supported |= perf_mem_event__supported(mnt, pmu, e);
>> +                       if (e->supported) {
>> +                               found = true;
>> +                               break;
>> +                       }
>>                 }
>> -
>> -               if (e->supported)
>> -                       found = true;
>>         }
>>
>>         return found ? 0 : -ENOENT;
>> @@ -167,13 +169,10 @@ static void perf_mem_events__print_unsupport_hybrid(struct perf_mem_event *e,
>>                                                     int idx)
>>  {
>>         const char *mnt = sysfs__mount();
>> -       char sysfs_name[100];
>>         struct perf_pmu *pmu = NULL;
>>
>>         while ((pmu = perf_pmus__scan(pmu)) != NULL) {
>> -               scnprintf(sysfs_name, sizeof(sysfs_name), e->sysfs_name,
>> -                         pmu->name);
>> -               if (!perf_mem_event__supported(mnt, sysfs_name)) {
>> +               if (!perf_mem_event__supported(mnt, pmu, e)) {
>>                         pr_err("failed: event '%s' not supported\n",
>>                                perf_mem_events__name(idx, pmu->name));
>>                 }
>> @@ -183,6 +182,7 @@ static void perf_mem_events__print_unsupport_hybrid(struct perf_mem_event *e,
>>  int perf_mem_events__record_args(const char **rec_argv, int *argv_nr,
>>                                  char **rec_tmp, int *tmp_nr)
>>  {
>> +       const char *mnt = sysfs__mount();
>>         int i = *argv_nr, k = 0;
>>         struct perf_mem_event *e;
>>
>> @@ -211,6 +211,9 @@ int perf_mem_events__record_args(const char **rec_argv, int *argv_nr,
>>                         while ((pmu = perf_pmus__scan(pmu)) != NULL) {
>>                                 const char *s = perf_mem_events__name(j, pmu->name);
>>
>> +                               if (!perf_mem_event__supported(mnt, pmu, e))
>> +                                       continue;
>> +
>>                                 rec_argv[i++] = "-e";
>>                                 if (s) {
>>                                         char *copy = strdup(s);
>> --
>> 2.35.1
>>