Re: [PATCH V6] powercap/drivers/idle_injection: Add an idle injection framework

[Daniel Lezcano]

On 12/06/2018 19:35, Peter Zijlstra wrote:

[ ... ]

 Yes, drifting is not an issue if that happens. This scheme is simpler
 and safer than setting the timer ahead before waking up the tasks with
 the risk it expires before all the tasks ended their idle cycles.
>>>
>>> sloppy though..
>>
>> Ok, do you prefer to see the timer set in the wakeup function and thus
>> having a periodic tick for the idle injection ?
> 
> I think having a HRTIMER_RESTART handler that does hrtimer_forward() is
> the most sensible. You will end up having to deal with threads not being
> ready, but I think that's not a real problem.

Thanks for the restartable timer suggestion. I'm finishing polishing the
code and definitively the result is much more nicer.


-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH V6] powercap/drivers/idle_injection: Add an idle injection framework

[Rafael J. Wysocki]

On Wed, Jun 13, 2018 at 10:35 PM, Daniel Lezcano
 wrote:
> On 13/06/2018 22:32, Pandruvada, Srinivas wrote:
>> On Wed, 2018-06-13 at 22:19 +0200, Daniel Lezcano wrote:
>>> On 13/06/2018 22:07, Pandruvada, Srinivas wrote:
 On Wed, 2018-06-13 at 22:00 +0200, Daniel Lezcano wrote:
> On 13/06/2018 17:54, Pandruvada, Srinivas wrote:
>> On Tue, 2018-06-12 at 14:00 +0200, Daniel Lezcano wrote:
>>> Initially, the cpu_cooling device for ARM was changed by
>>> adding a
>>> new
>>> policy inserting idle cycles. The intel_powerclamp driver
>>> does a
>>> similar action.
>>>
>>> Instead of implementing idle injections privately in the
>>> cpu_cooling
>>> device, move the idle injection code in a dedicated framework
>>> and
>>> give
>>> the opportunity to other frameworks to make use of it.
>>>
>>> The framework relies on the smpboot kthreads which handles
>>> via
>>> its
>>> main loop the common code for hotplugging and [un]parking.
>>>
>>> This code was previously tested with the cpu cooling device
>>> and
>>> went
>>> through several iterations. It results now in split code and
>>> API
>>> exported in the header file. It was tested with the cpu
>>> cooling
>>> device
>>> with success.
>>>
>>
>> May be I have missed, but I don't see any use of powercap
>> sysfs.
>>
>> We created powercap sys that user space is presented a common
>> interface
>> for power capping. The RAPL driver was also submitted as
>> cooling
>> device
>> before, but suggestion was to create this powercap.
>>
>> So if powercap interface is not enough then may be we should
>> enhance
>> that.
>
> We are creating a framework for idle injection. This framework
> can
> then
> be used by the cpu_cooling device, the power_clamp and in
> addition a
> power capping for ARM (if it makes sense).

 But in this case, why in drivers/powercap folder as this is another
 framework?
>>>
>>> I presented at OSPM 2nd the cpu idle cooling device. It is ARM
>>> specific
>>> but has some similarities with the power clamp driver.
>>>
>>> Some board come with power numbers defined in their DT and from there
>>> we
>>> are able to compute a targeted power by injecting idle cycles.
>>>
>>> As the idle injection allows to do also power capping, Rafael told me
>>> to
>>> move that to a framework (may be better say a component with exported
>>> services or library) and put it in drivers/powercap.
>> So Rafael wants it to be in powercap.
>
> That is what I understood.

While this is not exactly power capping, the purpose of this mechanism
is to prevent too much power from being drawn by the system, so IMO it
falls under the same broad category.

Re: [PATCH V6] powercap/drivers/idle_injection: Add an idle injection framework

[Daniel Lezcano]

On 13/06/2018 22:32, Pandruvada, Srinivas wrote:
> On Wed, 2018-06-13 at 22:19 +0200, Daniel Lezcano wrote:
>> On 13/06/2018 22:07, Pandruvada, Srinivas wrote:
>>> On Wed, 2018-06-13 at 22:00 +0200, Daniel Lezcano wrote:
 On 13/06/2018 17:54, Pandruvada, Srinivas wrote:
> On Tue, 2018-06-12 at 14:00 +0200, Daniel Lezcano wrote:
>> Initially, the cpu_cooling device for ARM was changed by
>> adding a
>> new
>> policy inserting idle cycles. The intel_powerclamp driver
>> does a
>> similar action.
>>
>> Instead of implementing idle injections privately in the
>> cpu_cooling
>> device, move the idle injection code in a dedicated framework
>> and
>> give
>> the opportunity to other frameworks to make use of it.
>>
>> The framework relies on the smpboot kthreads which handles
>> via
>> its
>> main loop the common code for hotplugging and [un]parking.
>>
>> This code was previously tested with the cpu cooling device
>> and
>> went
>> through several iterations. It results now in split code and
>> API
>> exported in the header file. It was tested with the cpu
>> cooling
>> device
>> with success.
>>
>
> May be I have missed, but I don't see any use of powercap
> sysfs.
>
> We created powercap sys that user space is presented a common
> interface
> for power capping. The RAPL driver was also submitted as
> cooling
> device
> before, but suggestion was to create this powercap.
>
> So if powercap interface is not enough then may be we should
> enhance
> that.

 We are creating a framework for idle injection. This framework
 can
 then
 be used by the cpu_cooling device, the power_clamp and in
 addition a
 power capping for ARM (if it makes sense).
