Re: [PATCH 1/3] powerpc/pseries: Account for SPURR ticks on idle CPUs
Hello Nathan,
On Wed, Dec 04, 2019 at 04:24:52PM -0600, Nathan Lynch wrote:
> "Gautham R. Shenoy" writes:
> > diff --git a/arch/powerpc/kernel/idle.c b/arch/powerpc/kernel/idle.c
> > index a36fd05..708ec68 100644
> > --- a/arch/powerpc/kernel/idle.c
> > +++ b/arch/powerpc/kernel/idle.c
> > @@ -33,6 +33,8 @@
> > unsigned long cpuidle_disable = IDLE_NO_OVERRIDE;
> > EXPORT_SYMBOL(cpuidle_disable);
> >
> > +DEFINE_PER_CPU(u64, idle_spurr_cycles);
> > +
>
> Does idle_spurr_cycles need any special treatment for CPU
> online/offline?
If offline uses extended cede, then we need to take a snapshot of the
idle_spurr_cycles before going offline and add the delta once we are
back online. However, since the plan is to deprecate the use of
extended cede for CPU-Offline and use only rtas-stop-self, we don't
need any special handling there.
>
> > static int __init powersave_off(char *arg)
> > {
> > ppc_md.power_save = NULL;
> > diff --git a/drivers/cpuidle/cpuidle-pseries.c
> > b/drivers/cpuidle/cpuidle-pseries.c
> > index 74c2479..45e2be4 100644
> > --- a/drivers/cpuidle/cpuidle-pseries.c
> > +++ b/drivers/cpuidle/cpuidle-pseries.c
> > @@ -30,11 +30,14 @@ struct cpuidle_driver pseries_idle_driver = {
> > static struct cpuidle_state *cpuidle_state_table __read_mostly;
> > static u64 snooze_timeout __read_mostly;
> > static bool snooze_timeout_en __read_mostly;
> > +DECLARE_PER_CPU(u64, idle_spurr_cycles);
>
> This belongs in a header...
Will move it to the header file.
>
>
> > -static inline void idle_loop_prolog(unsigned long *in_purr)
> > +static inline void idle_loop_prolog(unsigned long *in_purr,
> > + unsigned long *in_spurr)
> > {
> > ppc64_runlatch_off();
> > *in_purr = mfspr(SPRN_PURR);
> > + *in_spurr = mfspr(SPRN_SPURR);
> > /*
> > * Indicate to the HV that we are idle. Now would be
> > * a good time to find other work to dispatch.
> > @@ -42,13 +45,16 @@ static inline void idle_loop_prolog(unsigned long
> > *in_purr)
> > get_lppaca()->idle = 1;
> > }
> >
> > -static inline void idle_loop_epilog(unsigned long in_purr)
> > +static inline void idle_loop_epilog(unsigned long in_purr,
> > + unsigned long in_spurr)
> > {
> > u64 wait_cycles;
> > + u64 *idle_spurr_cycles_ptr = this_cpu_ptr(_spurr_cycles);
> >
> > wait_cycles = be64_to_cpu(get_lppaca()->wait_state_cycles);
> > wait_cycles += mfspr(SPRN_PURR) - in_purr;
> > get_lppaca()->wait_state_cycles = cpu_to_be64(wait_cycles);
> > + *idle_spurr_cycles_ptr += mfspr(SPRN_SPURR) - in_spurr;
>
> ... and the sampling and increment logic probably should be further
> encapsulated in accessor functions that can be used in both the cpuidle
> driver and the default/generic idle implementation. Or is there some
> reason this is specific to the pseries cpuidle driver?
I am not sure if we use SPURR and PURR for performing accounting on
Bare-Metal systems. IIUC, the patches proposed by Kamalesh is only to
use idle_[s]purr and [s]purr on POWERVM LPARs. This is why I coded the
sampling/increment logic in the pseries cpuidle driver. But you are
right, in the absence of cpuidle, when we use the default idle
implementation, we will still need to note the value of
idle_purr/spurr.
--
Thanks and Regards
gautham.
Re: [PATCH 1/3] powerpc/pseries: Account for SPURR ticks on idle CPUs
"Gautham R. Shenoy" writes:
> diff --git a/arch/powerpc/kernel/idle.c b/arch/powerpc/kernel/idle.c
> index a36fd05..708ec68 100644
> --- a/arch/powerpc/kernel/idle.c
> +++ b/arch/powerpc/kernel/idle.c
> @@ -33,6 +33,8 @@
> unsigned long cpuidle_disable = IDLE_NO_OVERRIDE;
> EXPORT_SYMBOL(cpuidle_disable);
>
> +DEFINE_PER_CPU(u64, idle_spurr_cycles);
> +
Does idle_spurr_cycles need any special treatment for CPU
online/offline?
> static int __init powersave_off(char *arg)
> {
> ppc_md.power_save = NULL;
> diff --git a/drivers/cpuidle/cpuidle-pseries.c
> b/drivers/cpuidle/cpuidle-pseries.c
> index 74c2479..45e2be4 100644
> --- a/drivers/cpuidle/cpuidle-pseries.c
> +++ b/drivers/cpuidle/cpuidle-pseries.c
> @@ -30,11 +30,14 @@ struct cpuidle_driver pseries_idle_driver = {
> static struct cpuidle_state *cpuidle_state_table __read_mostly;
> static u64 snooze_timeout __read_mostly;
> static bool snooze_timeout_en __read_mostly;
> +DECLARE_PER_CPU(u64, idle_spurr_cycles);
This belongs in a header...
> -static inline void idle_loop_prolog(unsigned long *in_purr)
> +static inline void idle_loop_prolog(unsigned long *in_purr,
> + unsigned long *in_spurr)
> {
> ppc64_runlatch_off();
> *in_purr = mfspr(SPRN_PURR);
> + *in_spurr = mfspr(SPRN_SPURR);
> /*
>* Indicate to the HV that we are idle. Now would be
>* a good time to find other work to dispatch.
> @@ -42,13 +45,16 @@ static inline void idle_loop_prolog(unsigned long
> *in_purr)
> get_lppaca()->idle = 1;
> }
>
> -static inline void idle_loop_epilog(unsigned long in_purr)
> +static inline void idle_loop_epilog(unsigned long in_purr,
> + unsigned long in_spurr)
> {
> u64 wait_cycles;
> + u64 *idle_spurr_cycles_ptr = this_cpu_ptr(_spurr_cycles);
>
> wait_cycles = be64_to_cpu(get_lppaca()->wait_state_cycles);
> wait_cycles += mfspr(SPRN_PURR) - in_purr;
> get_lppaca()->wait_state_cycles = cpu_to_be64(wait_cycles);
> + *idle_spurr_cycles_ptr += mfspr(SPRN_SPURR) - in_spurr;
... and the sampling and increment logic probably should be further
encapsulated in accessor functions that can be used in both the cpuidle
driver and the default/generic idle implementation. Or is there some
reason this is specific to the pseries cpuidle driver?
Re: [PATCH 1/3] powerpc/pseries: Account for SPURR ticks on idle CPUs
On 11/27/19 5:31 PM, Gautham R. Shenoy wrote: > From: "Gautham R. Shenoy" > > On PSeries LPARs, to compute the utilization, tools such as lparstat > need to know the [S]PURR ticks when the CPUs were busy or idle. > > In the pseries cpuidle driver, we keep track of the idle PURR ticks in > the VPA variable "wait_state_cycles". This patch extends the support > to account for the idle SPURR ticks. Thanks for working on it. > > Signed-off-by: Gautham R. Shenoy Reviewed-by: Kamalesh Babulal
[PATCH 1/3] powerpc/pseries: Account for SPURR ticks on idle CPUs
From: "Gautham R. Shenoy"
On PSeries LPARs, to compute the utilization, tools such as lparstat
need to know the [S]PURR ticks when the CPUs were busy or idle.
In the pseries cpuidle driver, we keep track of the idle PURR ticks in
the VPA variable "wait_state_cycles". This patch extends the support
to account for the idle SPURR ticks.
Signed-off-by: Gautham R. Shenoy
---
arch/powerpc/kernel/idle.c| 2 ++
drivers/cpuidle/cpuidle-pseries.c | 28 +---
2 files changed, 19 insertions(+), 11 deletions(-)
diff --git a/arch/powerpc/kernel/idle.c b/arch/powerpc/kernel/idle.c
index a36fd05..708ec68 100644
--- a/arch/powerpc/kernel/idle.c
+++ b/arch/powerpc/kernel/idle.c
@@ -33,6 +33,8 @@
unsigned long cpuidle_disable = IDLE_NO_OVERRIDE;
EXPORT_SYMBOL(cpuidle_disable);
+DEFINE_PER_CPU(u64, idle_spurr_cycles);
+
static int __init powersave_off(char *arg)
{
ppc_md.power_save = NULL;
diff --git a/drivers/cpuidle/cpuidle-pseries.c
b/drivers/cpuidle/cpuidle-pseries.c
index 74c2479..45e2be4 100644
--- a/drivers/cpuidle/cpuidle-pseries.c
+++ b/drivers/cpuidle/cpuidle-pseries.c
@@ -30,11 +30,14 @@ struct cpuidle_driver pseries_idle_driver = {
static struct cpuidle_state *cpuidle_state_table __read_mostly;
static u64 snooze_timeout __read_mostly;
static bool snooze_timeout_en __read_mostly;
+DECLARE_PER_CPU(u64, idle_spurr_cycles);
-static inline void idle_loop_prolog(unsigned long *in_purr)
+static inline void idle_loop_prolog(unsigned long *in_purr,
+ unsigned long *in_spurr)
{
ppc64_runlatch_off();
*in_purr = mfspr(SPRN_PURR);
+ *in_spurr = mfspr(SPRN_SPURR);
/*
* Indicate to the HV that we are idle. Now would be
* a good time to find other work to dispatch.
@@ -42,13 +45,16 @@ static inline void idle_loop_prolog(unsigned long *in_purr)
get_lppaca()->idle = 1;
}
-static inline void idle_loop_epilog(unsigned long in_purr)
+static inline void idle_loop_epilog(unsigned long in_purr,
+ unsigned long in_spurr)
{
u64 wait_cycles;
+ u64 *idle_spurr_cycles_ptr = this_cpu_ptr(_spurr_cycles);
wait_cycles = be64_to_cpu(get_lppaca()->wait_state_cycles);
wait_cycles += mfspr(SPRN_PURR) - in_purr;
get_lppaca()->wait_state_cycles = cpu_to_be64(wait_cycles);
+ *idle_spurr_cycles_ptr += mfspr(SPRN_SPURR) - in_spurr;
get_lppaca()->idle = 0;
ppc64_runlatch_on();
@@ -58,12 +64,12 @@ static int snooze_loop(struct cpuidle_device *dev,
struct cpuidle_driver *drv,
int index)
{
- unsigned long in_purr;
+ unsigned long in_purr, in_spurr;
u64 snooze_exit_time;
set_thread_flag(TIF_POLLING_NRFLAG);
- idle_loop_prolog(_purr);
+ idle_loop_prolog(_purr, _spurr);
local_irq_enable();
snooze_exit_time = get_tb() + snooze_timeout;
@@ -87,7 +93,7 @@ static int snooze_loop(struct cpuidle_device *dev,
local_irq_disable();
- idle_loop_epilog(in_purr);
+ idle_loop_epilog(in_purr, in_spurr);
return index;
}
@@ -113,9 +119,9 @@ static int dedicated_cede_loop(struct cpuidle_device *dev,
struct cpuidle_driver *drv,
int index)
{
- unsigned long in_purr;
+ unsigned long in_purr, in_spurr;
- idle_loop_prolog(_purr);
+ idle_loop_prolog(_purr, _spurr);
get_lppaca()->donate_dedicated_cpu = 1;
HMT_medium();
@@ -124,7 +130,7 @@ static int dedicated_cede_loop(struct cpuidle_device *dev,
local_irq_disable();
get_lppaca()->donate_dedicated_cpu = 0;
- idle_loop_epilog(in_purr);
+ idle_loop_epilog(in_purr, in_spurr);
return index;
}
@@ -133,9 +139,9 @@ static int shared_cede_loop(struct cpuidle_device *dev,
struct cpuidle_driver *drv,
int index)
{
- unsigned long in_purr;
+ unsigned long in_purr, in_spurr;
- idle_loop_prolog(_purr);
+ idle_loop_prolog(_purr, _spurr);
/*
* Yield the processor to the hypervisor. We return if
@@ -147,7 +153,7 @@ static int shared_cede_loop(struct cpuidle_device *dev,
check_and_cede_processor();
local_irq_disable();
- idle_loop_epilog(in_purr);
+ idle_loop_epilog(in_purr, in_spurr);
return index;
}
--
1.9.4
