RE: [PATCH v2] ARM: perf: save/restore pmu registers in pm notifier
Hi Sudeep,
> -Original Message-
> From: Sudeep Holla [mailto:sudeep.ho...@arm.com]
> Sent: 2014年4月15日 21:06
> To: Neil Zhang
> Cc: Will Deacon; li...@arm.linux.org.uk; Sudeep Holla;
> linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2] ARM: perf: save/restore pmu registers in pm notifier
>
> Hi Will,
>
> Thanks for the response.
>
> Hi Neil,
>
> On 14/04/14 02:42, Neil Zhang wrote:
> > From: Sudeep KarkadaNagesha
> >
> > This adds core support for saving and restoring CPU PMU registers for
> > suspend/resume support i.e. deeper C-states in cpuidle terms.
> > This patch adds support only to ARMv7 PMU registers save/restore.
> > It needs to be extended to xscale and ARMv6 if needed.
> >
> > [Neil] We found that DS-5 not work on our CA7 based SoCs.
> > After debuging, found PMU registers were lost because of core power down.
> > Then i found Sudeep had a patch to fix it about two years ago but not
> > in the mainline, just port it.
> >
>
> The patch was written as PoC to support multiple PMUs back then and was
> clearly not intended for upstream. Will has already mentioned what this patch
> needs to improve on.
Thanks for the comments.
I think it can help others if it can be accepted by mainline.
>
> I have mentioned few comments which IMO is necessary.
>
> > Signed-off-by: Sudeep KarkadaNagesha
>
> > Signed-off-by: Neil Zhang
> > ---
> > arch/arm/include/asm/pmu.h | 11 +
> > arch/arm/kernel/perf_event_cpu.c | 29 ++-
> > arch/arm/kernel/perf_event_v7.c | 47
> ++
> > 3 files changed, 86 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm/include/asm/pmu.h b/arch/arm/include/asm/pmu.h
> > index ae1919b..f37f048 100644
> > --- a/arch/arm/include/asm/pmu.h
> > +++ b/arch/arm/include/asm/pmu.h
> > @@ -62,6 +62,15 @@ struct pmu_hw_events {
> > raw_spinlock_t pmu_lock;
> > };
> >
> > +struct cpupmu_regs {
> > + u32 pmc;
> > + u32 pmcntenset;
> > + u32 pmuseren;
> > + u32 pmintenset;
> > + u32 pmxevttype[8];
> > + u32 pmxevtcnt[8];
> > +};
> > +
>
> This can't be generic, it needs to be specific to each PMU implementations.
> I have clearly mentioned it in the commit log. It's better to move this out to
> each PMU specific implementations.
>
> > struct arm_pmu {
> > struct pmu pmu;
> > cpumask_t active_irqs;
> > @@ -83,6 +92,8 @@ struct arm_pmu {
> > int (*request_irq)(struct arm_pmu *, irq_handler_t handler);
> > void(*free_irq)(struct arm_pmu *);
> > int (*map_event)(struct perf_event *event);
> > + void(*save_regs)(struct arm_pmu *, struct cpupmu_regs *);
> > + void(*restore_regs)(struct arm_pmu *, struct cpupmu_regs *);
>
> Once cpupmu_regs becomes PMU specific, you can remove it from the above
> 2 function pointers.
>
> > int num_events;
> > atomic_tactive_events;
> > struct mutexreserve_mutex;
> > diff --git a/arch/arm/kernel/perf_event_cpu.c
> > b/arch/arm/kernel/perf_event_cpu.c
> > index 51798d7..7f1c756 100644
> > --- a/arch/arm/kernel/perf_event_cpu.c
> > +++ b/arch/arm/kernel/perf_event_cpu.c
> > @@ -19,6 +19,7 @@
> > #define pr_fmt(fmt) "CPU PMU: " fmt
> >
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -39,6 +40,7 @@ static DEFINE_PER_CPU(struct arm_pmu *,
> percpu_pmu);
> > static DEFINE_PER_CPU(struct perf_event * [ARMPMU_MAX_HWEVENTS],
> > hw_events); static DEFINE_PER_CPU(unsigned long
> > [BITS_TO_LONGS(ARMPMU_MAX_HWEVENTS)], used_mask); static
> > DEFINE_PER_CPU(struct pmu_hw_events, cpu_hw_events);
> > +static DEFINE_PER_CPU(struct cpupmu_regs, cpu_pmu_regs);
> >
> > /*
> > * Despite the names, these two functions are CPU-specific and are
> > used @@ -217,6 +219,23 @@ static struct notifier_block
> cpu_pmu_hotplug_notifier = {
> > .notifier_call = cpu_pmu_notify,
> > };
> >
> > +static int cpu_pmu_pm_notify(struct notifier_block *b,
> > + unsigned long action, void *hcpu)
>
> hcpu is misleading, the cpu_pm_notify doesn't pass the logical cpu index.
> IIRC its NULL, rename it.
Thanks for the detailed suggestions, I will refine it later.
>
> > +{
> > + struct cpupmu_regs *pmuregs = this_cpu_ptr(
Re: [PATCH v2] ARM: perf: save/restore pmu registers in pm notifier
Hi Will,
Thanks for the response.
Hi Neil,
On 14/04/14 02:42, Neil Zhang wrote:
> From: Sudeep KarkadaNagesha
>
> This adds core support for saving and restoring CPU PMU registers
> for suspend/resume support i.e. deeper C-states in cpuidle terms.
> This patch adds support only to ARMv7 PMU registers save/restore.
> It needs to be extended to xscale and ARMv6 if needed.
>
> [Neil] We found that DS-5 not work on our CA7 based SoCs.
> After debuging, found PMU registers were lost because of core power down.
> Then i found Sudeep had a patch to fix it about two years ago but not in
> the mainline, just port it.
>
The patch was written as PoC to support multiple PMUs back then and was clearly
not intended for upstream. Will has already mentioned what this patch needs to
improve on.
I have mentioned few comments which IMO is necessary.
> Signed-off-by: Sudeep KarkadaNagesha
> Signed-off-by: Neil Zhang
> ---
> arch/arm/include/asm/pmu.h | 11 +
> arch/arm/kernel/perf_event_cpu.c | 29 ++-
> arch/arm/kernel/perf_event_v7.c | 47
> ++
> 3 files changed, 86 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/include/asm/pmu.h b/arch/arm/include/asm/pmu.h
> index ae1919b..f37f048 100644
> --- a/arch/arm/include/asm/pmu.h
> +++ b/arch/arm/include/asm/pmu.h
> @@ -62,6 +62,15 @@ struct pmu_hw_events {
> raw_spinlock_t pmu_lock;
> };
>
> +struct cpupmu_regs {
> + u32 pmc;
> + u32 pmcntenset;
> + u32 pmuseren;
> + u32 pmintenset;
> + u32 pmxevttype[8];
> + u32 pmxevtcnt[8];
> +};
> +
This can't be generic, it needs to be specific to each PMU implementations.
I have clearly mentioned it in the commit log. It's better to move this out
to each PMU specific implementations.
> struct arm_pmu {
> struct pmu pmu;
> cpumask_t active_irqs;
> @@ -83,6 +92,8 @@ struct arm_pmu {
> int (*request_irq)(struct arm_pmu *, irq_handler_t handler);
> void(*free_irq)(struct arm_pmu *);
> int (*map_event)(struct perf_event *event);
> + void(*save_regs)(struct arm_pmu *, struct cpupmu_regs *);
> + void(*restore_regs)(struct arm_pmu *, struct cpupmu_regs *);
Once cpupmu_regs becomes PMU specific, you can remove it from the above 2
function pointers.
> int num_events;
> atomic_tactive_events;
> struct mutexreserve_mutex;
> diff --git a/arch/arm/kernel/perf_event_cpu.c
> b/arch/arm/kernel/perf_event_cpu.c
> index 51798d7..7f1c756 100644
> --- a/arch/arm/kernel/perf_event_cpu.c
> +++ b/arch/arm/kernel/perf_event_cpu.c
> @@ -19,6 +19,7 @@
> #define pr_fmt(fmt) "CPU PMU: " fmt
>
> #include
> +#include
> #include
> #include
> #include
> @@ -39,6 +40,7 @@ static DEFINE_PER_CPU(struct arm_pmu *, percpu_pmu);
> static DEFINE_PER_CPU(struct perf_event * [ARMPMU_MAX_HWEVENTS], hw_events);
> static DEFINE_PER_CPU(unsigned long [BITS_TO_LONGS(ARMPMU_MAX_HWEVENTS)],
> used_mask);
> static DEFINE_PER_CPU(struct pmu_hw_events, cpu_hw_events);
> +static DEFINE_PER_CPU(struct cpupmu_regs, cpu_pmu_regs);
>
> /*
> * Despite the names, these two functions are CPU-specific and are used
> @@ -217,6 +219,23 @@ static struct notifier_block cpu_pmu_hotplug_notifier = {
> .notifier_call = cpu_pmu_notify,
> };
>
> +static int cpu_pmu_pm_notify(struct notifier_block *b,
> + unsigned long action, void *hcpu)
hcpu is misleading, the cpu_pm_notify doesn't pass the logical cpu index.
IIRC its NULL, rename it.
> +{
> + struct cpupmu_regs *pmuregs = this_cpu_ptr(&cpu_pmu_regs);
> +
> + if (action == CPU_PM_ENTER && cpu_pmu->save_regs)
> + cpu_pmu->save_regs(cpu_pmu, pmuregs);
> + else if (action == CPU_PM_EXIT && cpu_pmu->restore_regs)
> + cpu_pmu->restore_regs(cpu_pmu, pmuregs);
> +
> + return NOTIFY_OK;
> +}
> +
> +static struct notifier_block cpu_pmu_pm_notifier = {
> + .notifier_call = cpu_pmu_pm_notify,
> +};
> +
> /*
> * PMU platform driver and devicetree bindings.
> */
> @@ -349,9 +368,17 @@ static int __init register_pmu_driver(void)
> if (err)
> return err;
>
> + err = cpu_pm_register_notifier(&cpu_pmu_pm_notifier);
> + if (err) {
> + unregister_cpu_notifier(&cpu_pmu_hotplug_notifier);
> + return err;
> + }
> +
> err = platform_driver_register(&cpu_pmu_driver);
> - if (err)
> + if (err) {
> + cpu_pm_unregister_notifier(&cpu_pmu_pm_notifier);
> unregister_cpu_notifier(&cpu_pmu_hotplug_notifier);
> + }
>
> return err;
> }
> diff --git a/arch/arm/kernel/perf_event_v7.c b/arch/arm/kernel/perf_event_v7.c
> index f4ef398..29ae8f1 100644
> --- a/arch/arm/kernel/perf_event_v7.c
> +++ b/arch/arm/kernel/perf_event_v7.c
> @@ -1237,6 +1237,51 @@ static void armv7_pmnc_d
RE: [PATCH v2] ARM: perf: save/restore pmu registers in pm notifier
> -Original Message- > From: Will Deacon [mailto:will.dea...@arm.com] > Sent: 2014年4月15日 20:50 > To: Neil Zhang > Cc: li...@arm.linux.org.uk; linux-arm-ker...@lists.infradead.org; > linux-kernel@vger.kernel.org; Sudeep Holla > Subject: Re: [PATCH v2] ARM: perf: save/restore pmu registers in pm notifier > > On Tue, Apr 15, 2014 at 01:46:08PM +0100, Neil Zhang wrote: > > > On Tue, Apr 15, 2014 at 01:37:17PM +0100, Neil Zhang wrote: > > > > > On Mon, Apr 14, 2014 at 02:42:22AM +0100, Neil Zhang wrote: > > > > > > From: Sudeep KarkadaNagesha > > > > > > > > > > > > This adds core support for saving and restoring CPU PMU > > > > > > registers for suspend/resume support i.e. deeper C-states in cpuidle > terms. > > > > > > This patch adds support only to ARMv7 PMU registers save/restore. > > > > > > It needs to be extended to xscale and ARMv6 if needed. > > > > > > > > > > > > [Neil] We found that DS-5 not work on our CA7 based SoCs. > > > > > > After debuging, found PMU registers were lost because of core > > > > > > power > > > down. > > > > > > Then i found Sudeep had a patch to fix it about two years ago > > > > > > but not in the mainline, just port it. > > > > > > > > > > What I don't like about this patch is that we're introducing > > > > > significant overhead for SoCs that don't require save/restore of > > > > > the PMU state. I'd much rather see core power down disabled > > > > > whilst the PMU is in use but, if that's not possible, then I think we > > > > > need > to: > > > > > > > > > > (1) Make this conditional for cores that really need it > > > > > > > > > > (2) Only save/restore if the PMU is in use (even better, just > save/restore > > > > > the live registers, but that's probably not worth the effort > > > > > initially). > > > > > > > > > > > > > The patch has check the ARMV7_PMNC_E bit when save / restore, so > > > > suppose only the core's that use PMU will do the save / restore work. > > > > > > Seems pretty fragile to base our save/restore decision on the value > > > of one of the registers, though. What happens if the control > > > register is zeroed by the core power down? > > > > > It will check the saved control value when restore, so is should be OK > > while control register is zeroed. > > Ah yes, I mixed up and save and restore functions. It's still horrible that we > have to read the control register unconditionally during the save though > -- it might be nicer if we simply register/unregister the notifier during > perf runs > (in the same way that we request/free the PMU IRQs). > Thanks for the comments, I will refine it according to your suggestion. > Will Best Regards, Neil Zhang
Re: [PATCH v2] ARM: perf: save/restore pmu registers in pm notifier
On Tue, Apr 15, 2014 at 01:46:08PM +0100, Neil Zhang wrote: > > On Tue, Apr 15, 2014 at 01:37:17PM +0100, Neil Zhang wrote: > > > > On Mon, Apr 14, 2014 at 02:42:22AM +0100, Neil Zhang wrote: > > > > > From: Sudeep KarkadaNagesha > > > > > > > > > > This adds core support for saving and restoring CPU PMU registers > > > > > for suspend/resume support i.e. deeper C-states in cpuidle terms. > > > > > This patch adds support only to ARMv7 PMU registers save/restore. > > > > > It needs to be extended to xscale and ARMv6 if needed. > > > > > > > > > > [Neil] We found that DS-5 not work on our CA7 based SoCs. > > > > > After debuging, found PMU registers were lost because of core power > > down. > > > > > Then i found Sudeep had a patch to fix it about two years ago but > > > > > not in the mainline, just port it. > > > > > > > > What I don't like about this patch is that we're introducing > > > > significant overhead for SoCs that don't require save/restore of the > > > > PMU state. I'd much rather see core power down disabled whilst the > > > > PMU is in use but, if that's not possible, then I think we need to: > > > > > > > > (1) Make this conditional for cores that really need it > > > > > > > > (2) Only save/restore if the PMU is in use (even better, just > > > > save/restore > > > > the live registers, but that's probably not worth the effort > > > > initially). > > > > > > > > > > The patch has check the ARMV7_PMNC_E bit when save / restore, so > > > suppose only the core's that use PMU will do the save / restore work. > > > > Seems pretty fragile to base our save/restore decision on the value of one > > of > > the registers, though. What happens if the control register is zeroed by the > > core power down? > > > It will check the saved control value when restore, so is should be OK > while control register is zeroed. Ah yes, I mixed up and save and restore functions. It's still horrible that we have to read the control register unconditionally during the save though -- it might be nicer if we simply register/unregister the notifier during perf runs (in the same way that we request/free the PMU IRQs). Will -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v2] ARM: perf: save/restore pmu registers in pm notifier
> -Original Message- > From: Will Deacon [mailto:will.dea...@arm.com] > Sent: 2014年4月15日 20:41 > To: Neil Zhang > Cc: li...@arm.linux.org.uk; linux-arm-ker...@lists.infradead.org; > linux-kernel@vger.kernel.org; Sudeep Holla > Subject: Re: [PATCH v2] ARM: perf: save/restore pmu registers in pm notifier > > On Tue, Apr 15, 2014 at 01:37:17PM +0100, Neil Zhang wrote: > > > On Mon, Apr 14, 2014 at 02:42:22AM +0100, Neil Zhang wrote: > > > > From: Sudeep KarkadaNagesha > > > > > > > > This adds core support for saving and restoring CPU PMU registers > > > > for suspend/resume support i.e. deeper C-states in cpuidle terms. > > > > This patch adds support only to ARMv7 PMU registers save/restore. > > > > It needs to be extended to xscale and ARMv6 if needed. > > > > > > > > [Neil] We found that DS-5 not work on our CA7 based SoCs. > > > > After debuging, found PMU registers were lost because of core power > down. > > > > Then i found Sudeep had a patch to fix it about two years ago but > > > > not in the mainline, just port it. > > > > > > What I don't like about this patch is that we're introducing > > > significant overhead for SoCs that don't require save/restore of the > > > PMU state. I'd much rather see core power down disabled whilst the > > > PMU is in use but, if that's not possible, then I think we need to: > > > > > > (1) Make this conditional for cores that really need it > > > > > > (2) Only save/restore if the PMU is in use (even better, just > > > save/restore > > > the live registers, but that's probably not worth the effort > > > initially). > > > > > > > The patch has check the ARMV7_PMNC_E bit when save / restore, so > > suppose only the core's that use PMU will do the save / restore work. > > Seems pretty fragile to base our save/restore decision on the value of one of > the registers, though. What happens if the control register is zeroed by the > core power down? > It will check the saved control value when restore, so is should be OK while control register is zeroed. > Will Best Regards, Neil Zhang
Re: [PATCH v2] ARM: perf: save/restore pmu registers in pm notifier
On Tue, Apr 15, 2014 at 01:37:17PM +0100, Neil Zhang wrote: > > On Mon, Apr 14, 2014 at 02:42:22AM +0100, Neil Zhang wrote: > > > From: Sudeep KarkadaNagesha > > > > > > This adds core support for saving and restoring CPU PMU registers for > > > suspend/resume support i.e. deeper C-states in cpuidle terms. > > > This patch adds support only to ARMv7 PMU registers save/restore. > > > It needs to be extended to xscale and ARMv6 if needed. > > > > > > [Neil] We found that DS-5 not work on our CA7 based SoCs. > > > After debuging, found PMU registers were lost because of core power down. > > > Then i found Sudeep had a patch to fix it about two years ago but not > > > in the mainline, just port it. > > > > What I don't like about this patch is that we're introducing significant > > overhead for SoCs that don't require save/restore of the PMU state. I'd much > > rather see core power down disabled whilst the PMU is in use but, if that's > > not > > possible, then I think we need to: > > > > (1) Make this conditional for cores that really need it > > > > (2) Only save/restore if the PMU is in use (even better, just save/restore > > the live registers, but that's probably not worth the effort > > initially). > > > > The patch has check the ARMV7_PMNC_E bit when save / restore, > so suppose only the core's that use PMU will do the save / restore work. Seems pretty fragile to base our save/restore decision on the value of one of the registers, though. What happens if the control register is zeroed by the core power down? Will -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v2] ARM: perf: save/restore pmu registers in pm notifier
Will, Thanks for the comments! > -Original Message- > From: Will Deacon [mailto:will.dea...@arm.com] > Sent: 2014年4月15日 16:48 > To: Neil Zhang > Cc: li...@arm.linux.org.uk; linux-arm-ker...@lists.infradead.org; > linux-kernel@vger.kernel.org; Sudeep Holla > Subject: Re: [PATCH v2] ARM: perf: save/restore pmu registers in pm notifier > > Hello, > > On Mon, Apr 14, 2014 at 02:42:22AM +0100, Neil Zhang wrote: > > From: Sudeep KarkadaNagesha > > > > This adds core support for saving and restoring CPU PMU registers for > > suspend/resume support i.e. deeper C-states in cpuidle terms. > > This patch adds support only to ARMv7 PMU registers save/restore. > > It needs to be extended to xscale and ARMv6 if needed. > > > > [Neil] We found that DS-5 not work on our CA7 based SoCs. > > After debuging, found PMU registers were lost because of core power down. > > Then i found Sudeep had a patch to fix it about two years ago but not > > in the mainline, just port it. > > What I don't like about this patch is that we're introducing significant > overhead for SoCs that don't require save/restore of the PMU state. I'd much > rather see core power down disabled whilst the PMU is in use but, if that's > not > possible, then I think we need to: > > (1) Make this conditional for cores that really need it > > (2) Only save/restore if the PMU is in use (even better, just save/restore > the live registers, but that's probably not worth the effort > initially). > The patch has check the ARMV7_PMNC_E bit when save / restore, so suppose only the core's that use PMU will do the save / restore work. > (3) Ensure we ->reset the PMU before doing the restore Ok, I can add it in the next version. > > Will Best Regards, Neil Zhang
Re: [PATCH v2] ARM: perf: save/restore pmu registers in pm notifier
Hello, On Mon, Apr 14, 2014 at 02:42:22AM +0100, Neil Zhang wrote: > From: Sudeep KarkadaNagesha > > This adds core support for saving and restoring CPU PMU registers > for suspend/resume support i.e. deeper C-states in cpuidle terms. > This patch adds support only to ARMv7 PMU registers save/restore. > It needs to be extended to xscale and ARMv6 if needed. > > [Neil] We found that DS-5 not work on our CA7 based SoCs. > After debuging, found PMU registers were lost because of core power down. > Then i found Sudeep had a patch to fix it about two years ago but not in > the mainline, just port it. What I don't like about this patch is that we're introducing significant overhead for SoCs that don't require save/restore of the PMU state. I'd much rather see core power down disabled whilst the PMU is in use but, if that's not possible, then I think we need to: (1) Make this conditional for cores that really need it (2) Only save/restore if the PMU is in use (even better, just save/restore the live registers, but that's probably not worth the effort initially). (3) Ensure we ->reset the PMU before doing the restore Will -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
