[PATCH V7 0/2] genirq: arm64: perf: support for percpu pmu interrupt

[PATCH V7 0/2] genirq: arm64: perf: support for percpu pmu interrupt

[Vinayak Kale]

This patch series adds support to handle interrupt registration/deregistration
in arm64 pmu driver when pmu interrupt type is percpu.

Changelog:
V7:
* In arm64 pmu driver: Instead of passing 'struct arm_pmu' pointer, pass the
  irq number directly to armpmu_[enable/disable]_percpu_irq().
  Clean-up the percpu active irq cpumask set/clear. 

V6:
* In arm64 pmu driver: Use macro 'IRQ_TYPE_NONE' while passing irq type value
  to enable_percpu_irq(). Change 'irqs' etc variables as unsigned, modify the
  check for 'irqs' inside armpmu_[release/reserve]_hardware().

V5:
* In irqdesc.h: Added Chris Smith's sign-off.
  In arm64 pmu driver: Handle the invalid irq-0 case for platform_get_irq().

V4:
* In arm64 pmu driver: Avoid using irq_to_desc() to check validity of irq.

V3:
* Remove validity check for 'desc' from accessor function in irqdesc.h .
  Instead, check the irq 'desc' validity in arm64 pmu driver.

V2:
* To determine whether an IRQ is percpu or not, added an accessor function in
  irqdesc.h . This approach was used by Chris Smith here[1] for similar changes
  in arm pmu driver.
* In arm64 pmu driver: Got rid of unnecessary pointer typecastings.

[1] http://lkml.indiana.edu/hypermail/linux/kernel/1207.3/02955.html

Vinayak Kale (2):
  genirq: Add an accessor for IRQ_PER_CPU flag
  arm64: perf: add support for percpu pmu interrupt

 arch/arm64/kernel/perf_event.c |  108 +++++++++++++++++++++++++++++-----------
 include/linux/irqdesc.h        |    8 +++
 2 files changed, 86 insertions(+), 30 deletions(-)

-- 
1.7.9.5

[PATCH V7 1/2] genirq: Add an accessor for IRQ_PER_CPU flag

[Vinayak Kale]

This patch adds an accessor function for IRQ_PER_CPU flag.
The accessor function is useful to determine whether an IRQ is percpu or not.

This patch is based on an older patch posted by Chris Smith here [1].
There is a minor change w.r.t. Chris's original patch: The accessor function
is renamed as 'irq_is_percpu' instead of 'irq_is_per_cpu'.

[1]: http://lkml.indiana.edu/hypermail/linux/kernel/1207.3/02955.html

Signed-off-by: Chris Smith <chris.smith@st.com>
Signed-off-by: Vinayak Kale <vkale@apm.com>
Acked-by: Will Deacon <will.deacon@arm.com>
---
 include/linux/irqdesc.h |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
index 56fb646..26e2661 100644
--- a/include/linux/irqdesc.h
+++ b/include/linux/irqdesc.h
@@ -152,6 +152,14 @@ static inline int irq_balancing_disabled(unsigned int irq)
 	return desc->status_use_accessors & IRQ_NO_BALANCING_MASK;
 }
 
+static inline int irq_is_percpu(unsigned int irq)
+{
+	struct irq_desc *desc;
+
+	desc = irq_to_desc(irq);
+	return desc->status_use_accessors & IRQ_PER_CPU;
+}
+
 static inline void
 irq_set_lockdep_class(unsigned int irq, struct lock_class_key *class)
 {
-- 
1.7.9.5

[PATCH V7 1/2] genirq: Add an accessor for IRQ_PER_CPU flag

[Thomas Gleixner]

On Wed, 4 Dec 2013, Vinayak Kale wrote:

> This patch adds an accessor function for IRQ_PER_CPU flag.
> The accessor function is useful to determine whether an IRQ is percpu or not.
> 
> This patch is based on an older patch posted by Chris Smith here [1].
> There is a minor change w.r.t. Chris's original patch: The accessor function
> is renamed as 'irq_is_percpu' instead of 'irq_is_per_cpu'.
> 
> [1]: http://lkml.indiana.edu/hypermail/linux/kernel/1207.3/02955.html
> 
> Signed-off-by: Chris Smith <chris.smith@st.com>
> Signed-off-by: Vinayak Kale <vkale@apm.com>
> Acked-by: Will Deacon <will.deacon@arm.com>

For the genirq part:
    Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

Please take that through the SoC tree which carries the actual user of
this.

Thanks,

	tglx

[PATCH V7 2/2] arm64: perf: add support for percpu pmu interrupt

[Vinayak Kale]

Add support for irq registration when pmu interrupt is percpu.

Signed-off-by: Vinayak Kale <vkale@apm.com>
Signed-off-by: Tuan Phan <tphan@apm.com>
---
 arch/arm64/kernel/perf_event.c |  108 +++++++++++++++++++++++++++++-----------
 1 file changed, 78 insertions(+), 30 deletions(-)

diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index cea1594..d8e6667 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -22,6 +22,7 @@
 
 #include <linux/bitmap.h>
 #include <linux/interrupt.h>
+#include <linux/irq.h>
 #include <linux/kernel.h>
 #include <linux/export.h>
 #include <linux/perf_event.h>
@@ -363,26 +364,52 @@ validate_group(struct perf_event *event)
 }
 
 static void
