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 ?


-- 
 <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