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;
MIME-Version: 1.0
References: <2764850.e9J7NaK4W3@kreacher> <7927358.NyiUUSuA9g@kreacher> <YTBPR01MB3262DB49961DBCCE96C51AB8C47D9@YTBPR01MB3262.CANPRD01.PROD.OUTLOOK.COM>
In-Reply-To: <YTBPR01MB3262DB49961DBCCE96C51AB8C47D9@YTBPR01MB3262.CANPRD01.PROD.OUTLOOK.COM>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Tue, 30 Mar 2021 16:47:56 +0200
Message-ID: <CAJZ5v0g75+AFyQ-SP=ckKR5M171i7RaR1Qj66rUpHZujCFMqbA@mail.gmail.com>
Subject: Re: [PATCH v1 5/5] cpuidle: menu: Take negative "sleep length" values
 into account
To:     "Zhou Ti (x2019cwm)" <x2019cwm@stfx.ca>
Cc:     "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        Linux PM <linux-pm@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Frederic Weisbecker <frederic@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Thomas Gleixner <tglx@linutronix.de>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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.