[PATCH 0/4] arm_pmu: Add PERF_PMU_CAP_EXTENDED_HW_TYPE capability

[PATCH 0/4] arm_pmu: Add PERF_PMU_CAP_EXTENDED_HW_TYPE capability

[James Clark]

This came out of the discussion here [1]. It seems like we can get some
extra big.LITTLE stuff working pretty easily. The test issues mentioned
in the linked thread are actually fairly unrelated and I've fixed them
in a different set on the list.

After adding it in the first commit, the remaining ones tidy up a
related capability that doesn't do anything any more.

I've added a fixes tag for the commit where
PERF_PMU_CAP_EXTENDED_HW_TYPE was originally added because it probably
should have been added to the Arm PMU at the same time. It doesn't apply
cleanly that far back because another capability was added between then,
but the resolution is trivial.

Thanks
James

[1]: https://lore.kernel.org/linux-perf-users/CAP-5=fVkRc9=ySJ=fG-SQ8oAKmE_1mhHHzSASmGHUsda5Qy92A@mail.gmail.com/T/#t

James Clark (4):
  arm_pmu: Add PERF_PMU_CAP_EXTENDED_HW_TYPE capability
  perf/x86: Remove unused PERF_PMU_CAP_HETEROGENEOUS_CPUS capability
  arm_pmu: Remove unused PERF_PMU_CAP_HETEROGENEOUS_CPUS capability
  perf: Remove unused PERF_PMU_CAP_HETEROGENEOUS_CPUS capability

 arch/x86/events/core.c     |  1 -
 drivers/perf/arm_pmu.c     | 10 ++++++----
 include/linux/perf_event.h |  2 +-
 3 files changed, 7 insertions(+), 6 deletions(-)

-- 
2.34.1

[PATCH 1/4] arm_pmu: Add PERF_PMU_CAP_EXTENDED_HW_TYPE capability

[James Clark]

This capability gives us the ability to open PERF_TYPE_HARDWARE and
PERF_TYPE_HW_CACHE events on a specific PMU for free. All the
implementation is contained in the Perf core and tool code so no change
to the Arm PMU driver is needed.

The following basic use case now results in Perf opening the event on
all PMUs rather than picking only one in an unpredictable way:

  $ perf stat -e cycles -- taskset --cpu-list 0,1 stress -c 2

   Performance counter stats for 'taskset --cpu-list 0,1 stress -c 2':

         963279620      armv8_cortex_a57/cycles/                (99.19%)
         752745657      armv8_cortex_a53/cycles/                (94.80%)

Fixes: 55bcf6ef314a ("perf: Extend PERF_TYPE_HARDWARE and PERF_TYPE_HW_CACHE")
Suggested-by: Ian Rogers <irogers@google.com>
Signed-off-by: James Clark <james.clark@arm.com>
---
 drivers/perf/arm_pmu.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
index 277e29fbd504..d8844a9461a2 100644
--- a/drivers/perf/arm_pmu.c
+++ b/drivers/perf/arm_pmu.c
@@ -875,8 +875,13 @@ struct arm_pmu *armpmu_alloc(void)
 		 * configuration (e.g. big.LITTLE). This is not an uncore PMU,
 		 * and we have taken ctx sharing into account (e.g. with our
 		 * pmu::filter callback and pmu::event_init group validation).
+		 *
+		 * PERF_PMU_CAP_EXTENDED_HW_TYPE is required to open the legacy
+		 * PERF_TYPE_HARDWARE and PERF_TYPE_HW_CACHE events on a
+		 * specific PMU.
 		 */
-		.capabilities	= PERF_PMU_CAP_HETEROGENEOUS_CPUS | PERF_PMU_CAP_EXTENDED_REGS,
+		.capabilities	= PERF_PMU_CAP_HETEROGENEOUS_CPUS | PERF_PMU_CAP_EXTENDED_REGS |
+				  PERF_PMU_CAP_EXTENDED_HW_TYPE,
 	};
 
 	pmu->attr_groups[ARMPMU_ATTR_GROUP_COMMON] =
-- 
2.34.1

Re: [PATCH 1/4] arm_pmu: Add PERF_PMU_CAP_EXTENDED_HW_TYPE capability

[Ian Rogers]

