Hi Athira,
Some comments below :)
Athira Rajeev <atraj...@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 <atraj...@linux.vnet.ibm.com>
> Reviewed-by: Madhavan Srinivasan <ma...@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
