| Message ID | 1582262314-8319-2-git-send-email-ego@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Track and expose idle PURR and SPURR ticks | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (65b2623f395a4e25ab3ff4cff1c9c7623619a22d) |
| snowpatch_ozlabs/checkpatch | warning | total: 0 errors, 1 warnings, 0 checks, 94 lines checked |
| snowpatch_ozlabs/needsstable | success | Patch has no Fixes tags |
"Gautham R. Shenoy" <ego@linux.vnet.ibm.com> writes: > From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> > > Currently prior to entering an idle state on a Linux Guest, the > pseries cpuidle driver implement an idle_loop_prolog() and > idle_loop_epilog() functions which ensure that idle_purr is correctly > computed, and the hypervisor is informed that the CPU cycles have been > donated. > > These prolog and epilog functions are also required in the default > idle call, i.e pseries_lpar_idle(). Hence move these accessor > functions to a common header file and call them from > pseries_lpar_idle(). Since the existing header files such as > asm/processor.h have enough clutter, create a new header file > asm/idle.h. > > Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com> > --- > arch/powerpc/include/asm/idle.h | 27 +++++++++++++++++++++++++++ > arch/powerpc/platforms/pseries/setup.c | 7 +++++-- > drivers/cpuidle/cpuidle-pseries.c | 24 +----------------------- > 3 files changed, 33 insertions(+), 25 deletions(-) > create mode 100644 arch/powerpc/include/asm/idle.h > > diff --git a/arch/powerpc/include/asm/idle.h b/arch/powerpc/include/asm/idle.h > new file mode 100644 > index 0000000..f32a7d8 > --- /dev/null > +++ b/arch/powerpc/include/asm/idle.h > @@ -0,0 +1,27 @@ > +#ifndef _ASM_POWERPC_IDLE_H > +#define _ASM_POWERPC_IDLE_H > +#include <asm/runlatch.h> > + > +static inline void idle_loop_prolog(unsigned long *in_purr) > +{ > + ppc64_runlatch_off(); > + *in_purr = mfspr(SPRN_PURR); > + /* > + * Indicate to the HV that we are idle. Now would be > + * a good time to find other work to dispatch. > + */ > + get_lppaca()->idle = 1; > +} > + > +static inline void idle_loop_epilog(unsigned long in_purr) > +{ > + u64 wait_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); > + get_lppaca()->idle = 0; > + > + ppc64_runlatch_on(); > +} > +#endif Looks fine and correct as a cleanup, but asm/include/idle.h and idle_loop_prolog, idle_loop_epilog, strike me as too generic for pseries-specific code.
Hello Nathan, On Fri, Feb 21, 2020 at 09:03:16AM -0600, Nathan Lynch wrote: > "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> writes: > > > From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> > > > > Currently prior to entering an idle state on a Linux Guest, the > > pseries cpuidle driver implement an idle_loop_prolog() and > > idle_loop_epilog() functions which ensure that idle_purr is correctly > > computed, and the hypervisor is informed that the CPU cycles have been > > donated. > > > > These prolog and epilog functions are also required in the default > > idle call, i.e pseries_lpar_idle(). Hence move these accessor > > functions to a common header file and call them from > > pseries_lpar_idle(). Since the existing header files such as > > asm/processor.h have enough clutter, create a new header file > > asm/idle.h. > > > > Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com> > > --- > > arch/powerpc/include/asm/idle.h | 27 +++++++++++++++++++++++++++ > > arch/powerpc/platforms/pseries/setup.c | 7 +++++-- > > drivers/cpuidle/cpuidle-pseries.c | 24 +----------------------- > > 3 files changed, 33 insertions(+), 25 deletions(-) > > create mode 100644 arch/powerpc/include/asm/idle.h > > > > diff --git a/arch/powerpc/include/asm/idle.h b/arch/powerpc/include/asm/idle.h > > new file mode 100644 > > index 0000000..f32a7d8 > > --- /dev/null > > +++ b/arch/powerpc/include/asm/idle.h > > @@ -0,0 +1,27 @@ > > +#ifndef _ASM_POWERPC_IDLE_H > > +#define _ASM_POWERPC_IDLE_H > > +#include <asm/runlatch.h> > > + > > +static inline void idle_loop_prolog(unsigned long *in_purr) > > +{ > > + ppc64_runlatch_off(); > > + *in_purr = mfspr(SPRN_PURR); > > + /* > > + * Indicate to the HV that we are idle. Now would be > > + * a good time to find other work to dispatch. > > + */ > > + get_lppaca()->idle = 1; > > +} > > + > > +static inline void idle_loop_epilog(unsigned long in_purr) > > +{ > > + u64 wait_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); > > + get_lppaca()->idle = 0; > > + > > + ppc64_runlatch_on(); > > +} > > +#endif > > Looks fine and correct as a cleanup, but asm/include/idle.h and > idle_loop_prolog, idle_loop_epilog, strike me as too generic for > pseries-specific code. Should it be prefixed with pseries , i.e pseries_idle_prolog() and pseries_idle_epilog() ? Also, I am planning another round of cleanup to move all the idle-related declaration from asm/include/processor.h to asm/include/idle.h
Gautham R Shenoy <ego@linux.vnet.ibm.com> writes: > On Fri, Feb 21, 2020 at 09:03:16AM -0600, Nathan Lynch wrote: >> Looks fine and correct as a cleanup, but asm/include/idle.h and >> idle_loop_prolog, idle_loop_epilog, strike me as too generic for >> pseries-specific code. > > Should it be prefixed with pseries , i.e pseries_idle_prolog() > and pseries_idle_epilog() ? Yes that seems appropriate to me.
diff --git a/arch/powerpc/include/asm/idle.h b/arch/powerpc/include/asm/idle.h new file mode 100644 index 0000000..f32a7d8 --- /dev/null +++ b/arch/powerpc/include/asm/idle.h @@ -0,0 +1,27 @@ +#ifndef _ASM_POWERPC_IDLE_H +#define _ASM_POWERPC_IDLE_H +#include <asm/runlatch.h> + +static inline void idle_loop_prolog(unsigned long *in_purr) +{ + ppc64_runlatch_off(); + *in_purr = mfspr(SPRN_PURR); + /* + * Indicate to the HV that we are idle. Now would be + * a good time to find other work to dispatch. + */ + get_lppaca()->idle = 1; +} + +static inline void idle_loop_epilog(unsigned long in_purr) +{ + u64 wait_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); + get_lppaca()->idle = 0; + + ppc64_runlatch_on(); +} +#endif diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c index 0c8421d..ffd4d59 100644 --- a/arch/powerpc/platforms/pseries/setup.c +++ b/arch/powerpc/platforms/pseries/setup.c @@ -68,6 +68,7 @@ #include <asm/isa-bridge.h> #include <asm/security_features.h> #include <asm/asm-const.h> +#include <asm/idle.h> #include <asm/swiotlb.h> #include <asm/svm.h> @@ -319,6 +320,8 @@ static int alloc_dispatch_log_kmem_cache(void) static void pseries_lpar_idle(void) { + unsigned long in_purr; + /* * Default handler to go into low thread priority and possibly * low power mode by ceding processor to hypervisor @@ -328,7 +331,7 @@ static void pseries_lpar_idle(void) return; /* Indicate to hypervisor that we are idle. */ - get_lppaca()->idle = 1; + idle_loop_prolog(&in_purr); /* * Yield the processor to the hypervisor. We return if @@ -339,7 +342,7 @@ static void pseries_lpar_idle(void) */ cede_processor(); - get_lppaca()->idle = 0; + idle_loop_epilog(in_purr); } /* diff --git a/drivers/cpuidle/cpuidle-pseries.c b/drivers/cpuidle/cpuidle-pseries.c index 74c2479..fc9dee9c 100644 --- a/drivers/cpuidle/cpuidle-pseries.c +++ b/drivers/cpuidle/cpuidle-pseries.c @@ -19,6 +19,7 @@ #include <asm/machdep.h> #include <asm/firmware.h> #include <asm/runlatch.h> +#include <asm/idle.h> #include <asm/plpar_wrappers.h> struct cpuidle_driver pseries_idle_driver = { @@ -31,29 +32,6 @@ struct cpuidle_driver pseries_idle_driver = { static u64 snooze_timeout __read_mostly; static bool snooze_timeout_en __read_mostly; -static inline void idle_loop_prolog(unsigned long *in_purr) -{ - ppc64_runlatch_off(); - *in_purr = mfspr(SPRN_PURR); - /* - * Indicate to the HV that we are idle. Now would be - * a good time to find other work to dispatch. - */ - get_lppaca()->idle = 1; -} - -static inline void idle_loop_epilog(unsigned long in_purr) -{ - u64 wait_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); - get_lppaca()->idle = 0; - - ppc64_runlatch_on(); -} - static int snooze_loop(struct cpuidle_device *dev, struct cpuidle_driver *drv, int index)