On Mon, Jul 10, 2023 at 5:22 AM James Clark <james.clark@arm.com> wrote:
>
> This capability gives us the ability to open PERF_TYPE_HARDWARE and
> PERF_TYPE_HW_CACHE events on a specific PMU for free. All the
> implementation is contained in the Perf core and tool code so no change
> to the Arm PMU driver is needed.
>
> The following basic use case now results in Perf opening the event on
> all PMUs rather than picking only one in an unpredictable way:
>
>   $ perf stat -e cycles -- taskset --cpu-list 0,1 stress -c 2
>
>    Performance counter stats for 'taskset --cpu-list 0,1 stress -c 2':
>
>          963279620      armv8_cortex_a57/cycles/                (99.19%)
>          752745657      armv8_cortex_a53/cycles/                (94.80%)
>
> Fixes: 55bcf6ef314a ("perf: Extend PERF_TYPE_HARDWARE and PERF_TYPE_HW_CACHE")
> Suggested-by: Ian Rogers <irogers@google.com>
> Signed-off-by: James Clark <james.clark@arm.com>

Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  drivers/perf/arm_pmu.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
> index 277e29fbd504..d8844a9461a2 100644
> --- a/drivers/perf/arm_pmu.c
> +++ b/drivers/perf/arm_pmu.c
> @@ -875,8 +875,13 @@ struct arm_pmu *armpmu_alloc(void)
>                  * configuration (e.g. big.LITTLE). This is not an uncore PMU,
>                  * and we have taken ctx sharing into account (e.g. with our
>                  * pmu::filter callback and pmu::event_init group validation).
> +                *
> +                * PERF_PMU_CAP_EXTENDED_HW_TYPE is required to open the legacy
> +                * PERF_TYPE_HARDWARE and PERF_TYPE_HW_CACHE events on a
> +                * specific PMU.
>                  */
> -               .capabilities   = PERF_PMU_CAP_HETEROGENEOUS_CPUS | PERF_PMU_CAP_EXTENDED_REGS,
> +               .capabilities   = PERF_PMU_CAP_HETEROGENEOUS_CPUS | PERF_PMU_CAP_EXTENDED_REGS |
> +                                 PERF_PMU_CAP_EXTENDED_HW_TYPE,
>         };
>
>         pmu->attr_groups[ARMPMU_ATTR_GROUP_COMMON] =
> --
> 2.34.1
>

Re: [PATCH 1/4] arm_pmu: Add PERF_PMU_CAP_EXTENDED_HW_TYPE capability

[Anshuman Khandual]

On 7/10/23 17:51, James Clark wrote:
> This capability gives us the ability to open PERF_TYPE_HARDWARE and
> PERF_TYPE_HW_CACHE events on a specific PMU for free. All the
> implementation is contained in the Perf core and tool code so no change
> to the Arm PMU driver is needed.
> 
> The following basic use case now results in Perf opening the event on
> all PMUs rather than picking only one in an unpredictable way:
> 
>   $ perf stat -e cycles -- taskset --cpu-list 0,1 stress -c 2
> 
>    Performance counter stats for 'taskset --cpu-list 0,1 stress -c 2':
> 
>          963279620      armv8_cortex_a57/cycles/                (99.19%)
>          752745657      armv8_cortex_a53/cycles/                (94.80%)
> 
> Fixes: 55bcf6ef314a ("perf: Extend PERF_TYPE_HARDWARE and PERF_TYPE_HW_CACHE")
> Suggested-by: Ian Rogers <irogers@google.com>
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
>  drivers/perf/arm_pmu.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
> index 277e29fbd504..d8844a9461a2 100644
> --- a/drivers/perf/arm_pmu.c
> +++ b/drivers/perf/arm_pmu.c
> @@ -875,8 +875,13 @@ struct arm_pmu *armpmu_alloc(void)
>  		 * configuration (e.g. big.LITTLE). This is not an uncore PMU,
>  		 * and we have taken ctx sharing into account (e.g. with our
>  		 * pmu::filter callback and pmu::event_init group validation).
> +		 *
> +		 * PERF_PMU_CAP_EXTENDED_HW_TYPE is required to open the legacy

s/legacy/generic ? These hardware events are still around.

> +		 * PERF_TYPE_HARDWARE and PERF_TYPE_HW_CACHE events on a
> +		 * specific PMU.
>  		 */
> -		.capabilities	= PERF_PMU_CAP_HETEROGENEOUS_CPUS | PERF_PMU_CAP_EXTENDED_REGS,
> +		.capabilities	= PERF_PMU_CAP_HETEROGENEOUS_CPUS | PERF_PMU_CAP_EXTENDED_REGS |
> +				  PERF_PMU_CAP_EXTENDED_HW_TYPE,
>  	};
>  
>  	pmu->attr_groups[ARMPMU_ATTR_GROUP_COMMON] =

Re: [PATCH 1/4] arm_pmu: Add PERF_PMU_CAP_EXTENDED_HW_TYPE capability

[James Clark]

On 11/07/2023 13:01, Anshuman Khandual wrote:
> 
> 
> On 7/10/23 17:51, James Clark wrote:
>> This capability gives us the ability to open PERF_TYPE_HARDWARE and
>> PERF_TYPE_HW_CACHE events on a specific PMU for free. All the
>> implementation is contained in the Perf core and tool code so no change
>> to the Arm PMU driver is needed.
>>
>> The following basic use case now results in Perf opening the event on
>> all PMUs rather than picking only one in an unpredictable way:
>>
>>   $ perf stat -e cycles -- taskset --cpu-list 0,1 stress -c 2
>>
>>    Performance counter stats for 'taskset --cpu-list 0,1 stress -c 2':
>>
>>          963279620      armv8_cortex_a57/cycles/                (99.19%)
>>          752745657      armv8_cortex_a53/cycles/                (94.80%)
>>
>> Fixes: 55bcf6ef314a ("perf: Extend PERF_TYPE_HARDWARE and PERF_TYPE_HW_CACHE")
>> Suggested-by: Ian Rogers <irogers@google.com>
>> Signed-off-by: James Clark <james.clark@arm.com>
>> ---
>>  drivers/perf/arm_pmu.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
>> index 277e29fbd504..d8844a9461a2 100644
>> --- a/drivers/perf/arm_pmu.c
>> +++ b/drivers/perf/arm_pmu.c
>> @@ -875,8 +875,13 @@ struct arm_pmu *armpmu_alloc(void)
>>  		 * configuration (e.g. big.LITTLE). This is not an uncore PMU,
>>  		 * and we have taken ctx sharing into account (e.g. with our
>>  		 * pmu::filter callback and pmu::event_init group validation).
>> +		 *
>> +		 * PERF_PMU_CAP_EXTENDED_HW_TYPE is required to open the legacy
> 
> s/legacy/generic ? These hardware events are still around.

