[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <8ebd3fba-f2a1-9fbd-917e-05d5d1a41735@linaro.org>
Date: Wed, 21 Dec 2022 21:43:24 +0100
From: Daniel Lezcano <daniel.lezcano@...aro.org>
To: srinivas pandruvada <srinivas.pandruvada@...ux.intel.com>,
rafael@...nel.org
Cc: linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC][PATCH] thermal/idle_inject: Support 100% idle injection
On 21/12/2022 21:36, srinivas pandruvada wrote:
> On Wed, 2022-12-21 at 14:43 +0100, Daniel Lezcano wrote:
>>
>> Hi Srinivas,
>>
>>
>> On 09/12/2022 02:36, Srinivas Pandruvada wrote:
>>> The users of idle injection framework allow 100% idle injection.
>>> For
>>> example: thermal/cpuidle_cooling.c driver. When the ratio set to
>>> 100%,
>>> the runtime_duration becomes zero.
>>>
>>> In the function idle_inject_set_duration() in idle injection
>>> framework
>>> run_duration_us == 0 is silently ignored, without any error (it is
>>> a
>>> void function). So, the caller will assume that everything is fine
>>> and
>>> 100% idle is effective. But in reality the idle inject will be
>>> whatever
>>> set before.
>>
>> Good catch
>>
>>> There are two options:
>>> - The caller change their max state to 99% instead of 100% and
>>> document that 100% is not supported by idle inject framework
>>> - Support 100% idle support in idle inject framework
>>
>> Yes, from my POV a CPU being impossible to cool down for any reason
>> should end up by staying off.
>>
>>> Since there are other protections via RT throttling, this framework
>>> can
>>> allow 100% idle. The RT throttling will be activated at 95% idle by
>>> default. The caller disabling RT throttling and injecting 100%
>>> idle,
>>> should be aware that CPU can't be used at all.
>>
>> Would it make sense to write a trace in this case ?
>
> There is one printk already:
> printk_deferred_once("sched: RT throttling activated\n")
> You mean we should add
>
> trace_sched_* for this?
I meant the CPU is going 100% idle
>>> The idle inject timer is started for (run_duration_us +
>>> idle_duration_us)
>>> duration. Hence replace (run_duration_us && idle_duration_us) with
>>> (run_duration_us + idle_duration_us) in the function
>>> idle_inject_set_duration().
>>
>> Sounds good to me
>>
> I will submit a patch for this.
>
> Thanks,
> Srinivas
>
>>> Signed-off-by: Srinivas Pandruvada
>>> <srinivas.pandruvada@...ux.intel.com>
>>> ---
>>> drivers/powercap/idle_inject.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/powercap/idle_inject.c
>>> b/drivers/powercap/idle_inject.c
>>> index f48e71501429..4a4fe60d2563 100644
>>> --- a/drivers/powercap/idle_inject.c
>>> +++ b/drivers/powercap/idle_inject.c
>>> @@ -184,7 +184,7 @@ void idle_inject_set_duration(struct
>>> idle_inject_device *ii_dev,
>>> unsigned int run_duration_us,
>>> unsigned int idle_duration_us)
>>> {
>>> - if (run_duration_us && idle_duration_us) {
>>> + if (run_duration_us + idle_duration_us) {
>>> WRITE_ONCE(ii_dev->run_duration_us,
>>> run_duration_us);
>>> WRITE_ONCE(ii_dev->idle_duration_us,
>>> idle_duration_us);
>>> }
>>
>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
Powered by blists - more mailing lists