[PATCH] drivers: perf: arm: implement CPU_PM notifier

[PATCH] drivers: perf: arm: implement CPU_PM notifier

[Lorenzo Pieralisi]

When a CPU is suspended (either through suspend-to-RAM or CPUidle),
its PMU registers content can be lost, which means that counters
registers values that were initialized on power down entry have to be
reprogrammed on power-up to make sure the counters set-up is preserved
(ie on power-up registers take the reset values on Cold or Warm reset,
which can be architecturally UNKNOWN).

To guarantee seamless profiling conditions across a core power down
this patch adds a CPU PM notifier to ARM pmus, that upon CPU PM
entry/exit from low-power states saves/restores the pmu registers
set-up (by using the ARM perf API), so that the power-down/up cycle does
not affect the perf behaviour (apart from a black-out period between
power-up/down CPU PM notifications that is unavoidable).

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Ashwin Chaugule <ashwin.chaugule@linaro.org>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Kevin Hilman <khilman@baylibre.com>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Mark Rutland <mark.rutland@arm.com>
---
 drivers/perf/arm_pmu.c       | 95 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/perf/arm_pmu.h |  1 +
 2 files changed, 96 insertions(+)

diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
index 166637f..12317a9 100644
--- a/drivers/perf/arm_pmu.c
+++ b/drivers/perf/arm_pmu.c
@@ -13,6 +13,7 @@
 
 #include <linux/bitmap.h>
 #include <linux/cpumask.h>
+#include <linux/cpu_pm.h>
 #include <linux/export.h>
 #include <linux/kernel.h>
 #include <linux/of_device.h>
@@ -710,6 +711,93 @@ static int cpu_pmu_notify(struct notifier_block *b, unsigned long action,
 	return NOTIFY_OK;
 }
 