True, I thought I saw it mentioned that way somewhere, but I can
probably just remove it altogether. PERF_TYPE_HARDWARE and
PERF_TYPE_HW_CACHE is enough.


> 
>> +		 * PERF_TYPE_HARDWARE and PERF_TYPE_HW_CACHE events on a
>> +		 * specific PMU.
>>  		 */
>> -		.capabilities	= PERF_PMU_CAP_HETEROGENEOUS_CPUS | PERF_PMU_CAP_EXTENDED_REGS,
>> +		.capabilities	= PERF_PMU_CAP_HETEROGENEOUS_CPUS | PERF_PMU_CAP_EXTENDED_REGS |
>> +				  PERF_PMU_CAP_EXTENDED_HW_TYPE,
>>  	};
>>  
>>  	pmu->attr_groups[ARMPMU_ATTR_GROUP_COMMON] =

Re: [PATCH 1/4] arm_pmu: Add PERF_PMU_CAP_EXTENDED_HW_TYPE capability

[Ian Rogers]

On Tue, Jul 11, 2023 at 7:12 AM James Clark <james.clark@arm.com> wrote:
>
>
>
> On 11/07/2023 13:01, Anshuman Khandual wrote:
> >
> >
> > On 7/10/23 17:51, James Clark wrote:
> >> This capability gives us the ability to open PERF_TYPE_HARDWARE and
> >> PERF_TYPE_HW_CACHE events on a specific PMU for free. All the
> >> implementation is contained in the Perf core and tool code so no change
> >> to the Arm PMU driver is needed.
> >>
> >> The following basic use case now results in Perf opening the event on
> >> all PMUs rather than picking only one in an unpredictable way:
> >>
> >>   $ perf stat -e cycles -- taskset --cpu-list 0,1 stress -c 2
> >>
> >>    Performance counter stats for 'taskset --cpu-list 0,1 stress -c 2':
> >>
> >>          963279620      armv8_cortex_a57/cycles/                (99.19%)
> >>          752745657      armv8_cortex_a53/cycles/                (94.80%)
> >>
> >> Fixes: 55bcf6ef314a ("perf: Extend PERF_TYPE_HARDWARE and PERF_TYPE_HW_CACHE")
> >> Suggested-by: Ian Rogers <irogers@google.com>
> >> Signed-off-by: James Clark <james.clark@arm.com>

Hi ARM Linux and ARM Linux PMU people,

Could this patch be picked up for Linux 6.5? I don't see it in the
tree and it seems a shame to have to wait for it. The other patches do
cleanup and so waiting for 6.6 seems okay.

Thanks,
Ian

> >> ---
> >>  drivers/perf/arm_pmu.c | 7 ++++++-
> >>  1 file changed, 6 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
> >> index 277e29fbd504..d8844a9461a2 100644
> >> --- a/drivers/perf/arm_pmu.c
> >> +++ b/drivers/perf/arm_pmu.c
> >> @@ -875,8 +875,13 @@ struct arm_pmu *armpmu_alloc(void)
> >>               * configuration (e.g. big.LITTLE). This is not an uncore PMU,
> >>               * and we have taken ctx sharing into account (e.g. with our
> >>               * pmu::filter callback and pmu::event_init group validation).
> >> +             *
> >> +             * PERF_PMU_CAP_EXTENDED_HW_TYPE is required to open the legacy
> >
> > s/legacy/generic ? These hardware events are still around.
>
> True, I thought I saw it mentioned that way somewhere, but I can
> probably just remove it altogether. PERF_TYPE_HARDWARE and
> PERF_TYPE_HW_CACHE is enough.
>
>
> >
> >> +             * PERF_TYPE_HARDWARE and PERF_TYPE_HW_CACHE events on a
> >> +             * specific PMU.
> >>               */
> >> -            .capabilities   = PERF_PMU_CAP_HETEROGENEOUS_CPUS | PERF_PMU_CAP_EXTENDED_REGS,
> >> +            .capabilities   = PERF_PMU_CAP_HETEROGENEOUS_CPUS | PERF_PMU_CAP_EXTENDED_REGS |
> >> +                              PERF_PMU_CAP_EXTENDED_HW_TYPE,
> >>      };
> >>
> >>      pmu->attr_groups[ARMPMU_ATTR_GROUP_COMMON] =

Re: [PATCH 1/4] arm_pmu: Add PERF_PMU_CAP_EXTENDED_HW_TYPE capability

[Will Deacon]

