SLW: Remove stop1_lite and stop2_lite

Message ID 1525941957-9140-1-git-send-email-akshay.adiga@linux.vnet.ibm.com
State Superseded
Headers show
Series
  • SLW: Remove stop1_lite and stop2_lite
Related show

Commit Message

Akshay Adiga May 10, 2018, 8:45 a.m.
Lite states don't give up the SMT resources, can potentially have a
performance impact on sibling threads. At latencies of stop1_lite and
stop2_lite , lite states dont make sense and can have performance
impact.

Signed-off-by: Akshay Adiga <akshay.adiga@linux.vnet.ibm.com>
---
 hw/slw.c | 29 +----------------------------
 1 file changed, 1 insertion(+), 28 deletions(-)

Comments

Akshay Adiga May 15, 2018, 6:38 a.m. | #1
On Thu, May 10, 2018 at 02:15:57PM +0530, Akshay Adiga wrote:
> Lite states don't give up the SMT resources, can potentially have a
> performance impact on sibling threads. At latencies of stop1_lite and
> stop2_lite , lite states dont make sense and can have performance
> impact.
> 
> Signed-off-by: Akshay Adiga <akshay.adiga@linux.vnet.ibm.com>
> ---
>  hw/slw.c | 29 +----------------------------
>  1 file changed, 1 insertion(+), 28 deletions(-)
> 
> diff --git a/hw/slw.c b/hw/slw.c
> index c059d9c..513be7b 100644
> --- a/hw/slw.c
> +++ b/hw/slw.c
> @@ -529,20 +529,7 @@ static struct cpu_idle_states power9_cpu_idle_states[] = {
>  				 | OPAL_PM_PSSCR_ESL \
>  				 | OPAL_PM_PSSCR_EC,
>  		.pm_ctrl_reg_mask = OPAL_PM_PSSCR_MASK },
> -	{
> -		.name = "stop1_lite", /* Enter stop1 with no state loss */
> -		.latency_ns = 4900,
> -		.residency_ns = 49000,
> -		.flags = 0*OPAL_PM_DEC_STOP \
> -		       | 0*OPAL_PM_TIMEBASE_STOP  \
> -		       | 0*OPAL_PM_LOSE_USER_CONTEXT \
> -		       | 0*OPAL_PM_LOSE_HYP_CONTEXT \
> -		       | 0*OPAL_PM_LOSE_FULL_CONTEXT \
> -		       | 1*OPAL_PM_STOP_INST_FAST,
> -		.pm_ctrl_reg_val = OPAL_PM_PSSCR_RL(1) \
> -				 | OPAL_PM_PSSCR_MTL(3) \
> -				 | OPAL_PM_PSSCR_TR(3),
> -		.pm_ctrl_reg_mask = OPAL_PM_PSSCR_MASK },
> +
>  	{
>  		.name = "stop1",
>  		.latency_ns = 5000,
> @@ -560,20 +547,6 @@ static struct cpu_idle_states power9_cpu_idle_states[] = {
>  				 | OPAL_PM_PSSCR_EC,
>  		.pm_ctrl_reg_mask = OPAL_PM_PSSCR_MASK },
>  	{
> -		.name = "stop2_lite", /* Enter stop2 with no state loss */
> -		.latency_ns = 9900,
> -		.residency_ns = 99000,
> -		.flags = 0*OPAL_PM_DEC_STOP \
> -		       | 0*OPAL_PM_TIMEBASE_STOP  \
> -		       | 0*OPAL_PM_LOSE_USER_CONTEXT \
> -		       | 0*OPAL_PM_LOSE_HYP_CONTEXT \
> -		       | 0*OPAL_PM_LOSE_FULL_CONTEXT \
> -		       | 1*OPAL_PM_STOP_INST_FAST,
> -		.pm_ctrl_reg_val = OPAL_PM_PSSCR_RL(2) \
> -				 | OPAL_PM_PSSCR_MTL(3) \
> -				 | OPAL_PM_PSSCR_TR(3),
> -		.pm_ctrl_reg_mask = OPAL_PM_PSSCR_MASK },
> -	{
>  		.name = "stop2",
>  		.latency_ns = 10000,
>  		.residency_ns = 100000,
> -- 
> 2.5.5
> 

This is a followup on the previous patch  : https://patchwork.ozlabs.org/patch/906582/ 

Nick,Stewart does this look good ?
Nicholas Piggin May 15, 2018, 7:21 a.m. | #2
On Tue, 15 May 2018 12:08:31 +0530
Akshay Adiga <akshay.adiga@linux.vnet.ibm.com> wrote:

> On Thu, May 10, 2018 at 02:15:57PM +0530, Akshay Adiga wrote:
> > Lite states don't give up the SMT resources, can potentially have a
> > performance impact on sibling threads. At latencies of stop1_lite
> > and stop2_lite , lite states dont make sense and can have
> > performance impact.
> > 
> > Signed-off-by: Akshay Adiga <akshay.adiga@linux.vnet.ibm.com>
> > ---
> >  hw/slw.c | 29 +----------------------------
> >  1 file changed, 1 insertion(+), 28 deletions(-)
> > 
> > diff --git a/hw/slw.c b/hw/slw.c
> > index c059d9c..513be7b 100644
> > --- a/hw/slw.c
> > +++ b/hw/slw.c
> > @@ -529,20 +529,7 @@ static struct cpu_idle_states
> > power9_cpu_idle_states[] = { | OPAL_PM_PSSCR_ESL \
> >  				 | OPAL_PM_PSSCR_EC,
> >  		.pm_ctrl_reg_mask = OPAL_PM_PSSCR_MASK },
> > -	{
> > -		.name = "stop1_lite", /* Enter stop1 with no state
> > loss */
> > -		.latency_ns = 4900,
> > -		.residency_ns = 49000,
> > -		.flags = 0*OPAL_PM_DEC_STOP \
> > -		       | 0*OPAL_PM_TIMEBASE_STOP  \
> > -		       | 0*OPAL_PM_LOSE_USER_CONTEXT \
> > -		       | 0*OPAL_PM_LOSE_HYP_CONTEXT \
> > -		       | 0*OPAL_PM_LOSE_FULL_CONTEXT \
> > -		       | 1*OPAL_PM_STOP_INST_FAST,
> > -		.pm_ctrl_reg_val = OPAL_PM_PSSCR_RL(1) \
> > -				 | OPAL_PM_PSSCR_MTL(3) \
> > -				 | OPAL_PM_PSSCR_TR(3),
> > -		.pm_ctrl_reg_mask = OPAL_PM_PSSCR_MASK },
> > +
> >  	{
> >  		.name = "stop1",
> >  		.latency_ns = 5000,
> > @@ -560,20 +547,6 @@ static struct cpu_idle_states
> > power9_cpu_idle_states[] = { | OPAL_PM_PSSCR_EC,
> >  		.pm_ctrl_reg_mask = OPAL_PM_PSSCR_MASK },
> >  	{
> > -		.name = "stop2_lite", /* Enter stop2 with no state
> > loss */
> > -		.latency_ns = 9900,
> > -		.residency_ns = 99000,
> > -		.flags = 0*OPAL_PM_DEC_STOP \
> > -		       | 0*OPAL_PM_TIMEBASE_STOP  \
> > -		       | 0*OPAL_PM_LOSE_USER_CONTEXT \
> > -		       | 0*OPAL_PM_LOSE_HYP_CONTEXT \
> > -		       | 0*OPAL_PM_LOSE_FULL_CONTEXT \
> > -		       | 1*OPAL_PM_STOP_INST_FAST,
> > -		.pm_ctrl_reg_val = OPAL_PM_PSSCR_RL(2) \
> > -				 | OPAL_PM_PSSCR_MTL(3) \
> > -				 | OPAL_PM_PSSCR_TR(3),
> > -		.pm_ctrl_reg_mask = OPAL_PM_PSSCR_MASK },
> > -	{
> >  		.name = "stop2",
> >  		.latency_ns = 10000,
> >  		.residency_ns = 100000,
> > -- 
> > 2.5.5
> >   
> 
> This is a followup on the previous patch  :
> https://patchwork.ozlabs.org/patch/906582/ 
> 
> Nick,Stewart does this look good ?
> 

Yes I think so. The rest we can look at tuning with Linux and dt
parameters, but I think this should avoid most of the pathological
cases and work with existing kernels, without overly hard coding
policy.

I would add a comment as to why we're leaving those states out
though.

Thanks,
Nick
Akshay Adiga May 17, 2018, 5:26 a.m. | #3
On Tue, May 15, 2018 at 05:21:04PM +1000, Nicholas Piggin wrote:
> On Tue, 15 May 2018 12:08:31 +0530
> Akshay Adiga <akshay.adiga@linux.vnet.ibm.com> wrote:
> 
> > On Thu, May 10, 2018 at 02:15:57PM +0530, Akshay Adiga wrote:
> > > Lite states don't give up the SMT resources, can potentially have a
> > > performance impact on sibling threads. At latencies of stop1_lite
> > > and stop2_lite , lite states dont make sense and can have
> > > performance impact.
> > > 
> > > Signed-off-by: Akshay Adiga <akshay.adiga@linux.vnet.ibm.com>
> > >   
> > 
> > This is a followup on the previous patch  :
> > https://patchwork.ozlabs.org/patch/906582/ 
> > 
> > Nick,Stewart does this look good ?
> > 
> 
> Yes I think so. The rest we can look at tuning with Linux and dt
> parameters, but I think this should avoid most of the pathological
> cases and work with existing kernels, without overly hard coding
> policy.
>
> I would add a comment as to why we're leaving those states out
> though.

I dont have problem adding a comment. But it will be like adding a
comment for the code that does removed code. Anyways, we have the
the reason mentioned in git log.
> 
> Thanks,
> Nick
>

Patch

diff --git a/hw/slw.c b/hw/slw.c
index c059d9c..513be7b 100644
--- a/hw/slw.c
+++ b/hw/slw.c
@@ -529,20 +529,7 @@  static struct cpu_idle_states power9_cpu_idle_states[] = {
 				 | OPAL_PM_PSSCR_ESL \
 				 | OPAL_PM_PSSCR_EC,
 		.pm_ctrl_reg_mask = OPAL_PM_PSSCR_MASK },
-	{
-		.name = "stop1_lite", /* Enter stop1 with no state loss */
-		.latency_ns = 4900,
-		.residency_ns = 49000,
-		.flags = 0*OPAL_PM_DEC_STOP \
-		       | 0*OPAL_PM_TIMEBASE_STOP  \
-		       | 0*OPAL_PM_LOSE_USER_CONTEXT \
-		       | 0*OPAL_PM_LOSE_HYP_CONTEXT \
-		       | 0*OPAL_PM_LOSE_FULL_CONTEXT \
-		       | 1*OPAL_PM_STOP_INST_FAST,
-		.pm_ctrl_reg_val = OPAL_PM_PSSCR_RL(1) \
-				 | OPAL_PM_PSSCR_MTL(3) \
-				 | OPAL_PM_PSSCR_TR(3),
-		.pm_ctrl_reg_mask = OPAL_PM_PSSCR_MASK },
+
 	{
 		.name = "stop1",
 		.latency_ns = 5000,
@@ -560,20 +547,6 @@  static struct cpu_idle_states power9_cpu_idle_states[] = {
 				 | OPAL_PM_PSSCR_EC,
 		.pm_ctrl_reg_mask = OPAL_PM_PSSCR_MASK },
 	{
-		.name = "stop2_lite", /* Enter stop2 with no state loss */
-		.latency_ns = 9900,
-		.residency_ns = 99000,
-		.flags = 0*OPAL_PM_DEC_STOP \
-		       | 0*OPAL_PM_TIMEBASE_STOP  \
-		       | 0*OPAL_PM_LOSE_USER_CONTEXT \
-		       | 0*OPAL_PM_LOSE_HYP_CONTEXT \
-		       | 0*OPAL_PM_LOSE_FULL_CONTEXT \
-		       | 1*OPAL_PM_STOP_INST_FAST,
-		.pm_ctrl_reg_val = OPAL_PM_PSSCR_RL(2) \
-				 | OPAL_PM_PSSCR_MTL(3) \
-				 | OPAL_PM_PSSCR_TR(3),
-		.pm_ctrl_reg_mask = OPAL_PM_PSSCR_MASK },
-	{
 		.name = "stop2",
 		.latency_ns = 10000,
 		.residency_ns = 100000,