Re: [RFC] Fix SMP brokenness for PF_FREEZE and make freezing usable for other purposes

Kirill Korotaev
2005-06-28T07:30:59Z

>>>-static inline int freezing(struct task_struct *p)
>>>-{
>>>- return p->flags & PF_FREEZE;
>>>-}
>>>+#if defined(CONFIG_PM) || defined(CONFIG_MIGRATE)
>>
>><<<< why not to make a single option CONFIG_REFRIGERATOR? It looks to be a
>>more robust way, since there are multiple users of it.
>
>
> Yes. That may be better. We can do that once the migration code is
> finished and when we know what kind of CONFIG_XXX the migration code
> really needs.
>
>
>>>+#ifdef CONFIG_PM
>>
>><<<< is it intentionaly? or you just lost CONFIG_MIGRATE?
> It is intentional. freeze_processes and thaw_processes are only needed
> for suspend. One only needs to freeze a couple of processes for process
> migration.
But PM and your migrate code can be not the only users of it.

>><<<< I still think this refrigerator is racy with freeze_processes():
>><<<< scenarios:
>><<<< scenario 1
>><<<<
>>task1 -> freeze_processes(): taskXXX ->refrigerator()
>> checked (task->flags & PF_FROZEN) == 0 cur->flags |= PF_FROZEN
>> clear TIF_FREEZE
>> <sleep on thaw>
>> set TIF_FREEZING
>> clear PF_FROZEN
>>
>><<<< so the task awakes with TIF_FREEZE flag set!!!
>
>
> Hmm... If we wait to clear both flags until after the completion
> notification then we do not have the race right? But then we need to move
> the signal recalc since it tests for TIF_FREEZE too.
It is almost ok, but it is still not fine :)

look what happens if you call freeze/unfreeze in a loop:

refrigerator:
awakes

freezer:
check PF_FROZEN, it is still set, skips task and thinks it is finished
freezing.

refrigerator:
clears PF_FROZEN and TIF_FREEZE and returns.

I think you can fix this by moving PF_FROZEN check and set in both
places under siglock.

Kirill

>><<<< scenario 2
>><<<< look at error path in freeze_processes (on timeout), it is broken as
>>well. You need to wakeup tasks there...
>
>
> Ok. How about this additional patch? This still requires that process
> freezing does not immediately occurr again after the completion
> handler. All of this is iffy due to not having a real lock protecting all
> these values and we may still need to add some barriers.
>
> Index: linux-2.6.12/kernel/power/process.c
> ===================================================================
> --- linux-2.6.12.orig/kernel/power/process.c 2005-06-28 06:34:52.000000000 +0000
> +++ linux-2.6.12/kernel/power/process.c 2005-06-28 06:40:28.000000000 +0000
> @@ -47,12 +47,13 @@ int freeze_processes(void)
> unsigned long flags;
> if (!freezeable(p))
> continue;
> - if ((p->flags & PF_FROZEN) ||
> - (p->state == TASK_TRACED) ||
> + if ((p->state == TASK_TRACED) ||
> (p->state == TASK_STOPPED))
> continue;
>
> set_thread_flag(TIF_FREEZE);
> + if (p->flags & PF_FROZEN)
> + continue;
> spin_lock_irqsave(&p->sighand->siglock, flags);
> signal_wake_up(p, 0);
> spin_unlock_irqrestore(&p->sighand->siglock, flags);
> @@ -63,6 +64,8 @@ int freeze_processes(void)
> if (time_after(jiffies, start_time + TIMEOUT)) {
> printk( "\n" );
> printk(KERN_ERR " stopping tasks failed (%d tasks remaining)\n", todo );
> + complete_all(&thaw);
> + up(&freezer_sem);
> return todo;
> }
> } while(todo);
> Index: linux-2.6.12/kernel/sched.c
> ===================================================================
> --- linux-2.6.12.orig/kernel/sched.c 2005-06-28 06:34:52.000000000 +0000
> +++ linux-2.6.12/kernel/sched.c 2005-06-28 06:37:36.000000000 +0000
> @@ -5210,13 +5210,13 @@ DECLARE_COMPLETION(thaw);
> void refrigerator(void)
> {
> current->flags |= PF_FROZEN;
> + wait_for_completion(&thaw);
> clear_thread_flag(TIF_FREEZE);
> + current->flags &= ~PF_FROZEN;
> /* A fake signal 0 may have been sent. Recalculate sigpending */
> spin_lock_irq(&current->sighand->siglock);
> recalc_sigpending();
> spin_unlock_irq(&current->sighand->siglock);
> - wait_for_completion(&thaw);
> - current->flags &= ~PF_FROZEN;
> }
> EXPORT_SYMBOL(refrigerator);
> #endif