On Thu, Jul 20, 2023 at 10:12:21AM -0700, Ian Rogers wrote:
> On Tue, Jul 11, 2023 at 7:12 AM James Clark <james.clark@arm.com> wrote:
> >
> >
> >
> > On 11/07/2023 13:01, Anshuman Khandual wrote:
> > >
> > >
> > > On 7/10/23 17:51, James Clark wrote:
> > >> This capability gives us the ability to open PERF_TYPE_HARDWARE and
> > >> PERF_TYPE_HW_CACHE events on a specific PMU for free. All the
> > >> implementation is contained in the Perf core and tool code so no change
> > >> to the Arm PMU driver is needed.
> > >>
> > >> The following basic use case now results in Perf opening the event on
> > >> all PMUs rather than picking only one in an unpredictable way:
> > >>
> > >>   $ perf stat -e cycles -- taskset --cpu-list 0,1 stress -c 2
> > >>
> > >>    Performance counter stats for 'taskset --cpu-list 0,1 stress -c 2':
> > >>
> > >>          963279620      armv8_cortex_a57/cycles/                (99.19%)
> > >>          752745657      armv8_cortex_a53/cycles/                (94.80%)
> > >>
> > >> Fixes: 55bcf6ef314a ("perf: Extend PERF_TYPE_HARDWARE and PERF_TYPE_HW_CACHE")
> > >> Suggested-by: Ian Rogers <irogers@google.com>
> > >> Signed-off-by: James Clark <james.clark@arm.com>
> 
> Hi ARM Linux and ARM Linux PMU people,
> 
> Could this patch be picked up for Linux 6.5? I don't see it in the
> tree and it seems a shame to have to wait for it. The other patches do
> cleanup and so waiting for 6.6 seems okay.

I'm only taking fixes for 6.5 and I don't think this qualifies.

If it was an oversight introduced during the recent merge window, then
I'd be happier fixing it up, but 55bcf6ef314a was merged ages ago (v5.12?),
so I think we can wait.

I'll be queuing perf changes for 6.6 next week, so I'll look at this
then.

Cheers,

Will

Re: [PATCH 1/4] arm_pmu: Add PERF_PMU_CAP_EXTENDED_HW_TYPE capability

[James Clark]

On 21/07/2023 11:21, Will Deacon wrote:
> On Thu, Jul 20, 2023 at 10:12:21AM -0700, Ian Rogers wrote:
>> On Tue, Jul 11, 2023 at 7:12 AM James Clark <james.clark@arm.com> wrote:
>>>
>>>
>>>
>>> On 11/07/2023 13:01, Anshuman Khandual wrote:
>>>>
>>>>
>>>> On 7/10/23 17:51, James Clark wrote:
>>>>> This capability gives us the ability to open PERF_TYPE_HARDWARE and
>>>>> PERF_TYPE_HW_CACHE events on a specific PMU for free. All the
>>>>> implementation is contained in the Perf core and tool code so no change
>>>>> to the Arm PMU driver is needed.
>>>>>
>>>>> The following basic use case now results in Perf opening the event on
>>>>> all PMUs rather than picking only one in an unpredictable way:
>>>>>
>>>>>   $ perf stat -e cycles -- taskset --cpu-list 0,1 stress -c 2
>>>>>
>>>>>    Performance counter stats for 'taskset --cpu-list 0,1 stress -c 2':
>>>>>
>>>>>          963279620      armv8_cortex_a57/cycles/                (99.19%)
>>>>>          752745657      armv8_cortex_a53/cycles/                (94.80%)
>>>>>
>>>>> Fixes: 55bcf6ef314a ("perf: Extend PERF_TYPE_HARDWARE and PERF_TYPE_HW_CACHE")
>>>>> Suggested-by: Ian Rogers <irogers@google.com>
>>>>> Signed-off-by: James Clark <james.clark@arm.com>
>>
>> Hi ARM Linux and ARM Linux PMU people,
>>
>> Could this patch be picked up for Linux 6.5? I don't see it in the
>> tree and it seems a shame to have to wait for it. The other patches do
>> cleanup and so waiting for 6.6 seems okay.
> 
> I'm only taking fixes for 6.5 and I don't think this qualifies.
> 
> If it was an oversight introduced during the recent merge window, then
> I'd be happier fixing it up, but 55bcf6ef314a was merged ages ago (v5.12?),
> so I think we can wait.
> 
> I'll be queuing perf changes for 6.6 next week, so I'll look at this
> then.
> 
> Cheers,
> 
> Will

Hi Will,

Thanks for looking at this. I've sent a v2 with Anshuman's fixes.

James

[PATCH 2/4] perf/x86: Remove unused PERF_PMU_CAP_HETEROGENEOUS_CPUS capability

[James Clark]

Since commit bd2756811766 ("perf: Rewrite core context handling") the
relationship between perf_event_context and PMUs has changed so that
the error scenario that PERF_PMU_CAP_HETEROGENEOUS_CPUS originally
silenced no longer exists.

Remove the capability to avoid confusion that it actually influences
any perf core behavior. This change should be a no-op.

Signed-off-by: James Clark <james.clark@arm.com>
---
 arch/x86/events/core.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index 9d248703cbdd..2353aaf0b248 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -2168,7 +2168,6 @@ static int __init init_hw_perf_events(void)
 			hybrid_pmu->pmu = pmu;
 			hybrid_pmu->pmu.type = -1;
 			hybrid_pmu->pmu.attr_update = x86_pmu.attr_update;
