Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   "Zhou Ti (x2019cwm)" <x2019cwm@stfx.ca>
To:     "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        Linux PM <linux-pm@vger.kernel.org>
CC:     LKML <linux-kernel@vger.kernel.org>,
        Frederic Weisbecker <frederic@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Thomas Gleixner <tglx@linutronix.de>
Subject: Re: [PATCH v1 5/5] cpuidle: menu: Take negative "sleep length" values
 into account
Thread-Topic: [PATCH v1 5/5] cpuidle: menu: Take negative "sleep length"
 values into account
Thread-Index: AQHXJMqW2cL1o6UBuEaCN31sThmNSKqbxtcz
Date:   Tue, 30 Mar 2021 01:59:40 +0000
Message-ID: <YTBPR01MB3262DB49961DBCCE96C51AB8C47D9@YTBPR01MB3262.CANPRD01.PROD.OUTLOOK.COM>
References: <2764850.e9J7NaK4W3@kreacher>,<7927358.NyiUUSuA9g@kreacher>
In-Reply-To: <7927358.NyiUUSuA9g@kreacher>
Accept-Language: en-CA, zh-CN, en-US
Content-Language: en-CA
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: e0302d3a-5216-464d-dc71-08d8f31f7b40
X-MS-Exchange-CrossTenant-originalarrivaltime: 30 Mar 2021 01:59:40.8510
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 8c46abc7-960b-4124-8950-1628b2b192f9
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: /obn9Fg2RdzBuYYRSsjNtXmjGgLwpFMM4umR6sK5mlybw4FL2VtSadTyQS6vel9FenPjdhWsNUsSUDqwELb/TA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: YT1PR01MB2412
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Mon 2021-03-29 14:37 Rafael J. Wysocki wrote:=0A=
> Make the menu governor check the tick_nohz_get_next_hrtimer()=0A=
> return value so as to avoid dealing with negative "sleep length"=0A=
> values and make it use that value directly when the tick is stopped.=0A=
> =0A=
> While at it, rename local variable delta_next in menu_select() to=0A=
> delta_tick which better reflects its purpose.=0A=
> =0A=
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>=0A=
> ---=0A=
>  drivers/cpuidle/governors/menu.c |   17 +++++++++++------=0A=
>  1 file changed, 11 insertions(+), 6 deletions(-)=0A=
> =0A=
> Index: linux-pm/drivers/cpuidle/governors/menu.c=0A=
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=0A=
> --- linux-pm.orig/drivers/cpuidle/governors/menu.c=0A=
> +++ linux-pm/drivers/cpuidle/governors/menu.c=0A=
> @@ -271,7 +271,7 @@ static int menu_select(struct cpuidle_dr=0A=
>          u64 predicted_ns;=0A=
>          u64 interactivity_req;=0A=
>          unsigned long nr_iowaiters;=0A=
> -       ktime_t delta_next;=0A=
> +       ktime_t delta, delta_tick;=0A=
>          int i, idx;=0A=
>  =0A=
>          if (data->needs_update) {=0A=
> @@ -280,7 +280,12 @@ static int menu_select(struct cpuidle_dr=0A=
>          }=0A=
>  =0A=
>          /* determine the expected residency time, round up */=0A=
> -       data->next_timer_ns =3D tick_nohz_get_sleep_length(&delta_next);=
=0A=
> +       delta =3D tick_nohz_get_sleep_length(&delta_tick);=0A=
> +       if (unlikely(delta < 0)) {=0A=
> +               delta =3D 0;=0A=
> +               delta_tick =3D 0;=0A=
> +       }=0A=
> +       data->next_timer_ns =3D delta;=0A=
>  =0A=
>          nr_iowaiters =3D nr_iowait_cpu(dev->cpu);=0A=
>          data->bucket =3D which_bucket(data->next_timer_ns, nr_iowaiters)=
;=0A=
> @@ -318,7 +323,7 @@ static int menu_select(struct cpuidle_dr=0A=
>                   * state selection.=0A=
>                   */=0A=
>                  if (predicted_ns < TICK_NSEC)=0A=
> -                       predicted_ns =3D delta_next;=0A=
> +                       predicted_ns =3D data->next_timer_ns;=0A=
>          } else {=0A=
>                  /*=0A=
>                   * Use the performance multiplier and the user-configura=
ble=0A=
> @@ -377,7 +382,7 @@ static int menu_select(struct cpuidle_dr=0A=
>                           * stuck in the shallow one for too long.=0A=
>                           */=0A=
>                          if (drv->states[idx].target_residency_ns < TICK_=
NSEC &&=0A=
> -                           s->target_residency_ns <=3D delta_next)=0A=
> +                           s->target_residency_ns <=3D delta_tick)=0A=
>                                  idx =3D i;=0A=
>  =0A=
>                          return idx;=0A=
> @@ -399,7 +404,7 @@ static int menu_select(struct cpuidle_dr=0A=
>               predicted_ns < TICK_NSEC) && !tick_nohz_tick_stopped()) {=
=0A=
>                  *stop_tick =3D false;=0A=
>  =0A=
> -               if (idx > 0 && drv->states[idx].target_residency_ns > del=
ta_next) {=0A=
> +               if (idx > 0 && drv->states[idx].target_residency_ns > del=
ta_tick) {=0A=
>                          /*=0A=
>                           * The tick is not going to be stopped and the t=
arget=0A=
>                           * residency of the state to be returned is not =
within=0A=
> @@ -411,7 +416,7 @@ static int menu_select(struct cpuidle_dr=0A=
>                                          continue;=0A=
>  =0A=
>                                  idx =3D i;=0A=
> -                               if (drv->states[i].target_residency_ns <=
=3D delta_next)=0A=
> +                               if (drv->states[i].target_residency_ns <=
=3D delta_tick)=0A=
>                                          break;=0A=
>                          }=0A=
>                  }=0A=
=0A=
How about this.=0A=
I think it's possible to avoid the new variable delta.=0A=
=0A=
---=0A=
=0A=
--- linux-pm/drivers/cpuidle/governors/menu.c.orig	2021-03-29 22:44:02.3169=
71970 -0300=0A=
+++ linux-pm/drivers/cpuidle/governors/menu.c	2021-03-29 22:51:15.804377168=
 -0300=0A=
@@ -271,7 +271,7 @@ static int menu_select(struct cpuidle_dr=0A=
 	u64 predicted_ns;=0A=
 	u64 interactivity_req;=0A=
 	unsigned long nr_iowaiters;=0A=
-	ktime_t delta_next;=0A=
+	ktime_t delta_tick;=0A=
 	int i, idx;=0A=
 =0A=
 	if (data->needs_update) {=0A=
@@ -280,7 +280,12 @@ static int menu_select(struct cpuidle_dr=0A=
 	}=0A=
 =0A=
 	/* determine the expected residency time, round up */=0A=
-	data->next_timer_ns =3D tick_nohz_get_sleep_length(&delta_next);=0A=
+	data->next_timer_ns =3D tick_nohz_get_sleep_length(&delta_tick);=0A=
+=0A=
+	if (unlikely(data->next_timer_ns >> 63)) {=0A=
+		data->next_timer_ns =3D 0;=0A=
+		delta_tick =3D 0;=0A=
+	}=0A=
 =0A=
 	nr_iowaiters =3D nr_iowait_cpu(dev->cpu);=0A=
 	data->bucket =3D which_bucket(data->next_timer_ns, nr_iowaiters);=0A=
@@ -318,7 +323,7 @@ static int menu_select(struct cpuidle_dr=0A=
 		 * state selection.=0A=
 		 */=0A=
 		if (predicted_ns < TICK_NSEC)=0A=
-			predicted_ns =3D delta_next;=0A=
+			predicted_ns =3D data->next_timer_ns;=0A=
 	} else {=0A=
 		/*=0A=
 		 * Use the performance multiplier and the user-configurable=0A=
@@ -377,7 +382,7 @@ static int menu_select(struct cpuidle_dr=0A=
 			 * stuck in the shallow one for too long.=0A=
 			 */=0A=
 			if (drv->states[idx].target_residency_ns < TICK_NSEC &&=0A=
-			    s->target_residency_ns <=3D delta_next)=0A=
+			    s->target_residency_ns <=3D delta_tick)=0A=
 				idx =3D i;=0A=
 =0A=
 			return idx;=0A=
@@ -399,7 +404,7 @@ static int menu_select(struct cpuidle_dr=0A=
 	     predicted_ns < TICK_NSEC) && !tick_nohz_tick_stopped()) {=0A=
 		*stop_tick =3D false;=0A=
 =0A=
-		if (idx > 0 && drv->states[idx].target_residency_ns > delta_next) {=0A=
+		if (idx > 0 && drv->states[idx].target_residency_ns > delta_tick) {=0A=
 			/*=0A=
 			 * The tick is not going to be stopped and the target=0A=
 			 * residency of the state to be returned is not within=0A=
@@ -411,7 +416,7 @@ static int menu_select(struct cpuidle_dr=0A=
 					continue;=0A=
 =0A=
 				idx =3D i;=0A=
-				if (drv->states[i].target_residency_ns <=3D delta_next)=0A=
+				if (drv->states[i].target_residency_ns <=3D delta_tick)=0A=
 					break;=0A=
 			}=0A=
 		}=0A=