>>>
>>> But in this case, why in drivers/powercap folder as this is another
>>> framework?
>>
>> I presented at OSPM 2nd the cpu idle cooling device. It is ARM
>> specific
>> but has some similarities with the power clamp driver.
>>
>> Some board come with power numbers defined in their DT and from there
>> we
>> are able to compute a targeted power by injecting idle cycles.
>>
>> As the idle injection allows to do also power capping, Rafael told me
>> to
>> move that to a framework (may be better say a component with exported
>> services or library) and put it in drivers/powercap.
> So Rafael wants it to be in powercap.

That is what I understood.




-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH V6] powercap/drivers/idle_injection: Add an idle injection framework

[Pandruvada, Srinivas]

On Wed, 2018-06-13 at 22:19 +0200, Daniel Lezcano wrote:
> On 13/06/2018 22:07, Pandruvada, Srinivas wrote:
> > On Wed, 2018-06-13 at 22:00 +0200, Daniel Lezcano wrote:
> > > On 13/06/2018 17:54, Pandruvada, Srinivas wrote:
> > > > On Tue, 2018-06-12 at 14:00 +0200, Daniel Lezcano wrote:
> > > > > Initially, the cpu_cooling device for ARM was changed by
> > > > > adding a
> > > > > new
> > > > > policy inserting idle cycles. The intel_powerclamp driver
> > > > > does a
> > > > > similar action.
> > > > > 
> > > > > Instead of implementing idle injections privately in the
> > > > > cpu_cooling
> > > > > device, move the idle injection code in a dedicated framework
> > > > > and
> > > > > give
> > > > > the opportunity to other frameworks to make use of it.
> > > > > 
> > > > > The framework relies on the smpboot kthreads which handles
> > > > > via
> > > > > its
> > > > > main loop the common code for hotplugging and [un]parking.
> > > > > 
> > > > > This code was previously tested with the cpu cooling device
> > > > > and
> > > > > went
> > > > > through several iterations. It results now in split code and
> > > > > API
> > > > > exported in the header file. It was tested with the cpu
> > > > > cooling
> > > > > device
> > > > > with success.
> > > > > 
> > > > 
> > > > May be I have missed, but I don't see any use of powercap
> > > > sysfs.
> > > > 
> > > > We created powercap sys that user space is presented a common
> > > > interface
> > > > for power capping. The RAPL driver was also submitted as
> > > > cooling
> > > > device
> > > > before, but suggestion was to create this powercap.
> > > > 
> > > > So if powercap interface is not enough then may be we should
> > > > enhance
> > > > that.
> > > 
> > > We are creating a framework for idle injection. This framework
> > > can
> > > then
> > > be used by the cpu_cooling device, the power_clamp and in
> > > addition a
> > > power capping for ARM (if it makes sense).
> > 
> > But in this case, why in drivers/powercap folder as this is another
> > framework?
> 
> I presented at OSPM 2nd the cpu idle cooling device. It is ARM
> specific
> but has some similarities with the power clamp driver.
> 
> Some board come with power numbers defined in their DT and from there
> we
> are able to compute a targeted power by injecting idle cycles.
> 
> As the idle injection allows to do also power capping, Rafael told me
> to
> move that to a framework (may be better say a component with exported
> services or library) and put it in drivers/powercap.
So Rafael wants it to be in powercap.


smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH V6] powercap/drivers/idle_injection: Add an idle injection framework

[Daniel Lezcano]

On 13/06/2018 22:07, Pandruvada, Srinivas wrote:
> On Wed, 2018-06-13 at 22:00 +0200, Daniel Lezcano wrote:
>> On 13/06/2018 17:54, Pandruvada, Srinivas wrote:
>>> On Tue, 2018-06-12 at 14:00 +0200, Daniel Lezcano wrote:
 Initially, the cpu_cooling device for ARM was changed by adding a
 new
 policy inserting idle cycles. The intel_powerclamp driver does a
 similar action.

 Instead of implementing idle injections privately in the
 cpu_cooling
 device, move the idle injection code in a dedicated framework and
 give
 the opportunity to other frameworks to make use of it.

 The framework relies on the smpboot kthreads which handles via
 its
 main loop the common code for hotplugging and [un]parking.

 This code was previously tested with the cpu cooling device and
 went
 through several iterations. It results now in split code and API
 exported in the header file. It was tested with the cpu cooling
 device
 with success.

>>>
>>> May be I have missed, but I don't see any use of powercap sysfs.
>>>
>>> We created powercap sys that user space is presented a common
>>> interface
>>> for power capping. The RAPL driver was also submitted as cooling
>>> device
>>> before, but suggestion was to create this powercap.
>>>
>>> So if powercap interface is not enough then may be we should
>>> enhance
>>> that.
>>
>> We are creating a framework for idle injection. This framework can
>> then
>> be used by the cpu_cooling device, the power_clamp and in addition a
>> power capping for ARM (if it makes sense).
> But in this case, why in drivers/powercap folder as this is another
> framework?

I presented at OSPM 2nd the cpu idle cooling device. It is ARM specific
but has some similarities with the power clamp driver.

Some board come with power numbers defined in their DT and from there we
are able to compute a targeted power by injecting idle cycles.

As the idle injection allows to do also power capping, Rafael told me to
move that to a framework (may be better say a component with exported
services or library) and put it in drivers/powercap.

>From there we can implement a power capping, a cpu idle cooling device
and power_clamp (if the maintainer is willing to) using the same API.



-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH V6] powercap/drivers/idle_injection: Add an idle injection framework

[Pandruvada, Srinivas]

