| Message ID | 20221209013640.943210-1-srinivas.pandruvada@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] thermal/idle_inject: Support 100% idle injection | expand |
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 ? > 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 > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.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); > }
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? > > > 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@linux.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); > > } >
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@linux.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); >>> } >> >
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); }
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. 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 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. 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(). Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> --- drivers/powercap/idle_inject.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)