Re: [PATCH v2 2/4] powercap: idle_inject: Add prepare/complete callbacks

[srinivas pandruvada]

On Thu, 2023-01-12 at 18:25 +0100, Rafael J. Wysocki wrote:
> On Thu, Jan 12, 2023 at 3:53 PM Rafael J. Wysocki <rafael@xxxxxxxxxx>
> wrote:
> > 
> > On Wed, Nov 30, 2022 at 12:34 AM Srinivas Pandruvada
> > <srinivas.pandruvada@xxxxxxxxxxxxxxx> wrote:
> > > 
> > > The actual idle percentage can be less than the desired because
> > > of
> > > interrupts.
> > 
> > This is somewhat unclear.
> > 
Will try to make it better.

> > > Since the objective for CPU Idle injection is for thermal
> > > control, there should be some way to compensate for lost idle
> > > percentage.
> > 
> > What does "lost idle percentage" mean here?
User space wants 50% idle, but because of interrupts on target cpus we
were less than 50% idle. This may be because of too many interrupt
wakes. If we measure from the hardware counters, this may say we were
45% idle. So we were 5% less than user desired setting.

> > 
> > > Some architectures provide interface to get actual idle percent
> > > observed
> > > by the hardware. So, the idle percent can be adjusted using the
> > > hardware
> > > feedback. For example, Intel CPUs provides package idle counters,
> > > which
> > > is currently used by intel powerclamp driver to adjust idle time.
> > > 
> > > The only way this can be done currently is by monitoring hardware
> > > idle
> > > percent from a different software thread. This can be avoided by
> > > adding
> > > callbacks.
> > > 
> > > Add a capability to register a prepare and complete callback
> > > during idle
> > > inject registry. Add a new register function
> > > idle_inject_register_full()
> > > which also allows to register callbacks.
> > > 
> > > If they are not NULL, then prepare callback is called before
> > > calling
> > > play_idle_precise() and complete callback is called after calling
> > > play_idle_precise().
> > > 
> > > If prepare callback is present and returns non 0 value then
> > > play_idle_precise() is not called to avoid over compensation.
> > 
> > This mechanism isn't particularly straightforward, but maybe
> > there's
> > no better way.
> > 
> > > Signed-off-by: Srinivas Pandruvada
> > > <srinivas.pandruvada@xxxxxxxxxxxxxxx>
> > > ---
> > > v2
> > > - Replace begin/end with prepare/complete
> > > - Add new interface idle_inject_register_full with callbacks
> > > - Update kernel doc
> > > - Update commit description
> > > 
> > >  drivers/powercap/idle_inject.c | 62
> > > +++++++++++++++++++++++++++++++---
> > >  include/linux/idle_inject.h    |  4 +++
> > >  2 files changed, 62 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/powercap/idle_inject.c
> > > b/drivers/powercap/idle_inject.c
> > > index dfa989182e71..f48e71501429 100644
> > > --- a/drivers/powercap/idle_inject.c
> > > +++ b/drivers/powercap/idle_inject.c
> > > @@ -63,13 +63,31 @@ struct idle_inject_thread {
> > >   * @idle_duration_us: duration of CPU idle time to inject
> > >   * @run_duration_us: duration of CPU run time to allow
> > >   * @latency_us: max allowed latency
> > > + * @prepare: Callback function which is called before calling
> > > + *             play_idle_precise()
> > > + * @complete: Callback function which is called after calling
> > > + *             play_idle_precise()
> > 
> > What about:
> > 
> > @prepare: Optional callback deciding whether or not to skip idle
> > injection in the given cycle.
> > @complete: Optional callback updating the state after idle
> > injection.
Looks good.

> 
> One more thing: ->complete() is not even used by powerclamp AFAICS,
> so
> I wouldn't add it at this time, because it isn't clear if it's going
> to be useful at all.
Sure.

Thanks,
Srinivas