-			hybrid_pmu->pmu.capabilities |= PERF_PMU_CAP_HETEROGENEOUS_CPUS;
 			hybrid_pmu->pmu.capabilities |= PERF_PMU_CAP_EXTENDED_HW_TYPE;
 
 			err = perf_pmu_register(&hybrid_pmu->pmu, hybrid_pmu->name,
-- 
2.34.1

Re: [PATCH 2/4] perf/x86: Remove unused PERF_PMU_CAP_HETEROGENEOUS_CPUS capability

[Ian Rogers]

On Mon, Jul 10, 2023 at 5:22 AM James Clark <james.clark@arm.com> wrote:
>
> Since commit bd2756811766 ("perf: Rewrite core context handling") the
> relationship between perf_event_context and PMUs has changed so that
> the error scenario that PERF_PMU_CAP_HETEROGENEOUS_CPUS originally
> silenced no longer exists.
>
> Remove the capability to avoid confusion that it actually influences
> any perf core behavior. This change should be a no-op.
>
> Signed-off-by: James Clark <james.clark@arm.com>

Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  arch/x86/events/core.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> index 9d248703cbdd..2353aaf0b248 100644
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -2168,7 +2168,6 @@ static int __init init_hw_perf_events(void)
>                         hybrid_pmu->pmu = pmu;
>                         hybrid_pmu->pmu.type = -1;
>                         hybrid_pmu->pmu.attr_update = x86_pmu.attr_update;
> -                       hybrid_pmu->pmu.capabilities |= PERF_PMU_CAP_HETEROGENEOUS_CPUS;
>                         hybrid_pmu->pmu.capabilities |= PERF_PMU_CAP_EXTENDED_HW_TYPE;
>
>                         err = perf_pmu_register(&hybrid_pmu->pmu, hybrid_pmu->name,
> --
> 2.34.1
>

[PATCH 3/4] arm_pmu: Remove unused PERF_PMU_CAP_HETEROGENEOUS_CPUS capability

[James Clark]

Since commit bd2756811766 ("perf: Rewrite core context handling") the
relationship between perf_event_context and PMUs has changed so that
the error scenario that PERF_PMU_CAP_HETEROGENEOUS_CPUS originally
silenced no longer exists.

Remove the capability and associated comment to avoid confusion that it
actually influences any perf core behavior. This change should be a
no-op.

Signed-off-by: James Clark <james.clark@arm.com>
---
 drivers/perf/arm_pmu.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
index d8844a9461a2..297906df6628 100644
--- a/drivers/perf/arm_pmu.c
+++ b/drivers/perf/arm_pmu.c
@@ -872,15 +872,12 @@ struct arm_pmu *armpmu_alloc(void)
 		.attr_groups	= pmu->attr_groups,
 		/*
 		 * This is a CPU PMU potentially in a heterogeneous
-		 * configuration (e.g. big.LITTLE). This is not an uncore PMU,
-		 * and we have taken ctx sharing into account (e.g. with our
-		 * pmu::filter callback and pmu::event_init group validation).
-		 *
+		 * configuration (e.g. big.LITTLE) so
 		 * PERF_PMU_CAP_EXTENDED_HW_TYPE is required to open the legacy
 		 * PERF_TYPE_HARDWARE and PERF_TYPE_HW_CACHE events on a
 		 * specific PMU.
 		 */
-		.capabilities	= PERF_PMU_CAP_HETEROGENEOUS_CPUS | PERF_PMU_CAP_EXTENDED_REGS |
+		.capabilities	= PERF_PMU_CAP_EXTENDED_REGS |
 				  PERF_PMU_CAP_EXTENDED_HW_TYPE,
 	};
 
-- 
2.34.1

Re: [PATCH 3/4] arm_pmu: Remove unused PERF_PMU_CAP_HETEROGENEOUS_CPUS capability

[Ian Rogers]

On Mon, Jul 10, 2023 at 5:22 AM James Clark <james.clark@arm.com> wrote:
>
> Since commit bd2756811766 ("perf: Rewrite core context handling") the
> relationship between perf_event_context and PMUs has changed so that
> the error scenario that PERF_PMU_CAP_HETEROGENEOUS_CPUS originally
> silenced no longer exists.
>
> Remove the capability and associated comment to avoid confusion that it
> actually influences any perf core behavior. This change should be a
> no-op.
>
> Signed-off-by: James Clark <james.clark@arm.com>

Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  drivers/perf/arm_pmu.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
> index d8844a9461a2..297906df6628 100644
> --- a/drivers/perf/arm_pmu.c
> +++ b/drivers/perf/arm_pmu.c
> @@ -872,15 +872,12 @@ struct arm_pmu *armpmu_alloc(void)
>                 .attr_groups    = pmu->attr_groups,
>                 /*
>                  * This is a CPU PMU potentially in a heterogeneous
> -                * configuration (e.g. big.LITTLE). This is not an uncore PMU,
> -                * and we have taken ctx sharing into account (e.g. with our
> -                * pmu::filter callback and pmu::event_init group validation).
> -                *
> +                * configuration (e.g. big.LITTLE) so
>                  * PERF_PMU_CAP_EXTENDED_HW_TYPE is required to open the legacy
>                  * PERF_TYPE_HARDWARE and PERF_TYPE_HW_CACHE events on a
>                  * specific PMU.
>                  */
> -               .capabilities   = PERF_PMU_CAP_HETEROGENEOUS_CPUS | PERF_PMU_CAP_EXTENDED_REGS |
> +               .capabilities   = PERF_PMU_CAP_EXTENDED_REGS |
>                                   PERF_PMU_CAP_EXTENDED_HW_TYPE,
>         };
>
> --
> 2.34.1
>

