Re: [RFC][PATCH] thermal/idle_inject: Support 100% idle injection

[Daniel Lezcano]

On 21/12/2022 21:36, srinivas pandruvada wrote:
> On Wed, 2022-12-21 at 14:43 +0100, Daniel Lezcano wrote:
> > Hi Srinivas,
> >
> >
> > On 09/12/2022 02:36, Srinivas Pandruvada wrote:
> > > The users of idle injection framework allow 100% idle injection.
> > > For
> > > example: thermal/cpuidle_cooling.c driver. When the ratio set to
> > > 100%,
> > > the runtime_duration becomes zero.
> > >
> > > In the function idle_inject_set_duration() in idle injection
> > > framework
> > > run_duration_us == 0 is silently ignored, without any error (it is
> > > a
> > > void function). So, the caller will assume that everything is fine
> > > and
> > > 100% idle is effective. But in reality the idle inject will be
> > > whatever
> > > set before.
> >
> > Good catch
> >
> > > There are two options:
> > > - The caller change their max state to 99% instead of 100% and
> > > document that 100% is not supported by idle inject framework
> > > - Support 100% idle support in idle inject framework
> >
> > Yes, from my POV a CPU being impossible to cool down for any reason
> > should end up by staying off.
> >
> > > Since there are other protections via RT throttling, this framework
> > > can
> > > allow 100% idle. The RT throttling will be activated at 95% idle by
> > > default. The caller disabling RT throttling and injecting 100%
> > > idle,
> > > should be aware that CPU can't be used at all.
> >
> > Would it make sense to write a trace in this case ?
>
> There is one printk already:
> printk_deferred_once("sched: RT throttling activated\n")
> You mean we should add
>
> trace_sched_* for this?


I meant the CPU is going 100% idle


> > > The idle inject timer is started for (run_duration_us +
> > > idle_duration_us)
> > > duration. Hence replace (run_duration_us && idle_duration_us) with
> > > (run_duration_us + idle_duration_us) in the function
> > > idle_inject_set_duration().
> >
> > Sounds good to me
> I will submit a patch for this.
>
> Thanks,
> Srinivas
>
> > > Signed-off-by: Srinivas Pandruvada
> > > <srinivas.pandruvada@xxxxxxxxxxxxxxx>
> > > ---
> > >    drivers/powercap/idle_inject.c | 2 +-
> > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/powercap/idle_inject.c
> > > b/drivers/powercap/idle_inject.c
> > > index f48e71501429..4a4fe60d2563 100644
> > > --- a/drivers/powercap/idle_inject.c
> > > +++ b/drivers/powercap/idle_inject.c
> > > @@ -184,7 +184,7 @@ void idle_inject_set_duration(struct
> > > idle_inject_device *ii_dev,
> > >                                unsigned int run_duration_us,
> > >                                unsigned int idle_duration_us)
> > >    {
> > > -       if (run_duration_us && idle_duration_us) {
> > > +       if (run_duration_us + idle_duration_us) {
> > >                  WRITE_ONCE(ii_dev->run_duration_us,
> > > run_duration_us);
> > >                  WRITE_ONCE(ii_dev->idle_duration_us,
> > > idle_duration_us);
> > >          }

--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog