| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 29 Nov 2023 08:52:13 -0500
From: "Liang, Kan" <kan.liang@...ux.intel.com>
To: Ian Rogers <irogers@...gle.com>
Cc: acme@...nel.org, mingo@...hat.com, peterz@...radead.org,
mark.rutland@....com, namhyung@...nel.org, jolsa@...nel.org,
adrian.hunter@...el.com, ravi.bangoria@....com,
linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org,
Ammy Yi <ammy.yi@...el.com>
Subject: Re: [PATCH] perf mem: Fix perf mem error on hybrid
On 2023-11-29 1:24 a.m., Ian Rogers wrote:
> On Tue, Nov 28, 2023 at 12:39 PM <kan.liang@...ux.intel.com> wrote:
>>
>> From: Kan Liang <kan.liang@...ux.intel.com>
>>
>> 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@...el.com>
>> Tested-by: Ammy Yi <ammy.yi@...el.com>
>> Signed-off-by: Kan Liang <kan.liang@...ux.intel.com>
>> ---
>> 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
>>
Powered by blists - more mailing lists