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

2018-06-08 Thread Daniel Lezcano
On 08/06/2018 06:48, Viresh Kumar wrote: > On 07-06-18, 16:11, Daniel Lezcano wrote: I'm wondering if we can have a CPU hotplugged right after the 'put_online_cpus', resulting in the 'should park' flag set and then the thread goes in the kthread_parkme instead of jumping back the idl

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

2018-06-07 Thread Viresh Kumar
On 07-06-18, 16:11, Daniel Lezcano wrote: > >> I'm wondering if we can have a CPU hotplugged right after the > >> 'put_online_cpus', resulting in the 'should park' flag set and then the > >> thread goes in the kthread_parkme instead of jumping back the idle > >> injection function and decrease the

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

2018-06-07 Thread Daniel Lezcano
On 06/06/2018 12:45, Viresh Kumar wrote: > On 06-06-18, 12:22, Daniel Lezcano wrote: >> (mb() are done in the atomic operations AFAICT). > > AFAIU, it is required to make sure the operations are seen in a particular > order > on another CPU and the compiler doesn't reorganize code to optimize it.

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

2018-06-07 Thread Daniel Lezcano
On 07/06/2018 11:39, Peter Zijlstra wrote: > On Thu, Jun 07, 2018 at 11:32:01AM +0200, Peter Zijlstra wrote: >> On Thu, Jun 07, 2018 at 11:09:13AM +0200, Daniel Lezcano wrote: >>> On 07/06/2018 10:49, Viresh Kumar wrote: On 07-06-18, 10:46, Daniel Lezcano wrote: > Yes, correct. > >

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

2018-06-07 Thread Peter Zijlstra
On Thu, Jun 07, 2018 at 11:32:01AM +0200, Peter Zijlstra wrote: > On Thu, Jun 07, 2018 at 11:09:13AM +0200, Daniel Lezcano wrote: > > On 07/06/2018 10:49, Viresh Kumar wrote: > > > On 07-06-18, 10:46, Daniel Lezcano wrote: > > >> Yes, correct. > > >> > > >> But if we don't care about who wins to st

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

2018-06-07 Thread Peter Zijlstra
On Thu, Jun 07, 2018 at 11:09:13AM +0200, Daniel Lezcano wrote: > On 07/06/2018 10:49, Viresh Kumar wrote: > > On 07-06-18, 10:46, Daniel Lezcano wrote: > >> Yes, correct. > >> > >> But if we don't care about who wins to store to value, is there a risk > >> of scramble variable if we just assign a

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

2018-06-07 Thread Peter Zijlstra
On Thu, Jun 07, 2018 at 02:19:21PM +0530, Viresh Kumar wrote: > On 07-06-18, 10:46, Daniel Lezcano wrote: > > Yes, correct. > > > > But if we don't care about who wins to store to value, is there a risk > > of scramble variable if we just assign a value ? > > Normally no, as the compiler wouldn't

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

2018-06-07 Thread Daniel Lezcano
On 07/06/2018 10:49, Viresh Kumar wrote: > On 07-06-18, 10:46, Daniel Lezcano wrote: >> Yes, correct. >> >> But if we don't care about who wins to store to value, is there a risk >> of scramble variable if we just assign a value ? > > Normally no, as the compiler wouldn't screw it up badly. But th

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

2018-06-07 Thread Viresh Kumar
On 07-06-18, 10:46, Daniel Lezcano wrote: > Yes, correct. > > But if we don't care about who wins to store to value, is there a risk > of scramble variable if we just assign a value ? Normally no, as the compiler wouldn't screw it up badly. But there is no rule which stops the compiler from doing

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

2018-06-07 Thread Daniel Lezcano
On 07/06/2018 10:42, Viresh Kumar wrote: > On 07-06-18, 10:32, Peter Zijlstra wrote: >> On Thu, Jun 07, 2018 at 10:18:27AM +0200, Daniel Lezcano wrote: >>> So IIUC, neither atomic or WRITE|READ_ONCE are necessary in this code >>> because of the wake_up_process() barrier is enough, right ? >> >> I d

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

2018-06-07 Thread Viresh Kumar
On 07-06-18, 10:32, Peter Zijlstra wrote: > On Thu, Jun 07, 2018 at 10:18:27AM +0200, Daniel Lezcano wrote: > > So IIUC, neither atomic or WRITE|READ_ONCE are necessary in this code > > because of the wake_up_process() barrier is enough, right ? > > I didn't look hard enough; if there ever is a ti

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

2018-06-07 Thread Peter Zijlstra
On Thu, Jun 07, 2018 at 10:18:27AM +0200, Daniel Lezcano wrote: > So IIUC, neither atomic or WRITE|READ_ONCE are necessary in this code > because of the wake_up_process() barrier is enough, right ? I didn't look hard enough; if there ever is a time where the loads and stores happen concurrently, y

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

