[v1,5/5] cpuidle: menu: Take negative "sleep length" values into account
diff mbox series

Message ID 7927358.NyiUUSuA9g@kreacher
State New, archived
Headers show
Series
  • cpuidle: Take possible negative "sleep length" values into account
Related show

Commit Message

Rafael J. Wysocki March 29, 2021, 6:37 p.m. UTC
From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Subject: [PATCH] cpuidle: menu: Take negative "sleep length" values into account

Make the menu governor check the tick_nohz_get_next_hrtimer()
return value so as to avoid dealing with negative "sleep length"
values and make it use that value directly when the tick is stopped.

While at it, rename local variable delta_next in menu_select() to
delta_tick which better reflects its purpose.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpuidle/governors/menu.c |   17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

Comments

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

How about this.
I think it's possible to avoid the new variable delta.

---

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

Well, not really.  Using a new local var is cleaner IMO.

Patch
diff mbox series

Index: linux-pm/drivers/cpuidle/governors/menu.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/governors/menu.c
+++ linux-pm/drivers/cpuidle/governors/menu.c
@@ -271,7 +271,7 @@  static int menu_select(struct cpuidle_dr
 	u64 predicted_ns;
 	u64 interactivity_req;
 	unsigned long nr_iowaiters;
-	ktime_t delta_next;
+	ktime_t delta, delta_tick;
 	int i, idx;
 
 	if (data->needs_update) {
@@ -280,7 +280,12 @@  static int menu_select(struct cpuidle_dr
 	}
 
 	/* determine the expected residency time, round up */
-	data->next_timer_ns = tick_nohz_get_sleep_length(&delta_next);
+	delta = tick_nohz_get_sleep_length(&delta_tick);
+	if (unlikely(delta < 0)) {
+		delta = 0;
+		delta_tick = 0;
+	}
+	data->next_timer_ns = delta;
 
 	nr_iowaiters = nr_iowait_cpu(dev->cpu);
 	data->bucket = which_bucket(data->next_timer_ns, nr_iowaiters);
@@ -318,7 +323,7 @@  static int menu_select(struct cpuidle_dr
 		 * state selection.
 		 */
 		if (predicted_ns < TICK_NSEC)
-			predicted_ns = delta_next;
+			predicted_ns = data->next_timer_ns;
 	} else {
 		/*
 		 * Use the performance multiplier and the user-configurable
@@ -377,7 +382,7 @@  static int menu_select(struct cpuidle_dr
 			 * stuck in the shallow one for too long.
 			 */
 			if (drv->states[idx].target_residency_ns < TICK_NSEC &&
-			    s->target_residency_ns <= delta_next)
+			    s->target_residency_ns <= delta_tick)
 				idx = i;
 
 			return idx;
@@ -399,7 +404,7 @@  static int menu_select(struct cpuidle_dr
 	     predicted_ns < TICK_NSEC) && !tick_nohz_tick_stopped()) {
 		*stop_tick = false;
 
-		if (idx > 0 && drv->states[idx].target_residency_ns > delta_next) {
+		if (idx > 0 && drv->states[idx].target_residency_ns > delta_tick) {
 			/*
 			 * The tick is not going to be stopped and the target
 			 * residency of the state to be returned is not within
@@ -411,7 +416,7 @@  static int menu_select(struct cpuidle_dr
 					continue;
 
 				idx = i;
-				if (drv->states[i].target_residency_ns <= delta_next)
+				if (drv->states[i].target_residency_ns <= delta_tick)
 					break;
 			}
 		}