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

[Rafael J. Wysocki]

On Thu, Dec 22, 2022 at 6:36 PM srinivas pandruvada
<srinivas.pandruvada@xxxxxxxxxxxxxxx> wrote:
>
> Hi Daniel,
>
> On Thu, 2022-12-22 at 10:50 +0100, Daniel Lezcano wrote:
> >
> > Hi Srinivas,
> >
> >
> > On 21/12/2022 21:58, srinivas pandruvada wrote:
> > > Hi Daniel,
> > >
> > > On Wed, 2022-12-21 at 15:52 +0100, Daniel Lezcano wrote:
> > > >
> > > > Hi Srinivas,
> > > >
> > > > On 30/11/2022 00:34, Srinivas Pandruvada wrote:
> > > > > The actual idle percentage can be less than the desired because
> > > > > of
> > > > > interrupts. Since the objective for CPU Idle injection is for
> > > > > thermal
> > > > > control, there should be some way to compensate for lost idle
> > > > > percentage.
> > > > > 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.
> > > > Can you provide an example in terms of timings?
> > > >
> > > > I'm not getting how 'prepare' would do by returning a positive
> > > > value
> > > > to
> > > > skip the play_idle_precise() and what will do 'complete' ?
> > > >
> > > intel_powerclamp has a logic where if the current idle percentage
> > > observed from hardware is more than the desired target inject
> > > percent,
> > > it skips calling play_idle().
> > >
> > > For example if you want to inject 50% idle and system is naturally
> > > idle
> > > for 60%, there is no use of calling play_idle in the idle injection
> > > framework to induce more idle. In this way a workload can run
> > > immediately.
> > >
> > > So trying to emulate the same logic by using powercap/idle_inject
> > > framework. So prepare() callback in the intel_powerclamp driver
> > > calls
> > > the existing function to check if idle-inject should skip for this
> > > time
> > > or not.
> >
> > The function 'prepare' has the 'cpu' parameter. How can it compare
> > with
> > the desired idle duration as this information is not passed to the
> > callback ?
> Good question.
>
> Calling driver knows what idle_duration he set.
> In my first version, I passed *idle_duration (with current
> idle_duration set), so the caller can change this for the current
> play_idle call if required for one time.
>
> But in powerclamp case we either skip the whole play_idle or not. It
> doesn't change idle duration. So didn't add.
>
> But we can add this back.

I don't think that it is necessary at this point.

Since powerclamp is the only user and it doesn't need idle_duration, I
would just not add it ATM.

I have a couple of other comments to the patch, but let me send them separately.