Re: [PATCH] powerpc/perf: Add support for caps under sysfs in powerpc

[Athira Rajeev <atrajeev@linux.vnet.ibm.com>]

> On 06-May-2022, at 6:55 PM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> 
> Hi Athira,
> 
> Some comments below :)
> 
> Athira Rajeev <atrajeev@linux.vnet.ibm.com> writes:
>> Add caps support under "/sys/bus/event_source/devices/<pmu>/"
>> for powerpc. This directory can be used to expose some of the
>> specific features that powerpc PMU supports to the user.
>> Example: pmu_name. The name of PMU registered will depend on
>> platform, say power9 or power10 or it could be Generic Compat
>> PMU.
> 
> Is there precedent for adding a "caps" directory? ie. do other PMUs on
> other architectures already do that?
Hi Michael,

Thanks for the review comments.

Yes, There are other PMU’s on other architectures already having “caps” support.
I will add them in changelog for reference in V2.
> 
> Is there precedent for adding "pmu_name"?
> 
> I don't see any mention of them in Documentation/ABI anywhere.
> 
> If we're the first to do that we should add it to the documentation.
> 
> As this would set a precedent for other PMUs, please Cc the perf
> maintainers on v2.
> 
>> Currently the only way to know which is the registered
>> PMU is from the dmesg logs. But clearing the dmesg will make it
>> difficult to know exact PMU backend used. And even extracting
>> from dmesg will be complicated, as we need  to parse the dmesg
>> logs and add filters for pmu name. Whereas by exposing it via
>> caps will make it easy as we just need to directly read it from
>> the sysfs.
>> 
>> Add a caps directory to /sys/bus/event_source/devices/cpu/
>> for power8, power9, power10 and generic compat PMU.
>> 
>> The information exposed currently:
>> - pmu_name : Underlying PMU name from the driver
>> 
>> Example result with power9 pmu:
>> 
>> # ls /sys/bus/event_source/devices/cpu/caps
>> pmu_name
>> 
>> # cat /sys/bus/event_source/devices/cpu/caps/pmu_name
>> power9
>> 
>> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
>> Reviewed-by: Madhavan Srinivasan <maddy@linux.ibm.com>
>> ---
>> arch/powerpc/perf/generic-compat-pmu.c | 20 ++++++++++++++++++++
>> arch/powerpc/perf/power10-pmu.c        | 20 ++++++++++++++++++++
>> arch/powerpc/perf/power8-pmu.c         | 20 ++++++++++++++++++++
>> arch/powerpc/perf/power9-pmu.c         | 20 ++++++++++++++++++++
>> 4 files changed, 80 insertions(+)
>> 
>> diff --git a/arch/powerpc/perf/generic-compat-pmu.c b/arch/powerpc/perf/generic-compat-pmu.c
>> index f3db88aee4dd..7b5fe2d89007 100644
>> --- a/arch/powerpc/perf/generic-compat-pmu.c
>> +++ b/arch/powerpc/perf/generic-compat-pmu.c
>> @@ -151,9 +151,29 @@ static const struct attribute_group generic_compat_pmu_format_group = {
>> 	.attrs = generic_compat_pmu_format_attr,
>> };
>> 
>> +static ssize_t pmu_name_show(struct device *cdev,
>> +		struct device_attribute *attr,
>> +		char *buf)
>> +{
>> +	return snprintf(buf, PAGE_SIZE, "generic_compat_pmu");
>> +}
> 
> That's not a great name, now that it's exposed to userspace.
> 
> For starters it's only generic on Book3S, and if you look at
> init_generic_compat_pmu() it's really a "ISA >= v3.0 fallback PMU" - or
> something like that.
> 
>> +static DEVICE_ATTR_RO(pmu_name);
>> +
>> +static struct attribute *generic_compat_pmu_caps_attrs[] = {
>> +	&dev_attr_pmu_name.attr,
>> +	NULL
>> +};
>> +
>> +static struct attribute_group generic_compat_pmu_caps_group = {
>> +	.name  = "caps",
>> +	.attrs = generic_compat_pmu_caps_attrs,
>> +};
>> +
>> static const struct attribute_group *generic_compat_pmu_attr_groups[] = {
>> 	&generic_compat_pmu_format_group,
>> 	&generic_compat_pmu_events_group,
>> +	&generic_compat_pmu_caps_group,
>> 	NULL,
>> };
>> 
>> diff --git a/arch/powerpc/perf/power10-pmu.c b/arch/powerpc/perf/power10-pmu.c
>> index d3398100a60f..a622ff783719 100644
>> --- a/arch/powerpc/perf/power10-pmu.c
>> +++ b/arch/powerpc/perf/power10-pmu.c
>> @@ -258,6 +258,25 @@ static const struct attribute_group power10_pmu_format_group = {
>> 	.attrs = power10_pmu_format_attr,
>> };
>> 
>> +static ssize_t pmu_name_show(struct device *cdev,
>> +		struct device_attribute *attr,
>> +		char *buf)
>> +{
>> +	return snprintf(buf, PAGE_SIZE, "power10");
> 
> I believe that should use sysfs_emit().

Sure will change this.
> 
>> +}
>> +
>> +static DEVICE_ATTR_RO(pmu_name);
>> +
>> +static struct attribute *power10_pmu_caps_attrs[] = {
>> +	&dev_attr_pmu_name.attr,
>> +	NULL
>> +};
>> +
>> +static struct attribute_group power10_pmu_caps_group = {
>> +	.name  = "caps",
>> +	.attrs = power10_pmu_caps_attrs,
>> +};
>> +
>> static const struct attribute_group *power10_pmu_attr_groups_dd1[] = {
>> 	&power10_pmu_format_group,
>> 	&power10_pmu_events_group_dd1,
>> @@ -267,6 +286,7 @@ static const struct attribute_group *power10_pmu_attr_groups_dd1[] = {
>> static const struct attribute_group *power10_pmu_attr_groups[] = {
>> 	&power10_pmu_format_group,
>> 	&power10_pmu_events_group,
>> +	&power10_pmu_caps_group,
>> 	NULL,
>> };
> 
> There's a lot of boiler plate repeated for each PMU.
> 
> We already have power_pmu->name, can we use that and make the show
> function generic at least in core-book3s.c ?

Sure, I will rework on V2 to make the show function generic in core-book3s.
For the Generic Compat PMU, now that I am going to use show function from core-book3s, the
name exposed will be that of power_pmu->name

Thanks
Athira 
> 
> cheers