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

[Michael Ellerman <mpe@ellerman.id.au>]

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?

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().

> +}
> +
> +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 ?

cheers