Re: [PATCH 3/4] arm_pmu: Remove unused PERF_PMU_CAP_HETEROGENEOUS_CPUS capability

[Anshuman Khandual]

On 7/10/23 17:51, James Clark wrote:
> Since commit bd2756811766 ("perf: Rewrite core context handling") the
> relationship between perf_event_context and PMUs has changed so that
> the error scenario that PERF_PMU_CAP_HETEROGENEOUS_CPUS originally
> silenced no longer exists.
> 
> Remove the capability and associated comment to avoid confusion that it
> actually influences any perf core behavior. This change should be a
> no-op.
> 
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
>  drivers/perf/arm_pmu.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
> index d8844a9461a2..297906df6628 100644
> --- a/drivers/perf/arm_pmu.c
> +++ b/drivers/perf/arm_pmu.c
> @@ -872,15 +872,12 @@ struct arm_pmu *armpmu_alloc(void)
>  		.attr_groups	= pmu->attr_groups,
>  		/*
>  		 * This is a CPU PMU potentially in a heterogeneous
> -		 * configuration (e.g. big.LITTLE). This is not an uncore PMU,
> -		 * and we have taken ctx sharing into account (e.g. with our
> -		 * pmu::filter callback and pmu::event_init group validation).
> -		 *
> +		 * configuration (e.g. big.LITTLE) so
>  		 * PERF_PMU_CAP_EXTENDED_HW_TYPE is required to open the legacy
>  		 * PERF_TYPE_HARDWARE and PERF_TYPE_HW_CACHE events on a
>  		 * specific PMU.
>  		 */
> -		.capabilities	= PERF_PMU_CAP_HETEROGENEOUS_CPUS | PERF_PMU_CAP_EXTENDED_REGS |
> +		.capabilities	= PERF_PMU_CAP_EXTENDED_REGS |
>  				  PERF_PMU_CAP_EXTENDED_HW_TYPE,

Small nit, the second line could be folded back into the first one.

>  	};
>  

Re: [PATCH 3/4] arm_pmu: Remove unused PERF_PMU_CAP_HETEROGENEOUS_CPUS capability

[James Clark]

On 11/07/2023 13:08, Anshuman Khandual wrote:
> 
> 
> On 7/10/23 17:51, James Clark wrote:
>> Since commit bd2756811766 ("perf: Rewrite core context handling") the
>> relationship between perf_event_context and PMUs has changed so that
>> the error scenario that PERF_PMU_CAP_HETEROGENEOUS_CPUS originally
>> silenced no longer exists.
>>
>> Remove the capability and associated comment to avoid confusion that it
>> actually influences any perf core behavior. This change should be a
>> no-op.
>>
>> Signed-off-by: James Clark <james.clark@arm.com>
>> ---
>>  drivers/perf/arm_pmu.c | 7 ++-----
>>  1 file changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
>> index d8844a9461a2..297906df6628 100644
>> --- a/drivers/perf/arm_pmu.c
>> +++ b/drivers/perf/arm_pmu.c
>> @@ -872,15 +872,12 @@ struct arm_pmu *armpmu_alloc(void)
>>  		.attr_groups	= pmu->attr_groups,
>>  		/*
>>  		 * This is a CPU PMU potentially in a heterogeneous
>> -		 * configuration (e.g. big.LITTLE). This is not an uncore PMU,
>> -		 * and we have taken ctx sharing into account (e.g. with our
>> -		 * pmu::filter callback and pmu::event_init group validation).
>> -		 *
>> +		 * configuration (e.g. big.LITTLE) so
>>  		 * PERF_PMU_CAP_EXTENDED_HW_TYPE is required to open the legacy
>>  		 * PERF_TYPE_HARDWARE and PERF_TYPE_HW_CACHE events on a
>>  		 * specific PMU.
>>  		 */
>> -		.capabilities	= PERF_PMU_CAP_HETEROGENEOUS_CPUS | PERF_PMU_CAP_EXTENDED_REGS |
>> +		.capabilities	= PERF_PMU_CAP_EXTENDED_REGS |
>>  				  PERF_PMU_CAP_EXTENDED_HW_TYPE,
> 
> Small nit, the second line could be folded back into the first one.

.clang-format has a column limit of 80 which is why it gets folded. The
kernel coding style guide also still says 80 is a strongly preferred
limit so I'll probably leave it.

> 
>>  	};
>>  

[PATCH 4/4] perf: Remove unused PERF_PMU_CAP_HETEROGENEOUS_CPUS capability

[James Clark]

Since commit bd2756811766 ("perf: Rewrite core context handling") the
relationship between perf_event_context and PMUs has changed so that
the error scenario that PERF_PMU_CAP_HETEROGENEOUS_CPUS originally
silenced no longer exists.

Remove the capability to avoid confusion that it actually influences
any perf core behavior. This change should be a no-op.