On Wed, 2018-06-13 at 22:00 +0200, Daniel Lezcano wrote:
> On 13/06/2018 17:54, Pandruvada, Srinivas wrote:
> > On Tue, 2018-06-12 at 14:00 +0200, Daniel Lezcano wrote:
> > > Initially, the cpu_cooling device for ARM was changed by adding a
> > > new
> > > policy inserting idle cycles. The intel_powerclamp driver does a
> > > similar action.
> > > 
> > > Instead of implementing idle injections privately in the
> > > cpu_cooling
> > > device, move the idle injection code in a dedicated framework and
> > > give
> > > the opportunity to other frameworks to make use of it.
> > > 
> > > The framework relies on the smpboot kthreads which handles via
> > > its
> > > main loop the common code for hotplugging and [un]parking.
> > > 
> > > This code was previously tested with the cpu cooling device and
> > > went
> > > through several iterations. It results now in split code and API
> > > exported in the header file. It was tested with the cpu cooling
> > > device
> > > with success.
> > > 
> > 
> > May be I have missed, but I don't see any use of powercap sysfs.
> > 
> > We created powercap sys that user space is presented a common
> > interface
> > for power capping. The RAPL driver was also submitted as cooling
> > device
> > before, but suggestion was to create this powercap.
> > 
> > So if powercap interface is not enough then may be we should
> > enhance
> > that.
> 
> We are creating a framework for idle injection. This framework can
> then
> be used by the cpu_cooling device, the power_clamp and in addition a
> power capping for ARM (if it makes sense).
But in this case, why in drivers/powercap folder as this is another
framework?

Thanks,
Srinivas

> 
> 
> 

smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH V6] powercap/drivers/idle_injection: Add an idle injection framework

[Daniel Lezcano]

On 13/06/2018 17:54, Pandruvada, Srinivas wrote:
> On Tue, 2018-06-12 at 14:00 +0200, Daniel Lezcano wrote:
>> Initially, the cpu_cooling device for ARM was changed by adding a new
>> policy inserting idle cycles. The intel_powerclamp driver does a
>> similar action.
>>
>> Instead of implementing idle injections privately in the cpu_cooling
>> device, move the idle injection code in a dedicated framework and
>> give
>> the opportunity to other frameworks to make use of it.
>>
>> The framework relies on the smpboot kthreads which handles via its
>> main loop the common code for hotplugging and [un]parking.
>>
>> This code was previously tested with the cpu cooling device and went
>> through several iterations. It results now in split code and API
>> exported in the header file. It was tested with the cpu cooling
>> device
>> with success.
>>
> May be I have missed, but I don't see any use of powercap sysfs.
> 
> We created powercap sys that user space is presented a common interface
> for power capping. The RAPL driver was also submitted as cooling device
> before, but suggestion was to create this powercap.
> 
> So if powercap interface is not enough then may be we should enhance
> that.

We are creating a framework for idle injection. This framework can then
be used by the cpu_cooling device, the power_clamp and in addition a
power capping for ARM (if it makes sense).



-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH V6] powercap/drivers/idle_injection: Add an idle injection framework

[Pandruvada, Srinivas]

On Tue, 2018-06-12 at 14:00 +0200, Daniel Lezcano wrote:
> Initially, the cpu_cooling device for ARM was changed by adding a new
> policy inserting idle cycles. The intel_powerclamp driver does a
> similar action.
> 
> Instead of implementing idle injections privately in the cpu_cooling
> device, move the idle injection code in a dedicated framework and
> give
> the opportunity to other frameworks to make use of it.
> 
> The framework relies on the smpboot kthreads which handles via its
> main loop the common code for hotplugging and [un]parking.
> 
> This code was previously tested with the cpu cooling device and went
> through several iterations. It results now in split code and API
> exported in the header file. It was tested with the cpu cooling
> device
> with success.
> 
May be I have missed, but I don't see any use of powercap sysfs.

We created powercap sys that user space is presented a common interface
for power capping. The RAPL driver was also submitted as cooling device
before, but suggestion was to create this powercap.

So if powercap interface is not enough then may be we should enhance
that.

Thanks,
Srinivas

