| Message ID | 20110829140549.GD18871@mtj.dyndns.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 08/29, Tejun Heo wrote: > > --- work.orig/kernel/freezer.c > +++ work/kernel/freezer.c > @@ -60,6 +60,7 @@ bool __refrigerator(bool check_kthr_stop > */ > spin_lock_irq(&freezer_lock); > current->flags |= PF_FROZEN; > +refreeze: > spin_unlock_irq(&freezer_lock); > > save = current->state; > @@ -78,8 +79,10 @@ bool __refrigerator(bool check_kthr_stop > schedule(); > } > > - /* leave FROZEN */ > + /* leave FROZEN after checking freezing() holding freezer_lock */ > spin_lock_irq(&freezer_lock); > + if (freezing(current)) > + goto refreeze; Looks like, you should move "save = current->state" up then. Oleg.
On 08/29, Oleg Nesterov wrote: > > On 08/29, Tejun Heo wrote: > > > > --- work.orig/kernel/freezer.c > > +++ work/kernel/freezer.c > > @@ -60,6 +60,7 @@ bool __refrigerator(bool check_kthr_stop > > */ > > spin_lock_irq(&freezer_lock); > > current->flags |= PF_FROZEN; > > +refreeze: > > spin_unlock_irq(&freezer_lock); > > > > save = current->state; > > @@ -78,8 +79,10 @@ bool __refrigerator(bool check_kthr_stop > > schedule(); > > } > > > > - /* leave FROZEN */ > > + /* leave FROZEN after checking freezing() holding freezer_lock */ > > spin_lock_irq(&freezer_lock); > > + if (freezing(current)) > > + goto refreeze; > > Looks like, you should move "save = current->state" up then. Hmm. And afaics there is another problem. This can "livelock" if check_kthr_stop && kthread_should_stop(). May be we should consolidate the freezer_lock's sections, something like below? Hmm. But I got lost a bit. Why do we need freezer_lock to set/clear PF_FROZEN ? OK, the code below takes freezer_lock for freezing(). Is there any other reason? Oleg. bool __refrigerator(bool check_kthr_stop) { /* Hmm, should we be allowed to suspend when there are realtime processes around? */ bool was_frozen = false; long save; save = current->state; pr_debug("%s entered refrigerator\n", current->comm); for (;;) { set_current_state(TASK_UNINTERRUPTIBLE); spin_lock_irq(&freezer_lock); current->flags |= PF_FROZEN; if (!freezing(current) || (check_kthr_stop && kthread_should_stop())) current->flags &= ~PF_FROZEN; spin_unlock_irq(&freezer_lock); if (!current->flags & PF_FROZEN) break; was_frozen = true; schedule(); } spin_lock_irq(¤t->sighand->siglock); recalc_sigpending(); /* We sent fake signal, clean it up */ spin_unlock_irq(¤t->sighand->siglock); pr_debug("%s left refrigerator\n", current->comm); /* * Restore saved task state before returning. The mb'd version * needs to be used; otherwise, it might silently break * synchronization which depends on ordered task state change. */ set_current_state(save); return was_frozen; }
On 08/29, Oleg Nesterov wrote: > > Hmm. But I got lost a bit. Why do we need freezer_lock to set/clear > PF_FROZEN ? OK, the code below takes freezer_lock for freezing(). > Is there any other reason? Ah, at least we need it to serialize with __thaw_task() which does "if (frozen) wake_up()". Oleg.
Index: work/kernel/freezer.c =================================================================== --- work.orig/kernel/freezer.c +++ work/kernel/freezer.c @@ -60,6 +60,7 @@ bool __refrigerator(bool check_kthr_stop */ spin_lock_irq(&freezer_lock); current->flags |= PF_FROZEN; +refreeze: spin_unlock_irq(&freezer_lock); save = current->state; @@ -78,8 +79,10 @@ bool __refrigerator(bool check_kthr_stop schedule(); } - /* leave FROZEN */ + /* leave FROZEN after checking freezing() holding freezer_lock */ spin_lock_irq(&freezer_lock); + if (freezing(current)) + goto refreeze; current->flags &= ~PF_FROZEN; spin_unlock_irq(&freezer_lock);
If another freeze happens before all tasks leave FROZEN state after being thawed, the freezer can see the existing FROZEN and consider the tasks to be frozen but they can clear FROZEN without checking the new freezing(). Check freezing() while holding freezer_lock before clearing FROZEN. Signed-off-by: Tejun Heo <tj@kernel.org> Reported-by: Oleg Nesterov <oleg@redhat.com> Cc: "Rafael J. Wysocki" <rjw@sisk.pl> --- kernel/freezer.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)