Re: [PATCH v4 2/2] CPU hotplug, Freezer: Synchronize CPU hotplugand Freezer

[Srivatsa S. Bhat]

On 10/28/2011 01:43 AM, Rafael J. Wysocki wrote:
> Hi,
> 
> On Thursday, October 27, 2011, Srivatsa S. Bhat wrote:
>> Prevent CPU hotplug and the freezer from racing with each other, to ensure
>> that during the *entire duration* for which the callbacks for CPU hotplug
>> notifications such as CPU_ONLINE[_FROZEN], CPU_DEAD[_FROZEN] etc are being
>> executed, the state of the system (with respect to the tasks being frozen
>> or not) remains constant.
>>
>> This patches hooks the CPU hotplug infrastructure onto the freezer
>> notifications (PM_FREEZE_PREPARE and PM_POST_THAW) and thus synchronizes
>> with the freezer.
>>
>> Specifically,
>>
>> * Upon the PM_FREEZE_PREPARE notification, the CPU hotplug callback disables
>>   future (regular) CPU hotplugging and also ensures that any currently running
>>   CPU hotplug operation is completed before allowing the freezer to continue
>>   any further.
>>
>> * Upon the PM_POST_THAW notification, the CPU hotplug callback re-enables
>>   regular CPU hotplug.
>>
>> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@xxxxxxxxxxxxxxxxxx>
>> ---
>>
>>  kernel/cpu.c |   76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 76 insertions(+), 0 deletions(-)
>>
>> diff --git a/kernel/cpu.c b/kernel/cpu.c
>> index 12b7458..61985ce 100644
>> --- a/kernel/cpu.c
>> +++ b/kernel/cpu.c
>> @@ -15,6 +15,7 @@
>>  #include <linux/stop_machine.h>
>>  #include <linux/mutex.h>
>>  #include <linux/gfp.h>
>> +#include <linux/suspend.h>
>>  
>>  #ifdef CONFIG_SMP
>>  /* Serializes the updates to cpu_online_mask, cpu_present_mask */
>> @@ -478,6 +479,81 @@ static int alloc_frozen_cpus(void)
>>  core_initcall(alloc_frozen_cpus);
>>  #endif /* CONFIG_PM_SLEEP_SMP */
>>  
>> +
>> +#ifdef CONFIG_FREEZER
>> +
>> +/*
>> + * Avoid CPU hotplug racing with the freezer subsystem, by disabling CPU
>> + * hotplug when tasks are about to be frozen.
>> + *
>> + * Also, don't allow the freezer subsystem to continue until any currently
>> + * running CPU hotplug operation gets completed.
>> + * To modify the 'cpu_hotplug_disabled' flag, we need to acquire the
>> + * 'cpu_add_remove_lock'. And this same lock is also taken by the regular
>> + * CPU hotplug path and released only after it is complete. Thus, we
>> + * (and hence the freezer) will block here until any currently running CPU
>> + * hotplug operation is completed.
>> + */
>> +static void cpu_hotplug_freezer_block_begin(void)
>> +{
>> +	cpu_maps_update_begin();
>> +	cpu_hotplug_disabled = 1;
>> +	cpu_maps_update_done();
>> +}
>> +
>> +
>> +/*
>> + * When thawing of tasks is complete, re-enable CPU hotplug (which had been
>> + * disabled while beginning to freeze tasks).
>> + */
>> +static void cpu_hotplug_freezer_block_done(void)
>> +{
>> +	cpu_maps_update_begin();
>> +	cpu_hotplug_disabled = 0;
>> +	cpu_maps_update_done();
>> +}
>> +
> 
> I wonder if the new PM notifier events are really necessary?
> 
> Why don't you just call cpu_hotplug_freezer_block_begin() (perhaps
> with a better name?) directly from freeze_processes()?  And analogously
> for cpu_hotplug_freezer_block_done() and thaw_processes()?
> 

Yes, we can definitely do that. 

But the reason why I chose to introduce new notifiers was to make this
more extensible (because we know that at least 2 subsystems would benefit
from mutually excluding themselves from the freezer, namely CPU hotplug
and x86 microcode).
http://thread.gmane.org/gmane.linux.kernel/1198291/focus=1200591

But now that I think of it, hooking onto the freezer notifiers wouldn't
solve the microcode cases since usermodehelper_disable() is called
_before_ freezing tasks... :(

So we should probably call the functions directly like you suggested.. 

But I really didn't want to clutter the freezer call path because of problems
elsewhere. So I felt freezer notifiers would be a cleaner way of dealing with
such things. Also, since freezer is a generic subsystem that could be used
for purposes other than S3/S4 as well (I have heard of attempts to use freezer
during tracing), wouldn't it be better to introduce new notifiers to
announce the begin and end of freezer activity to interested subsystems?
(and then use them to solve the CPU hotplug issue like this patch does...)

Please let me know your suggestions.

-- 
Regards,
Srivatsa S. Bhat  <srivatsa.bhat@xxxxxxxxxxxxxxxxxx>
Linux Technology Center,
IBM India Systems and Technology Lab
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/