> Signed-off-by: Daniel Lezcano 
> Cc: Viresh Kumar 
> Cc: Eduardo Valentin 
> Cc: Javi Merino 
> Cc: Leo Yan 
> Cc: Kevin Wangtao 
> Cc: Vincent Guittot 
> Cc: Rui Zhang 
> Cc: Daniel Thompson 
> Cc: Peter Zijlstra 
> Cc: Andrea Parri 
> ---
> V6:
>- Move count to wake up function (Viresh Kumar)
>- Split atomic/non-atomic context to wake up tasks
>- Add the park callback to handle unplug/inject race
>- Replace atomic by READ_ONCE and WRITE_ONCE (Peter Zijlstra)
> 
> V5:
>- Move init_completion in the init function (Viresh Kumar)
>- Increment task count in the wakeup function (Viresh Kumar)
>- Remove rollback at init time (Viresh Kumar)
> 
> V4:
>- Wait for completion when stopping (Viresh Kumar)
>- Check init already done and rollback (Viresh Kumar)
> 
> V3:
>- Fixed typos (Viresh Kumar)
>- Removed extra blank line (Viresh Kumar)
>- Added full stop (Viresh Kumar)
>- Fixed Return kerneldoc format (Viresh Kumar)
>- Fixed multiple kthreads initialization (Viresh Kumar)
>- Fixed rollbacking the actions in the unregister function (Viresh
> Kumar)
>- Make idle injection kthreads name hardcoded
>- Kthreads are created in the initcall process
> 
>  V2: Fixed checkpatch warnings
> 
> Signed-off-by: Daniel Lezcano 
> ---
>  drivers/powercap/Kconfig  |  10 +
>  drivers/powercap/Makefile |   1 +
>  drivers/powercap/idle_injection.c | 414
> ++
>  include/linux/idle_injection.h|  29 +++
>  4 files changed, 454 insertions(+)
>  create mode 100644 drivers/powercap/idle_injection.c
>  create mode 100644 include/linux/idle_injection.h
> 
> diff --git a/drivers/powercap/Kconfig b/drivers/powercap/Kconfig
> index 85727ef..a767ef2 100644
> --- a/drivers/powercap/Kconfig
> +++ b/drivers/powercap/Kconfig
> @@ -29,4 +29,14 @@ config INTEL_RAPL
> controller, CPU core (Power Plance 0), graphics uncore
> (Power Plane
> 1), etc.
>  
> +config IDLE_INJECTION
> + bool "Idle injection framework"
> + depends on CPU_IDLE
> + default n
> + help
> +   This enables support for the idle injection framework. It
> +   provides a way to force idle periods on a set of specified
> +   CPUs for power capping. Idle period can be injected
> +   synchronously on a set of specified CPUs or alternatively
> +   on a per CPU basis.
>  endif
> diff --git a/drivers/powercap/Makefile b/drivers/powercap/Makefile
> index 0a21ef3..c3bbfee 100644
> --- a/drivers/powercap/Makefile
> +++ b/drivers/powercap/Makefile
> @@ -1,2 +1,3 @@
>  obj-$(CONFIG_POWERCAP)   += powercap_sys.o
>  obj-$(CONFIG_INTEL_RAPL) += intel_rapl.o
> +obj-$(CONFIG_IDLE_INJECTION) += idle_injection.o
> diff --git a/drivers/powercap/idle_injection.c
> b/drivers/powercap/idle_injection.c
> new file mode 100644
> index 000..1ff1a46
> --- /dev/null
> +++ b/drivers/powercap/idle_injection.c
> @@ -0,0 +1,414 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2018 Linaro Limited
> + *
> + * Author: Daniel Lezcano 
> + *
> + * The idle injection framework proposes a way to force a cpu to
> enter
> + * an idle state during a specified amount of time for a specified
> + * period.
> + *
> + * It relies on the smpboot kthreads which handles, via its main
> loop,
> + * the common code for hotplugging and [un]parking.
> + *
> + * At init time, all the kthreads are created and parked.
> + *
> + * A cpumask is specified as parameter for the idle injection
> + * registering function. The kthreads will be synchronized regarding
> + * this cpumask.
> + *
> + * The idle + run duration is specified via the helpers and then the
> + * idle injection can be started at this point.
> + *
> + * A kthread will call play_idle() with the specified idle duration
> + * from above.
> + *
> + * A timer is set

Re: [PATCH V6] powercap/drivers/idle_injection: Add an idle injection framework

[Viresh Kumar]

On 13-06-18, 11:03, Daniel Lezcano wrote:
> nr_threads(smpboot) <> nr_threads(idleinject)
> 
> If we are facing races issues, it is because we are trying to avoid
> using locks in the code path. With lock and proper refcounting that
> should be solved, AFAICT there are similar races with inodes.

I am not sure if locking can make things better here. We aren't using them as we
don't really need them AFAIU :)

-- 
viresh

Re: [PATCH V6] powercap/drivers/idle_injection: Add an idle injection framework

[Daniel Lezcano]

On 13/06/2018 10:55, Viresh Kumar wrote:
> On 12-06-18, 19:35, Peter Zijlstra wrote:
>> On Tue, Jun 12, 2018 at 07:02:57PM +0200, Daniel Lezcano wrote:
>>> Mmh, it is unclear for me if the park() vs wakeup() can happen at the
>>> same time.
>>>
>>> If the park() function is called, that means the hotplug is allowed.
>>
>> No, it means we're inside hot-un-plug, but that doesn't stop the hrtimer
>> from firing.
>>
>>> If the hotplug is allowed, we can modify the online mask.
>>>
>>> What happens with the online mask when we are processing it in an
>>> interrupt context ?
>>
>> RCU-like, if you observe a CPU in the online mask, it will stay
>> available, but the bit might get cleared.
>>
 Maybe avoid the issue entire by having a
 {period,idle} tuple, where your old run := period - idle.
>>>
>>> Can you elaborate ? I don't get it.
>>
>> Have a period parameter that specifies the interval in which you have
>> one injected idle, and specify for how long you want to inject idle;
>> then obviously idle < period.
>>
>> Furthermore, should you not be using hrtimer_forward(&timer,
>> idle_duration + run_duration) instead? AFAICT the current scheme is
>> prone to drifting.
>
> (I assume you meant setting the timer in the wakeup task function).
>
> Yes, drifting is not an issue if that happens. This scheme is simpler
> and safer than setting the timer ahead before waking up the tasks with
> the risk it expires before all the tasks ended their idle cycles.

 sloppy though..
>>>
>>> Ok, do you prefer to see the timer set in the wakeup function and thus
>>> having a periodic tick for the idle injection ?
>>
>> I think having a HRTIMER_RESTART handler that does hrtimer_forward() is
>> the most sensible. You will end up having to deal with threads not being
>> ready, but I think that's not a real problem.
> 
> Honestly speaking I am not sure if we can fix all the races we have identified
> until now, with the current design and the fear of using resources after 
> getting
> freed will always be there. Too many corner cases really.
> 
> The requirement we have is to make sure no kthread or hrtimer is still in use
> when the ii_dev is freed from idle_injection_unregister().
> 
> May be the only sane way to make it work is to call
> smpboot_register_percpu_thread() from idle_injection_register() and then
> unregister those threads from idle_injection_unregister().

nr_threads(smpboot) <> nr_threads(idleinject)

If we are facing races issues, it is because we are trying to avoid
using locks in the code path. With lock and proper refcounting that
should be solved, AFAICT there are similar races with inodes.

Furthermore, hrtimer_forward approach is probably cleaner.



-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH V6] powercap/drivers/idle_injection: Add an idle injection framework

[Viresh Kumar]

