| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAJZ5v0jD-a__naERH3MsB1yDQwNkwKcL=aSP3JEdiD11s76gDA@mail.gmail.com>
Date: Wed, 9 Nov 2022 15:48:13 +0100
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>
Cc: rui.zhang@...el.com, rafael@...nel.org, daniel.lezcano@...aro.org,
amitk@...nel.org, linux-kernel@...r.kernel.org,
linux-pm@...r.kernel.org
Subject: Re: [PATCH 2/4] powercap: idle_inject: Add begin/end callbacks
On Tue, Nov 8, 2022 at 4:04 AM Srinivas Pandruvada
<srinivas.pandruvada@...ux.intel.com> 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.
> 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.
> 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.
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>
> ---
> 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().
> {
> 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);
>
> --
Powered by blists - more mailing lists