Re: [PATCH 2/4] powercap: idle_inject: Add begin/end callbacks

[srinivas pandruvada]

On Wed, 2022-11-09 at 15:48 +0100, Rafael J. Wysocki wrote:
> On Tue, Nov 8, 2022 at 4:04 AM Srinivas Pandruvada
> <srinivas.pandruvada@xxxxxxxxxxxxxxx> wrote:
> > 
> > The actual CPU Idle percent can be different than what can be
> > observed
> > from the hardware.
> 
> Can you expand this a bit, please?  It is not clear what the "CPU
> idle
> percent" and "observed from the hardware" phrases mean here.
Sure.

> 
> > Since the objective for CPU Idle injection is for
> > thermal control, the idle percent observed by the hardware is more
> > relevant.
> > 
> > To account for hardware feedback the actual runtime/idle time
> > should be
> > adjusted.
> > 
> > Add a capability to register a begin and end callback during call
> > to
> 
> I would call them "prepare" and "complete" without the "idle_inject_"
> prefix.
OK

> 
> > idle_inject_register(). If they are not NULL, then begin callback
> > is
> > called before calling play_idle_precise() and end callback is
> > called
> > after play_idle_precise().
> > 
> > If begin callback is present and returns non 0 value then
> > play_idle_precise() is not called as it means there is some over
> > compensation.
> 
> This behavior needs to be documented somewhere other than the patch
> changelog.
We can add a kernel doc and add there.

> 
> > Signed-off-by: Srinivas Pandruvada <   
> > srinivas.pandruvada@xxxxxxxxxxxxxxx>
> > ---
> >  drivers/powercap/idle_inject.c    | 19 ++++++++++++++++++-
> >  drivers/thermal/cpuidle_cooling.c |  2 +-
> >  include/linux/idle_inject.h       |  4 +++-
> >  3 files changed, 22 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/powercap/idle_inject.c
> > b/drivers/powercap/idle_inject.c
> > index e73885bd9065..14968b0ff133 100644
> > --- a/drivers/powercap/idle_inject.c
> > +++ b/drivers/powercap/idle_inject.c
> > @@ -70,6 +70,8 @@ struct idle_inject_device {
> >         unsigned int idle_duration_us;
> >         unsigned int run_duration_us;
> >         unsigned int latency_us;
> > +       int (*idle_inject_begin)(unsigned int cpu);
> > +       void (*idle_inject_end)(unsigned int cpu);
> 
> The comment above needs to be updated.  Also please see the remark
> above regarding callback names.
> 
> >         unsigned long cpumask[];
> >  };
> > 
> > @@ -132,6 +134,7 @@ static void idle_inject_fn(unsigned int cpu)
> >  {
> >         struct idle_inject_device *ii_dev;
> >         struct idle_inject_thread *iit;
> > +       int ret;
> > 
> >         ii_dev = per_cpu(idle_inject_device, cpu);
> >         iit = per_cpu_ptr(&idle_inject_thread, cpu);
> > @@ -141,8 +144,18 @@ static void idle_inject_fn(unsigned int cpu)
> >          */
> >         iit->should_run = 0;
> > 
> > +       if (ii_dev->idle_inject_begin) {
> > +               ret = ii_dev->idle_inject_begin(cpu);
> > +               if (ret)
> > +                       goto skip;
> > +       }
> > +
> >         play_idle_precise(READ_ONCE(ii_dev->idle_duration_us) *
> > NSEC_PER_USEC,
> >                           READ_ONCE(ii_dev->latency_us) *
> > NSEC_PER_USEC);
> > +
> > +skip:
> > +       if (ii_dev->idle_inject_end)
> > +               ii_dev->idle_inject_end(cpu);
> >  }
> > 
> >  /**
> > @@ -302,7 +315,9 @@ static int idle_inject_should_run(unsigned int
> > cpu)
> >   * Return: NULL if memory allocation fails, idle injection control
> > device
> >   * pointer on success.
> >   */
> > -struct idle_inject_device *idle_inject_register(struct cpumask
> > *cpumask)
> > +struct idle_inject_device *idle_inject_register(struct cpumask
> > *cpumask,
> > +                                               int
> > (*idle_inject_begin)(unsigned int cpu),
> > +                                               void
> > (*idle_inject_end)(unsigned int cpu))
> 
> Instead of modifying this, I would add something like
> idle_inject_register_full() that will take the callback arguments and
> will be called internally by idle_inject_register().
OK

Thanks,
Srinivas

> 
> >  {
> >         struct idle_inject_device *ii_dev;
> >         int cpu, cpu_rb;
> > @@ -315,6 +330,8 @@ struct idle_inject_device
> > *idle_inject_register(struct cpumask *cpumask)
> >         hrtimer_init(&ii_dev->timer, CLOCK_MONOTONIC,
> > HRTIMER_MODE_REL);
> >         ii_dev->timer.function = idle_inject_timer_fn;
> >         ii_dev->latency_us = UINT_MAX;
> > +       ii_dev->idle_inject_begin = idle_inject_begin;
> > +       ii_dev->idle_inject_end = idle_inject_end;
> > 
> >         for_each_cpu(cpu, to_cpumask(ii_dev->cpumask)) {
> > 
> > diff --git a/drivers/thermal/cpuidle_cooling.c
> > b/drivers/thermal/cpuidle_cooling.c
> > index 4f41102e8b16..e8b35b3b5767 100644
> > --- a/drivers/thermal/cpuidle_cooling.c
> > +++ b/drivers/thermal/cpuidle_cooling.c
> > @@ -184,7 +184,7 @@ static int __cpuidle_cooling_register(struct
> > device_node *np,
> >                 goto out;
> >         }
> > 
> > -       ii_dev = idle_inject_register(drv->cpumask);
> > +       ii_dev = idle_inject_register(drv->cpumask, NULL, NULL);
> 
> So this change would not be necessary any more.
> 
> >         if (!ii_dev) {
> >                 ret = -EINVAL;
> >                 goto out_kfree;
> > diff --git a/include/linux/idle_inject.h
> > b/include/linux/idle_inject.h
> > index fb88e23a99d3..73f3414fafe2 100644
> > --- a/include/linux/idle_inject.h
> > +++ b/include/linux/idle_inject.h
> > @@ -11,7 +11,9 @@
> >  /* private idle injection device structure */
> >  struct idle_inject_device;
> > 
> > -struct idle_inject_device *idle_inject_register(struct cpumask
> > *cpumask);
> > +struct idle_inject_device *idle_inject_register(struct cpumask
> > *cpumask,
> > +                                               int
> > (*idle_inject_begin)(unsigned int cpu),
> > +                                               void
> > (*idle_inject_end)(unsigned int cpu));
> > 
> >  void idle_inject_unregister(struct idle_inject_device *ii_dev);
> > 
> > --