On 12-06-18, 19:35, Peter Zijlstra wrote:
> On Tue, Jun 12, 2018 at 07:02:57PM +0200, Daniel Lezcano wrote:
> > Mmh, it is unclear for me if the park() vs wakeup() can happen at the
> > same time.
> > 
> > If the park() function is called, that means the hotplug is allowed.
> 
> No, it means we're inside hot-un-plug, but that doesn't stop the hrtimer
> from firing.
> 
> > If the hotplug is allowed, we can modify the online mask.
> > 
> > What happens with the online mask when we are processing it in an
> > interrupt context ?
> 
> RCU-like, if you observe a CPU in the online mask, it will stay
> available, but the bit might get cleared.
> 
> > > Maybe avoid the issue entire by having a
> > > {period,idle} tuple, where your old run := period - idle.
> > 
> > Can you elaborate ? I don't get it.
> 
> Have a period parameter that specifies the interval in which you have
> one injected idle, and specify for how long you want to inject idle;
> then obviously idle < period.
> 
> > >>> Furthermore, should you not be using hrtimer_forward(&timer,
> > >>> idle_duration + run_duration) instead? AFAICT the current scheme is
> > >>> prone to drifting.
> > >>
> > >> (I assume you meant setting the timer in the wakeup task function).
> > >>
> > >> Yes, drifting is not an issue if that happens. This scheme is simpler
> > >> and safer than setting the timer ahead before waking up the tasks with
> > >> the risk it expires before all the tasks ended their idle cycles.
> > > 
> > > sloppy though..
> > 
> > Ok, do you prefer to see the timer set in the wakeup function and thus
> > having a periodic tick for the idle injection ?
> 
> I think having a HRTIMER_RESTART handler that does hrtimer_forward() is
> the most sensible. You will end up having to deal with threads not being
> ready, but I think that's not a real problem.

Honestly speaking I am not sure if we can fix all the races we have identified
until now, with the current design and the fear of using resources after getting
freed will always be there. Too many corner cases really.

The requirement we have is to make sure no kthread or hrtimer is still in use
when the ii_dev is freed from idle_injection_unregister().

May be the only sane way to make it work is to call
smpboot_register_percpu_thread() from idle_injection_register() and then
unregister those threads from idle_injection_unregister().

-- 
viresh

Re: [PATCH V6] powercap/drivers/idle_injection: Add an idle injection framework

[Viresh Kumar]

On 12-06-18, 14:59, Peter Zijlstra wrote:
> On Tue, Jun 12, 2018 at 02:00:11PM +0200, Daniel Lezcano wrote:
> > +struct idle_injection_device {
> 
> remove this:
> > +   cpumask_var_t cpumask;
> 
> > +   struct hrtimer timer;
> > +   struct completion stop_complete;
> > +   unsigned int idle_duration_ms;
> > +   unsigned int run_duration_ms;
> > +   atomic_t count;
> 
> add:
>   unsigned long cpumask[0];
> > +};
> 
> 
> > +static struct idle_injection_device *ii_dev_alloc(void)
> > +{
> > +   struct idle_injection_device *ii_dev;
> > +
> > +   ii_dev = kzalloc(sizeof(*ii_dev), GFP_KERNEL);
> 
> use:
> 
>   sizeof(*ii_dev) + cpumask_size()
> 
> > +   if (!ii_dev)
> > +   return NULL;
> > +
> 
> delete:
> 
> > +   if (!alloc_cpumask_var(&ii_dev->cpumask, GFP_KERNEL)) {
> > +   kfree(ii_dev);
> > +   return NULL;
> > +   }
> > +
> > +   return ii_dev;
> > +}
> 
> And use:
> 
>   to_cpumask(ii_dev->cpumask)

What's the benefit of these changes? Is it just about not allocating memory
twice or more than that ?

And what could we do in situations where we need two cpumask variables (we have
a case in cpufreq core for that) ?

-- 
viresh

Re: [PATCH V6] powercap/drivers/idle_injection: Add an idle injection framework

[Peter Zijlstra]

On Tue, Jun 12, 2018 at 07:02:57PM +0200, Daniel Lezcano wrote:
> Mmh, it is unclear for me if the park() vs wakeup() can happen at the
> same time.
> 
> If the park() function is called, that means the hotplug is allowed.

No, it means we're inside hot-un-plug, but that doesn't stop the hrtimer
from firing.

> If the hotplug is allowed, we can modify the online mask.
> 
> What happens with the online mask when we are processing it in an
> interrupt context ?

RCU-like, if you observe a CPU in the online mask, it will stay
available, but the bit might get cleared.

> > Maybe avoid the issue entire by having a
> > {period,idle} tuple, where your old run := period - idle.
> 
> Can you elaborate ? I don't get it.

Have a period parameter that specifies the interval in which you have
one injected idle, and specify for how long you want to inject idle;
then obviously idle < period.

> >>> Furthermore, should you not be using hrtimer_forward(&timer,
> >>> idle_duration + run_duration) instead? AFAICT the current scheme is
> >>> prone to drifting.
> >>
> >> (I assume you meant setting the timer in the wakeup task function).
> >>
> >> Yes, drifting is not an issue if that happens. This scheme is simpler
> >> and safer than setting the timer ahead before waking up the tasks with
> >> the risk it expires before all the tasks ended their idle cycles.
> > 
> > sloppy though..
> 
> Ok, do you prefer to see the timer set in the wakeup function and thus
> having a periodic tick for the idle injection ?

I think having a HRTIMER_RESTART handler that does hrtimer_forward() is
the most sensible. You will end up having to deal with threads not being
ready, but I think that's not a real problem.

Re: [PATCH V6] powercap/drivers/idle_injection: Add an idle injection framework

