| 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
| ||
|
Message-ID: <f5112f5e-c77c-4a9a-ac3b-66772adba471@linux.intel.com>
Date: Wed, 29 Nov 2023 16:15:16 -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 11:17 a.m., Ian Rogers wrote:
> On Wed, Nov 29, 2023 at 5:52 AM Liang, Kan <kan.liang@...ux.intel.com> wrote:
>>
>>
>>
>> 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.
>
> Right, that seems wrong. For one thing we're losing the compiler's
> format string argument checking, but hardcoding PMU names just seems
> to be something that will keep needing maintenance. This patch set
> looks to fix an Intel issue but in general it is increasing tech debt
> (or at least churning it) that will need cleaning up to do something
> with better error checking and more generic. perf_mem_event looks like
> a bad abstraction and then there are the integers whose special values
> hold meaning. Could this fix come with some cleanup? It wouldn't seem
> wrong to me to add notions of memory events to the PMU abstraction. As
> it stands this scnprintf looks wrong in non-Intel cases.
>
The problem is that different ARCHs check different things. Arm and AMD
checks the PMU name, while Intel and Power checks the specific events.
It's hard to have a unified scnprintf.
But we can abstract them into two cases, PMU name and event name. We use
a different scnprintf to handle them.
How about something as below?
diff --git a/tools/perf/arch/x86/util/mem-events.c
b/tools/perf/arch/x86/util/mem-events.c
index 191b372f9a2d..4ef70fb9132b 100644
--- a/tools/perf/arch/x86/util/mem-events.c
+++ b/tools/perf/arch/x86/util/mem-events.c
@@ -17,8 +17,8 @@ static char mem_stores_name[100];
#define E(t, n, s) { .tag = t, .name = n, .sysfs_name = s }
static struct perf_mem_event
perf_mem_events_intel[PERF_MEM_EVENTS__MAX] = {
- E("ldlat-loads", "%s/mem-loads,ldlat=%u/P", "%s/events/mem-loads"),
- E("ldlat-stores", "%s/mem-stores/P", "%s/events/mem-stores"),
+ E("ldlat-loads", "%s/mem-loads,ldlat=%u/P", "events/mem-loads"),
+ E("ldlat-stores", "%s/mem-stores/P", "events/mem-stores"),
E(NULL, NULL, NULL),
};
diff --git a/tools/perf/util/mem-events.c b/tools/perf/util/mem-events.c
index 3a2e3687878c..ba88cb3d804f 100644
--- a/tools/perf/util/mem-events.c
+++ b/tools/perf/util/mem-events.c
@@ -8,6 +8,7 @@
#include <unistd.h>
#include <api/fs/fs.h>
#include <linux/kernel.h>
+#include <linux/string.h>
#include "map_symbol.h"
#include "mem-events.h"
#include "debug.h"
@@ -20,8 +21,8 @@ unsigned int perf_mem_events__loads_ldlat = 30;
#define E(t, n, s) { .tag = t, .name = n, .sysfs_name = s }
static struct perf_mem_event perf_mem_events[PERF_MEM_EVENTS__MAX] = {
- E("ldlat-loads", "cpu/mem-loads,ldlat=%u/P", "cpu/events/mem-loads"),
- E("ldlat-stores", "cpu/mem-stores/P", "cpu/events/mem-stores"),
+ E("ldlat-loads", "cpu/mem-loads,ldlat=%u/P", "events/mem-loads"),
+ E("ldlat-stores", "cpu/mem-stores/P", "events/mem-stores"),
E(NULL, NULL, NULL),
};
#undef E
@@ -103,12 +104,14 @@ int perf_mem_events__parse(const char *str)
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);
- scnprintf(path, PATH_MAX, "%s/devices/%s", mnt, sysfs_name);
+ if (strstarts(e->sysfs_name, "event/"))
+ scnprintf(path, PATH_MAX, "%s/devices/%s/%s", mnt, pmu->name,
e->sysfs_name);
+ else
+ scnprintf(path, PATH_MAX, "%s/devices/%s", mnt, e->sysfs_name);
+
return !stat(path, &st);
}
Thanks,
Kan
> Thanks,
> Ian
>
>> 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