[PATCH 3/3] cpuidle: Select a different state on tick_broadcast_enter() failures

[Rafael J. Wysocki]

On Wed, Aug 21, 2024 at 1:15 PM Dhruva Gole <d-gole at ti.com> wrote:
>
> Hi,
>
> On May 10, 2015 at 01:19:52 +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki at intel.com>
> >
> > If tick_broadcast_enter() fails in cpuidle_enter_state(),
> > try to find another idle state to enter instead of invoking
> > default_idle_call() immediately and returning -EBUSY which
> > should increase the chances of saving some energy in those
> > cases.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki at intel.com>
> > ---
>
> Found this during code review, hence dug up this old thread again,
>
> >  drivers/cpuidle/cpuidle.c |   20 +++++++++++++++-----
> >  1 file changed, 15 insertions(+), 5 deletions(-)
> >
> > Index: linux-pm/drivers/cpuidle/cpuidle.c
> > ===================================================================
> > --- linux-pm.orig/drivers/cpuidle/cpuidle.c
> > +++ linux-pm/drivers/cpuidle/cpuidle.c
> > @@ -73,7 +73,10 @@ int cpuidle_play_dead(void)
> >  }
> >
> >  static int find_deepest_state(struct cpuidle_driver *drv,
> > -                           struct cpuidle_device *dev, bool freeze)
> > +                           struct cpuidle_device *dev,
> > +                           unsigned int max_latency,
> > +                           unsigned int forbidden_flags,
> > +                           bool freeze)
> >  {
> >       unsigned int latency_req = 0;
> >       int i, ret = freeze ? -1 : CPUIDLE_DRIVER_STATE_START - 1;
> > @@ -83,6 +86,8 @@ static int find_deepest_state(struct cpu
> >               struct cpuidle_state_usage *su = &dev->states_usage[i];
> >
> >               if (s->disabled || su->disable || s->exit_latency <= latency_req
> > +                 || s->exit_latency > max_latency
> > +                 || (s->flags & forbidden_flags)
> >                   || (freeze && !s->enter_freeze))
> >                       continue;
> >
> > @@ -100,7 +105,7 @@ static int find_deepest_state(struct cpu
> >  int cpuidle_find_deepest_state(struct cpuidle_driver *drv,
> >                              struct cpuidle_device *dev)
> >  {
> > -     return find_deepest_state(drv, dev, false);
> > +     return find_deepest_state(drv, dev, UINT_MAX, 0, false);
> >  }
> >
> >  static void enter_freeze_proper(struct cpuidle_driver *drv,
> > @@ -139,7 +144,7 @@ int cpuidle_enter_freeze(struct cpuidle_
> >        * that interrupts won't be enabled when it exits and allows the tick to
> >        * be frozen safely.
> >        */
> > -     index = find_deepest_state(drv, dev, true);
> > +     index = find_deepest_state(drv, dev, UINT_MAX, 0, true);
> >       if (index >= 0)
> >               enter_freeze_proper(drv, dev, index);
> >
> > @@ -168,8 +173,13 @@ int cpuidle_enter_state(struct cpuidle_d
> >        * CPU as a broadcast timer, this call may fail if it is not available.
> >        */
> >       if (broadcast && tick_broadcast_enter()) {
> > -             default_idle_call();
> > -             return -EBUSY;
> > +             index = find_deepest_state(drv, dev, target_state->exit_latency,
> > +                                        CPUIDLE_FLAG_TIMER_STOP, false);
> > +             if (index < 0) {
>
> Would this condition ever meet?
> If you see, the ret inside find_deepest_state is always starting with a 0 and
> then nobody is ever really making it negative again. So the func either
> returns a 0 or some positive value right?
>
> Since nobody has probably raised an issue about this in 9 years, is this
> basically dead code inside the if?

Yes, it is dead code now.

> Let me know what needs to be done here, I'd be happy to patch this up.

Please feel free to send a patch removing the redundant check.

> > +                     default_idle_call();
> > +                     return -EBUSY;
> > +             }
> > +             target_state = &drv->states[index];
> >       }
> >
> >       /* Take note of the planned idle state. */
> >
>
> --