[Daniel Lezcano]

On 12/06/2018 17:58, Peter Zijlstra wrote:
> On Tue, Jun 12, 2018 at 04:37:17PM +0200, Daniel Lezcano wrote:
>> On 12/06/2018 16:06, Peter Zijlstra wrote:
>>> On Tue, Jun 12, 2018 at 03:02:14PM +0200, Daniel Lezcano wrote:
 On 12/06/2018 14:52, Peter Zijlstra wrote:
> In this case, you can do:

 That is what we had before but we change the code to set the count
 before waking up the task, so compute the cpumask_weight of the
 resulting AND right before this loop.

> +   for_each_cpu_and(cpu, &ii_dev->cpumask, cpu_online_mask) {
> +   iit = per_cpu_ptr(&idle_injection_thread, cpu);
> +   iit->should_run = 1;
> +   wake_up_process(iit->tsk);
> +   }
>>>
>>>
>>> Ah, I see, but since you do:
>>>
>>> if (atomic_dec_and_test())
>>>   last_man()
>>>
>>> where that last_man() thing will start a timer, there is no real problem
>>> with doing atomic_inc() with before wake_up_process().
>>
>> Viresh was worried about the scenario:
>>
>> https://lkml.org/lkml/2018/6/5/276
> 
> Ah, but I think you have more races, for instance look at wakeup vs
> park, what if wakeup sets should_run after you've just checked it?
> 
> Then you have an inc without a dec.

Mmh, it is unclear for me if the park() vs wakeup() can happen at the
same time.

If the park() function is called, that means the hotplug is allowed.

If the hotplug is allowed, we can modify the online mask.

What happens with the online mask when we are processing it in an
interrupt context ?

>>> Also, last_man() uses @run_duration, but the way I read it, the timer is
>>> for waking things up again, this means it is in fact the sleep duration,
>>> no?
>>
>> No, it is the next idle injection deadline, meanwhile we let the system
>> continue running.
>>
>> The sleep duration is managed by another timer in play_idle().
> 
> No, that's the idle duration. 

Ah, ok. I misunderstood what you meant by 'sleep' duration.

> Maybe avoid the issue entire by having a
> {period,idle} tuple, where your old run := period - idle.

Can you elaborate ? I don't get it.

>>> Furthermore, should you not be using hrtimer_forward(&timer,
>>> idle_duration + run_duration) instead? AFAICT the current scheme is
>>> prone to drifting.
>>
>> (I assume you meant setting the timer in the wakeup task function).
>>
>> Yes, drifting is not an issue if that happens. This scheme is simpler
>> and safer than setting the timer ahead before waking up the tasks with
>> the risk it expires before all the tasks ended their idle cycles.
> 
> sloppy though..

Ok, do you prefer to see the timer set in the wakeup function and thus
having a periodic tick for the idle injection ?


-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH V6] powercap/drivers/idle_injection: Add an idle injection framework

[Peter Zijlstra]

On Tue, Jun 12, 2018 at 04:37:17PM +0200, Daniel Lezcano wrote:
> On 12/06/2018 16:06, Peter Zijlstra wrote:
> > On Tue, Jun 12, 2018 at 03:02:14PM +0200, Daniel Lezcano wrote:
> >> On 12/06/2018 14:52, Peter Zijlstra wrote:
> >>> In this case, you can do:
> >>
> >> That is what we had before but we change the code to set the count
> >> before waking up the task, so compute the cpumask_weight of the
> >> resulting AND right before this loop.
> >>
> >>> +   for_each_cpu_and(cpu, &ii_dev->cpumask, cpu_online_mask) {
> >>> +   iit = per_cpu_ptr(&idle_injection_thread, cpu);
> >>> +   iit->should_run = 1;
> >>> +   wake_up_process(iit->tsk);
> >>> +   }
> > 
> > 
> > Ah, I see, but since you do:
> > 
> > if (atomic_dec_and_test())
> >   last_man()
> > 
> > where that last_man() thing will start a timer, there is no real problem
> > with doing atomic_inc() with before wake_up_process().
> 
> Viresh was worried about the scenario:
> 
> https://lkml.org/lkml/2018/6/5/276

Ah, but I think you have more races, for instance look at wakeup vs
park, what if wakeup sets should_run after you've just checked it?

Then you have an inc without a dec.

> > Also, last_man() uses @run_duration, but the way I read it, the timer is
> > for waking things up again, this means it is in fact the sleep duration,
> > no?
> 
> No, it is the next idle injection deadline, meanwhile we let the system
> continue running.
> 
> The sleep duration is managed by another timer in play_idle().

No, that's the idle duration. Maybe avoid the issue entire by having a
{period,idle} tuple, where your old run := period - idle.

> > Furthermore, should you not be using hrtimer_forward(&timer,
> > idle_duration + run_duration) instead? AFAICT the current scheme is
> > prone to drifting.
> 
> (I assume you meant setting the timer in the wakeup task function).
> 
> Yes, drifting is not an issue if that happens. This scheme is simpler
> and safer than setting the timer ahead before waking up the tasks with
> the risk it expires before all the tasks ended their idle cycles.

sloppy though..

Re: [PATCH V6] powercap/drivers/idle_injection: Add an idle injection framework

[Daniel Lezcano]

On 12/06/2018 16:26, Peter Zijlstra wrote:
> On Tue, Jun 12, 2018 at 02:00:11PM +0200, Daniel Lezcano wrote:
>> +void idle_injection_stop(struct idle_injection_device *ii_dev)
>> +{
>> +pr_debug("Stopping injecting idle cycles on CPUs '%*pbl'\n",
>> + cpumask_pr_args(ii_dev->cpumask));
>> +
>> +idle_injection_set_duration(ii_dev, 0, 0);
>> +
>> +wait_for_completion_interruptible(&ii_dev->stop_complete);
>> +}
> 
> interruptible?

