Messages in this thread | | | Date | Tue, 26 Apr 2022 16:55:26 +0530 | Subject | Re: [PATCH 1/6] perf/amd/ibs: Add support for L3 miss filtering | From | Ravi Bangoria <> |
| |
On 26-Apr-22 2:48 PM, Robert Richter wrote: > On 25.04.22 10:13:18, Ravi Bangoria wrote: >> IBS L3 miss filtering works by tagging an instruction on IBS counter >> overflow and generating an NMI if the tagged instruction causes an L3 >> miss. Samples without an L3 miss are discarded and counter is reset >> with random value (between 1-15 for fetch pmu and 1-127 for op pmu). >> This helps in reducing sampling overhead when user is interested only >> in such samples. One of the use case of such filtered samples is to >> feed data to page-migration daemon in tiered memory systems. >> >> Add support for L3 miss filtering in IBS driver via new pmu attribute >> "l3missonly". Example usage: >> >> # perf record -a -e ibs_op/l3missonly=1/ --raw-samples sleep 5 >> >> Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com> >> --- >> arch/x86/events/amd/ibs.c | 42 ++++++++++++++++++++++--------- >> arch/x86/include/asm/perf_event.h | 3 +++ >> 2 files changed, 33 insertions(+), 12 deletions(-) >> >> diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c >> index 9739019d4b67..a5303d62060c 100644 >> --- a/arch/x86/events/amd/ibs.c >> +++ b/arch/x86/events/amd/ibs.c >> @@ -520,16 +520,12 @@ static void perf_ibs_read(struct perf_event *event) { } >> >> PMU_FORMAT_ATTR(rand_en, "config:57"); >> PMU_FORMAT_ATTR(cnt_ctl, "config:19"); >> +PMU_EVENT_ATTR_STRING(l3missonly, fetch_l3missonly, "config:59"); >> +PMU_EVENT_ATTR_STRING(l3missonly, op_l3missonly, "config:16"); >> >> -static struct attribute *ibs_fetch_format_attrs[] = { >> - &format_attr_rand_en.attr, >> - NULL, >> -}; >> - >> -static struct attribute *ibs_op_format_attrs[] = { >> - NULL, /* &format_attr_cnt_ctl.attr if IBS_CAPS_OPCNT */ >> - NULL, >> -}; >> +/* size = nr attrs plus NULL at the end */ >> +static struct attribute *ibs_fetch_format_attrs[3]; >> +static struct attribute *ibs_op_format_attrs[3]; > > Define a macro for the array size.
Except defining size of the above arrays, there is no use of such macros. So I don't feel the need of it.
> >> >> static struct perf_ibs perf_ibs_fetch = { >> .pmu = { >> @@ -759,9 +755,9 @@ static __init int perf_ibs_pmu_init(struct perf_ibs *perf_ibs, char *name) >> return ret; >> } >> >> -static __init void perf_event_ibs_init(void) >> +static __init void perf_ibs_fetch_prepare(void) > > Since this actually initializes the pmu, let's call that > perf_ibs_fetch_init().
Sure
> > For low level init functions it would be good to keep track of the > return code even if it is later not evaluated. So these kind of > function should return an error code.
Sure
> >> { >> - struct attribute **attr = ibs_op_format_attrs; >> + struct attribute **format_attrs = perf_ibs_fetch.format_attrs; > > I think we could keep this short here with 'attr'. > >> >> /* >> * Some chips fail to reset the fetch count when it is written; instead >> @@ -773,11 +769,22 @@ static __init void perf_event_ibs_init(void) >> if (boot_cpu_data.x86 == 0x19 && boot_cpu_data.x86_model < 0x10) >> perf_ibs_fetch.fetch_ignore_if_zero_rip = 1; >> >> + *format_attrs++ = &format_attr_rand_en.attr; >> + if (ibs_caps & IBS_CAPS_ZEN4IBSEXTENSIONS) { >> + perf_ibs_fetch.config_mask |= IBS_FETCH_L3MISSONLY; >> + *format_attrs++ = &fetch_l3missonly.attr.attr; >> + } > > You should also write the terminating NULL pointer here, though the > mem is preinitialized zero.
That seems unnecessary
> >> + >> perf_ibs_pmu_init(&perf_ibs_fetch, "ibs_fetch"); >> +} >> + >> +static __init void perf_ibs_op_prepare(void) >> +{ >> + struct attribute **format_attrs = perf_ibs_op.format_attrs; >> >> if (ibs_caps & IBS_CAPS_OPCNT) { >> perf_ibs_op.config_mask |= IBS_OP_CNT_CTL; >> - *attr++ = &format_attr_cnt_ctl.attr; >> + *format_attrs++ = &format_attr_cnt_ctl.attr; >> } >> >> if (ibs_caps & IBS_CAPS_OPCNTEXT) { >> @@ -786,7 +793,18 @@ static __init void perf_event_ibs_init(void) >> perf_ibs_op.cnt_mask |= IBS_OP_MAX_CNT_EXT_MASK; >> } >> >> + if (ibs_caps & IBS_CAPS_ZEN4IBSEXTENSIONS) { >> + perf_ibs_op.config_mask |= IBS_OP_L3MISSONLY; >> + *format_attrs++ = &op_l3missonly.attr.attr; >> + } >> + >> perf_ibs_pmu_init(&perf_ibs_op, "ibs_op"); >> +} > > Same for this function: *_init(), error code, attrs, terminating NULL. > >> + >> +static __init void perf_event_ibs_init(void) >> +{ >> + perf_ibs_fetch_prepare(); >> + perf_ibs_op_prepare(); >> >> register_nmi_handler(NMI_LOCAL, perf_ibs_nmi_handler, 0, "perf_ibs"); >> pr_info("perf: AMD IBS detected (0x%08x)\n", ibs_caps); > > The function is now small enough to be squashed into amd_ibs_init().
It's small enough but it still make sense to keep this function, as there is an empty version of it when (CONFIG_PERF_EVENTS=n && CONFIG_CPU_SUP_AMD=n).
Thanks for the review, Ravi
| |