linux-kernel - Re: [PATCH 3/3] cpuidle: Select a different state on tick_broadcast

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAJZ5v0goEQC3xF11wOm7q3TA51WfaM_g1zBV+nKpVYyvH5fmag@mail.gmail.com>
Date: Wed, 21 Aug 2024 13:25:31 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Dhruva Gole <d-gole@...com>
Cc: "Rafael J. Wysocki" <rjw@...ysocki.net>, Preeti U Murthy <preeti@...ux.vnet.ibm.com>, peterz@...radead.org, 
	rlippert@...gle.com, daniel.lezcano@...aro.org, linux-pm@...r.kernel.org, 
	linus.walleij@...aro.org, rafael.j.wysocki@...el.com, 
	linux-kernel@...r.kernel.org, mingo@...hat.com, sudeep.holla@....com, 
	tglx@...utronix.de, linuxppc-dev@...ts.ozlabs.org, khilman@...com, 
	Ulf Hansson <ulf.hansson@...aro.org>
Subject: Re: [PATCH 3/3] cpuidle: Select a different state on
 tick_broadcast_enter() failures

On Wed, Aug 21, 2024 at 1:15 PM Dhruva Gole <d-gole@...com> wrote:
>
> Hi,
>
> On May 10, 2015 at 01:19:52 +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@...el.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@...el.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. */
> >
>
> --