2018-06-07 Thread Daniel Lezcano
On 06/06/2018 17:02, Peter Zijlstra wrote: > On Wed, Jun 06, 2018 at 03:42:08PM +0200, Daniel Lezcano wrote: >> On 06/06/2018 14:23, Peter Zijlstra wrote: >>> On Tue, Jun 05, 2018 at 11:16:40AM +0200, Daniel Lezcano wrote: + atomic_t idle_duration_ms; + atomic_t run_duration_ms; >>> >>>

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

2018-06-06 Thread Peter Zijlstra
On Wed, Jun 06, 2018 at 03:42:08PM +0200, Daniel Lezcano wrote: > On 06/06/2018 14:23, Peter Zijlstra wrote: > > On Tue, Jun 05, 2018 at 11:16:40AM +0200, Daniel Lezcano wrote: > >> + atomic_t idle_duration_ms; > >> + atomic_t run_duration_ms; > > > >> + idle_duration_ms = atomic_read(&ii_dev->

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

2018-06-06 Thread Daniel Lezcano
On 06/06/2018 14:23, Peter Zijlstra wrote: > On Tue, Jun 05, 2018 at 11:16:40AM +0200, Daniel Lezcano wrote: >> +atomic_t idle_duration_ms; >> +atomic_t run_duration_ms; > >> +idle_duration_ms = atomic_read(&ii_dev->idle_duration_ms); > >> +run_duration_ms = atomic_read(&ii_dev->r

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

2018-06-06 Thread Peter Zijlstra
On Wed, Jun 06, 2018 at 02:05:39PM +0200, Andrea Parri wrote: > Hi Daniel, Viresh, > > On Wed, Jun 06, 2018 at 04:15:28PM +0530, Viresh Kumar wrote: > > On 06-06-18, 12:22, Daniel Lezcano wrote: > > > (mb() are done in the atomic operations AFAICT). > > To do my bit, not all atomic ops do/imply m

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

2018-06-06 Thread Peter Zijlstra
On Tue, Jun 05, 2018 at 11:16:40AM +0200, Daniel Lezcano wrote: > + atomic_t idle_duration_ms; > + atomic_t run_duration_ms; > + idle_duration_ms = atomic_read(&ii_dev->idle_duration_ms); > + run_duration_ms = atomic_read(&ii_dev->run_duration_ms); > + atomic_set(&ii_dev->run

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

2018-06-06 Thread Andrea Parri
Hi Daniel, Viresh, On Wed, Jun 06, 2018 at 04:15:28PM +0530, Viresh Kumar wrote: > On 06-06-18, 12:22, Daniel Lezcano wrote: > > (mb() are done in the atomic operations AFAICT). To do my bit, not all atomic ops do/imply memory barriers; e.g., [from Documentation/atomic_t.txt] - non-RMW oper

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

2018-06-06 Thread Viresh Kumar
On 06-06-18, 12:22, Daniel Lezcano wrote: > (mb() are done in the atomic operations AFAICT). AFAIU, it is required to make sure the operations are seen in a particular order on another CPU and the compiler doesn't reorganize code to optimize it. For example, in our case what if the compiler reorg

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

2018-06-06 Thread Daniel Lezcano
On 06/06/2018 06:27, Viresh Kumar wrote: > On 05-06-18, 16:54, Daniel Lezcano wrote: >> On 05/06/2018 12:39, Viresh Kumar wrote: >> I don't think you are doing a mistake. Even if this can happen >> theoretically, I don't think practically that is the case. >> >> The play_idle() has 1ms minimum slee

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

2018-06-05 Thread Viresh Kumar
On 05-06-18, 16:54, Daniel Lezcano wrote: > On 05/06/2018 12:39, Viresh Kumar wrote: > I don't think you are doing a mistake. Even if this can happen > theoretically, I don't think practically that is the case. > > The play_idle() has 1ms minimum sleep time. > > The scenario you are describing me

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

2018-06-05 Thread Daniel Lezcano
On 05/06/2018 12:39, Viresh Kumar wrote: > On 05-06-18, 11:16, Daniel Lezcano wrote: >> diff --git a/drivers/powercap/idle_injection.c >> b/drivers/powercap/idle_injection.c >> +/** >> + * idle_injection_wakeup - Wake up all idle injection threads >> + * @ii_dev: the idle injection device >> + * >

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

2018-06-05 Thread Viresh Kumar
On 05-06-18, 11:16, Daniel Lezcano wrote: > diff --git a/drivers/powercap/idle_injection.c > b/drivers/powercap/idle_injection.c > +/** > + * idle_injection_wakeup - Wake up all idle injection threads > + * @ii_dev: the idle injection device > + * > + * Every idle injection task belonging to the i

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

2018-06-05 Thread Daniel Lezcano
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 opp