Right, can be removed.


-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH V6] powercap/drivers/idle_injection: Add an idle injection framework

[Daniel Lezcano]

On 12/06/2018 16:23, Peter Zijlstra wrote:
> On Tue, Jun 12, 2018 at 02:00:11PM +0200, Daniel Lezcano wrote:
>> +static void idle_injection_last_man(struct idle_injection_device *ii_dev)
>> +{
>> +unsigned int run_duration_ms;
>> +
>> +run_duration_ms = READ_ONCE(ii_dev->run_duration_ms);
>> +if (run_duration_ms) {
>> +hrtimer_start(&ii_dev->timer, ms_to_ktime(run_duration_ms),
>> +  HRTIMER_MODE_REL_PINNED);
> 
> What's the point of PINNED here? AFAICT this gets called from a
> different CPU every time.

Right, I will remove it.

-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH V6] powercap/drivers/idle_injection: Add an idle injection framework

[Daniel Lezcano]

On 12/06/2018 16:06, Peter Zijlstra wrote:
> On Tue, Jun 12, 2018 at 03:02:14PM +0200, Daniel Lezcano wrote:
>> On 12/06/2018 14:52, Peter Zijlstra wrote:
>>> In this case, you can do:
>>
>> That is what we had before but we change the code to set the count
>> before waking up the task, so compute the cpumask_weight of the
>> resulting AND right before this loop.
>>
>>> +   for_each_cpu_and(cpu, &ii_dev->cpumask, cpu_online_mask) {
>>> +   iit = per_cpu_ptr(&idle_injection_thread, cpu);
>>> +   iit->should_run = 1;
>>> +   wake_up_process(iit->tsk);
>>> +   }
> 
> 
> Ah, I see, but since you do:
> 
>   if (atomic_dec_and_test())
> last_man()
> 
> where that last_man() thing will start a timer, there is no real problem
> with doing atomic_inc() with before wake_up_process().

Viresh was worried about the scenario:

https://lkml.org/lkml/2018/6/5/276


> Yes, it allows doing last_man, too often, but repeated hrtimer_start()
> will DTRT and reprogram the timer.
> 
> Also, last_man() uses @run_duration, but the way I read it, the timer is
> for waking things up again, this means it is in fact the sleep duration,
> no?

No, it is the next idle injection deadline, meanwhile we let the system
continue running.

The sleep duration is managed by another timer in play_idle().

> Furthermore, should you not be using hrtimer_forward(&timer,
> idle_duration + run_duration) instead? AFAICT the current scheme is
> prone to drifting.

(I assume you meant setting the timer in the wakeup task function).

Yes, drifting is not an issue if that happens. This scheme is simpler
and safer than setting the timer ahead before waking up the tasks with
the risk it expires before all the tasks ended their idle cycles.



-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH V6] powercap/drivers/idle_injection: Add an idle injection framework

[Peter Zijlstra]

On Tue, Jun 12, 2018 at 02:00:11PM +0200, Daniel Lezcano wrote:
> +void idle_injection_stop(struct idle_injection_device *ii_dev)
> +{
> + pr_debug("Stopping injecting idle cycles on CPUs '%*pbl'\n",
> +  cpumask_pr_args(ii_dev->cpumask));
> +
> + idle_injection_set_duration(ii_dev, 0, 0);
> +
> + wait_for_completion_interruptible(&ii_dev->stop_complete);
> +}

interruptible?

Re: [PATCH V6] powercap/drivers/idle_injection: Add an idle injection framework

[Peter Zijlstra]

On Tue, Jun 12, 2018 at 02:00:11PM +0200, Daniel Lezcano wrote:
> +static void idle_injection_last_man(struct idle_injection_device *ii_dev)
> +{
> + unsigned int run_duration_ms;
> +
> + run_duration_ms = READ_ONCE(ii_dev->run_duration_ms);
> + if (run_duration_ms) {
> + hrtimer_start(&ii_dev->timer, ms_to_ktime(run_duration_ms),
> +   HRTIMER_MODE_REL_PINNED);

What's the point of PINNED here? AFAICT this gets called from a
different CPU every time.

> + return;
> + }
> +
> + complete(&ii_dev->stop_complete);
> +}

Re: [PATCH V6] powercap/drivers/idle_injection: Add an idle injection framework

[Peter Zijlstra]

On Tue, Jun 12, 2018 at 03:02:14PM +0200, Daniel Lezcano wrote:
> On 12/06/2018 14:52, Peter Zijlstra wrote:
> > In this case, you can do:
> 
> That is what we had before but we change the code to set the count
> before waking up the task, so compute the cpumask_weight of the
> resulting AND right before this loop.
> 
> > +   for_each_cpu_and(cpu, &ii_dev->cpumask, cpu_online_mask) {
> > +   iit = per_cpu_ptr(&idle_injection_thread, cpu);
> > +   iit->should_run = 1;
> > +   wake_up_process(iit->tsk);
> > +   }


Ah, I see, but since you do:

if (atomic_dec_and_test())
  last_man()

where that last_man() thing will start a timer, there is no real problem
with doing atomic_inc() with before wake_up_process().

Yes, it allows doing last_man, too often, but repeated hrtimer_start()
will DTRT and reprogram the timer.

Also, last_man() uses @run_duration, but the way I read it, the timer is
for waking things up again, this means it is in fact the sleep duration,
no?

Furthermore, should you not be using hrtimer_forward(&timer,
idle_duration + run_duration) instead? AFAICT the current scheme is
prone to drifting.

Re: [PATCH V6] powercap/drivers/idle_injection: Add an idle injection framework

