[PATCH] Powerpc/perf: Wire up PMI throttling

[PATCH] Powerpc/perf: Wire up PMI throttling

[Ravi Bangoria]

Commit 14c63f17b1fde ("perf: Drop sample rate when sampling is too
slow") introduced a way to throttle PMU interrupts if we're spending
too much time just processing those. Wire up powerpc PMI handler to
use this infrastructure.

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
 arch/powerpc/kernel/traps.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 9a86572db1ef..44f85fa22356 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -18,6 +18,7 @@
 #include <linux/errno.h>
 #include <linux/sched.h>
 #include <linux/sched/debug.h>
+#include <linux/sched/clock.h>
 #include <linux/kernel.h>
 #include <linux/mm.h>
 #include <linux/pkeys.h>
@@ -1803,9 +1804,12 @@ void vsx_unavailable_tm(struct pt_regs *regs)
 
 void performance_monitor_exception(struct pt_regs *regs)
 {
+	u64 start_clock;
 	__this_cpu_inc(irq_stat.pmu_irqs);
 
+	start_clock = sched_clock();
 	perf_irq(regs);
+	perf_sample_event_took(sched_clock() - start_clock);
 }
 
 #ifdef CONFIG_PPC_ADV_DEBUG_REGS
-- 
2.19.1

Re: [PATCH] Powerpc/perf: Wire up PMI throttling

[Michael Ellerman]

Ravi Bangoria <ravi.bangoria@linux.ibm.com> writes:

> Commit 14c63f17b1fde ("perf: Drop sample rate when sampling is too
> slow") introduced a way to throttle PMU interrupts if we're spending
> too much time just processing those. Wire up powerpc PMI handler to
> use this infrastructure.

To be clear we have throttling of the *rate* of interrupts, but this
adds throttling based on the *time taken* to process the interrupts. Or
at least that's my understanding?

> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index 9a86572db1ef..44f85fa22356 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -18,6 +18,7 @@
>  #include <linux/errno.h>
>  #include <linux/sched.h>
>  #include <linux/sched/debug.h>
> +#include <linux/sched/clock.h>
>  #include <linux/kernel.h>
>  #include <linux/mm.h>
>  #include <linux/pkeys.h>
> @@ -1803,9 +1804,12 @@ void vsx_unavailable_tm(struct pt_regs *regs)
>  
>  void performance_monitor_exception(struct pt_regs *regs)
>  {
> +	u64 start_clock;
>  	__this_cpu_inc(irq_stat.pmu_irqs);
>  
> +	start_clock = sched_clock();
>  	perf_irq(regs);
> +	perf_sample_event_took(sched_clock() - start_clock);
>  }

Despite the name, perf_irq() may not actually be the perf IRQ handler :)

It's a function pointer which might call perf or might call oprofile, or
a dummy handler.

I don't think we should be calling perf_sample_event_took() if we're not
actually using perf, that is wasteful at best.

So the timing logic should go in the perf specific handler I think.
ie. perf_event_interrupt().

cheers

Re: [PATCH] Powerpc/perf: Wire up PMI throttling

[Ravi Bangoria]

On 11/15/18 6:13 PM, Michael Ellerman wrote:
> Ravi Bangoria <ravi.bangoria@linux.ibm.com> writes:
> 
>> Commit 14c63f17b1fde ("perf: Drop sample rate when sampling is too
>> slow") introduced a way to throttle PMU interrupts if we're spending
>> too much time just processing those. Wire up powerpc PMI handler to
>> use this infrastructure.
> 
> To be clear we have throttling of the *rate* of interrupts, but this
> adds throttling based on the *time taken* to process the interrupts. Or
> at least that's my understanding?

Right. This throttles based on time taken to process the interrupts.

> 
>> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
>> index 9a86572db1ef..44f85fa22356 100644
>> --- a/arch/powerpc/kernel/traps.c
>> +++ b/arch/powerpc/kernel/traps.c
>> @@ -18,6 +18,7 @@
>>  #include <linux/errno.h>
>>  #include <linux/sched.h>
>>  #include <linux/sched/debug.h>
>> +#include <linux/sched/clock.h>
>>  #include <linux/kernel.h>
>>  #include <linux/mm.h>
>>  #include <linux/pkeys.h>
>> @@ -1803,9 +1804,12 @@ void vsx_unavailable_tm(struct pt_regs *regs)
>>  
>>  void performance_monitor_exception(struct pt_regs *regs)
>>  {
>> +	u64 start_clock;
>>  	__this_cpu_inc(irq_stat.pmu_irqs);
>>  
>> +	start_clock = sched_clock();
>>  	perf_irq(regs);
>> +	perf_sample_event_took(sched_clock() - start_clock);
>>  }
> 
> Despite the name, perf_irq() may not actually be the perf IRQ handler :)
> 
> It's a function pointer which might call perf or might call oprofile, or
> a dummy handler.
> 
> I don't think we should be calling perf_sample_event_took() if we're not
> actually using perf, that is wasteful at best.
> 
> So the timing logic should go in the perf specific handler I think.
> ie. perf_event_interrupt().

Makes sense. I'll re-send with that change.

Thanks,
Ravi