Signed-off-by: James Clark <james.clark@arm.com>
---
 include/linux/perf_event.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index d5628a7b5eaa..3f4d941fd6c5 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -288,7 +288,7 @@ struct perf_event_pmu_context;
 #define PERF_PMU_CAP_EXTENDED_REGS		0x0008
 #define PERF_PMU_CAP_EXCLUSIVE			0x0010
 #define PERF_PMU_CAP_ITRACE			0x0020
-#define PERF_PMU_CAP_HETEROGENEOUS_CPUS		0x0040
+/* Unused					0x0040 */
 #define PERF_PMU_CAP_NO_EXCLUDE			0x0080
 #define PERF_PMU_CAP_AUX_OUTPUT			0x0100
 #define PERF_PMU_CAP_EXTENDED_HW_TYPE		0x0200
-- 
2.34.1

Re: [PATCH 4/4] perf: Remove unused PERF_PMU_CAP_HETEROGENEOUS_CPUS capability

[Ian Rogers]

On Mon, Jul 10, 2023 at 5:22 AM James Clark <james.clark@arm.com> wrote:
>
> Since commit bd2756811766 ("perf: Rewrite core context handling") the
> relationship between perf_event_context and PMUs has changed so that
> the error scenario that PERF_PMU_CAP_HETEROGENEOUS_CPUS originally
> silenced no longer exists.
>
> Remove the capability to avoid confusion that it actually influences
> any perf core behavior. This change should be a no-op.
>
> Signed-off-by: James Clark <james.clark@arm.com>

Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  include/linux/perf_event.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index d5628a7b5eaa..3f4d941fd6c5 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -288,7 +288,7 @@ struct perf_event_pmu_context;
>  #define PERF_PMU_CAP_EXTENDED_REGS             0x0008
>  #define PERF_PMU_CAP_EXCLUSIVE                 0x0010
>  #define PERF_PMU_CAP_ITRACE                    0x0020
> -#define PERF_PMU_CAP_HETEROGENEOUS_CPUS                0x0040
> +/* Unused                                      0x0040 */
>  #define PERF_PMU_CAP_NO_EXCLUDE                        0x0080
>  #define PERF_PMU_CAP_AUX_OUTPUT                        0x0100
>  #define PERF_PMU_CAP_EXTENDED_HW_TYPE          0x0200
> --
> 2.34.1
>

Re: [PATCH 4/4] perf: Remove unused PERF_PMU_CAP_HETEROGENEOUS_CPUS capability

[Anshuman Khandual]

On 7/10/23 17:51, James Clark wrote:
> Since commit bd2756811766 ("perf: Rewrite core context handling") the
> relationship between perf_event_context and PMUs has changed so that
> the error scenario that PERF_PMU_CAP_HETEROGENEOUS_CPUS originally
> silenced no longer exists.
> 
> Remove the capability to avoid confusion that it actually influences
> any perf core behavior. This change should be a no-op.
> 
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
>  include/linux/perf_event.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index d5628a7b5eaa..3f4d941fd6c5 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -288,7 +288,7 @@ struct perf_event_pmu_context;
>  #define PERF_PMU_CAP_EXTENDED_REGS		0x0008
>  #define PERF_PMU_CAP_EXCLUSIVE			0x0010
>  #define PERF_PMU_CAP_ITRACE			0x0020
> -#define PERF_PMU_CAP_HETEROGENEOUS_CPUS		0x0040
> +/* Unused					0x0040 */

Small nit, "Unused" marking might not be required here.

>  #define PERF_PMU_CAP_NO_EXCLUDE			0x0080
>  #define PERF_PMU_CAP_AUX_OUTPUT			0x0100
>  #define PERF_PMU_CAP_EXTENDED_HW_TYPE		0x0200

Re: [PATCH 4/4] perf: Remove unused PERF_PMU_CAP_HETEROGENEOUS_CPUS capability

[James Clark]

On 11/07/2023 13:10, Anshuman Khandual wrote:
> 
> 
> On 7/10/23 17:51, James Clark wrote:
>> Since commit bd2756811766 ("perf: Rewrite core context handling") the
>> relationship between perf_event_context and PMUs has changed so that
>> the error scenario that PERF_PMU_CAP_HETEROGENEOUS_CPUS originally
>> silenced no longer exists.
>>
>> Remove the capability to avoid confusion that it actually influences
>> any perf core behavior. This change should be a no-op.
>>
>> Signed-off-by: James Clark <james.clark@arm.com>
>> ---
>>  include/linux/perf_event.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
>> index d5628a7b5eaa..3f4d941fd6c5 100644
>> --- a/include/linux/perf_event.h
>> +++ b/include/linux/perf_event.h
>> @@ -288,7 +288,7 @@ struct perf_event_pmu_context;
>>  #define PERF_PMU_CAP_EXTENDED_REGS		0x0008
>>  #define PERF_PMU_CAP_EXCLUSIVE			0x0010
>>  #define PERF_PMU_CAP_ITRACE			0x0020
>> -#define PERF_PMU_CAP_HETEROGENEOUS_CPUS		0x0040
>> +/* Unused					0x0040 */
> 
> Small nit, "Unused" marking might not be required here.
> 

But then it would be very easy to miss that there is a free bit if I
don't leave the comment. Is it really better without it?

I could shift all the following ones down by one bit, but it would be a
lot of work to make sure that nobody has hard coded some check for one
of the bits instead of using the define somewhere.