[Daniel Lezcano]

On 12/06/2018 14:52, Peter Zijlstra wrote:
> On Tue, Jun 12, 2018 at 02:44:29PM +0200, Daniel Lezcano wrote:
>> On 12/06/2018 14:30, Peter Zijlstra wrote:
>>> On Tue, Jun 12, 2018 at 02:00:11PM +0200, Daniel Lezcano wrote:
 +static void __idle_injection_wakeup(struct idle_injection_device *ii_dev)
 +{
 +  struct idle_injection_thread *iit;
 +  struct cpumask tmp;
 +  unsigned int cpu;
 +
 +  cpumask_and(&tmp, ii_dev->cpumask, cpu_online_mask);
>>>
>>> You should not be having a cpumask on the stack. Those things can be
>>> ginormous.
>>
>> Ok, the kernel code uses of cpumask_t on the stack when dealing with
>> cpumask_and. I assume it is also not recommended.
> 
> Yes, that should all get fixed. It's mostly legacy code I suppose. It's
> been at least 10 years I think since we merged the whole
> CPUMASK_OFFSTACK stuff.
> 
>> What would be the best practice ? Allocate a per cpumask at init time as
>> a temporary mask to work with ?
> 
> In this case, you can do:

That is what we had before but we change the code to set the count
before waking up the task, so compute the cpumask_weight of the
resulting AND right before this loop.

> +   for_each_cpu_and(cpu, &ii_dev->cpumask, cpu_online_mask) {
> +   iit = per_cpu_ptr(&idle_injection_thread, cpu);
> +   iit->should_run = 1;
> +   wake_up_process(iit->tsk);
> +   }
> 


-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH V6] powercap/drivers/idle_injection: Add an idle injection framework

[Peter Zijlstra]

On Tue, Jun 12, 2018 at 02:00:11PM +0200, Daniel Lezcano wrote:
> +struct idle_injection_device {

remove this:
> + cpumask_var_t cpumask;

> + struct hrtimer timer;
> + struct completion stop_complete;
> + unsigned int idle_duration_ms;
> + unsigned int run_duration_ms;
> + atomic_t count;

add:
unsigned long cpumask[0];
> +};


> +static struct idle_injection_device *ii_dev_alloc(void)
> +{
> + struct idle_injection_device *ii_dev;
> +
> + ii_dev = kzalloc(sizeof(*ii_dev), GFP_KERNEL);

use:

sizeof(*ii_dev) + cpumask_size()

> + if (!ii_dev)
> + return NULL;
> +

delete:

> + if (!alloc_cpumask_var(&ii_dev->cpumask, GFP_KERNEL)) {
> + kfree(ii_dev);
> + return NULL;
> + }
> +
> + return ii_dev;
> +}

And use:

to_cpumask(ii_dev->cpumask)

Re: [PATCH V6] powercap/drivers/idle_injection: Add an idle injection framework

[Peter Zijlstra]

On Tue, Jun 12, 2018 at 02:44:29PM +0200, Daniel Lezcano wrote:
> On 12/06/2018 14:30, Peter Zijlstra wrote:
> > On Tue, Jun 12, 2018 at 02:00:11PM +0200, Daniel Lezcano wrote:
> >> +static void __idle_injection_wakeup(struct idle_injection_device *ii_dev)
> >> +{
> >> +  struct idle_injection_thread *iit;
> >> +  struct cpumask tmp;
> >> +  unsigned int cpu;
> >> +
> >> +  cpumask_and(&tmp, ii_dev->cpumask, cpu_online_mask);
> > 
> > You should not be having a cpumask on the stack. Those things can be
> > ginormous.
> 
> Ok, the kernel code uses of cpumask_t on the stack when dealing with
> cpumask_and. I assume it is also not recommended.

Yes, that should all get fixed. It's mostly legacy code I suppose. It's
been at least 10 years I think since we merged the whole
CPUMASK_OFFSTACK stuff.

> What would be the best practice ? Allocate a per cpumask at init time as
> a temporary mask to work with ?

In this case, you can do:

+   for_each_cpu_and(cpu, &ii_dev->cpumask, cpu_online_mask) {
+   iit = per_cpu_ptr(&idle_injection_thread, cpu);
+   iit->should_run = 1;
+   wake_up_process(iit->tsk);
+   }

Re: [PATCH V6] powercap/drivers/idle_injection: Add an idle injection framework

[Daniel Lezcano]

On 12/06/2018 14:30, Peter Zijlstra wrote:
> On Tue, Jun 12, 2018 at 02:00:11PM +0200, Daniel Lezcano wrote:
>> +static void __idle_injection_wakeup(struct idle_injection_device *ii_dev)
>> +{
>> +struct idle_injection_thread *iit;
>> +struct cpumask tmp;
>> +unsigned int cpu;
>> +
>> +cpumask_and(&tmp, ii_dev->cpumask, cpu_online_mask);
> 
> You should not be having a cpumask on the stack. Those things can be
> ginormous.

Ok, the kernel code uses of cpumask_t on the stack when dealing with
cpumask_and. I assume it is also not recommended.

What would be the best practice ? Allocate a per cpumask at init time as
a temporary mask to work with ?




-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH V6] powercap/drivers/idle_injection: Add an idle injection framework

[Peter Zijlstra]

On Tue, Jun 12, 2018 at 02:00:11PM +0200, Daniel Lezcano wrote:
> +static void __idle_injection_wakeup(struct idle_injection_device *ii_dev)
> +{
> + struct idle_injection_thread *iit;
> + struct cpumask tmp;
> + unsigned int cpu;
> +
> + cpumask_and(&tmp, ii_dev->cpumask, cpu_online_mask);

You should not be having a cpumask on the stack. Those things can be
ginormous.