From: Gautham R Shenoy <ego@linux.vnet.ibm.com> To: Nathan Lynch <nathanl@linux.ibm.com> Cc: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>, linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org, Michael Ellerman <mpe@ellerman.id.au>, Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com>, Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>, "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>, Tyrel Datwyler <tyreld@linux.ibm.com> Subject: Re: [PATCH 1/3] powerpc/pseries: Account for SPURR ticks on idle CPUs Date: Mon, 3 Feb 2020 10:15:16 +0530 [thread overview] Message-ID: <20200203044516.GA13468@in.ibm.com> (raw) In-Reply-To: <87o8wnu3t7.fsf@linux.ibm.com> Hello Nathan, On Wed, Dec 04, 2019 at 04:24:52PM -0600, Nathan Lynch wrote: > "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> 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(&idle_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.
WARNING: multiple messages have this Message-ID (diff)
From: Gautham R Shenoy <ego@linux.vnet.ibm.com> To: Nathan Lynch <nathanl@linux.ibm.com> Cc: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>, Tyrel Datwyler <tyreld@linux.ibm.com>, linux-kernel@vger.kernel.org, Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>, "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>, Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com>, linuxppc-dev@lists.ozlabs.org Subject: Re: [PATCH 1/3] powerpc/pseries: Account for SPURR ticks on idle CPUs Date: Mon, 3 Feb 2020 10:15:16 +0530 [thread overview] Message-ID: <20200203044516.GA13468@in.ibm.com> (raw) In-Reply-To: <87o8wnu3t7.fsf@linux.ibm.com> Hello Nathan, On Wed, Dec 04, 2019 at 04:24:52PM -0600, Nathan Lynch wrote: > "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> 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(&idle_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.
next prev parent reply other threads:[~2020-02-03 4:53 UTC|newest] Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-11-27 12:01 [PATCH 0/3] pseries: Track and expose idle PURR and SPURR ticks Gautham R. Shenoy 2019-11-27 12:01 ` Gautham R. Shenoy 2019-11-27 12:01 ` [PATCH 1/3] powerpc/pseries: Account for SPURR ticks on idle CPUs Gautham R. Shenoy 2019-11-27 12:01 ` Gautham R. Shenoy 2019-12-03 13:39 ` Kamalesh Babulal 2019-12-04 22:24 ` Nathan Lynch 2019-12-04 22:24 ` Nathan Lynch 2020-02-03 4:45 ` Gautham R Shenoy [this message] 2020-02-03 4:45 ` Gautham R Shenoy 2019-11-27 12:01 ` [PATCH 2/3] powerpc/sysfs: Show idle_purr and idle_spurr for every CPU Gautham R. Shenoy 2019-11-27 12:01 ` Gautham R. Shenoy 2019-12-03 13:37 ` Kamalesh Babulal 2019-12-04 12:37 ` Gautham R Shenoy 2019-12-04 12:37 ` Gautham R Shenoy 2019-12-03 21:02 ` kbuild test robot 2019-12-03 21:02 ` kbuild test robot 2019-12-03 21:02 ` kbuild test robot 2019-12-04 22:24 ` Nathan Lynch 2019-12-04 22:24 ` Nathan Lynch 2020-02-03 4:47 ` Gautham R Shenoy 2020-02-03 4:47 ` Gautham R Shenoy 2019-12-05 16:53 ` Naveen N. Rao 2019-12-05 16:53 ` Naveen N. Rao 2020-02-03 4:50 ` Gautham R Shenoy 2020-02-03 4:50 ` Gautham R Shenoy 2020-02-04 7:52 ` Naveen N. Rao 2020-02-04 7:52 ` Naveen N. Rao 2020-02-05 4:19 ` Gautham R Shenoy 2020-02-05 4:19 ` Gautham R Shenoy 2020-02-05 6:58 ` Naveen N. Rao 2020-02-05 6:58 ` Naveen N. Rao 2020-02-05 7:08 ` Christophe Leroy 2020-02-05 8:07 ` Naveen N. Rao 2020-02-05 8:07 ` Naveen N. Rao 2019-11-27 12:01 ` [PATCH 3/3] Documentation: Document sysfs interfaces purr, spurr, idle_purr, idle_spurr Gautham R. Shenoy 2019-11-27 12:01 ` Gautham R. Shenoy 2019-12-04 22:25 ` Nathan Lynch 2019-12-04 22:25 ` Nathan Lynch 2019-12-04 22:24 ` [PATCH 0/3] pseries: Track and expose idle PURR and SPURR ticks Nathan Lynch 2019-12-04 22:24 ` Nathan Lynch 2019-12-05 15:03 ` Kamalesh Babulal 2019-12-05 15:03 ` Kamalesh Babulal 2019-12-05 16:16 ` Nathan Lynch 2019-12-05 16:16 ` Nathan Lynch 2019-12-05 17:25 ` Naveen N. Rao 2019-12-05 17:25 ` Naveen N. Rao 2019-12-06 9:14 ` Naveen N. Rao 2019-12-06 9:14 ` Naveen N. Rao 2020-02-04 9:12 ` Kamalesh Babulal 2020-02-04 9:12 ` Kamalesh Babulal
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200203044516.GA13468@in.ibm.com \ --to=ego@linux.vnet.ibm.com \ --cc=kamalesh@linux.vnet.ibm.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linuxppc-dev@lists.ozlabs.org \ --cc=mpe@ellerman.id.au \ --cc=nathanl@linux.ibm.com \ --cc=naveen.n.rao@linux.vnet.ibm.com \ --cc=svaidy@linux.vnet.ibm.com \ --cc=tyreld@linux.ibm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.