>>  #define PERF_PMU_CAP_NO_EXCLUDE			0x0080
>>  #define PERF_PMU_CAP_AUX_OUTPUT			0x0100
>>  #define PERF_PMU_CAP_EXTENDED_HW_TYPE		0x0200

Re: [PATCH 4/4] perf: Remove unused PERF_PMU_CAP_HETEROGENEOUS_CPUS capability

[Anshuman Khandual]

On 7/11/23 19:45, James Clark wrote:
> 
> 
> On 11/07/2023 13:10, Anshuman Khandual wrote:
>>
>>
>> On 7/10/23 17:51, James Clark wrote:
>>> Since commit bd2756811766 ("perf: Rewrite core context handling") the
>>> relationship between perf_event_context and PMUs has changed so that
>>> the error scenario that PERF_PMU_CAP_HETEROGENEOUS_CPUS originally
>>> silenced no longer exists.
>>>
>>> Remove the capability to avoid confusion that it actually influences
>>> any perf core behavior. This change should be a no-op.
>>>
>>> Signed-off-by: James Clark <james.clark@arm.com>
>>> ---
>>>  include/linux/perf_event.h | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
>>> index d5628a7b5eaa..3f4d941fd6c5 100644
>>> --- a/include/linux/perf_event.h
>>> +++ b/include/linux/perf_event.h
>>> @@ -288,7 +288,7 @@ struct perf_event_pmu_context;
>>>  #define PERF_PMU_CAP_EXTENDED_REGS		0x0008
>>>  #define PERF_PMU_CAP_EXCLUSIVE			0x0010
>>>  #define PERF_PMU_CAP_ITRACE			0x0020
>>> -#define PERF_PMU_CAP_HETEROGENEOUS_CPUS		0x0040
>>> +/* Unused					0x0040 */
>>
>> Small nit, "Unused" marking might not be required here.
>>
> 
> But then it would be very easy to miss that there is a free bit if I
> don't leave the comment. Is it really better without it?
> 
> I could shift all the following ones down by one bit, but it would be a

Sounds as a better option IMHO.

> lot of work to make sure that nobody has hard coded some check for one
> of the bits instead of using the define somewhere.

These are not user visible ABI and hence defined in include/linux/perf_event.h
to be used by drivers registering a PMU for capability enumeration. I am just
wondering why they might have been hard coded any where ?

> 
>>>  #define PERF_PMU_CAP_NO_EXCLUDE			0x0080
>>>  #define PERF_PMU_CAP_AUX_OUTPUT			0x0100
>>>  #define PERF_PMU_CAP_EXTENDED_HW_TYPE		0x0200

Re: [PATCH 4/4] perf: Remove unused PERF_PMU_CAP_HETEROGENEOUS_CPUS capability

[James Clark]

On 13/07/2023 08:30, Anshuman Khandual wrote:
> 
> 
> On 7/11/23 19:45, James Clark wrote:
>>
>>
>> On 11/07/2023 13:10, Anshuman Khandual wrote:
>>>
>>>
>>> On 7/10/23 17:51, James Clark wrote:
>>>> Since commit bd2756811766 ("perf: Rewrite core context handling") the
>>>> relationship between perf_event_context and PMUs has changed so that
>>>> the error scenario that PERF_PMU_CAP_HETEROGENEOUS_CPUS originally
>>>> silenced no longer exists.
>>>>
>>>> Remove the capability to avoid confusion that it actually influences
>>>> any perf core behavior. This change should be a no-op.
>>>>
>>>> Signed-off-by: James Clark <james.clark@arm.com>
>>>> ---
>>>>  include/linux/perf_event.h | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
>>>> index d5628a7b5eaa..3f4d941fd6c5 100644
>>>> --- a/include/linux/perf_event.h
>>>> +++ b/include/linux/perf_event.h
>>>> @@ -288,7 +288,7 @@ struct perf_event_pmu_context;
>>>>  #define PERF_PMU_CAP_EXTENDED_REGS		0x0008
>>>>  #define PERF_PMU_CAP_EXCLUSIVE			0x0010
>>>>  #define PERF_PMU_CAP_ITRACE			0x0020
>>>> -#define PERF_PMU_CAP_HETEROGENEOUS_CPUS		0x0040
>>>> +/* Unused					0x0040 */
>>>
>>> Small nit, "Unused" marking might not be required here.
>>>
>>
>> But then it would be very easy to miss that there is a free bit if I
>> don't leave the comment. Is it really better without it?
>>
>> I could shift all the following ones down by one bit, but it would be a
> 
> Sounds as a better option IMHO.
> 
>> lot of work to make sure that nobody has hard coded some check for one
>> of the bits instead of using the define somewhere.
> 
> These are not user visible ABI and hence defined in include/linux/perf_event.h
> to be used by drivers registering a PMU for capability enumeration. I am just
> wondering why they might have been hard coded any where ?
> 

You never know, I've seen worse! I suppose we'll find out if I change it
and anything breaks. It will probably be fine though, I'll make the
change on the next version.

>>
>>>>  #define PERF_PMU_CAP_NO_EXCLUDE			0x0080
>>>>  #define PERF_PMU_CAP_AUX_OUTPUT			0x0100
>>>>  #define PERF_PMU_CAP_EXTENDED_HW_TYPE		0x0200