+#ifdef CONFIG_CPU_PM
+static void cpu_pm_pmu_setup(struct arm_pmu *armpmu, unsigned long cmd)
+{
+	struct pmu_hw_events *hw_events = this_cpu_ptr(armpmu->hw_events);
+	struct perf_event *event;
+	int idx;
+
+	for (idx = 0; idx < armpmu->num_events; idx++) {
+		/*
+		 * If the counter is not used skip it, there is no
+		 * need of stopping/restarting it.
+		 */
+		if (!test_bit(idx, hw_events->used_mask))
+			continue;
+
+		event = hw_events->events[idx];
+
+		switch (cmd) {
+		case CPU_PM_ENTER:
+			/*
+			 * Stop and update the counter
+			 */
+			armpmu_stop(event, PERF_EF_UPDATE);
+			break;
+		case CPU_PM_EXIT:
+		case CPU_PM_ENTER_FAILED:
+			 /* Restore and enable the counter */
+			armpmu_start(event, PERF_EF_RELOAD);
+			break;
+		default:
+			break;
+		}
+	}
+}
+
+static int cpu_pm_pmu_notify(struct notifier_block *b, unsigned long cmd,
+			     void *v)
+{
+	struct arm_pmu *armpmu = container_of(b, struct arm_pmu, cpu_pm_nb);
+	struct pmu_hw_events *hw_events = this_cpu_ptr(armpmu->hw_events);
+	int enabled = bitmap_weight(hw_events->used_mask, armpmu->num_events);
+
+	if (!cpumask_test_cpu(smp_processor_id(), &armpmu->supported_cpus))
+		return NOTIFY_DONE;
+
+	/*
+	 * Always reset the PMU registers on power-up even if
+	 * there are no events running.
+	 */
+	if (cmd == CPU_PM_EXIT && armpmu->reset)
+		armpmu->reset(armpmu);
+
+	if (!enabled)
+		return NOTIFY_OK;
+
+	switch (cmd) {
+	case CPU_PM_ENTER:
+		armpmu->stop(armpmu);
+		cpu_pm_pmu_setup(armpmu, cmd);
+		break;
+	case CPU_PM_EXIT:
+		cpu_pm_pmu_setup(armpmu, cmd);
+	case CPU_PM_ENTER_FAILED:
+		armpmu->start(armpmu);
+		break;
+	default:
+		return NOTIFY_DONE;
+	}
+
+	return NOTIFY_OK;
+}
+
+static int cpu_pm_pmu_register(struct arm_pmu *cpu_pmu)
+{
+	cpu_pmu->cpu_pm_nb.notifier_call = cpu_pm_pmu_notify;
+	return cpu_pm_register_notifier(&cpu_pmu->cpu_pm_nb);
+}
+
+static void cpu_pm_pmu_unregister(struct arm_pmu *cpu_pmu)
+{
+	cpu_pm_unregister_notifier(&cpu_pmu->cpu_pm_nb);
+}
+#else
+static inline int cpu_pm_pmu_register(struct arm_pmu *cpu_pmu) { return 0; }
+static inline void cpu_pm_pmu_unregister(struct arm_pmu *cpu_pmu) { }
+#endif
+
 static int cpu_pmu_init(struct arm_pmu *cpu_pmu)
 {
 	int err;
@@ -725,6 +813,10 @@ static int cpu_pmu_init(struct arm_pmu *cpu_pmu)
 	if (err)
 		goto out_hw_events;
 
+	err = cpu_pm_pmu_register(cpu_pmu);
+	if (err)
+		goto out_unregister;
+
 	for_each_possible_cpu(cpu) {
 		struct pmu_hw_events *events = per_cpu_ptr(cpu_hw_events, cpu);
 		raw_spin_lock_init(&events->pmu_lock);
@@ -746,6 +838,8 @@ static int cpu_pmu_init(struct arm_pmu *cpu_pmu)
 
 	return 0;
 
+out_unregister:
+	unregister_cpu_notifier(&cpu_pmu->hotplug_nb);
 out_hw_events:
 	free_percpu(cpu_hw_events);
 	return err;
@@ -753,6 +847,7 @@ out_hw_events:
 
 static void cpu_pmu_destroy(struct arm_pmu *cpu_pmu)
 {
+	cpu_pm_pmu_unregister(cpu_pmu);
 	unregister_cpu_notifier(&cpu_pmu->hotplug_nb);
 	free_percpu(cpu_pmu->hw_events);
 }
diff --git a/include/linux/perf/arm_pmu.h b/include/linux/perf/arm_pmu.h
index 83b5e34..9ffa316 100644
--- a/include/linux/perf/arm_pmu.h
+++ b/include/linux/perf/arm_pmu.h
@@ -107,6 +107,7 @@ struct arm_pmu {
 	struct platform_device	*plat_device;
 	struct pmu_hw_events	__percpu *hw_events;
 	struct notifier_block	hotplug_nb;
+	struct notifier_block	cpu_pm_nb;
 };
 
 #define to_arm_pmu(p) (container_of(p, struct arm_pmu, pmu))
-- 
2.5.1

[PATCH] drivers: perf: arm: implement CPU_PM notifier

[Mathieu Poirier]

On 23 February 2016 at 11:22, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> When a CPU is suspended (either through suspend-to-RAM or CPUidle),
> its PMU registers content can be lost, which means that counters
> registers values that were initialized on power down entry have to be
> reprogrammed on power-up to make sure the counters set-up is preserved
> (ie on power-up registers take the reset values on Cold or Warm reset,
> which can be architecturally UNKNOWN).
>
> To guarantee seamless profiling conditions across a core power down
> this patch adds a CPU PM notifier to ARM pmus, that upon CPU PM
> entry/exit from low-power states saves/restores the pmu registers
> set-up (by using the ARM perf API), so that the power-down/up cycle does
> not affect the perf behaviour (apart from a black-out period between
> power-up/down CPU PM notifications that is unavoidable).
>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Ashwin Chaugule <ashwin.chaugule@linaro.org>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Kevin Hilman <khilman@baylibre.com>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> ---
>  drivers/perf/arm_pmu.c       | 95 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/perf/arm_pmu.h |  1 +
>  2 files changed, 96 insertions(+)
>
> diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
> index 166637f..12317a9 100644
> --- a/drivers/perf/arm_pmu.c
> +++ b/drivers/perf/arm_pmu.c
> @@ -13,6 +13,7 @@
>
>  #include <linux/bitmap.h>
>  #include <linux/cpumask.h>
> +#include <linux/cpu_pm.h>
>  #include <linux/export.h>
>  #include <linux/kernel.h>
>  #include <linux/of_device.h>
> @@ -710,6 +711,93 @@ static int cpu_pmu_notify(struct notifier_block *b, unsigned long action,
>         return NOTIFY_OK;
>  }
>
> +#ifdef CONFIG_CPU_PM
> +static void cpu_pm_pmu_setup(struct arm_pmu *armpmu, unsigned long cmd)
> +{
> +       struct pmu_hw_events *hw_events = this_cpu_ptr(armpmu->hw_events);
> +       struct perf_event *event;
> +       int idx;
> +
> +       for (idx = 0; idx < armpmu->num_events; idx++) {
> +               /*
> +                * If the counter is not used skip it, there is no
> +                * need of stopping/restarting it.
> +                */
> +               if (!test_bit(idx, hw_events->used_mask))
> +                       continue;
> +
> +               event = hw_events->events[idx];
> +
> +               switch (cmd) {
> +               case CPU_PM_ENTER:
> +                       /*
> +                        * Stop and update the counter
> +                        */
> +                       armpmu_stop(event, PERF_EF_UPDATE);
> +                       break;
> +               case CPU_PM_EXIT:
> +               case CPU_PM_ENTER_FAILED:
> +                        /* Restore and enable the counter */
> +                       armpmu_start(event, PERF_EF_RELOAD);
> +                       break;
> +               default:
> +                       break;
> +               }
> +       }
> +}
> +
> +static int cpu_pm_pmu_notify(struct notifier_block *b, unsigned long cmd,
> +                            void *v)
> +{
> +       struct arm_pmu *armpmu = container_of(b, struct arm_pmu, cpu_pm_nb);
> +       struct pmu_hw_events *hw_events = this_cpu_ptr(armpmu->hw_events);
> +       int enabled = bitmap_weight(hw_events->used_mask, armpmu->num_events);
> +
> +       if (!cpumask_test_cpu(smp_processor_id(), &armpmu->supported_cpus))
> +               return NOTIFY_DONE;
> +
> +       /*
> +        * Always reset the PMU registers on power-up even if
> +        * there are no events running.
> +        */
> +       if (cmd == CPU_PM_EXIT && armpmu->reset)
> +               armpmu->reset(armpmu);

I think this patch does the right thing but I can't get the above
reset.  Wouldn't it be better to do the reset as part of the
CPU_PM_EXIT case below?  At this point nothing tells us the CPU won't
go back down before the event is enabled, wasting the cycle needed to
reset the PMU.

Thanks,
Mathieu

> +
> +       if (!enabled)
> +               return NOTIFY_OK;
> +
> +       switch (cmd) {
> +       case CPU_PM_ENTER:
> +               armpmu->stop(armpmu);
> +               cpu_pm_pmu_setup(armpmu, cmd);
> +               break;
> +       case CPU_PM_EXIT:
> +               cpu_pm_pmu_setup(armpmu, cmd);
> +       case CPU_PM_ENTER_FAILED:
> +               armpmu->start(armpmu);
> +               break;
> +       default:
> +               return NOTIFY_DONE;
> +       }
> +
> +       return NOTIFY_OK;
> +}
> +
> +static int cpu_pm_pmu_register(struct arm_pmu *cpu_pmu)
> +{
> +       cpu_pmu->cpu_pm_nb.notifier_call = cpu_pm_pmu_notify;
> +       return cpu_pm_register_notifier(&cpu_pmu->cpu_pm_nb);
> +}
> +
> +static void cpu_pm_pmu_unregister(struct arm_pmu *cpu_pmu)
> +{
> +       cpu_pm_unregister_notifier(&cpu_pmu->cpu_pm_nb);
> +}
> +#else
> +static inline int cpu_pm_pmu_register(struct arm_pmu *cpu_pmu) { return 0; }
> +static inline void cpu_pm_pmu_unregister(struct arm_pmu *cpu_pmu) { }
> +#endif
> +
>  static int cpu_pmu_init(struct arm_pmu *cpu_pmu)
>  {
>         int err;
> @@ -725,6 +813,10 @@ static int cpu_pmu_init(struct arm_pmu *cpu_pmu)
>         if (err)
>                 goto out_hw_events;
>
> +       err = cpu_pm_pmu_register(cpu_pmu);
> +       if (err)
> +               goto out_unregister;
> +
>         for_each_possible_cpu(cpu) {
>                 struct pmu_hw_events *events = per_cpu_ptr(cpu_hw_events, cpu);
>                 raw_spin_lock_init(&events->pmu_lock);
> @@ -746,6 +838,8 @@ static int cpu_pmu_init(struct arm_pmu *cpu_pmu)
>
>         return 0;
>
> +out_unregister:
> +       unregister_cpu_notifier(&cpu_pmu->hotplug_nb);
>  out_hw_events:
>         free_percpu(cpu_hw_events);
>         return err;
> @@ -753,6 +847,7 @@ out_hw_events:
>
>  static void cpu_pmu_destroy(struct arm_pmu *cpu_pmu)
>  {
> +       cpu_pm_pmu_unregister(cpu_pmu);
>         unregister_cpu_notifier(&cpu_pmu->hotplug_nb);
>         free_percpu(cpu_pmu->hw_events);
>  }
> diff --git a/include/linux/perf/arm_pmu.h b/include/linux/perf/arm_pmu.h
> index 83b5e34..9ffa316 100644
> --- a/include/linux/perf/arm_pmu.h
> +++ b/include/linux/perf/arm_pmu.h
> @@ -107,6 +107,7 @@ struct arm_pmu {
>         struct platform_device  *plat_device;
>         struct pmu_hw_events    __percpu *hw_events;
>         struct notifier_block   hotplug_nb;
> +       struct notifier_block   cpu_pm_nb;
>  };
>
>  #define to_arm_pmu(p) (container_of(p, struct arm_pmu, pmu))
> --
> 2.5.1
>

[PATCH] drivers: perf: arm: implement CPU_PM notifier

[Lorenzo Pieralisi]

On Wed, Feb 24, 2016 at 09:20:22AM -0700, Mathieu Poirier wrote:
> On 23 February 2016 at 11:22, Lorenzo Pieralisi

[...]

> > +static int cpu_pm_pmu_notify(struct notifier_block *b, unsigned long cmd,
> > +                            void *v)
> > +{
> > +       struct arm_pmu *armpmu = container_of(b, struct arm_pmu, cpu_pm_nb);
> > +       struct pmu_hw_events *hw_events = this_cpu_ptr(armpmu->hw_events);
> > +       int enabled = bitmap_weight(hw_events->used_mask, armpmu->num_events);
> > +
> > +       if (!cpumask_test_cpu(smp_processor_id(), &armpmu->supported_cpus))
> > +               return NOTIFY_DONE;
> > +
> > +       /*
> > +        * Always reset the PMU registers on power-up even if
> > +        * there are no events running.
> > +        */
> > +       if (cmd == CPU_PM_EXIT && armpmu->reset)
> > +               armpmu->reset(armpmu);
> 
> I think this patch does the right thing but I can't get the above
> reset.  Wouldn't it be better to do the reset as part of the
> CPU_PM_EXIT case below?  At this point nothing tells us the CPU won't
> go back down before the event is enabled, wasting the cycle needed to
> reset the PMU.

The logic goes, if the cpu is woken up and it has no events enabled,
if we do not reset it (mind, ->reset here sets the PMU register values
to a sane default, some of them are architecturally UNKNOWN on reset, it
does NOT reset the PMU) _and_ we subsequently install an event on it we
do have a problem, that's why whenever a core gets out of low-power we
have to reset its pmu.

Does it make sense ?

Thanks,
Lorenzo

> 
> Thanks,
> Mathieu
> 
> > +
> > +       if (!enabled)
> > +               return NOTIFY_OK;
> > +
> > +       switch (cmd) {
> > +       case CPU_PM_ENTER:
> > +               armpmu->stop(armpmu);
> > +               cpu_pm_pmu_setup(armpmu, cmd);
> > +               break;
> > +       case CPU_PM_EXIT:
> > +               cpu_pm_pmu_setup(armpmu, cmd);
> > +       case CPU_PM_ENTER_FAILED:
> > +               armpmu->start(armpmu);
> > +               break;
> > +       default:
> > +               return NOTIFY_DONE;
> > +       }
> > +
> > +       return NOTIFY_OK;
> > +}
> > +
> > +static int cpu_pm_pmu_register(struct arm_pmu *cpu_pmu)
> > +{
> > +       cpu_pmu->cpu_pm_nb.notifier_call = cpu_pm_pmu_notify;
> > +       return cpu_pm_register_notifier(&cpu_pmu->cpu_pm_nb);
> > +}
> > +
> > +static void cpu_pm_pmu_unregister(struct arm_pmu *cpu_pmu)
> > +{
> > +       cpu_pm_unregister_notifier(&cpu_pmu->cpu_pm_nb);
> > +}
> > +#else
> > +static inline int cpu_pm_pmu_register(struct arm_pmu *cpu_pmu) { return 0; }
> > +static inline void cpu_pm_pmu_unregister(struct arm_pmu *cpu_pmu) { }
> > +#endif
> > +
> >  static int cpu_pmu_init(struct arm_pmu *cpu_pmu)
> >  {
> >         int err;
> > @@ -725,6 +813,10 @@ static int cpu_pmu_init(struct arm_pmu *cpu_pmu)
> >         if (err)
> >                 goto out_hw_events;
> >
> > +       err = cpu_pm_pmu_register(cpu_pmu);
> > +       if (err)
> > +               goto out_unregister;
> > +
> >         for_each_possible_cpu(cpu) {
> >                 struct pmu_hw_events *events = per_cpu_ptr(cpu_hw_events, cpu);
> >                 raw_spin_lock_init(&events->pmu_lock);
> > @@ -746,6 +838,8 @@ static int cpu_pmu_init(struct arm_pmu *cpu_pmu)
> >
> >         return 0;
> >
> > +out_unregister:
> > +       unregister_cpu_notifier(&cpu_pmu->hotplug_nb);
> >  out_hw_events:
> >         free_percpu(cpu_hw_events);
> >         return err;
> > @@ -753,6 +847,7 @@ out_hw_events:
> >
> >  static void cpu_pmu_destroy(struct arm_pmu *cpu_pmu)
> >  {
> > +       cpu_pm_pmu_unregister(cpu_pmu);
> >         unregister_cpu_notifier(&cpu_pmu->hotplug_nb);
> >         free_percpu(cpu_pmu->hw_events);
> >  }
> > diff --git a/include/linux/perf/arm_pmu.h b/include/linux/perf/arm_pmu.h
> > index 83b5e34..9ffa316 100644
> > --- a/include/linux/perf/arm_pmu.h
> > +++ b/include/linux/perf/arm_pmu.h
> > @@ -107,6 +107,7 @@ struct arm_pmu {
> >         struct platform_device  *plat_device;
> >         struct pmu_hw_events    __percpu *hw_events;
> >         struct notifier_block   hotplug_nb;
> > +       struct notifier_block   cpu_pm_nb;
> >  };
> >
> >  #define to_arm_pmu(p) (container_of(p, struct arm_pmu, pmu))
> > --
> > 2.5.1
> >
> 

[PATCH] drivers: perf: arm: implement CPU_PM notifier

[Ashwin Chaugule]

Hi Lorenzo,

On 24 February 2016 at 12:35, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> On Wed, Feb 24, 2016 at 09:20:22AM -0700, Mathieu Poirier wrote:
>> On 23 February 2016 at 11:22, Lorenzo Pieralisi
>
> [...]
>
>> > +static int cpu_pm_pmu_notify(struct notifier_block *b, unsigned long cmd,
>> > +                            void *v)
>> > +{
>> > +       struct arm_pmu *armpmu = container_of(b, struct arm_pmu, cpu_pm_nb);
>> > +       struct pmu_hw_events *hw_events = this_cpu_ptr(armpmu->hw_events);
>> > +       int enabled = bitmap_weight(hw_events->used_mask, armpmu->num_events);
>> > +
>> > +       if (!cpumask_test_cpu(smp_processor_id(), &armpmu->supported_cpus))
>> > +               return NOTIFY_DONE;
>> > +
>> > +       /*
>> > +        * Always reset the PMU registers on power-up even if
>> > +        * there are no events running.
>> > +        */
>> > +       if (cmd == CPU_PM_EXIT && armpmu->reset)
>> > +               armpmu->reset(armpmu);
>>
>> I think this patch does the right thing but I can't get the above
>> reset.  Wouldn't it be better to do the reset as part of the
>> CPU_PM_EXIT case below?  At this point nothing tells us the CPU won't
>> go back down before the event is enabled, wasting the cycle needed to
>> reset the PMU.
>
> The logic goes, if the cpu is woken up and it has no events enabled,
> if we do not reset it (mind, ->reset here sets the PMU register values
> to a sane default, some of them are architecturally UNKNOWN on reset, it
> does NOT reset the PMU) _and_ we subsequently install an event on it we
> do have a problem, that's why whenever a core gets out of low-power we
> have to reset its pmu.

Dont we blow out the previous value in the counter when installing an
event? Or has that changed lately? IIRC, there was some initial value
we'd program into the counter to avoid missing the overflow event.
(sorry its been too long) ;)

Cheers,
Ashwin.

[PATCH] drivers: perf: arm: implement CPU_PM notifier

[Lorenzo Pieralisi]

On Wed, Feb 24, 2016 at 02:53:02PM -0500, Ashwin Chaugule wrote:
> Hi Lorenzo,
> 
> On 24 February 2016 at 12:35, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> > On Wed, Feb 24, 2016 at 09:20:22AM -0700, Mathieu Poirier wrote:
> >> On 23 February 2016 at 11:22, Lorenzo Pieralisi
> >
> > [...]
> >
> >> > +static int cpu_pm_pmu_notify(struct notifier_block *b, unsigned long cmd,
> >> > +                            void *v)
> >> > +{
> >> > +       struct arm_pmu *armpmu = container_of(b, struct arm_pmu, cpu_pm_nb);
> >> > +       struct pmu_hw_events *hw_events = this_cpu_ptr(armpmu->hw_events);
> >> > +       int enabled = bitmap_weight(hw_events->used_mask, armpmu->num_events);
> >> > +
> >> > +       if (!cpumask_test_cpu(smp_processor_id(), &armpmu->supported_cpus))
> >> > +               return NOTIFY_DONE;
> >> > +
> >> > +       /*
> >> > +        * Always reset the PMU registers on power-up even if
> >> > +        * there are no events running.
> >> > +        */
> >> > +       if (cmd == CPU_PM_EXIT && armpmu->reset)
> >> > +               armpmu->reset(armpmu);
> >>
> >> I think this patch does the right thing but I can't get the above
> >> reset.  Wouldn't it be better to do the reset as part of the
> >> CPU_PM_EXIT case below?  At this point nothing tells us the CPU won't
> >> go back down before the event is enabled, wasting the cycle needed to
> >> reset the PMU.
> >
> > The logic goes, if the cpu is woken up and it has no events enabled,
> > if we do not reset it (mind, ->reset here sets the PMU register values
> > to a sane default, some of them are architecturally UNKNOWN on reset, it
> > does NOT reset the PMU) _and_ we subsequently install an event on it we
> > do have a problem, that's why whenever a core gets out of low-power we
> > have to reset its pmu.
> 
> Dont we blow out the previous value in the counter when installing an
> event? Or has that changed lately? IIRC, there was some initial value
> we'd program into the counter to avoid missing the overflow event.
> (sorry its been too long) ;)

If you mean there is no need of resetting the value since we are
overwriting it anyway you should see ->reset from a PMU unit POW
not just an event. If you look at the ->reset method for eg v8, you
will see that the reset method operates on all counters and the PMU
unit as a whole, that's the only sane way of setting up the PMU unit
before enabling single counters (some registers are UNKNOWN at reset).

To make my reply to Mathieu clearer, the ->reset method contains
operations (eg writing PMCR counters reset bits) that do carry out
counters reset, what I wanted to say is that the ->reset method
does not by itself drive the PMU HW reset signal, that's what the
power controller does when it resets the CPU on power up.

The PMU ->reset method must be called on a cpu on power-up, to make
sure PMU HW is set-up to sane default values and ready to be used (ie
install counters), for instance on v8 all counters must be disabled
(irq inclusive) and reset, that's what armv8pmu_reset() does.

I hope this makes things clearer.

Thanks,
Lorenzo

[PATCH] drivers: perf: arm: implement CPU_PM notifier

[Kevin Hilman]

Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> writes:

> When a CPU is suspended (either through suspend-to-RAM or CPUidle),
> its PMU registers content can be lost, which means that counters
> registers values that were initialized on power down entry have to be
> reprogrammed on power-up to make sure the counters set-up is preserved

This assumes that the PMUs are always sharing a power rail with a
specific CPU, not the cluster.  Is that a reliable assumption?

Kevin

[PATCH] drivers: perf: arm: implement CPU_PM notifier

[Lorenzo Pieralisi]

On Wed, Feb 24, 2016 at 05:10:20PM -0800, Kevin Hilman wrote:
> Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> writes:
> 
> > When a CPU is suspended (either through suspend-to-RAM or CPUidle),
> > its PMU registers content can be lost, which means that counters
> > registers values that were initialized on power down entry have to be
> > reprogrammed on power-up to make sure the counters set-up is preserved
> 
> This assumes that the PMUs are always sharing a power rail with a
> specific CPU, not the cluster.  Is that a reliable assumption?

As reliable as assuming the GIC HW state needs save/restore on idle-state
entry, if we have no power domains information linked to CPU PM
notifiers saving/restoring everything every time an idle state deeper
than wfi is entered is the best we can do.

As far as this patch is concerned, if the PMU is on a different power
domain than the CPU, I agree that stopping/resetting/restoring the
PMU registers is a waste of cycles (I will test it also by triggering
the notifiers on wfi, to make sure the reset is not disruptive on
a cpu that is not powered off), I see no alternative other than disabling
idle to carry out profiling sanely (or implement CPU PM notifiers with
runtime PM).

Thanks,
Lorenzo

[PATCH] drivers: perf: arm: implement CPU_PM notifier

[Mathieu Poirier]

On 25 February 2016 at 02:44, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> On Wed, Feb 24, 2016 at 05:10:20PM -0800, Kevin Hilman wrote:
>> Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> writes:
>>
>> > When a CPU is suspended (either through suspend-to-RAM or CPUidle),
>> > its PMU registers content can be lost, which means that counters
>> > registers values that were initialized on power down entry have to be
>> > reprogrammed on power-up to make sure the counters set-up is preserved
>>
>> This assumes that the PMUs are always sharing a power rail with a
>> specific CPU, not the cluster.  Is that a reliable assumption?
>
> As reliable as assuming the GIC HW state needs save/restore on idle-state
> entry, if we have no power domains information linked to CPU PM
> notifiers saving/restoring everything every time an idle state deeper
> than wfi is entered is the best we can do.
>
> As far as this patch is concerned, if the PMU is on a different power
> domain than the CPU, I agree that stopping/resetting/restoring the
> PMU registers is a waste of cycles (I will test it also by triggering
> the notifiers on wfi, to make sure the reset is not disruptive on
> a cpu that is not powered off), I see no alternative other than disabling
> idle to carry out profiling sanely (or implement CPU PM notifiers with
> runtime PM).

I totally understand - I'm faced with the same problem on Coresight.
To me the ultimate solution is still to integrate CPU power domain
management with runtime PM (like we talked about many times before).
I know Lina is working on something that will get us close to that but
it's not finished yet.

>
> Thanks,
> Lorenzo

[PATCH] drivers: perf: arm: implement CPU_PM notifier

[Mathieu Poirier]

On 24 February 2016 at 10:35, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> On Wed, Feb 24, 2016 at 09:20:22AM -0700, Mathieu Poirier wrote:
>> On 23 February 2016 at 11:22, Lorenzo Pieralisi
>
> [...]
>
>> > +static int cpu_pm_pmu_notify(struct notifier_block *b, unsigned long cmd,
>> > +                            void *v)
>> > +{
>> > +       struct arm_pmu *armpmu = container_of(b, struct arm_pmu, cpu_pm_nb);
>> > +       struct pmu_hw_events *hw_events = this_cpu_ptr(armpmu->hw_events);
>> > +       int enabled = bitmap_weight(hw_events->used_mask, armpmu->num_events);
>> > +
>> > +       if (!cpumask_test_cpu(smp_processor_id(), &armpmu->supported_cpus))
>> > +               return NOTIFY_DONE;
>> > +
>> > +       /*
>> > +        * Always reset the PMU registers on power-up even if
>> > +        * there are no events running.
>> > +        */
>> > +       if (cmd == CPU_PM_EXIT && armpmu->reset)
>> > +               armpmu->reset(armpmu);
>>
>> I think this patch does the right thing but I can't get the above
>> reset.  Wouldn't it be better to do the reset as part of the
>> CPU_PM_EXIT case below?  At this point nothing tells us the CPU won't
>> go back down before the event is enabled, wasting the cycle needed to
>> reset the PMU.
>
> The logic goes, if the cpu is woken up and it has no events enabled,
> if we do not reset it (mind, ->reset here sets the PMU register values
> to a sane default, some of them are architecturally UNKNOWN on reset, it
> does NOT reset the PMU) _and_ we subsequently install an event on it we
> do have a problem, that's why whenever a core gets out of low-power we
> have to reset its pmu.
>
> Does it make sense ?

Ok, I understand the context and your solution will work.

Isn't there a flag somewhere in the PMU registers that indicate the
status has been lost due a loss of power?  If so I think the best
solution is to probe that flag when an event gets installed.  If the
unit has gone through a power down the reset can be done before
installing the event.

Mathieu

>
> Thanks,
> Lorenzo
>
>>
>> Thanks,
>> Mathieu
>>
>> > +
>> > +       if (!enabled)
>> > +               return NOTIFY_OK;
>> > +
>> > +       switch (cmd) {
>> > +       case CPU_PM_ENTER:
>> > +               armpmu->stop(armpmu);
>> > +               cpu_pm_pmu_setup(armpmu, cmd);
>> > +               break;
>> > +       case CPU_PM_EXIT:
>> > +               cpu_pm_pmu_setup(armpmu, cmd);
>> > +       case CPU_PM_ENTER_FAILED:
>> > +               armpmu->start(armpmu);
>> > +               break;
>> > +       default:
>> > +               return NOTIFY_DONE;
>> > +       }
>> > +
>> > +       return NOTIFY_OK;
>> > +}
>> > +
>> > +static int cpu_pm_pmu_register(struct arm_pmu *cpu_pmu)
>> > +{
>> > +       cpu_pmu->cpu_pm_nb.notifier_call = cpu_pm_pmu_notify;
>> > +       return cpu_pm_register_notifier(&cpu_pmu->cpu_pm_nb);
>> > +}
>> > +
>> > +static void cpu_pm_pmu_unregister(struct arm_pmu *cpu_pmu)
>> > +{
>> > +       cpu_pm_unregister_notifier(&cpu_pmu->cpu_pm_nb);
>> > +}
>> > +#else
>> > +static inline int cpu_pm_pmu_register(struct arm_pmu *cpu_pmu) { return 0; }
>> > +static inline void cpu_pm_pmu_unregister(struct arm_pmu *cpu_pmu) { }
>> > +#endif
>> > +
>> >  static int cpu_pmu_init(struct arm_pmu *cpu_pmu)
>> >  {
>> >         int err;
>> > @@ -725,6 +813,10 @@ static int cpu_pmu_init(struct arm_pmu *cpu_pmu)
>> >         if (err)
>> >                 goto out_hw_events;
>> >
>> > +       err = cpu_pm_pmu_register(cpu_pmu);
>> > +       if (err)
>> > +               goto out_unregister;
>> > +
>> >         for_each_possible_cpu(cpu) {
>> >                 struct pmu_hw_events *events = per_cpu_ptr(cpu_hw_events, cpu);
>> >                 raw_spin_lock_init(&events->pmu_lock);
>> > @@ -746,6 +838,8 @@ static int cpu_pmu_init(struct arm_pmu *cpu_pmu)
>> >
>> >         return 0;
>> >
>> > +out_unregister:
>> > +       unregister_cpu_notifier(&cpu_pmu->hotplug_nb);
>> >  out_hw_events:
>> >         free_percpu(cpu_hw_events);
>> >         return err;
>> > @@ -753,6 +847,7 @@ out_hw_events:
>> >
>> >  static void cpu_pmu_destroy(struct arm_pmu *cpu_pmu)
>> >  {
>> > +       cpu_pm_pmu_unregister(cpu_pmu);
>> >         unregister_cpu_notifier(&cpu_pmu->hotplug_nb);
>> >         free_percpu(cpu_pmu->hw_events);
>> >  }
>> > diff --git a/include/linux/perf/arm_pmu.h b/include/linux/perf/arm_pmu.h
>> > index 83b5e34..9ffa316 100644
>> > --- a/include/linux/perf/arm_pmu.h
>> > +++ b/include/linux/perf/arm_pmu.h
>> > @@ -107,6 +107,7 @@ struct arm_pmu {
>> >         struct platform_device  *plat_device;
>> >         struct pmu_hw_events    __percpu *hw_events;
>> >         struct notifier_block   hotplug_nb;
>> > +       struct notifier_block   cpu_pm_nb;
>> >  };
>> >
>> >  #define to_arm_pmu(p) (container_of(p, struct arm_pmu, pmu))
>> > --
>> > 2.5.1
>> >
>>

[PATCH] drivers: perf: arm: implement CPU_PM notifier

[Will Deacon]

On Thu, Feb 25, 2016 at 09:32:17AM -0700, Mathieu Poirier wrote:
> On 25 February 2016 at 02:44, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> > On Wed, Feb 24, 2016 at 05:10:20PM -0800, Kevin Hilman wrote:
> >> Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> writes:
> >>
> >> > When a CPU is suspended (either through suspend-to-RAM or CPUidle),
> >> > its PMU registers content can be lost, which means that counters
> >> > registers values that were initialized on power down entry have to be
> >> > reprogrammed on power-up to make sure the counters set-up is preserved
> >>
> >> This assumes that the PMUs are always sharing a power rail with a
> >> specific CPU, not the cluster.  Is that a reliable assumption?
> >
> > As reliable as assuming the GIC HW state needs save/restore on idle-state
> > entry, if we have no power domains information linked to CPU PM
> > notifiers saving/restoring everything every time an idle state deeper
> > than wfi is entered is the best we can do.
> >
> > As far as this patch is concerned, if the PMU is on a different power
> > domain than the CPU, I agree that stopping/resetting/restoring the
> > PMU registers is a waste of cycles (I will test it also by triggering
> > the notifiers on wfi, to make sure the reset is not disruptive on
> > a cpu that is not powered off), I see no alternative other than disabling
> > idle to carry out profiling sanely (or implement CPU PM notifiers with
> > runtime PM).
> 
> I totally understand - I'm faced with the same problem on Coresight.
> To me the ultimate solution is still to integrate CPU power domain
> management with runtime PM (like we talked about many times before).
> I know Lina is working on something that will get us close to that but
> it's not finished yet.

I've been waiting for that code for *years* now...

Ultimately, Juno-r2 loses its PMU state over idle if you run mainline
on it and, worse still, you end up getting junk numbers back to userspace,
so it's not even obvious what happened.

I'm not aware of any flags that indicate loss of state.

Will

[PATCH] drivers: perf: arm: implement CPU_PM notifier

[Lorenzo Pieralisi]

On Thu, Feb 25, 2016 at 04:43:54PM +0000, Will Deacon wrote:
> On Thu, Feb 25, 2016 at 09:32:17AM -0700, Mathieu Poirier wrote:
> > On 25 February 2016 at 02:44, Lorenzo Pieralisi
> > <lorenzo.pieralisi@arm.com> wrote:
> > > On Wed, Feb 24, 2016 at 05:10:20PM -0800, Kevin Hilman wrote:
> > >> Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> writes:
> > >>
> > >> > When a CPU is suspended (either through suspend-to-RAM or CPUidle),
> > >> > its PMU registers content can be lost, which means that counters
> > >> > registers values that were initialized on power down entry have to be
> > >> > reprogrammed on power-up to make sure the counters set-up is preserved
> > >>
> > >> This assumes that the PMUs are always sharing a power rail with a
> > >> specific CPU, not the cluster.  Is that a reliable assumption?
> > >
> > > As reliable as assuming the GIC HW state needs save/restore on idle-state
> > > entry, if we have no power domains information linked to CPU PM
> > > notifiers saving/restoring everything every time an idle state deeper
> > > than wfi is entered is the best we can do.
> > >
> > > As far as this patch is concerned, if the PMU is on a different power
> > > domain than the CPU, I agree that stopping/resetting/restoring the
> > > PMU registers is a waste of cycles (I will test it also by triggering
> > > the notifiers on wfi, to make sure the reset is not disruptive on
> > > a cpu that is not powered off), I see no alternative other than disabling
> > > idle to carry out profiling sanely (or implement CPU PM notifiers with
> > > runtime PM).
> > 
> > I totally understand - I'm faced with the same problem on Coresight.
> > To me the ultimate solution is still to integrate CPU power domain
> > management with runtime PM (like we talked about many times before).
> > I know Lina is working on something that will get us close to that but
> > it's not finished yet.
> 
> I've been waiting for that code for *years* now...
> 
> Ultimately, Juno-r2 loses its PMU state over idle if you run mainline
> on it and, worse still, you end up getting junk numbers back to userspace,
> so it's not even obvious what happened.

I hacked the idle driver to trigger notifiers on wfi (so PMU state is
kept intact on idle exit - to mimic idle states that do not destroy PMU
context) and still get some reasonable perf stats, this needs more testing
but it means that save/restore still work on idle states that do not destroy
PMU context (ok, we add a profile black out period between CPU PM entry and
exit, that can't be avoided).

> I'm not aware of any flags that indicate loss of state.

kvm notifier had to add a check (in hyp_init_cpu_pm_notifier()), since
restoring KVM hooks if the cpu was not reset triggers kernel errors,
point is, we need to have a hook (this is generic arm perf core, it has
to work on all arm pmus) that allows us detecting if a reset happened,
question is if we should bother (and if it is doable), given the test
above, I am happy to implement it.

I definitely agree it is time to fix this, it has been hanging in
the balance for too long.

Lorenzo

[PATCH] drivers: perf: arm: implement CPU_PM notifier

[Kevin Hilman]

Will Deacon <will.deacon@arm.com> writes:

> On Thu, Feb 25, 2016 at 09:32:17AM -0700, Mathieu Poirier wrote:
>> On 25 February 2016 at 02:44, Lorenzo Pieralisi
>> <lorenzo.pieralisi@arm.com> wrote:
>> > On Wed, Feb 24, 2016 at 05:10:20PM -0800, Kevin Hilman wrote:
>> >> Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> writes:
>> >>
>> >> > When a CPU is suspended (either through suspend-to-RAM or CPUidle),
>> >> > its PMU registers content can be lost, which means that counters
>> >> > registers values that were initialized on power down entry have to be
>> >> > reprogrammed on power-up to make sure the counters set-up is preserved
>> >>
>> >> This assumes that the PMUs are always sharing a power rail with a
>> >> specific CPU, not the cluster.  Is that a reliable assumption?
>> >
>> > As reliable as assuming the GIC HW state needs save/restore on idle-state
>> > entry, if we have no power domains information linked to CPU PM
>> > notifiers saving/restoring everything every time an idle state deeper
>> > than wfi is entered is the best we can do.
>> >
>> > As far as this patch is concerned, if the PMU is on a different power
>> > domain than the CPU, I agree that stopping/resetting/restoring the
>> > PMU registers is a waste of cycles (I will test it also by triggering
>> > the notifiers on wfi, to make sure the reset is not disruptive on
>> > a cpu that is not powered off), I see no alternative other than disabling
>> > idle to carry out profiling sanely (or implement CPU PM notifiers with
>> > runtime PM).
>> 
>> I totally understand - I'm faced with the same problem on Coresight.
>> To me the ultimate solution is still to integrate CPU power domain
>> management with runtime PM (like we talked about many times before).
>> I know Lina is working on something that will get us close to that but
>> it's not finished yet.
>
> I've been waiting for that code for *years* now...

It's still coming... ;)

But, I didn't mean to suggest this patch should wait for a PM domain
based solution, I just wanted to be clear about the assumptions.  I have
no objections to this patch as it fixes a long-standing issue.

Acked-by: Kevin Hilman <khilman@baylibre.com>

Kevin

[PATCH] drivers: perf: arm: implement CPU_PM notifier

[Ashwin Chaugule]

On 24 February 2016 at 17:31, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> On Wed, Feb 24, 2016 at 02:53:02PM -0500, Ashwin Chaugule wrote:
>> Hi Lorenzo,
>>
>> On 24 February 2016 at 12:35, Lorenzo Pieralisi
>> <lorenzo.pieralisi@arm.com> wrote:
>> > On Wed, Feb 24, 2016 at 09:20:22AM -0700, Mathieu Poirier wrote:
>> >> On 23 February 2016 at 11:22, Lorenzo Pieralisi
>> >
>> > [...]
>> >
>> >> > +static int cpu_pm_pmu_notify(struct notifier_block *b, unsigned long cmd,
>> >> > +                            void *v)
>> >> > +{
>> >> > +       struct arm_pmu *armpmu = container_of(b, struct arm_pmu, cpu_pm_nb);
>> >> > +       struct pmu_hw_events *hw_events = this_cpu_ptr(armpmu->hw_events);
>> >> > +       int enabled = bitmap_weight(hw_events->used_mask, armpmu->num_events);
>> >> > +
>> >> > +       if (!cpumask_test_cpu(smp_processor_id(), &armpmu->supported_cpus))
>> >> > +               return NOTIFY_DONE;
>> >> > +
>> >> > +       /*
>> >> > +        * Always reset the PMU registers on power-up even if
>> >> > +        * there are no events running.
>> >> > +        */
>> >> > +       if (cmd == CPU_PM_EXIT && armpmu->reset)
>> >> > +               armpmu->reset(armpmu);
>> >>
>> >> I think this patch does the right thing but I can't get the above
>> >> reset.  Wouldn't it be better to do the reset as part of the
>> >> CPU_PM_EXIT case below?  At this point nothing tells us the CPU won't
>> >> go back down before the event is enabled, wasting the cycle needed to
>> >> reset the PMU.
>> >
>> > The logic goes, if the cpu is woken up and it has no events enabled,
>> > if we do not reset it (mind, ->reset here sets the PMU register values
>> > to a sane default, some of them are architecturally UNKNOWN on reset, it
>> > does NOT reset the PMU) _and_ we subsequently install an event on it we
>> > do have a problem, that's why whenever a core gets out of low-power we
>> > have to reset its pmu.
>>
>> Dont we blow out the previous value in the counter when installing an
>> event? Or has that changed lately? IIRC, there was some initial value
>> we'd program into the counter to avoid missing the overflow event.
>> (sorry its been too long) ;)
>
> If you mean there is no need of resetting the value since we are
> overwriting it anyway you should see ->reset from a PMU unit POW
> not just an event. If you look at the ->reset method for eg v8, you
> will see that the reset method operates on all counters and the PMU
> unit as a whole, that's the only sane way of setting up the PMU unit
> before enabling single counters (some registers are UNKNOWN at reset).
>
> To make my reply to Mathieu clearer, the ->reset method contains
> operations (eg writing PMCR counters reset bits) that do carry out
> counters reset, what I wanted to say is that the ->reset method
> does not by itself drive the PMU HW reset signal, that's what the
> power controller does when it resets the CPU on power up.
>
> The PMU ->reset method must be called on a cpu on power-up, to make
> sure PMU HW is set-up to sane default values and ready to be used (ie
> install counters), for instance on v8 all counters must be disabled
> (irq inclusive) and reset, that's what armv8pmu_reset() does.
>
> I hope this makes things clearer.
>

Indeed. Thanks for the explanation. The patch looks good to me.

Acked-by: Ashwin Chaugule <ashwin.chaugule@linaro.org>


> Thanks,
> Lorenzo