+armpmu_disable_percpu_irq(void *data)
+{
+	disable_percpu_irq((long)data);
+}
+
+static void
 armpmu_release_hardware(struct arm_pmu *armpmu)
 {
-	int i, irq, irqs;
+	int irq;
+	unsigned int i, irqs;
 	struct platform_device *pmu_device = armpmu->plat_device;
 
 	irqs = min(pmu_device->num_resources, num_possible_cpus());
+	if (!irqs)
+		return;
 
-	for (i = 0; i < irqs; ++i) {
-		if (!cpumask_test_and_clear_cpu(i, &armpmu->active_irqs))
-			continue;
-		irq = platform_get_irq(pmu_device, i);
-		if (irq >= 0)
-			free_irq(irq, armpmu);
+	irq = platform_get_irq(pmu_device, 0);
+	if (irq <= 0)
+		return;
+
+	if (irq_is_percpu(irq)) {
+		cpumask_clear(&armpmu->active_irqs);
+		on_each_cpu(armpmu_disable_percpu_irq, (void *)(long)irq, 1);
+		free_percpu_irq(irq, &cpu_hw_events);
+	} else {
+		for (i = 0; i < irqs; ++i) {
+			if (!cpumask_test_and_clear_cpu(i, &armpmu->active_irqs))
+				continue;
+			irq = platform_get_irq(pmu_device, i);
+			if (irq > 0)
+				free_irq(irq, armpmu);
+		}
 	}
 }
 
+static void
+armpmu_enable_percpu_irq(void *data)
+{
+	enable_percpu_irq((long)data, IRQ_TYPE_NONE);
+}
+
 static int
 armpmu_reserve_hardware(struct arm_pmu *armpmu)
 {
-	int i, err, irq, irqs;
+	int err, irq;
+	unsigned int i, irqs;
 	struct platform_device *pmu_device = armpmu->plat_device;
 
 	if (!pmu_device) {
@@ -391,39 +418,60 @@ armpmu_reserve_hardware(struct arm_pmu *armpmu)
 	}
 
 	irqs = min(pmu_device->num_resources, num_possible_cpus());
-	if (irqs < 1) {
+	if (!irqs) {
 		pr_err("no irqs for PMUs defined\n");
 		return -ENODEV;
 	}
 
-	for (i = 0; i < irqs; ++i) {
-		err = 0;
-		irq = platform_get_irq(pmu_device, i);
-		if (irq < 0)
-			continue;
+	irq = platform_get_irq(pmu_device, 0);
+	if (irq <= 0) {
+		pr_err("failed to get valid irq for PMU device\n");
+		return -ENODEV;
+	}
 
-		/*
-		 * If we have a single PMU interrupt that we can't shift,
-		 * assume that we're running on a uniprocessor machine and
-		 * continue. Otherwise, continue without this interrupt.
-		 */
-		if (irq_set_affinity(irq, cpumask_of(i)) && irqs > 1) {
-			pr_warning("unable to set irq affinity (irq=%d, cpu=%u)\n",
-				    irq, i);
-			continue;
-		}
+	if (irq_is_percpu(irq)) {
+		err = request_percpu_irq(irq, armpmu->handle_irq,
+				"arm-pmu", &cpu_hw_events);
 
-		err = request_irq(irq, armpmu->handle_irq,
-				  IRQF_NOBALANCING,
-				  "arm-pmu", armpmu);
 		if (err) {
-			pr_err("unable to request IRQ%d for ARM PMU counters\n",
-				irq);
+			pr_err("unable to request percpu IRQ%d for ARM PMU counters\n",
+					irq);
 			armpmu_release_hardware(armpmu);
 			return err;
 		}
 
-		cpumask_set_cpu(i, &armpmu->active_irqs);
+		on_each_cpu(armpmu_enable_percpu_irq, (void *)(long)irq, 1);
+		cpumask_setall(&armpmu->active_irqs);
+	} else {
+		for (i = 0; i < irqs; ++i) {
+			err = 0;
+			irq = platform_get_irq(pmu_device, i);
+			if (irq <= 0)
+				continue;
+
+			/*
+			 * If we have a single PMU interrupt that we can't shift,
+			 * assume that we're running on a uniprocessor machine and
+			 * continue. Otherwise, continue without this interrupt.
+			 */
+			if (irq_set_affinity(irq, cpumask_of(i)) && irqs > 1) {
+				pr_warning("unable to set irq affinity (irq=%d, cpu=%u)\n",
+						irq, i);
+				continue;
+			}
+
+			err = request_irq(irq, armpmu->handle_irq,
+					IRQF_NOBALANCING,
+					"arm-pmu", armpmu);
+			if (err) {
+				pr_err("unable to request IRQ%d for ARM PMU counters\n",
+						irq);
+				armpmu_release_hardware(armpmu);
+				return err;
+			}
+
+			cpumask_set_cpu(i, &armpmu->active_irqs);
+		}
 	}
 
 	return 0;
-- 
1.7.9.5

[PATCH V7 2/2] arm64: perf: add support for percpu pmu interrupt

[Will Deacon]

Hi Vinayak,

On Wed, Dec 04, 2013 at 10:09:51AM +0000, Vinayak Kale wrote:
> Add support for irq registration when pmu interrupt is percpu.

Getting closer...

> Signed-off-by: Vinayak Kale <vkale@apm.com>
> Signed-off-by: Tuan Phan <tphan@apm.com>
> ---
>  arch/arm64/kernel/perf_event.c |  108 +++++++++++++++++++++++++++++-----------
>  1 file changed, 78 insertions(+), 30 deletions(-)
> 
> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> index cea1594..d8e6667 100644
> --- a/arch/arm64/kernel/perf_event.c
> +++ b/arch/arm64/kernel/perf_event.c
> @@ -22,6 +22,7 @@
>  
>  #include <linux/bitmap.h>
>  #include <linux/interrupt.h>
> +#include <linux/irq.h>
>  #include <linux/kernel.h>
>  #include <linux/export.h>
>  #include <linux/perf_event.h>
> @@ -363,26 +364,52 @@ validate_group(struct perf_event *event)
>  }
>  
>  static void
> +armpmu_disable_percpu_irq(void *data)
> +{
> +	disable_percpu_irq((long)data);
> +}

Given that we wait for the CPUs to finish enabling/disabling the IRQ, I
actually meant pass the pointer to the IRQ, which removes the horrible
casts in the caller.

> +	if (irq_is_percpu(irq)) {
> +		cpumask_clear(&armpmu->active_irqs);

Thanks for moving the mask manipulation out. It now makes it obvious that we
don't care about the mask at all for PPIs, so that can be removed (the code
you have is racy against hotplug anyway).

I took the liberty of writing a fixup for you (see below). Can you test it
on your platform please?

Cheers,

Will

--->8

diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index 503c1eeedc1c..5b1cd792274a 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -366,7 +366,8 @@ validate_group(struct perf_event *event)
 static void
 armpmu_disable_percpu_irq(void *data)
 {
-       disable_percpu_irq((long)data);
+       unsigned int irq = *(unsigned int *)data;
+       disable_percpu_irq(irq);
 }
 
 static void
@@ -385,8 +386,7 @@ armpmu_release_hardware(struct arm_pmu *armpmu)
                return;
 
        if (irq_is_percpu(irq)) {
-               cpumask_clear(&armpmu->active_irqs);
-               on_each_cpu(armpmu_disable_percpu_irq, (void *)(long)irq, 1);
+               on_each_cpu(armpmu_disable_percpu_irq, &irq, 1);
                free_percpu_irq(irq, &cpu_hw_events);
        } else {
                for (i = 0; i < irqs; ++i) {
@@ -402,7 +402,8 @@ armpmu_release_hardware(struct arm_pmu *armpmu)
 static void
 armpmu_enable_percpu_irq(void *data)
 {
-       enable_percpu_irq((long)data, IRQ_TYPE_NONE);
+       unsigned int irq = *(unsigned int *)data;
+       enable_percpu_irq(irq, IRQ_TYPE_NONE);
 }
 
 static int
@@ -440,8 +441,7 @@ armpmu_reserve_hardware(struct arm_pmu *armpmu)
                        return err;
                }
 
-               on_each_cpu(armpmu_enable_percpu_irq, (void *)(long)irq, 1);
-               cpumask_setall(&armpmu->active_irqs);
+               on_each_cpu(armpmu_enable_percpu_irq, &irq, 1);
        } else {
                for (i = 0; i < irqs; ++i) {
                        err = 0;

[PATCH V7 2/2] arm64: perf: add support for percpu pmu interrupt

[Vinayak Kale]

Hi Will,


On Mon, Dec 9, 2013 at 10:20 PM, Will Deacon <will.deacon@arm.com> wrote:
> Hi Vinayak,
>
> On Wed, Dec 04, 2013 at 10:09:51AM +0000, Vinayak Kale wrote:
>> Add support for irq registration when pmu interrupt is percpu.
>
> Getting closer...
>
>> Signed-off-by: Vinayak Kale <vkale@apm.com>
>> Signed-off-by: Tuan Phan <tphan@apm.com>
>> ---
>>  arch/arm64/kernel/perf_event.c |  108 +++++++++++++++++++++++++++++-----------
>>  1 file changed, 78 insertions(+), 30 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
>> index cea1594..d8e6667 100644
>> --- a/arch/arm64/kernel/perf_event.c
>> +++ b/arch/arm64/kernel/perf_event.c
>> @@ -22,6 +22,7 @@
>>
>>  #include <linux/bitmap.h>
>>  #include <linux/interrupt.h>
>> +#include <linux/irq.h>
>>  #include <linux/kernel.h>
>>  #include <linux/export.h>
>>  #include <linux/perf_event.h>
>> @@ -363,26 +364,52 @@ validate_group(struct perf_event *event)
>>  }
>>
>>  static void
>> +armpmu_disable_percpu_irq(void *data)
>> +{
>> +     disable_percpu_irq((long)data);
>> +}
>
> Given that we wait for the CPUs to finish enabling/disabling the IRQ, I
> actually meant pass the pointer to the IRQ, which removes the horrible
> casts in the caller.
>
>> +     if (irq_is_percpu(irq)) {
>> +             cpumask_clear(&armpmu->active_irqs);
>
> Thanks for moving the mask manipulation out. It now makes it obvious that we
> don't care about the mask at all for PPIs, so that can be removed (the code
> you have is racy against hotplug anyway).
>
> I took the liberty of writing a fixup for you (see below). Can you test it
> on your platform please?

Below fixup works fine on APM platform.
Do you want me to send this fixup as part of next revision of the
patch or will you apply it yourself? (For later case, you have my ack)

>
> Cheers,
>
> Will
>
> --->8
>
> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> index 503c1eeedc1c..5b1cd792274a 100644
> --- a/arch/arm64/kernel/perf_event.c
> +++ b/arch/arm64/kernel/perf_event.c
> @@ -366,7 +366,8 @@ validate_group(struct perf_event *event)
>  static void
>  armpmu_disable_percpu_irq(void *data)
>  {
> -       disable_percpu_irq((long)data);
> +       unsigned int irq = *(unsigned int *)data;
> +       disable_percpu_irq(irq);
>  }
>
>  static void
> @@ -385,8 +386,7 @@ armpmu_release_hardware(struct arm_pmu *armpmu)
>                 return;
>
>         if (irq_is_percpu(irq)) {
> -               cpumask_clear(&armpmu->active_irqs);
> -               on_each_cpu(armpmu_disable_percpu_irq, (void *)(long)irq, 1);
> +               on_each_cpu(armpmu_disable_percpu_irq, &irq, 1);
>                 free_percpu_irq(irq, &cpu_hw_events);
>         } else {
>                 for (i = 0; i < irqs; ++i) {
> @@ -402,7 +402,8 @@ armpmu_release_hardware(struct arm_pmu *armpmu)
>  static void
>  armpmu_enable_percpu_irq(void *data)
>  {
> -       enable_percpu_irq((long)data, IRQ_TYPE_NONE);
> +       unsigned int irq = *(unsigned int *)data;
> +       enable_percpu_irq(irq, IRQ_TYPE_NONE);
>  }
>
>  static int
> @@ -440,8 +441,7 @@ armpmu_reserve_hardware(struct arm_pmu *armpmu)
>                         return err;
>                 }
>
> -               on_each_cpu(armpmu_enable_percpu_irq, (void *)(long)irq, 1);
> -               cpumask_setall(&armpmu->active_irqs);
> +               on_each_cpu(armpmu_enable_percpu_irq, &irq, 1);
>         } else {
>                 for (i = 0; i < irqs; ++i) {
>                         err = 0;

Acked-by: Vinayak Kale <vkale@apm.com>

[PATCH V7 2/2] arm64: perf: add support for percpu pmu interrupt

[Vinayak Kale]

Hi Will,

On Tue, Dec 10, 2013 at 1:00 PM, Vinayak Kale <vkale@apm.com> wrote:
> Hi Will,
>
>
> On Mon, Dec 9, 2013 at 10:20 PM, Will Deacon <will.deacon@arm.com> wrote:
>> Hi Vinayak,
>>
>> On Wed, Dec 04, 2013 at 10:09:51AM +0000, Vinayak Kale wrote:
>>> Add support for irq registration when pmu interrupt is percpu.
>>
>> Getting closer...
>>
>>> Signed-off-by: Vinayak Kale <vkale@apm.com>
>>> Signed-off-by: Tuan Phan <tphan@apm.com>
>>> ---
>>>  arch/arm64/kernel/perf_event.c |  108 +++++++++++++++++++++++++++++-----------
>>>  1 file changed, 78 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
>>> index cea1594..d8e6667 100644
>>> --- a/arch/arm64/kernel/perf_event.c
>>> +++ b/arch/arm64/kernel/perf_event.c
>>> @@ -22,6 +22,7 @@
>>>
>>>  #include <linux/bitmap.h>
>>>  #include <linux/interrupt.h>
>>> +#include <linux/irq.h>
>>>  #include <linux/kernel.h>
>>>  #include <linux/export.h>
>>>  #include <linux/perf_event.h>
>>> @@ -363,26 +364,52 @@ validate_group(struct perf_event *event)
>>>  }
>>>
>>>  static void
>>> +armpmu_disable_percpu_irq(void *data)
>>> +{
>>> +     disable_percpu_irq((long)data);
>>> +}
>>
>> Given that we wait for the CPUs to finish enabling/disabling the IRQ, I
>> actually meant pass the pointer to the IRQ, which removes the horrible
>> casts in the caller.
>>
>>> +     if (irq_is_percpu(irq)) {
>>> +             cpumask_clear(&armpmu->active_irqs);
>>
>> Thanks for moving the mask manipulation out. It now makes it obvious that we
>> don't care about the mask at all for PPIs, so that can be removed (the code
>> you have is racy against hotplug anyway).
>>
>> I took the liberty of writing a fixup for you (see below). Can you test it
>> on your platform please?
>
> Below fixup works fine on APM platform.
> Do you want me to send this fixup as part of next revision of the
> patch or will you apply it yourself? (For later case, you have my ack)

Any comments? Do I need to send the fix-up in next revision of patch?

Thanks
-Vinayak

[PATCH V7 2/2] arm64: perf: add support for percpu pmu interrupt

[Will Deacon]

On Thu, Dec 12, 2013 at 06:33:59AM +0000, Vinayak Kale wrote:
> > Below fixup works fine on APM platform.
> > Do you want me to send this fixup as part of next revision of the
> > patch or will you apply it yourself? (For later case, you have my ack)
> 
> Any comments? Do I need to send the fix-up in next revision of patch?

Looks fine to me; I've folded in my changes and applied it to my aarch64
branch. I'll queue it for 3.14 when Catalin and I start putting that
together.

Will

[PATCH V7 2/2] arm64: perf: add support for percpu pmu interrupt

[Vinayak Kale]

On Thu, Dec 12, 2013 at 5:16 PM, Will Deacon <will.deacon@arm.com> wrote:
> On Thu, Dec 12, 2013 at 06:33:59AM +0000, Vinayak Kale wrote:
>> > Below fixup works fine on APM platform.
>> > Do you want me to send this fixup as part of next revision of the
>> > patch or will you apply it yourself? (For later case, you have my ack)
>>
>> Any comments? Do I need to send the fix-up in next revision of patch?
>
> Looks fine to me; I've folded in my changes and applied it to my aarch64
> branch. I'll queue it for 3.14 when Catalin and I start putting that
> together.

Okay. Thanks :-)