Messages in this thread | | | Date | Tue, 26 Apr 2022 17:10:51 +0530 | Subject | Re: [PATCH 2/6] perf/amd/ibs: Advertise zen4_ibs_extensions as pmu capability attribute | From | Ravi Bangoria <> |
| |
On 26-Apr-22 3:27 PM, Robert Richter wrote: > On 25.04.22 10:13:19, Ravi Bangoria wrote: >> PMU driver can advertise certain feature via capability attribute('caps' >> sysfs directory) which can be consumed by userspace tools like perf. Add >> zen4_ibs_extensions capability attribute for IBS pmus. This attribute >> will be enabled when CPUID_Fn8000001B_EAX[11] is set. >> >> Without patch: >> >> $ ls /sys/bus/event_source/devices/ibs_op/caps >> ls: cannot access '/sys/.../ibs_op/caps': No such file or directory >> >> With patch: >> >> $ ls /sys/bus/event_source/devices/ibs_op/caps >> zen4_ibs_extensions >> >> Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com> >> --- >> arch/x86/events/amd/ibs.c | 34 ++++++++++++++++++++++++++++------ >> 1 file changed, 28 insertions(+), 6 deletions(-) >> >> diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c >> index a5303d62060c..54e12bd7843e 100644 >> --- a/arch/x86/events/amd/ibs.c >> +++ b/arch/x86/events/amd/ibs.c >> @@ -95,8 +95,10 @@ struct perf_ibs { >> struct cpu_perf_ibs __percpu *pcpu; >> >> struct attribute **format_attrs; >> + struct attribute **caps_attrs; >> struct attribute_group format_group; >> - const struct attribute_group *attr_groups[2]; >> + struct attribute_group caps_group; >> + const struct attribute_group *attr_groups[3]; > > Also add a macro for max groups.
Same as previous patch, except defining size of the array, it won't be used anywhere else.
> >> >> u64 (*get_count)(u64 config); >> }; >> @@ -522,10 +524,13 @@ 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"); >> +PMU_EVENT_ATTR_STRING(zen4_ibs_extensions, zen4_ibs_extensions, "1"); >> >> /* size = nr attrs plus NULL at the end */ >> static struct attribute *ibs_fetch_format_attrs[3]; >> static struct attribute *ibs_op_format_attrs[3]; >> +static struct attribute *ibs_fetch_caps_attrs[2]; >> +static struct attribute *ibs_op_caps_attrs[2]; >> >> static struct perf_ibs perf_ibs_fetch = { >> .pmu = { >> @@ -548,6 +553,7 @@ static struct perf_ibs perf_ibs_fetch = { >> .offset_mask = { MSR_AMD64_IBSFETCH_REG_MASK }, >> .offset_max = MSR_AMD64_IBSFETCH_REG_COUNT, >> .format_attrs = ibs_fetch_format_attrs, >> + .caps_attrs = ibs_fetch_caps_attrs, >> >> .get_count = get_ibs_fetch_count, >> }; >> @@ -574,6 +580,7 @@ static struct perf_ibs perf_ibs_op = { >> .offset_mask = { MSR_AMD64_IBSOP_REG_MASK }, >> .offset_max = MSR_AMD64_IBSOP_REG_COUNT, >> .format_attrs = ibs_op_format_attrs, >> + .caps_attrs = ibs_op_caps_attrs, >> >> .get_count = get_ibs_op_count, >> }; >> @@ -728,6 +735,7 @@ static __init int perf_ibs_pmu_init(struct perf_ibs *perf_ibs, char *name) >> { >> struct cpu_perf_ibs __percpu *pcpu; >> int ret; >> + int i = 0; > > 'group'? Use a pointer to iterate over groups here same as for attrs? > > struct attribute_group **group = &perf_ibs->attr_groups;
Sure.
> >> >> pcpu = alloc_percpu(struct cpu_perf_ibs); >> if (!pcpu) >> @@ -736,16 +744,26 @@ static __init int perf_ibs_pmu_init(struct perf_ibs *perf_ibs, char *name) >> perf_ibs->pcpu = pcpu; >> >> /* register attributes */ >> + memset(&perf_ibs->attr_groups, 0, sizeof(perf_ibs->attr_groups)); > > With a termination below this could be dropped.
I think we can just remove this line. perf_ibs points to either perf_ibs_fetch or perf_ibs_op, both of which are global variables and would be preinitialized with 0s.
> >> if (perf_ibs->format_attrs[0]) { >> memset(&perf_ibs->format_group, 0, sizeof(perf_ibs->format_group)); >> - perf_ibs->format_group.name = "format"; >> - perf_ibs->format_group.attrs = perf_ibs->format_attrs; >> + perf_ibs->format_group.name = "format"; >> + perf_ibs->format_group.attrs = perf_ibs->format_attrs; >> >> - memset(&perf_ibs->attr_groups, 0, sizeof(perf_ibs->attr_groups)); >> - perf_ibs->attr_groups[0] = &perf_ibs->format_group; >> - perf_ibs->pmu.attr_groups = perf_ibs->attr_groups; >> + perf_ibs->attr_groups[i++] = &perf_ibs->format_group; > > So this could be: > > *groups++ = &perf_ibs->format_group > >> } >> >> + if (perf_ibs->caps_attrs[0]) { >> + memset(&perf_ibs->caps_group, 0, sizeof(perf_ibs->caps_group)); >> + perf_ibs->caps_group.name = "caps"; >> + perf_ibs->caps_group.attrs = perf_ibs->caps_attrs; >> + >> + perf_ibs->attr_groups[i++] = &perf_ibs->caps_group; > > Similar here. > >> + } > > Add a terminating NULL pointer for groups here. > >> + >> + if (i) > > if (perf_ibs->attr_groups[0]) > ... > >> + perf_ibs->pmu.attr_groups = perf_ibs->attr_groups; >> + >> ret = perf_pmu_register(&perf_ibs->pmu, name, -1); >> if (ret) { >> perf_ibs->pcpu = NULL; >> @@ -758,6 +776,7 @@ static __init int perf_ibs_pmu_init(struct perf_ibs *perf_ibs, char *name) >> static __init void perf_ibs_fetch_prepare(void) >> { >> struct attribute **format_attrs = perf_ibs_fetch.format_attrs; >> + struct attribute **caps_attr = perf_ibs_fetch.caps_attrs; > > Now I see why it was renamed, ok.
Yup.
Thanks for the review, Ravi
| |