Messages in this thread Patch in this message | | | From | "Rafael J. Wysocki" <> | Date | Thu, 12 Jan 2023 15:53:45 +0100 | Subject | Re: [PATCH v2 2/4] powercap: idle_inject: Add prepare/complete callbacks |
| |
On Wed, Nov 30, 2022 at 12:34 AM Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> wrote: > > The actual idle percentage can be less than the desired because of > interrupts.
This is somewhat unclear.
> 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?
> 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@linux.intel.com> > --- > 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.
> * @cpumask: mask of CPUs affected by idle injection > + * > + * This structure is used to define per instance idle inject device data. Each > + * instance has an idle duration, a run duration and mask of CPUs to inject > + * idle. > + * Actual idle is injected by calling kernel scheduler interface > + * play_idle_precise(). There are two optional callbacks which the caller can > + * register by calling idle_inject_register_full(): > + * prepare() - This callback is called just before calling play_idle_precise() > + * If this callback returns non zero value then > + * play_idle_precise() is not called. This means skip injecting > + * idle during this period. > + * complete() - This callback is called after calling play_idle_precise().
I would keep the format of the comment more consistent with the general information at the top and the member descriptions at the bottom.
> */ > struct idle_inject_device { > struct hrtimer timer; > unsigned int idle_duration_us; > unsigned int run_duration_us; > unsigned int latency_us; > + int (*prepare)(unsigned int cpu);
Can it be bool?
> + void (*complete)(unsigned int cpu); > unsigned long cpumask[]; > }; > > @@ -132,6 +150,7 @@ static void idle_inject_fn(unsigned int cpu) > { > struct idle_inject_device *ii_dev; > struct idle_inject_thread *iit; > + int ret;
This is redundant.
> > ii_dev = per_cpu(idle_inject_device, cpu); > iit = per_cpu_ptr(&idle_inject_thread, cpu); > @@ -141,8 +160,18 @@ static void idle_inject_fn(unsigned int cpu) > */ > iit->should_run = 0; > > + if (ii_dev->prepare) { > + ret = ii_dev->prepare(cpu); > + if (ret) > + goto skip; > + }
Because the above can be written as
if (ii_dev->prepare && ii_dev->prepare(cpu)) 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->complete) > + ii_dev->complete(cpu); > } > > /** > @@ -295,17 +324,23 @@ static int idle_inject_should_run(unsigned int cpu) > } > > /** > - * idle_inject_register - initialize idle injection on a set of CPUs > + * idle_inject_register_full - initialize idle injection on a set of CPUs > * @cpumask: CPUs to be affected by idle injection > + * @prepare: callback called before calling play_idle_precise() > + * @complete: callback called after calling play_idle_precise()
IMO it would be slightly cleaner to say "invoked" instead of "called".
> * > * This function creates an idle injection control device structure for the > - * given set of CPUs and initializes the timer associated with it. It does not > - * start any injection cycles. > + * given set of CPUs and initializes the timer associated with it. This > + * function also allows to register prepare() and complete() callbacks. > + * It does not start any injection cycles. > * > * 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_full(struct cpumask *cpumask, > + int (*prepare)(unsigned int cpu), > + void (*complete)(unsigned int cpu)) > { > struct idle_inject_device *ii_dev; > int cpu, cpu_rb; > @@ -318,6 +353,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->prepare = prepare; > + ii_dev->complete = complete; > > for_each_cpu(cpu, to_cpumask(ii_dev->cpumask)) { > > @@ -342,6 +379,23 @@ struct idle_inject_device *idle_inject_register(struct cpumask *cpumask) > > return NULL; > } > +EXPORT_SYMBOL_NS_GPL(idle_inject_register_full, IDLE_INJECT); > + > +/** > + * idle_inject_register - initialize idle injection on a set of CPUs > + * @cpumask: CPUs to be affected by idle injection > + * > + * This function creates an idle injection control device structure for the > + * given set of CPUs and initializes the timer associated with it. It does not > + * start any injection cycles. > + * > + * Return: NULL if memory allocation fails, idle injection control device > + * pointer on success.
It would be sufficient to say "Pass @cpumask to idle_inject_register_full() to initialize idle injection on a set of CPUs".
> + */ > +struct idle_inject_device *idle_inject_register(struct cpumask *cpumask) > +{ > + return idle_inject_register_full(cpumask, NULL, NULL); > +} > EXPORT_SYMBOL_NS_GPL(idle_inject_register, IDLE_INJECT); > > /** > diff --git a/include/linux/idle_inject.h b/include/linux/idle_inject.h > index fb88e23a99d3..e18d89793490 100644 > --- a/include/linux/idle_inject.h > +++ b/include/linux/idle_inject.h > @@ -13,6 +13,10 @@ struct idle_inject_device; > > struct idle_inject_device *idle_inject_register(struct cpumask *cpumask); > > +struct idle_inject_device *idle_inject_register_full(struct cpumask *cpumask, > + int (*prepare)(unsigned int cpu), > + void (*complete)(unsigned int cpu)); > + > void idle_inject_unregister(struct idle_inject_device *ii_dev); > > int idle_inject_start(struct idle_inject_device *ii_dev); > --
| |