| Message ID | 20231207192338.400336-2-kan.liang@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Clean up perf mem | expand |
On Thu, Dec 07, 2023 at 11:23:34AM -0800, kan.liang@linux.intel.com wrote: > From: Kan Liang <kan.liang@linux.intel.com> > > With the mem_events, perf doesn't need to read sysfs for each PMU to > find the mem-events-supported PMU. The patch also makes it possible to > clean up the related __weak functions later. > > The patch is only to add the mem_events into the perf_pmu for all ARCHs. > It will be used in the later cleanup patches. > > Reviewed-by: Ian Rogers <irogers@google.com> > Tested-by: Ravi Bangoria <ravi.bangoria@amd.com> > Signed-off-by: Kan Liang <kan.liang@linux.intel.com> > --- > tools/perf/arch/arm64/util/mem-events.c | 4 ++-- > tools/perf/arch/arm64/util/mem-events.h | 7 +++++++ > tools/perf/arch/arm64/util/pmu.c | 6 ++++++ > tools/perf/arch/s390/util/pmu.c | 3 +++ > tools/perf/arch/x86/util/mem-events.c | 4 ++-- > tools/perf/arch/x86/util/mem-events.h | 9 +++++++++ > tools/perf/arch/x86/util/pmu.c | 7 +++++++ > tools/perf/util/mem-events.c | 2 +- > tools/perf/util/mem-events.h | 1 + > tools/perf/util/pmu.c | 4 +++- > tools/perf/util/pmu.h | 7 +++++++ > 11 files changed, 48 insertions(+), 6 deletions(-) > create mode 100644 tools/perf/arch/arm64/util/mem-events.h > create mode 100644 tools/perf/arch/x86/util/mem-events.h > > diff --git a/tools/perf/arch/arm64/util/mem-events.c b/tools/perf/arch/arm64/util/mem-events.c > index 3bcc5c7035c2..aaa4804922b4 100644 > --- a/tools/perf/arch/arm64/util/mem-events.c > +++ b/tools/perf/arch/arm64/util/mem-events.c > @@ -4,7 +4,7 @@ > > #define E(t, n, s) { .tag = t, .name = n, .sysfs_name = s } > > -static struct perf_mem_event perf_mem_events[PERF_MEM_EVENTS__MAX] = { > +struct perf_mem_event perf_mem_events_arm[PERF_MEM_EVENTS__MAX] = { > E("spe-load", "arm_spe_0/ts_enable=1,pa_enable=1,load_filter=1,store_filter=0,min_latency=%u/", "arm_spe_0"), > E("spe-store", "arm_spe_0/ts_enable=1,pa_enable=1,load_filter=0,store_filter=1/", "arm_spe_0"), > E("spe-ldst", "arm_spe_0/ts_enable=1,pa_enable=1,load_filter=1,store_filter=1,min_latency=%u/", "arm_spe_0"), > @@ -17,7 +17,7 @@ struct perf_mem_event *perf_mem_events__ptr(int i) > if (i >= PERF_MEM_EVENTS__MAX) > return NULL; > > - return &perf_mem_events[i]; > + return &perf_mem_events_arm[i]; I recognized that it's hard code to "arm_spe_0", which might break if system registers different Arm SPE groups. But this is not the issue introduced by this patch, we might need to consider to fix it later. > } > > const char *perf_mem_events__name(int i, const char *pmu_name __maybe_unused) > diff --git a/tools/perf/arch/arm64/util/mem-events.h b/tools/perf/arch/arm64/util/mem-events.h > new file mode 100644 > index 000000000000..5fc50be4be38 > --- /dev/null > +++ b/tools/perf/arch/arm64/util/mem-events.h > @@ -0,0 +1,7 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef _ARM64_MEM_EVENTS_H > +#define _ARM64_MEM_EVENTS_H > + > +extern struct perf_mem_event perf_mem_events_arm[PERF_MEM_EVENTS__MAX]; > + > +#endif /* _ARM64_MEM_EVENTS_H */ > diff --git a/tools/perf/arch/arm64/util/pmu.c b/tools/perf/arch/arm64/util/pmu.c > index 2a4eab2d160e..06ec9b838807 100644 > --- a/tools/perf/arch/arm64/util/pmu.c > +++ b/tools/perf/arch/arm64/util/pmu.c > @@ -8,6 +8,12 @@ > #include <api/fs/fs.h> > #include <math.h> > > +void perf_pmu__arch_init(struct perf_pmu *pmu) > +{ > + if (!strcmp(pmu->name, "arm_spe_0")) > + pmu->mem_events = perf_mem_events_arm; This is not right and it should cause building failure on aarch64. aarch64 reuses aarch32's file arch/arm/util/pmu.c, and this file has already defined perf_pmu__arch_init(), you should add above change in the file arch/arm/util/pmu.c. Now I cannot access a machine for testing Arm SPE, but I will play a bit for this patch set to ensure it can pass compilation. After that, I will seek Arm maintainers/reviewers help for the test. Thanks, Leo
On 2023-12-08 5:29 a.m., Leo Yan wrote: > On Thu, Dec 07, 2023 at 11:23:34AM -0800, kan.liang@linux.intel.com wrote: >> From: Kan Liang <kan.liang@linux.intel.com> >> >> With the mem_events, perf doesn't need to read sysfs for each PMU to >> find the mem-events-supported PMU. The patch also makes it possible to >> clean up the related __weak functions later. >> >> The patch is only to add the mem_events into the perf_pmu for all ARCHs. >> It will be used in the later cleanup patches. >> >> Reviewed-by: Ian Rogers <irogers@google.com> >> Tested-by: Ravi Bangoria <ravi.bangoria@amd.com> >> Signed-off-by: Kan Liang <kan.liang@linux.intel.com> >> --- >> tools/perf/arch/arm64/util/mem-events.c | 4 ++-- >> tools/perf/arch/arm64/util/mem-events.h | 7 +++++++ >> tools/perf/arch/arm64/util/pmu.c | 6 ++++++ >> tools/perf/arch/s390/util/pmu.c | 3 +++ >> tools/perf/arch/x86/util/mem-events.c | 4 ++-- >> tools/perf/arch/x86/util/mem-events.h | 9 +++++++++ >> tools/perf/arch/x86/util/pmu.c | 7 +++++++ >> tools/perf/util/mem-events.c | 2 +- >> tools/perf/util/mem-events.h | 1 + >> tools/perf/util/pmu.c | 4 +++- >> tools/perf/util/pmu.h | 7 +++++++ >> 11 files changed, 48 insertions(+), 6 deletions(-) >> create mode 100644 tools/perf/arch/arm64/util/mem-events.h >> create mode 100644 tools/perf/arch/x86/util/mem-events.h >> >> diff --git a/tools/perf/arch/arm64/util/mem-events.c b/tools/perf/arch/arm64/util/mem-events.c >> index 3bcc5c7035c2..aaa4804922b4 100644 >> --- a/tools/perf/arch/arm64/util/mem-events.c >> +++ b/tools/perf/arch/arm64/util/mem-events.c >> @@ -4,7 +4,7 @@ >> >> #define E(t, n, s) { .tag = t, .name = n, .sysfs_name = s } >> >> -static struct perf_mem_event perf_mem_events[PERF_MEM_EVENTS__MAX] = { >> +struct perf_mem_event perf_mem_events_arm[PERF_MEM_EVENTS__MAX] = { >> E("spe-load", "arm_spe_0/ts_enable=1,pa_enable=1,load_filter=1,store_filter=0,min_latency=%u/", "arm_spe_0"), >> E("spe-store", "arm_spe_0/ts_enable=1,pa_enable=1,load_filter=0,store_filter=1/", "arm_spe_0"), >> E("spe-ldst", "arm_spe_0/ts_enable=1,pa_enable=1,load_filter=1,store_filter=1,min_latency=%u/", "arm_spe_0"), >> @@ -17,7 +17,7 @@ struct perf_mem_event *perf_mem_events__ptr(int i) >> if (i >= PERF_MEM_EVENTS__MAX) >> return NULL; >> >> - return &perf_mem_events[i]; >> + return &perf_mem_events_arm[i]; > > I recognized that it's hard code to "arm_spe_0", which might break if > system registers different Arm SPE groups. But this is not the issue > introduced by this patch, we might need to consider to fix it later. > >> } >> >> const char *perf_mem_events__name(int i, const char *pmu_name __maybe_unused) >> diff --git a/tools/perf/arch/arm64/util/mem-events.h b/tools/perf/arch/arm64/util/mem-events.h >> new file mode 100644 >> index 000000000000..5fc50be4be38 >> --- /dev/null >> +++ b/tools/perf/arch/arm64/util/mem-events.h >> @@ -0,0 +1,7 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +#ifndef _ARM64_MEM_EVENTS_H >> +#define _ARM64_MEM_EVENTS_H >> + >> +extern struct perf_mem_event perf_mem_events_arm[PERF_MEM_EVENTS__MAX]; >> + >> +#endif /* _ARM64_MEM_EVENTS_H */ >> diff --git a/tools/perf/arch/arm64/util/pmu.c b/tools/perf/arch/arm64/util/pmu.c >> index 2a4eab2d160e..06ec9b838807 100644 >> --- a/tools/perf/arch/arm64/util/pmu.c >> +++ b/tools/perf/arch/arm64/util/pmu.c >> @@ -8,6 +8,12 @@ >> #include <api/fs/fs.h> >> #include <math.h> >> >> +void perf_pmu__arch_init(struct perf_pmu *pmu) >> +{ >> + if (!strcmp(pmu->name, "arm_spe_0")) >> + pmu->mem_events = perf_mem_events_arm; > > This is not right and it should cause building failure on aarch64. > > aarch64 reuses aarch32's file arch/arm/util/pmu.c, and this file has > already defined perf_pmu__arch_init(), you should add above change in > the file arch/arm/util/pmu.c. > Sure. > Now I cannot access a machine for testing Arm SPE, but I will play > a bit for this patch set to ensure it can pass compilation. After > that, I will seek Arm maintainers/reviewers help for the test. > Thanks. I guess I will hold the v3 until the test is done in case there are other issues found in ARM. Thanks, Kan
Hi Kan, On Fri, Dec 08, 2023 at 01:14:28PM -0500, Liang, Kan wrote: [...] > > Now I cannot access a machine for testing Arm SPE, but I will play > > a bit for this patch set to ensure it can pass compilation. After > > that, I will seek Arm maintainers/reviewers help for the test. > > Thanks. I guess I will hold the v3 until the test is done in case there > are other issues found in ARM. I will hold on a bit for the test until this patch set addresses the concern for the breakage issues on Arm64. Please check my review in other replies. Thanks, Leo
On 2023-12-09 1:34 a.m., Leo Yan wrote: > Hi Kan, > > On Fri, Dec 08, 2023 at 01:14:28PM -0500, Liang, Kan wrote: > > [...] > >>> Now I cannot access a machine for testing Arm SPE, but I will play >>> a bit for this patch set to ensure it can pass compilation. After >>> that, I will seek Arm maintainers/reviewers help for the test. >> >> Thanks. I guess I will hold the v3 until the test is done in case there >> are other issues found in ARM. > > I will hold on a bit for the test until this patch set addresses the > concern for the breakage issues on Arm64. Please check my review in > other replies. The reviews in the other replies don't look like break any current usage on Arm64. I think the breakage issue is what you described in this patch, right? If we move the check of "arm_spe_0" to arch/arm/util/pmu.c, it seems we have to move the perf_mem_events_arm[] into arch/arm/util/mem-events.c as well. Is it OK? I'm not familiar with ARM and have no idea how those events are organized under arm64 and arm. Could you please send a fix for the building failure on aarch64? I will fold it into the V3. Thanks, Kan > > Thanks, > Leo >
On Mon, Dec 11, 2023 at 02:01:37PM -0500, Liang, Kan wrote: [...] > > I will hold on a bit for the test until this patch set addresses the > > concern for the breakage issues on Arm64. Please check my review in > > other replies. > > The reviews in the other replies don't look like break any current usage > on Arm64. I think the breakage issue is what you described in this > patch, right? I mentioned the breakage is in the patch 04, but I think the concern is dismissed. > If we move the check of "arm_spe_0" to arch/arm/util/pmu.c, it seems we > have to move the perf_mem_events_arm[] into arch/arm/util/mem-events.c > as well. Is it OK? No. For fixing Arm64 building, please refer: https://termbin.com/0dkn > I'm not familiar with ARM and have no idea how those events are > organized under arm64 and arm. Could you please send a fix for the > building failure on aarch64? I will fold it into the V3. After apply the change in above link on the top of your patch set, it can build successfully at my side. Hope it's helpful. Thanks, Leo > > Thanks, > Kan > > > > Thanks, > > Leo > >
On 2023-12-13 9:24 a.m., Leo Yan wrote: > On Mon, Dec 11, 2023 at 02:01:37PM -0500, Liang, Kan wrote: > > [...] > >>> I will hold on a bit for the test until this patch set addresses the >>> concern for the breakage issues on Arm64. Please check my review in >>> other replies. >> >> The reviews in the other replies don't look like break any current usage >> on Arm64. I think the breakage issue is what you described in this >> patch, right? > > I mentioned the breakage is in the patch 04, but I think the concern > is dismissed. > >> If we move the check of "arm_spe_0" to arch/arm/util/pmu.c, it seems we >> have to move the perf_mem_events_arm[] into arch/arm/util/mem-events.c >> as well. Is it OK? > > No. For fixing Arm64 building, please refer: > > https://termbin.com/0dkn That's great! Thanks a lot! > >> I'm not familiar with ARM and have no idea how those events are >> organized under arm64 and arm. Could you please send a fix for the >> building failure on aarch64? I will fold it into the V3. > > After apply the change in above link on the top of your patch set, > it can build successfully at my side. Hope it's helpful. > Yes, that's help a lot. I will send the V3 shortly. Thanks, Kan
diff --git a/tools/perf/arch/arm64/util/mem-events.c b/tools/perf/arch/arm64/util/mem-events.c index 3bcc5c7035c2..aaa4804922b4 100644 --- a/tools/perf/arch/arm64/util/mem-events.c +++ b/tools/perf/arch/arm64/util/mem-events.c @@ -4,7 +4,7 @@ #define E(t, n, s) { .tag = t, .name = n, .sysfs_name = s } -static struct perf_mem_event perf_mem_events[PERF_MEM_EVENTS__MAX] = { +struct perf_mem_event perf_mem_events_arm[PERF_MEM_EVENTS__MAX] = { E("spe-load", "arm_spe_0/ts_enable=1,pa_enable=1,load_filter=1,store_filter=0,min_latency=%u/", "arm_spe_0"), E("spe-store", "arm_spe_0/ts_enable=1,pa_enable=1,load_filter=0,store_filter=1/", "arm_spe_0"), E("spe-ldst", "arm_spe_0/ts_enable=1,pa_enable=1,load_filter=1,store_filter=1,min_latency=%u/", "arm_spe_0"), @@ -17,7 +17,7 @@ struct perf_mem_event *perf_mem_events__ptr(int i) if (i >= PERF_MEM_EVENTS__MAX) return NULL; - return &perf_mem_events[i]; + return &perf_mem_events_arm[i]; } const char *perf_mem_events__name(int i, const char *pmu_name __maybe_unused) diff --git a/tools/perf/arch/arm64/util/mem-events.h b/tools/perf/arch/arm64/util/mem-events.h new file mode 100644 index 000000000000..5fc50be4be38 --- /dev/null +++ b/tools/perf/arch/arm64/util/mem-events.h @@ -0,0 +1,7 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _ARM64_MEM_EVENTS_H +#define _ARM64_MEM_EVENTS_H + +extern struct perf_mem_event perf_mem_events_arm[PERF_MEM_EVENTS__MAX]; + +#endif /* _ARM64_MEM_EVENTS_H */ diff --git a/tools/perf/arch/arm64/util/pmu.c b/tools/perf/arch/arm64/util/pmu.c index 2a4eab2d160e..06ec9b838807 100644 --- a/tools/perf/arch/arm64/util/pmu.c +++ b/tools/perf/arch/arm64/util/pmu.c @@ -8,6 +8,12 @@ #include <api/fs/fs.h> #include <math.h> +void perf_pmu__arch_init(struct perf_pmu *pmu) +{ + if (!strcmp(pmu->name, "arm_spe_0")) + pmu->mem_events = perf_mem_events_arm; +} + const struct pmu_metrics_table *pmu_metrics_table__find(void) { struct perf_pmu *pmu; diff --git a/tools/perf/arch/s390/util/pmu.c b/tools/perf/arch/s390/util/pmu.c index 886c30e001fa..225d7dc2379c 100644 --- a/tools/perf/arch/s390/util/pmu.c +++ b/tools/perf/arch/s390/util/pmu.c @@ -19,4 +19,7 @@ void perf_pmu__arch_init(struct perf_pmu *pmu) !strcmp(pmu->name, S390_PMUPAI_EXT) || !strcmp(pmu->name, S390_PMUCPUM_CF)) pmu->selectable = true; + + if (pmu->is_core) + pmu->mem_events = perf_mem_events; } diff --git a/tools/perf/arch/x86/util/mem-events.c b/tools/perf/arch/x86/util/mem-events.c index 191b372f9a2d..2b81d229982c 100644 --- a/tools/perf/arch/x86/util/mem-events.c +++ b/tools/perf/arch/x86/util/mem-events.c @@ -16,13 +16,13 @@ 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] = { +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(NULL, NULL, NULL), }; -static struct perf_mem_event perf_mem_events_amd[PERF_MEM_EVENTS__MAX] = { +struct perf_mem_event perf_mem_events_amd[PERF_MEM_EVENTS__MAX] = { E(NULL, NULL, NULL), E(NULL, NULL, NULL), E("mem-ldst", "ibs_op//", "ibs_op"), diff --git a/tools/perf/arch/x86/util/mem-events.h b/tools/perf/arch/x86/util/mem-events.h new file mode 100644 index 000000000000..3959e427f482 --- /dev/null +++ b/tools/perf/arch/x86/util/mem-events.h @@ -0,0 +1,9 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _X86_MEM_EVENTS_H +#define _X86_MEM_EVENTS_H + +extern struct perf_mem_event perf_mem_events_intel[PERF_MEM_EVENTS__MAX]; + +extern struct perf_mem_event perf_mem_events_amd[PERF_MEM_EVENTS__MAX]; + +#endif /* _X86_MEM_EVENTS_H */ diff --git a/tools/perf/arch/x86/util/pmu.c b/tools/perf/arch/x86/util/pmu.c index 469555ae9b3c..cd22e80e5657 100644 --- a/tools/perf/arch/x86/util/pmu.c +++ b/tools/perf/arch/x86/util/pmu.c @@ -15,6 +15,7 @@ #include "../../../util/pmu.h" #include "../../../util/fncache.h" #include "../../../util/pmus.h" +#include "mem-events.h" #include "env.h" void perf_pmu__arch_init(struct perf_pmu *pmu __maybe_unused) @@ -30,6 +31,12 @@ void perf_pmu__arch_init(struct perf_pmu *pmu __maybe_unused) pmu->selectable = true; } #endif + + if (x86__is_amd_cpu()) { + if (!strcmp(pmu->name, "ibs_op")) + pmu->mem_events = perf_mem_events_amd; + } else if (pmu->is_core) + pmu->mem_events = perf_mem_events_intel; } int perf_pmus__num_mem_pmus(void) diff --git a/tools/perf/util/mem-events.c b/tools/perf/util/mem-events.c index 3a2e3687878c..0a8f415f5efe 100644 --- a/tools/perf/util/mem-events.c +++ b/tools/perf/util/mem-events.c @@ -19,7 +19,7 @@ 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] = { +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(NULL, NULL, NULL), diff --git a/tools/perf/util/mem-events.h b/tools/perf/util/mem-events.h index b40ad6ea93fc..8c5694b2d0b0 100644 --- a/tools/perf/util/mem-events.h +++ b/tools/perf/util/mem-events.h @@ -34,6 +34,7 @@ enum { }; extern unsigned int perf_mem_events__loads_ldlat; +extern struct perf_mem_event perf_mem_events[PERF_MEM_EVENTS__MAX]; int perf_mem_events__parse(const char *str); int perf_mem_events__init(void); diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c index 3c9609944a2f..3d4373b8ab63 100644 --- a/tools/perf/util/pmu.c +++ b/tools/perf/util/pmu.c @@ -986,8 +986,10 @@ static int pmu_max_precise(int dirfd, struct perf_pmu *pmu) } void __weak -perf_pmu__arch_init(struct perf_pmu *pmu __maybe_unused) +perf_pmu__arch_init(struct perf_pmu *pmu) { + if (pmu->is_core) + pmu->mem_events = perf_mem_events; } struct perf_pmu *perf_pmu__lookup(struct list_head *pmus, int dirfd, const char *name) diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h index 424c3fee0949..e35d985206db 100644 --- a/tools/perf/util/pmu.h +++ b/tools/perf/util/pmu.h @@ -10,6 +10,8 @@ #include <stdio.h> #include "parse-events.h" #include "pmu-events/pmu-events.h" +#include "map_symbol.h" +#include "mem-events.h" struct evsel_config_term; struct perf_cpu_map; @@ -162,6 +164,11 @@ struct perf_pmu { */ bool exclude_guest; } missing_features; + + /** + * @mem_events: List of the supported mem events + */ + struct perf_mem_event *mem_events; }; /** @perf_pmu__fake: A special global PMU used for testing. */