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

[Daniel Lezcano <daniel.lezcano@linaro.org>]

On 19/06/2018 08:22, Viresh Kumar wrote:
> On 19-06-18, 07:58, Daniel Lezcano wrote:
>> +++ b/drivers/powercap/idle_injection.c
>> @@ -0,0 +1,375 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright 2018 Linaro Limited
>> + *
>> + * Author: Daniel Lezcano <daniel.lezcano@linaro.org>
>> + *
>> + * 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.
>> + *
>> + * 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 after waking up all the tasks, to the next idle
>> + * injection cycle.
>> + *
>> + * The task handling the timer interrupt will wakeup all the kthreads
>> + * belonging to the cpumask.
>> + *
>> + * Stopping the idle injection is synchonuous, when the function
> 
>                                      synchronous
> 
>> + * returns, there is the guarantee there is no more idle injection
>> + * kthread in activity.
>> + *
>> + * It is up to the user of this framework to provide a lock at an
>> + * upper level to prevent stupid things to happen, like starting while
>> + * we are unregistering.
>> + */
> 
>> +static void idle_injection_wakeup(struct idle_injection_device *ii_dev)
>> +{
>> +	struct idle_injection_thread *iit;
>> +	unsigned int cpu;
>> +
>> +	for_each_cpu_and(cpu, to_cpumask(ii_dev->cpumask), cpu_online_mask) {
>> +		iit = per_cpu_ptr(&idle_injection_thread, cpu);
>> +		iit->should_run = 1;
>> +		wake_up_process(iit->tsk);
>> +	}
>> +}
> 
> Thread A                                        Thread B
> 
>                                                 CPU3 hotplug out
>                                                 -> idle_injection_park()
>                                                   iit(of-CPU3)->should_run = 0;
> 
> idle_injection_wakeup()
>  for_each_cpu_and(online)..
>    CPU3-selected
>                                                 clear CPU3 from cpu-online mask.
> 
> 
>    iit(of-CPU3)->should_run = 1;
>    wake_up_process()
> 
> With the above sequence of events, is it possible that the iit->should_run
> variable is set to 1 while the CPU is offlined ? And so the crash we discussed
> in the previous version may still exist ? Sorry I am not able to take my mind
> away from thinking about these stupid races :(

If I refer to previous Peter's comment about a similar race, I think it
is possible.

I guess setting the should_run flag to zero in the unpark() must fix the
issue also.


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