freezer: fixes &#38; simplifications

Cc Paul Menage <[email protected]>

Cc "Rafael J. Wysocki" <[email protected]>

Cc Matt Helsley <matthltc-r/[email protected]>

---
 kernel/cgroup_freezer.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

kernel/cgroup_freezer.c +7 -7

index 4e82525..56a457a 100644
@@ -308,24 +308,24 @@ static int freezer_change_state(struct cgroup *cgroup,
 	spin_lock_irq(&freezer->lock);
 
 	update_if_frozen(cgroup, freezer);
-	if (goal_state == freezer->state)
-		goto out;
-
-	freezer->state = goal_state;
 
 	switch (goal_state) {
 	case CGROUP_THAWED:
-		atomic_dec(&system_freezing_cnt);
+		if (freezer->state != CGROUP_THAWED)
+			atomic_dec(&system_freezing_cnt);
+		freezer->state = CGROUP_THAWED;
 		unfreeze_cgroup(cgroup, freezer);
 		break;
 	case CGROUP_FROZEN:
-		atomic_inc(&system_freezing_cnt);
+		if (freezer->state == CGROUP_THAWED)
+			atomic_inc(&system_freezing_cnt);
+		freezer->state = CGROUP_FREEZING;
 		retval = try_to_freeze_cgroup(cgroup, freezer);
 		break;
 	default:
 		BUG();
 	}
-out:
+
 	spin_unlock_irq(&freezer->lock);
 
 	return retval;

Signature · 1 line

1.7.6

Tejun Heo

2011-09-02 18:27 UTC #3 lore raw

PATCH 2/6 freezer: set PF_NOFREEZE on a dying task right before TASK_DEAD setting bug in freezer_change_state()

3fb45733df "freezer: make exiting tasks properly unfreezable" removed clear_freeze_flag() from exit path and set PF_NOFREEZE right after PTRACE_EVENT_EXIT; however, Oleg pointed out that following exit paths may cause interaction with device drivers after PM freezer consider the system frozen.

There's no try_to_freeze() call in the exit path and the only necessary guarantee is that freezer doesn't hang waiting for zombies. Set PF_NOFREEZE right before setting tsk->state to TASK_DEAD instead.

Signed-off-by Tejun Heo <[email protected]>

Cc "Rafael J. Wysocki" <[email protected]>

---
 kernel/exit.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

kernel/exit.c +4 -6

index ac58259..7b6c4fa 100644
@@ -913,12 +913,6 @@ NORET_TYPE void do_exit(long code)
 
 	ptrace_event(PTRACE_EVENT_EXIT, code);
 
-	/*
-	 * With ptrace notification done, there's no point in freezing from
-	 * here on.  Disallow freezing.
-	 */
-	current->flags |= PF_NOFREEZE;
-
 	validate_creds_for_do_exit(tsk);
 
 	/*

@@ -1044,6 +1038,10 @@ NORET_TYPE void do_exit(long code)
 
 	preempt_disable();
 	exit_rcu();
+
+	/* this task is now dead and freezer should ignore it */
+	current->flags |= PF_NOFREEZE;
+
 	/* causes final put_task_struct in finish_task_switch(). */
 	tsk->state = TASK_DEAD;
 	schedule();

Signature · 1 line

1.7.6

Tejun Heo

2011-09-02 18:27 UTC #4 lore raw

PATCH 3/6 freezer: restructure __refrigerator()

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().

Oleg suggested restructuring __refrigerator() such that there's single condition check section inside freezer_lock and sigpending is cleared afterwards, which fixes the problem and simplifies the code. Restructure accordingly.

-v2: Frozen loop exited without releasing freezer_lock. Fixed.

Signed-off-by Tejun Heo <[email protected]>

Cc "Rafael J. Wysocki" <[email protected]>

---
 kernel/freezer.c |   29 +++++++++++------------------
 1 files changed, 11 insertions(+), 18 deletions(-)

kernel/freezer.c +11 ~~-18~~

index 466ea6b..ebee1ab 100644
@@ -52,36 +52,29 @@ 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;
+	long save = current->state;
 
-	/*
-	 * No point in checking freezing() again - the caller already did.
-	 * Proceed to enter FROZEN.
-	 */
-	spin_lock_irq(&freezer_lock);
-	current->flags |= PF_FROZEN;
-	spin_unlock_irq(&freezer_lock);
-
-	save = current->state;
 	pr_debug("%s entered refrigerator\n", current->comm);
 
-	spin_lock_irq(&current->sighand->siglock);
-	recalc_sigpending(); /* We sent fake signal, clean it up */
-	spin_unlock_irq(&current->sighand->siglock);
-
 	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();
 	}
 
-	/* leave FROZEN */
-	spin_lock_irq(&freezer_lock);
-	current->flags &= ~PF_FROZEN;
-	spin_unlock_irq(&freezer_lock);
+	spin_lock_irq(&current->sighand->siglock);
+	recalc_sigpending(); /* We sent fake signal, clean it up */
+	spin_unlock_irq(&current->sighand->siglock);
 
 	pr_debug("%s left refrigerator\n", current->comm);

Signature · 1 line

1.7.6

Tejun Heo

2011-09-02 18:27 UTC #5 lore raw

PATCH 4/6 freezer: use lock_task_sighand() in fake_signal_wake_up()

cgroup_freezer calls freeze_task() without holding tasklist_lock and, if the task is exiting, its ->sighand may be gone by the time fake_signal_wake_up() is called. Use lock_task_sighand() instead of accessing ->sighand directly.

Signed-off-by Tejun Heo <[email protected]>

Cc "Rafael J. Wysocki" <[email protected]>

Cc Paul Menage <[email protected]>

---
 kernel/freezer.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

kernel/freezer.c +4 -3

index ebee1ab..9846133 100644
@@ -93,9 +93,10 @@ static void fake_signal_wake_up(struct task_struct *p)
 {
 	unsigned long flags;
 
-	spin_lock_irqsave(&p->sighand->siglock, flags);
-	signal_wake_up(p, 0);
-	spin_unlock_irqrestore(&p->sighand->siglock, flags);
+	if (lock_task_sighand(p, &flags)) {
+		signal_wake_up(p, 0);
+		unlock_task_sighand(p, &flags);
+	}
 }
 
 /**

Signature · 1 line

1.7.6

Tejun Heo

2011-09-02 18:27 UTC #6 lore raw

PATCH 5/6 freezer: remove unused @sig_only from freeze_task()

After 25a16d02ba "freezer: make freezing() test freeze conditions in effect instead of TIF_FREEZE", freezing() returns authoritative answer on whether the current task should freeze or not and freeze_task() doesn't need or use @sig_only. Remove it.

While at it, rewrite function comment for freeze_task() and rename @sig_only to @user_only in try_to_freeze_tasks().

This patch doesn't cause any functional change.

Signed-off-by Tejun Heo <[email protected]>

---
 include/linux/freezer.h |    2 +-
 kernel/cgroup_freezer.c |    4 ++--
 kernel/freezer.c        |   21 +++++++++------------
 kernel/power/process.c  |    8 ++++----
 4 files changed, 16 insertions(+), 19 deletions(-)

include/linux/freezer.h +1 -1

index 122b5ce..9193bae 100644
@@ -47,7 +47,7 @@ static inline bool try_to_freeze(void)
 	return __refrigerator(false);
 }
 
-extern bool freeze_task(struct task_struct *p, bool sig_only);
+extern bool freeze_task(struct task_struct *p);
 extern bool __set_freezable(bool with_signal);
 
 #ifdef CONFIG_CGROUP_FREEZER

kernel/cgroup_freezer.c +2 -2

index 56a457a..8753e7b 100644
@@ -206,7 +206,7 @@ static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task)
 
 	/* Locking avoids race with FREEZING -> THAWED transitions. */
 	if (freezer->state == CGROUP_FREEZING)
-		freeze_task(task, true);
+		freeze_task(task);
 	spin_unlock_irq(&freezer->lock);
 }

@@ -274,7 +274,7 @@ static int try_to_freeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
 
 	cgroup_iter_start(cgroup, &it);
 	while ((task = cgroup_iter_next(cgroup, &it))) {
-		if (!freeze_task(task, true))
+		if (!freeze_task(task))
 			continue;
 		if (frozen(task))
 			continue;

kernel/freezer.c +9 ~~-12~~

index 9846133..da76b64 100644
@@ -100,20 +100,17 @@ static void fake_signal_wake_up(struct task_struct *p)
 }
 
 /**
- *	freeze_task - send a freeze request to given task
- *	@p: task to send the request to
- *	@sig_only: if set, the request will only be sent if the task has the
- *		PF_FREEZER_NOSIG flag unset
- *	Return value: 'false', if @sig_only is set and the task has
- *		PF_FREEZER_NOSIG set or the task is frozen, 'true', otherwise
+ * freeze_task - send a freeze request to given task
+ * @p: task to send the request to
  *
- *	The freeze request is sent by setting the tasks's TIF_FREEZE flag and
- *	either sending a fake signal to it or waking it up, depending on whether
- *	or not it has PF_FREEZER_NOSIG set.  If @sig_only is set and the task
- *	has PF_FREEZER_NOSIG set (ie. it is a typical kernel thread), its
- *	TIF_FREEZE flag will not be set.
+ * If @p is freezing, the freeze request is sent by setting %TIF_FREEZE
+ * flag and either sending a fake signal to it or waking it up, depending
+ * on whether it has %PF_FREEZER_NOSIG set.
+ *
+ * RETURNS:
+ * %false, if @p is not freezing or already frozen; %true, otherwise
  */
-bool freeze_task(struct task_struct *p, bool sig_only)
+bool freeze_task(struct task_struct *p)
 {
 	unsigned long flags;

kernel/power/process.c +4 -4

index fe06ddf..ab4fd7b 100644
@@ -22,7 +22,7 @@
  */
 #define TIMEOUT	(20 * HZ)
 
-static int try_to_freeze_tasks(bool sig_only)
+static int try_to_freeze_tasks(bool user_only)
 {
 	struct task_struct *g, *p;
 	unsigned long end_time;

@@ -37,14 +37,14 @@ static int try_to_freeze_tasks(bool sig_only)
 
 	end_time = jiffies + TIMEOUT;
 
-	if (!sig_only)
+	if (!user_only)
 		freeze_workqueues_begin();
 
 	while (true) {
 		todo = 0;
 		read_lock(&tasklist_lock);
 		do_each_thread(g, p) {
-			if (p == current || !freeze_task(p, sig_only))
+			if (p == current || !freeze_task(p))
 				continue;
 
 			/*

@@ -65,7 +65,7 @@ static int try_to_freeze_tasks(bool sig_only)
 		} while_each_thread(g, p);
 		read_unlock(&tasklist_lock);
 
-		if (!sig_only) {
+		if (!user_only) {
 			wq_busy = freeze_workqueues_busy();
 			todo += wq_busy;
 		}

Signature · 1 line

1.7.6

Tejun Heo

2011-09-02 18:27 UTC #7 lore raw

PATCH 6/6 freezer: kill unused set_freezable_with_signal()

There's no in-kernel user of set_freezable_with_signal() and there's no plan to add one. Mixing TIF_SIGPENDING with kernel threads can lead to nasty corner cases as kernel threads never travel signal delivery path on their own.

e.g. the current implementation is buggy in the cancelation path of __thaw_task(). It calls recalc_sigpending_and_wake() in an attempt to clear TIF_SIGPENDING but the function never clears it regardless of sigpending state. This means that signallable freezable kthreads may continue executing with !freezing() && stuck TIF_SIGPENDING, which can be troublesome.

This patch removes set_freezable_with_signal() along with PF_FREEZER_NOSIG and recalc_sigpending*() calls in freezer. User tasks get TIF_SIGPENDING, kernel tasks get woken up and the spurious sigpending is dealt with in the usual signal delivery path.

Signed-off-by Tejun Heo <[email protected]>

---
 include/linux/freezer.h |   20 +-------------------
 include/linux/sched.h   |    1 -
 kernel/freezer.c        |   27 ++++++---------------------
 kernel/kthread.c        |    2 +-
 4 files changed, 8 insertions(+), 42 deletions(-)

include/linux/freezer.h +1 ~~-19~~

index 9193bae..7ffbf04 100644
@@ -48,7 +48,7 @@ static inline bool try_to_freeze(void)
 }
 
 extern bool freeze_task(struct task_struct *p);
-extern bool __set_freezable(bool with_signal);
+extern bool set_freezable(void);
 
 #ifdef CONFIG_CGROUP_FREEZER
 extern bool cgroup_freezing(struct task_struct *task);

@@ -104,23 +104,6 @@ static inline int freezer_should_skip(struct task_struct *p)
 }
 
 /*
- * Tell the freezer that the current task should be frozen by it
- */
-static inline bool set_freezable(void)
-{
-	return __set_freezable(false);
-}
-
-/*
- * Tell the freezer that the current task should be frozen by it and that it
- * should send a fake signal to the task to freeze it.
- */
-static inline bool set_freezable_with_signal(void)
-{
-	return __set_freezable(true);
-}
-
-/*
  * Freezer-friendly wrappers around wait_event_interruptible() and
  * wait_event_interruptible_timeout(), originally defined in <linux/wait.h>
  */

@@ -164,7 +147,6 @@ static inline void freezer_do_not_count(void) {}
 static inline void freezer_count(void) {}
 static inline int freezer_should_skip(struct task_struct *p) { return 0; }
 static inline void set_freezable(void) {}
-static inline void set_freezable_with_signal(void) {}
 
 #define wait_event_freezable(wq, condition)				\
 		wait_event_interruptible(wq, condition)

include/linux/sched.h +0 -1

index 1bb3356..6d45ce7 100644
@@ -1784,7 +1784,6 @@ extern void thread_group_times(struct task_struct *p, cputime_t *ut, cputime_t *
 #define PF_MEMPOLICY	0x10000000	/* Non-default NUMA mempolicy */
 #define PF_MUTEX_TESTER	0x20000000	/* Thread belongs to the rt mutex tester */
 #define PF_FREEZER_SKIP	0x40000000	/* Freezer should not count it as freezable */
-#define PF_FREEZER_NOSIG 0x80000000	/* Freezer won't send signals to it */
 
 /*
  * Only the _current_ task can read/write to tsk->flags, but other

kernel/freezer.c +6 ~~-21~~

index da76b64..770e6f5 100644
@@ -39,7 +39,7 @@ bool freezing_slow_path(struct task_struct *p)
 	if (pm_nosig_freezing || cgroup_freezing(p))
 		return true;
 
-	if (pm_freezing && !(p->flags & PF_FREEZER_NOSIG))
+	if (pm_freezing && !(p->flags & PF_KTHREAD))
 		return true;
 
 	return false;

@@ -72,10 +72,6 @@ bool __refrigerator(bool check_kthr_stop)
 		schedule();
 	}
 
-	spin_lock_irq(&current->sighand->siglock);
-	recalc_sigpending(); /* We sent fake signal, clean it up */
-	spin_unlock_irq(&current->sighand->siglock);
-
 	pr_debug("%s left refrigerator\n", current->comm);
 
 	/*

@@ -120,7 +116,7 @@ bool freeze_task(struct task_struct *p)
 		return false;
 	}
 
-	if (!(p->flags & PF_FREEZER_NOSIG)) {
+	if (!(p->flags & PF_KTHREAD)) {
 		fake_signal_wake_up(p);
 		/*
 		 * fake_signal_wake_up() goes through p's scheduler

@@ -145,28 +141,19 @@ void __thaw_task(struct task_struct *p)
 	 * be visible to @p as waking up implies wmb.  Waking up inside
 	 * freezer_lock also prevents wakeups from leaking outside
 	 * refrigerator.
-	 *
-	 * If !FROZEN, @p hasn't reached refrigerator, recalc sigpending to
-	 * avoid leaving dangling TIF_SIGPENDING behind.
 	 */
 	spin_lock_irqsave(&freezer_lock, flags);
-	if (frozen(p)) {
+	if (frozen(p))
 		wake_up_process(p);
-	} else {
-		spin_lock(&p->sighand->siglock);
-		recalc_sigpending_and_wake(p);
-		spin_unlock(&p->sighand->siglock);
-	}
 	spin_unlock_irqrestore(&freezer_lock, flags);
 }
 
 /**
- * __set_freezable - make %current freezable
- * @with_signal: do we want %TIF_SIGPENDING for notification too?
+ * set_freezable - make %current freezable
  *
  * Mark %current freezable and enter refrigerator if necessary.
  */
-bool __set_freezable(bool with_signal)
+bool set_freezable(void)
 {
 	might_sleep();

@@ -177,10 +164,8 @@ bool __set_freezable(bool with_signal)
 	 */
 	spin_lock_irq(&freezer_lock);
 	current->flags &= ~PF_NOFREEZE;
-	if (with_signal)
-		current->flags &= ~PF_FREEZER_NOSIG;
 	spin_unlock_irq(&freezer_lock);
 
 	return try_to_freeze();
 }
-EXPORT_SYMBOL(__set_freezable);
+EXPORT_SYMBOL(set_freezable);

kernel/kthread.c +1 -1

index a6cbeea..7a4c862 100644
@@ -282,7 +282,7 @@ int kthreadd(void *unused)
 	set_cpus_allowed_ptr(tsk, cpu_all_mask);
 	set_mems_allowed(node_states[N_HIGH_MEMORY]);
 
-	current->flags |= PF_NOFREEZE | PF_FREEZER_NOSIG;
+	current->flags |= PF_NOFREEZE;
 
 	for (;;) {
 		set_current_state(TASK_INTERRUPTIBLE);

Signature · 1 line

1.7.6

Tejun Heo

author

2011-09-02 18:27 UTC #8 lore raw

PATCH 1/6 cgroup_freezer: fix freezer->state setting bug in freezer_change_state()

-v2: Oleg pointed out that re-freezing FROZEN cgroup could increment
system_freezing_cnt. Fixed.

Signed-off-by Tejun Heo <[email protected]>

Cc Paul Menage <[email protected]>

Cc "Rafael J. Wysocki" <[email protected]>

Cc Matt Helsley <[email protected]>

---
 kernel/cgroup_freezer.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

kernel/cgroup_freezer.c +7 -7

index 4e82525..56a457a 100644
@@ -308,24 +308,24 @@ static int freezer_change_state(struct cgroup *cgroup,
 	spin_lock_irq(&freezer->lock);
 
 	update_if_frozen(cgroup, freezer);
-	if (goal_state == freezer->state)
-		goto out;
-
-	freezer->state = goal_state;
 
 	switch (goal_state) {
 	case CGROUP_THAWED:
-		atomic_dec(&system_freezing_cnt);
+		if (freezer->state != CGROUP_THAWED)
+			atomic_dec(&system_freezing_cnt);
+		freezer->state = CGROUP_THAWED;
 		unfreeze_cgroup(cgroup, freezer);
 		break;
 	case CGROUP_FROZEN:
-		atomic_inc(&system_freezing_cnt);
+		if (freezer->state == CGROUP_THAWED)
+			atomic_inc(&system_freezing_cnt);
+		freezer->state = CGROUP_FREEZING;
 		retval = try_to_freeze_cgroup(cgroup, freezer);
 		break;
 	default:
 		BUG();
 	}
-out:
+
 	spin_unlock_irq(&freezer->lock);
 
 	return retval;

Signature · 1 line

1.7.6

Tejun Heo

author

2011-09-02 18:28 UTC #9 lore raw

PATCH 1/6 cgroup_freezer: fix freezer->state setting bug in freezer_change_state()

-v2: Oleg pointed out that re-freezing FROZEN cgroup could increment
system_freezing_cnt. Fixed.

Signed-off-by Tejun Heo <[email protected]>

Cc Paul Menage <[email protected]>

Cc "Rafael J. Wysocki" <[email protected]>

Cc Matt Helsley <[email protected]>

---
 kernel/cgroup_freezer.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

kernel/cgroup_freezer.c +7 -7

index 4e82525..56a457a 100644
@@ -308,24 +308,24 @@ static int freezer_change_state(struct cgroup *cgroup,
 	spin_lock_irq(&freezer->lock);
 
 	update_if_frozen(cgroup, freezer);
-	if (goal_state == freezer->state)
-		goto out;
-
-	freezer->state = goal_state;
 
 	switch (goal_state) {
 	case CGROUP_THAWED:
-		atomic_dec(&system_freezing_cnt);
+		if (freezer->state != CGROUP_THAWED)
+			atomic_dec(&system_freezing_cnt);
+		freezer->state = CGROUP_THAWED;
 		unfreeze_cgroup(cgroup, freezer);
 		break;
 	case CGROUP_FROZEN:
-		atomic_inc(&system_freezing_cnt);
+		if (freezer->state == CGROUP_THAWED)
+			atomic_inc(&system_freezing_cnt);
+		freezer->state = CGROUP_FREEZING;
 		retval = try_to_freeze_cgroup(cgroup, freezer);
 		break;
 	default:
 		BUG();
 	}
-out:
+
 	spin_unlock_irq(&freezer->lock);
 
 	return retval;

Signature · 1 line

1.7.6

Oleg Nesterov

2011-09-04 18:02 UTC #10 lore raw

Re: [PATCH 1/6] cgroup_freezer: fix freezer->state setting bug in freezer_change_state()

Quoted 14 lines · Tejun Heo

kernel/cgroup_freezer.c +1 -5

@@ -308,24 +308,24 @@ static int freezer_change_state(struct cgroup *cgroup,
 	spin_lock_irq(&freezer->lock);
 
 	update_if_frozen(cgroup, freezer);
-	if (goal_state == freezer->state)
-		goto out;
-
-	freezer->state = goal_state;
 
 	switch (goal_state) {
 	case CGROUP_THAWED:
-		atomic_dec(&system_freezing_cnt);
+		if (freezer->state != CGROUP_THAWED)

kernel/cgroup_freezer.c static int freezer_change_state(struct cgroup *cgroup, · line 323

Quoted 10 lines

kernel/cgroup_freezer.c +5 -1

+			atomic_dec(&system_freezing_cnt);
+		freezer->state = CGROUP_THAWED;
 		unfreeze_cgroup(cgroup, freezer);
 		break;
 	case CGROUP_FROZEN:
-		atomic_inc(&system_freezing_cnt);
+		if (freezer->state == CGROUP_THAWED)
+			atomic_inc(&system_freezing_cnt);
+		freezer->state = CGROUP_FREEZING;
 		retval = try_to_freeze_cgroup(cgroup, freezer);

Cough. Now it doesn't look right to me ;)

If the user write "FROZEN" into the control file, we should not turn the CGROUP_FROZEN state into CGROUP_FREEZING. Probably this is harmless, but this looks wrong and this doesn't match the current behaviour, user-visible change.

Oleg.

Tejun Heo

author

2011-09-04 18:11 UTC #11 lore raw

Re: [PATCH 1/6] cgroup_freezer: fix freezer->state setting bug in freezer_change_state()

Quoted 6 lines · Oleg Nesterov

Cough. Now it doesn't look right to me ;)
If the user write "FROZEN" into the control file, we should not turn the CGROUP_FROZEN state into CGROUP_FREEZING. Probably this is harmless, but this looks wrong and this doesn't match the current behaviour, user-visible change.

It's not user-visible as the state is always updated before shown to userland. If we're gonna be re-priming freezing on each FROZEN write, resetting the state to freezing to force state re-calculation is logical thing to do.

Thanks.

Signature · 1 line

tejun

Oleg Nesterov

2011-09-04 18:20 UTC #12 lore raw

Re: [PATCH 1/6] cgroup_freezer: fix freezer->state setting bug in freezer_change_state()

Quoted 11 lines · Tejun Heo

Quoted 6 lines · Oleg Nesterov
Cough. Now it doesn't look right to me ;)
If the user write "FROZEN" into the control file, we should not turn the CGROUP_FROZEN state into CGROUP_FREEZING. Probably this is harmless, but this looks wrong and this doesn't match the current behaviour, user-visible change.
It's not user-visible as the state is always updated before shown to userland.

Ah, indeed, thanks.

OK.

So, the only proble I can see is that update_if_frozen() can hit BUG_ON(nfrozen > 0), but this is off-topic.

I think this patch is correct.

Oleg.

Oleg Nesterov

2011-09-04 18:20 UTC #13 lore raw

Re: [PATCH 1/6] cgroup_freezer: fix freezer->state setting bug in freezer_change_state()

Quoted 11 lines · Tejun Heo

Quoted 6 lines · Oleg Nesterov
Cough. Now it doesn't look right to me ;)
If the user write "FROZEN" into the control file, we should not turn the CGROUP_FROZEN state into CGROUP_FREEZING. Probably this is harmless, but this looks wrong and this doesn't match the current behaviour, user-visible change.
It's not user-visible as the state is always updated before shown to userland.

Ah, indeed, thanks.

OK.

So, the only proble I can see is that update_if_frozen() can hit BUG_ON(nfrozen > 0), but this is off-topic.

I think this patch is correct.

Oleg.

Tejun Heo

author

2011-09-04 18:11 UTC #14 lore raw

Re: [PATCH 1/6] cgroup_freezer: fix freezer->state setting bug in freezer_change_state()

Quoted 6 lines · Oleg Nesterov

Cough. Now it doesn't look right to me ;)
If the user write "FROZEN" into the control file, we should not turn the CGROUP_FROZEN state into CGROUP_FREEZING. Probably this is harmless, but this looks wrong and this doesn't match the current behaviour, user-visible change.

Thanks.

Signature · 1 line

tejun

Oleg Nesterov

2011-09-04 18:02 UTC #15 lore raw

Re: [PATCH 1/6] cgroup_freezer: fix freezer->state setting bug in freezer_change_state()

Quoted 14 lines · Tejun Heo

kernel/cgroup_freezer.c +1 -5

@@ -308,24 +308,24 @@ static int freezer_change_state(struct cgroup *cgroup,
 	spin_lock_irq(&freezer->lock);
 
 	update_if_frozen(cgroup, freezer);
-	if (goal_state == freezer->state)
-		goto out;
-
-	freezer->state = goal_state;
 
 	switch (goal_state) {
 	case CGROUP_THAWED:
-		atomic_dec(&system_freezing_cnt);
+		if (freezer->state != CGROUP_THAWED)

kernel/cgroup_freezer.c static int freezer_change_state(struct cgroup *cgroup, · line 323

Quoted 10 lines

kernel/cgroup_freezer.c +5 -1

+			atomic_dec(&system_freezing_cnt);
+		freezer->state = CGROUP_THAWED;
 		unfreeze_cgroup(cgroup, freezer);
 		break;
 	case CGROUP_FROZEN:
-		atomic_inc(&system_freezing_cnt);
+		if (freezer->state == CGROUP_THAWED)
+			atomic_inc(&system_freezing_cnt);
+		freezer->state = CGROUP_FREEZING;
 		retval = try_to_freeze_cgroup(cgroup, freezer);

Cough. Now it doesn't look right to me ;)

Oleg.

Tejun Heo

author

2011-09-02 18:27 UTC #16 lore raw

PATCH 2/6 freezer: set PF_NOFREEZE on a dying task right before TASK_DEAD setting bug in freezer_change_state()

Signed-off-by Tejun Heo <[email protected]>

Cc "Rafael J. Wysocki" <[email protected]>

---
 kernel/exit.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

kernel/exit.c +4 -6

index ac58259..7b6c4fa 100644
@@ -913,12 +913,6 @@ NORET_TYPE void do_exit(long code)
 
 	ptrace_event(PTRACE_EVENT_EXIT, code);
 
-	/*
-	 * With ptrace notification done, there's no point in freezing from
-	 * here on.  Disallow freezing.
-	 */
-	current->flags |= PF_NOFREEZE;
-
 	validate_creds_for_do_exit(tsk);
 
 	/*

@@ -1044,6 +1038,10 @@ NORET_TYPE void do_exit(long code)
 
 	preempt_disable();
 	exit_rcu();
+
+	/* this task is now dead and freezer should ignore it */
+	current->flags |= PF_NOFREEZE;
+
 	/* causes final put_task_struct in finish_task_switch(). */
 	tsk->state = TASK_DEAD;
 	schedule();

Signature · 1 line

1.7.6

Tejun Heo

author

2011-09-02 18:28 UTC #17 lore raw

PATCH 2/6 freezer: set PF_NOFREEZE on a dying task right before TASK_DEAD setting bug in freezer_change_state()

Signed-off-by Tejun Heo <[email protected]>

Cc "Rafael J. Wysocki" <[email protected]>

---
 kernel/exit.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

kernel/exit.c +4 -6

index ac58259..7b6c4fa 100644
@@ -913,12 +913,6 @@ NORET_TYPE void do_exit(long code)
 
 	ptrace_event(PTRACE_EVENT_EXIT, code);
 
-	/*
-	 * With ptrace notification done, there's no point in freezing from
-	 * here on.  Disallow freezing.
-	 */
-	current->flags |= PF_NOFREEZE;
-
 	validate_creds_for_do_exit(tsk);
 
 	/*

@@ -1044,6 +1038,10 @@ NORET_TYPE void do_exit(long code)
 
 	preempt_disable();
 	exit_rcu();
+
+	/* this task is now dead and freezer should ignore it */
+	current->flags |= PF_NOFREEZE;
+
 	/* causes final put_task_struct in finish_task_switch(). */
 	tsk->state = TASK_DEAD;
 	schedule();

Signature · 1 line

1.7.6

Tejun Heo

author

2011-09-02 18:27 UTC #18 lore raw

PATCH 3/6 freezer: restructure __refrigerator()

-v2: Frozen loop exited without releasing freezer_lock. Fixed.

Signed-off-by Tejun Heo <[email protected]>

Cc "Rafael J. Wysocki" <[email protected]>

---
 kernel/freezer.c |   29 +++++++++++------------------
 1 files changed, 11 insertions(+), 18 deletions(-)

kernel/freezer.c +11 ~~-18~~

index 466ea6b..ebee1ab 100644
@@ -52,36 +52,29 @@ 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;
+	long save = current->state;
 
-	/*
-	 * No point in checking freezing() again - the caller already did.
-	 * Proceed to enter FROZEN.
-	 */
-	spin_lock_irq(&freezer_lock);
-	current->flags |= PF_FROZEN;
-	spin_unlock_irq(&freezer_lock);
-
-	save = current->state;
 	pr_debug("%s entered refrigerator\n", current->comm);
 
-	spin_lock_irq(&current->sighand->siglock);
-	recalc_sigpending(); /* We sent fake signal, clean it up */
-	spin_unlock_irq(&current->sighand->siglock);
-
 	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();
 	}
 
-	/* leave FROZEN */
-	spin_lock_irq(&freezer_lock);
-	current->flags &= ~PF_FROZEN;
-	spin_unlock_irq(&freezer_lock);
+	spin_lock_irq(&current->sighand->siglock);
+	recalc_sigpending(); /* We sent fake signal, clean it up */
+	spin_unlock_irq(&current->sighand->siglock);
 
 	pr_debug("%s left refrigerator\n", current->comm);

Signature · 1 line

1.7.6

Tejun Heo

author

2011-09-02 18:28 UTC #19 lore raw

PATCH 3/6 freezer: restructure __refrigerator()

-v2: Frozen loop exited without releasing freezer_lock. Fixed.

Signed-off-by Tejun Heo <[email protected]>

Cc "Rafael J. Wysocki" <[email protected]>

---
 kernel/freezer.c |   29 +++++++++++------------------
 1 files changed, 11 insertions(+), 18 deletions(-)

kernel/freezer.c +11 ~~-18~~

index 466ea6b..ebee1ab 100644
@@ -52,36 +52,29 @@ 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;
+	long save = current->state;
 
-	/*
-	 * No point in checking freezing() again - the caller already did.
-	 * Proceed to enter FROZEN.
-	 */
-	spin_lock_irq(&freezer_lock);
-	current->flags |= PF_FROZEN;
-	spin_unlock_irq(&freezer_lock);
-
-	save = current->state;
 	pr_debug("%s entered refrigerator\n", current->comm);
 
-	spin_lock_irq(&current->sighand->siglock);
-	recalc_sigpending(); /* We sent fake signal, clean it up */
-	spin_unlock_irq(&current->sighand->siglock);
-
 	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();
 	}
 
-	/* leave FROZEN */
-	spin_lock_irq(&freezer_lock);
-	current->flags &= ~PF_FROZEN;
-	spin_unlock_irq(&freezer_lock);
+	spin_lock_irq(&current->sighand->siglock);
+	recalc_sigpending(); /* We sent fake signal, clean it up */
+	spin_unlock_irq(&current->sighand->siglock);
 
 	pr_debug("%s left refrigerator\n", current->comm);

Signature · 1 line

1.7.6

Tejun Heo

author

2011-09-02 18:27 UTC #20 lore raw

PATCH 4/6 freezer: use lock_task_sighand() in fake_signal_wake_up()

Signed-off-by Tejun Heo <[email protected]>

Cc "Rafael J. Wysocki" <[email protected]>

Cc Paul Menage <[email protected]>

---
 kernel/freezer.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

kernel/freezer.c +4 -3

index ebee1ab..9846133 100644
@@ -93,9 +93,10 @@ static void fake_signal_wake_up(struct task_struct *p)
 {
 	unsigned long flags;
 
-	spin_lock_irqsave(&p->sighand->siglock, flags);
-	signal_wake_up(p, 0);
-	spin_unlock_irqrestore(&p->sighand->siglock, flags);
+	if (lock_task_sighand(p, &flags)) {
+		signal_wake_up(p, 0);
+		unlock_task_sighand(p, &flags);
+	}
 }
 
 /**

Signature · 1 line

1.7.6

Tejun Heo

author

2011-09-02 18:28 UTC #21 lore raw

PATCH 4/6 freezer: use lock_task_sighand() in fake_signal_wake_up()

Signed-off-by Tejun Heo <[email protected]>

Cc "Rafael J. Wysocki" <[email protected]>

Cc Paul Menage <[email protected]>

---
 kernel/freezer.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

kernel/freezer.c +4 -3

index ebee1ab..9846133 100644
@@ -93,9 +93,10 @@ static void fake_signal_wake_up(struct task_struct *p)
 {
 	unsigned long flags;
 
-	spin_lock_irqsave(&p->sighand->siglock, flags);
-	signal_wake_up(p, 0);
-	spin_unlock_irqrestore(&p->sighand->siglock, flags);
+	if (lock_task_sighand(p, &flags)) {
+		signal_wake_up(p, 0);
+		unlock_task_sighand(p, &flags);
+	}
 }
 
 /**

Signature · 1 line

1.7.6

Tejun Heo

author

2011-09-02 18:27 UTC #22 lore raw

PATCH 5/6 freezer: remove unused @sig_only from freeze_task()

While at it, rewrite function comment for freeze_task() and rename @sig_only to @user_only in try_to_freeze_tasks().

This patch doesn't cause any functional change.

Signed-off-by Tejun Heo <[email protected]>

---
 include/linux/freezer.h |    2 +-
 kernel/cgroup_freezer.c |    4 ++--
 kernel/freezer.c        |   21 +++++++++------------
 kernel/power/process.c  |    8 ++++----
 4 files changed, 16 insertions(+), 19 deletions(-)

include/linux/freezer.h +1 -1

index 122b5ce..9193bae 100644
@@ -47,7 +47,7 @@ static inline bool try_to_freeze(void)
 	return __refrigerator(false);
 }
 
-extern bool freeze_task(struct task_struct *p, bool sig_only);
+extern bool freeze_task(struct task_struct *p);
 extern bool __set_freezable(bool with_signal);
 
 #ifdef CONFIG_CGROUP_FREEZER

kernel/cgroup_freezer.c +2 -2

index 56a457a..8753e7b 100644
@@ -206,7 +206,7 @@ static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task)
 
 	/* Locking avoids race with FREEZING -> THAWED transitions. */
 	if (freezer->state == CGROUP_FREEZING)
-		freeze_task(task, true);
+		freeze_task(task);
 	spin_unlock_irq(&freezer->lock);
 }

@@ -274,7 +274,7 @@ static int try_to_freeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
 
 	cgroup_iter_start(cgroup, &it);
 	while ((task = cgroup_iter_next(cgroup, &it))) {
-		if (!freeze_task(task, true))
+		if (!freeze_task(task))
 			continue;
 		if (frozen(task))
 			continue;

kernel/freezer.c +9 ~~-12~~

index 9846133..da76b64 100644
@@ -100,20 +100,17 @@ static void fake_signal_wake_up(struct task_struct *p)
 }
 
 /**
- *	freeze_task - send a freeze request to given task
- *	@p: task to send the request to
- *	@sig_only: if set, the request will only be sent if the task has the
- *		PF_FREEZER_NOSIG flag unset
- *	Return value: 'false', if @sig_only is set and the task has
- *		PF_FREEZER_NOSIG set or the task is frozen, 'true', otherwise
+ * freeze_task - send a freeze request to given task
+ * @p: task to send the request to
  *
- *	The freeze request is sent by setting the tasks's TIF_FREEZE flag and
- *	either sending a fake signal to it or waking it up, depending on whether
- *	or not it has PF_FREEZER_NOSIG set.  If @sig_only is set and the task
- *	has PF_FREEZER_NOSIG set (ie. it is a typical kernel thread), its
- *	TIF_FREEZE flag will not be set.
+ * If @p is freezing, the freeze request is sent by setting %TIF_FREEZE
+ * flag and either sending a fake signal to it or waking it up, depending
+ * on whether it has %PF_FREEZER_NOSIG set.
+ *
+ * RETURNS:
+ * %false, if @p is not freezing or already frozen; %true, otherwise
  */
-bool freeze_task(struct task_struct *p, bool sig_only)
+bool freeze_task(struct task_struct *p)
 {
 	unsigned long flags;

kernel/power/process.c +4 -4

index fe06ddf..ab4fd7b 100644
@@ -22,7 +22,7 @@
  */
 #define TIMEOUT	(20 * HZ)
 
-static int try_to_freeze_tasks(bool sig_only)
+static int try_to_freeze_tasks(bool user_only)
 {
 	struct task_struct *g, *p;
 	unsigned long end_time;

@@ -37,14 +37,14 @@ static int try_to_freeze_tasks(bool sig_only)
 
 	end_time = jiffies + TIMEOUT;
 
-	if (!sig_only)
+	if (!user_only)
 		freeze_workqueues_begin();
 
 	while (true) {
 		todo = 0;
 		read_lock(&tasklist_lock);
 		do_each_thread(g, p) {
-			if (p == current || !freeze_task(p, sig_only))
+			if (p == current || !freeze_task(p))
 				continue;
 
 			/*

@@ -65,7 +65,7 @@ static int try_to_freeze_tasks(bool sig_only)
 		} while_each_thread(g, p);
 		read_unlock(&tasklist_lock);
 
-		if (!sig_only) {
+		if (!user_only) {
 			wq_busy = freeze_workqueues_busy();
 			todo += wq_busy;
 		}

Signature · 1 line

1.7.6

Tejun Heo

author

2011-09-02 18:28 UTC #23 lore raw

PATCH 5/6 freezer: remove unused @sig_only from freeze_task()

While at it, rewrite function comment for freeze_task() and rename @sig_only to @user_only in try_to_freeze_tasks().

This patch doesn't cause any functional change.

Signed-off-by Tejun Heo <[email protected]>

---
 include/linux/freezer.h |    2 +-
 kernel/cgroup_freezer.c |    4 ++--
 kernel/freezer.c        |   21 +++++++++------------
 kernel/power/process.c  |    8 ++++----
 4 files changed, 16 insertions(+), 19 deletions(-)

include/linux/freezer.h +1 -1

index 122b5ce..9193bae 100644
@@ -47,7 +47,7 @@ static inline bool try_to_freeze(void)
 	return __refrigerator(false);
 }
 
-extern bool freeze_task(struct task_struct *p, bool sig_only);
+extern bool freeze_task(struct task_struct *p);
 extern bool __set_freezable(bool with_signal);
 
 #ifdef CONFIG_CGROUP_FREEZER

kernel/cgroup_freezer.c +2 -2

index 56a457a..8753e7b 100644
@@ -206,7 +206,7 @@ static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task)
 
 	/* Locking avoids race with FREEZING -> THAWED transitions. */
 	if (freezer->state == CGROUP_FREEZING)
-		freeze_task(task, true);
+		freeze_task(task);
 	spin_unlock_irq(&freezer->lock);
 }

@@ -274,7 +274,7 @@ static int try_to_freeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
 
 	cgroup_iter_start(cgroup, &it);
 	while ((task = cgroup_iter_next(cgroup, &it))) {
-		if (!freeze_task(task, true))
+		if (!freeze_task(task))
 			continue;
 		if (frozen(task))
 			continue;

kernel/freezer.c +9 ~~-12~~

index 9846133..da76b64 100644
@@ -100,20 +100,17 @@ static void fake_signal_wake_up(struct task_struct *p)
 }
 
 /**
- *	freeze_task - send a freeze request to given task
- *	@p: task to send the request to
- *	@sig_only: if set, the request will only be sent if the task has the
- *		PF_FREEZER_NOSIG flag unset
- *	Return value: 'false', if @sig_only is set and the task has
- *		PF_FREEZER_NOSIG set or the task is frozen, 'true', otherwise
+ * freeze_task - send a freeze request to given task
+ * @p: task to send the request to
  *
- *	The freeze request is sent by setting the tasks's TIF_FREEZE flag and
- *	either sending a fake signal to it or waking it up, depending on whether
- *	or not it has PF_FREEZER_NOSIG set.  If @sig_only is set and the task
- *	has PF_FREEZER_NOSIG set (ie. it is a typical kernel thread), its
- *	TIF_FREEZE flag will not be set.
+ * If @p is freezing, the freeze request is sent by setting %TIF_FREEZE
+ * flag and either sending a fake signal to it or waking it up, depending
+ * on whether it has %PF_FREEZER_NOSIG set.
+ *
+ * RETURNS:
+ * %false, if @p is not freezing or already frozen; %true, otherwise
  */
-bool freeze_task(struct task_struct *p, bool sig_only)
+bool freeze_task(struct task_struct *p)
 {
 	unsigned long flags;

kernel/power/process.c +4 -4

index fe06ddf..ab4fd7b 100644
@@ -22,7 +22,7 @@
  */
 #define TIMEOUT	(20 * HZ)
 
-static int try_to_freeze_tasks(bool sig_only)
+static int try_to_freeze_tasks(bool user_only)
 {
 	struct task_struct *g, *p;
 	unsigned long end_time;

@@ -37,14 +37,14 @@ static int try_to_freeze_tasks(bool sig_only)
 
 	end_time = jiffies + TIMEOUT;
 
-	if (!sig_only)
+	if (!user_only)
 		freeze_workqueues_begin();
 
 	while (true) {
 		todo = 0;
 		read_lock(&tasklist_lock);
 		do_each_thread(g, p) {
-			if (p == current || !freeze_task(p, sig_only))
+			if (p == current || !freeze_task(p))
 				continue;
 
 			/*

@@ -65,7 +65,7 @@ static int try_to_freeze_tasks(bool sig_only)
 		} while_each_thread(g, p);
 		read_unlock(&tasklist_lock);
 
-		if (!sig_only) {
+		if (!user_only) {
 			wq_busy = freeze_workqueues_busy();
 			todo += wq_busy;
 		}

Signature · 1 line

1.7.6

Tejun Heo

author

2011-09-02 18:27 UTC #24 lore raw

PATCH 6/6 freezer: kill unused set_freezable_with_signal()

Signed-off-by Tejun Heo <[email protected]>

---
 include/linux/freezer.h |   20 +-------------------
 include/linux/sched.h   |    1 -
 kernel/freezer.c        |   27 ++++++---------------------
 kernel/kthread.c        |    2 +-
 4 files changed, 8 insertions(+), 42 deletions(-)

include/linux/freezer.h +1 ~~-19~~

index 9193bae..7ffbf04 100644
@@ -48,7 +48,7 @@ static inline bool try_to_freeze(void)
 }
 
 extern bool freeze_task(struct task_struct *p);
-extern bool __set_freezable(bool with_signal);
+extern bool set_freezable(void);
 
 #ifdef CONFIG_CGROUP_FREEZER
 extern bool cgroup_freezing(struct task_struct *task);

@@ -104,23 +104,6 @@ static inline int freezer_should_skip(struct task_struct *p)
 }
 
 /*
- * Tell the freezer that the current task should be frozen by it
- */
-static inline bool set_freezable(void)
-{
-	return __set_freezable(false);
-}
-
-/*
- * Tell the freezer that the current task should be frozen by it and that it
- * should send a fake signal to the task to freeze it.
- */
-static inline bool set_freezable_with_signal(void)
-{
-	return __set_freezable(true);
-}
-
-/*
  * Freezer-friendly wrappers around wait_event_interruptible() and
  * wait_event_interruptible_timeout(), originally defined in <linux/wait.h>
  */

@@ -164,7 +147,6 @@ static inline void freezer_do_not_count(void) {}
 static inline void freezer_count(void) {}
 static inline int freezer_should_skip(struct task_struct *p) { return 0; }
 static inline void set_freezable(void) {}
-static inline void set_freezable_with_signal(void) {}
 
 #define wait_event_freezable(wq, condition)				\
 		wait_event_interruptible(wq, condition)

include/linux/sched.h +0 -1

index 1bb3356..6d45ce7 100644
@@ -1784,7 +1784,6 @@ extern void thread_group_times(struct task_struct *p, cputime_t *ut, cputime_t *
 #define PF_MEMPOLICY	0x10000000	/* Non-default NUMA mempolicy */
 #define PF_MUTEX_TESTER	0x20000000	/* Thread belongs to the rt mutex tester */
 #define PF_FREEZER_SKIP	0x40000000	/* Freezer should not count it as freezable */
-#define PF_FREEZER_NOSIG 0x80000000	/* Freezer won't send signals to it */
 
 /*
  * Only the _current_ task can read/write to tsk->flags, but other

kernel/freezer.c +6 ~~-21~~

index da76b64..770e6f5 100644
@@ -39,7 +39,7 @@ bool freezing_slow_path(struct task_struct *p)
 	if (pm_nosig_freezing || cgroup_freezing(p))
 		return true;
 
-	if (pm_freezing && !(p->flags & PF_FREEZER_NOSIG))
+	if (pm_freezing && !(p->flags & PF_KTHREAD))
 		return true;
 
 	return false;

@@ -72,10 +72,6 @@ bool __refrigerator(bool check_kthr_stop)
 		schedule();
 	}
 
-	spin_lock_irq(&current->sighand->siglock);
-	recalc_sigpending(); /* We sent fake signal, clean it up */
-	spin_unlock_irq(&current->sighand->siglock);
-
 	pr_debug("%s left refrigerator\n", current->comm);
 
 	/*

@@ -120,7 +116,7 @@ bool freeze_task(struct task_struct *p)
 		return false;
 	}
 
-	if (!(p->flags & PF_FREEZER_NOSIG)) {
+	if (!(p->flags & PF_KTHREAD)) {
 		fake_signal_wake_up(p);
 		/*
 		 * fake_signal_wake_up() goes through p's scheduler

@@ -145,28 +141,19 @@ void __thaw_task(struct task_struct *p)
 	 * be visible to @p as waking up implies wmb.  Waking up inside
 	 * freezer_lock also prevents wakeups from leaking outside
 	 * refrigerator.
-	 *
-	 * If !FROZEN, @p hasn't reached refrigerator, recalc sigpending to
-	 * avoid leaving dangling TIF_SIGPENDING behind.
 	 */
 	spin_lock_irqsave(&freezer_lock, flags);
-	if (frozen(p)) {
+	if (frozen(p))
 		wake_up_process(p);
-	} else {
-		spin_lock(&p->sighand->siglock);
-		recalc_sigpending_and_wake(p);
-		spin_unlock(&p->sighand->siglock);
-	}
 	spin_unlock_irqrestore(&freezer_lock, flags);
 }
 
 /**
- * __set_freezable - make %current freezable
- * @with_signal: do we want %TIF_SIGPENDING for notification too?
+ * set_freezable - make %current freezable
  *
  * Mark %current freezable and enter refrigerator if necessary.
  */
-bool __set_freezable(bool with_signal)
+bool set_freezable(void)
 {
 	might_sleep();

@@ -177,10 +164,8 @@ bool __set_freezable(bool with_signal)
 	 */
 	spin_lock_irq(&freezer_lock);
 	current->flags &= ~PF_NOFREEZE;
-	if (with_signal)
-		current->flags &= ~PF_FREEZER_NOSIG;
 	spin_unlock_irq(&freezer_lock);
 
 	return try_to_freeze();
 }
-EXPORT_SYMBOL(__set_freezable);
+EXPORT_SYMBOL(set_freezable);

kernel/kthread.c +1 -1

index a6cbeea..7a4c862 100644
@@ -282,7 +282,7 @@ int kthreadd(void *unused)
 	set_cpus_allowed_ptr(tsk, cpu_all_mask);
 	set_mems_allowed(node_states[N_HIGH_MEMORY]);
 
-	current->flags |= PF_NOFREEZE | PF_FREEZER_NOSIG;
+	current->flags |= PF_NOFREEZE;
 
 	for (;;) {
 		set_current_state(TASK_INTERRUPTIBLE);

Signature · 1 line

1.7.6

Tejun Heo

author

2011-09-02 18:28 UTC #25 lore raw

PATCH 6/6 freezer: kill unused set_freezable_with_signal()

Signed-off-by Tejun Heo <[email protected]>

---
 include/linux/freezer.h |   20 +-------------------
 include/linux/sched.h   |    1 -
 kernel/freezer.c        |   27 ++++++---------------------
 kernel/kthread.c        |    2 +-
 4 files changed, 8 insertions(+), 42 deletions(-)

include/linux/freezer.h +1 ~~-19~~

index 9193bae..7ffbf04 100644
@@ -48,7 +48,7 @@ static inline bool try_to_freeze(void)
 }
 
 extern bool freeze_task(struct task_struct *p);
-extern bool __set_freezable(bool with_signal);
+extern bool set_freezable(void);
 
 #ifdef CONFIG_CGROUP_FREEZER
 extern bool cgroup_freezing(struct task_struct *task);

@@ -104,23 +104,6 @@ static inline int freezer_should_skip(struct task_struct *p)
 }
 
 /*
- * Tell the freezer that the current task should be frozen by it
- */
-static inline bool set_freezable(void)
-{
-	return __set_freezable(false);
-}
-
-/*
- * Tell the freezer that the current task should be frozen by it and that it
- * should send a fake signal to the task to freeze it.
- */
-static inline bool set_freezable_with_signal(void)
-{
-	return __set_freezable(true);
-}
-
-/*
  * Freezer-friendly wrappers around wait_event_interruptible() and
  * wait_event_interruptible_timeout(), originally defined in <linux/wait.h>
  */

@@ -164,7 +147,6 @@ static inline void freezer_do_not_count(void) {}
 static inline void freezer_count(void) {}
 static inline int freezer_should_skip(struct task_struct *p) { return 0; }
 static inline void set_freezable(void) {}
-static inline void set_freezable_with_signal(void) {}
 
 #define wait_event_freezable(wq, condition)				\
 		wait_event_interruptible(wq, condition)

include/linux/sched.h +0 -1

index 1bb3356..6d45ce7 100644
@@ -1784,7 +1784,6 @@ extern void thread_group_times(struct task_struct *p, cputime_t *ut, cputime_t *
 #define PF_MEMPOLICY	0x10000000	/* Non-default NUMA mempolicy */
 #define PF_MUTEX_TESTER	0x20000000	/* Thread belongs to the rt mutex tester */
 #define PF_FREEZER_SKIP	0x40000000	/* Freezer should not count it as freezable */
-#define PF_FREEZER_NOSIG 0x80000000	/* Freezer won't send signals to it */
 
 /*
  * Only the _current_ task can read/write to tsk->flags, but other

kernel/freezer.c +6 ~~-21~~

index da76b64..770e6f5 100644
@@ -39,7 +39,7 @@ bool freezing_slow_path(struct task_struct *p)
 	if (pm_nosig_freezing || cgroup_freezing(p))
 		return true;
 
-	if (pm_freezing && !(p->flags & PF_FREEZER_NOSIG))
+	if (pm_freezing && !(p->flags & PF_KTHREAD))
 		return true;
 
 	return false;

@@ -72,10 +72,6 @@ bool __refrigerator(bool check_kthr_stop)
 		schedule();
 	}
 
-	spin_lock_irq(&current->sighand->siglock);
-	recalc_sigpending(); /* We sent fake signal, clean it up */
-	spin_unlock_irq(&current->sighand->siglock);
-
 	pr_debug("%s left refrigerator\n", current->comm);
 
 	/*

@@ -120,7 +116,7 @@ bool freeze_task(struct task_struct *p)
 		return false;
 	}
 
-	if (!(p->flags & PF_FREEZER_NOSIG)) {
+	if (!(p->flags & PF_KTHREAD)) {
 		fake_signal_wake_up(p);
 		/*
 		 * fake_signal_wake_up() goes through p's scheduler

@@ -145,28 +141,19 @@ void __thaw_task(struct task_struct *p)
 	 * be visible to @p as waking up implies wmb.  Waking up inside
 	 * freezer_lock also prevents wakeups from leaking outside
 	 * refrigerator.
-	 *
-	 * If !FROZEN, @p hasn't reached refrigerator, recalc sigpending to
-	 * avoid leaving dangling TIF_SIGPENDING behind.
 	 */
 	spin_lock_irqsave(&freezer_lock, flags);
-	if (frozen(p)) {
+	if (frozen(p))
 		wake_up_process(p);
-	} else {
-		spin_lock(&p->sighand->siglock);
-		recalc_sigpending_and_wake(p);
-		spin_unlock(&p->sighand->siglock);
-	}
 	spin_unlock_irqrestore(&freezer_lock, flags);
 }
 
 /**
- * __set_freezable - make %current freezable
- * @with_signal: do we want %TIF_SIGPENDING for notification too?
+ * set_freezable - make %current freezable
  *
  * Mark %current freezable and enter refrigerator if necessary.
  */
-bool __set_freezable(bool with_signal)
+bool set_freezable(void)
 {
 	might_sleep();

@@ -177,10 +164,8 @@ bool __set_freezable(bool with_signal)
 	 */
 	spin_lock_irq(&freezer_lock);
 	current->flags &= ~PF_NOFREEZE;
-	if (with_signal)
-		current->flags &= ~PF_FREEZER_NOSIG;
 	spin_unlock_irq(&freezer_lock);
 
 	return try_to_freeze();
 }
-EXPORT_SYMBOL(__set_freezable);
+EXPORT_SYMBOL(set_freezable);

kernel/kthread.c +1 -1

index a6cbeea..7a4c862 100644
@@ -282,7 +282,7 @@ int kthreadd(void *unused)
 	set_cpus_allowed_ptr(tsk, cpu_all_mask);
 	set_mems_allowed(node_states[N_HIGH_MEMORY]);
 
-	current->flags |= PF_NOFREEZE | PF_FREEZER_NOSIG;
+	current->flags |= PF_NOFREEZE;
 
 	for (;;) {
 		set_current_state(TASK_INTERRUPTIBLE);

Signature · 1 line

1.7.6

A message that is not in the archive (sent off-list or not yet received): 1314988070-12244-7-git-send-email-tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org

Oleg Nesterov

2011-09-04 18:46 UTC #27 lore raw

Re: [PATCH 6/6] freezer: kill unused set_freezable_with_signal()

Quoted 3 lines · Tejun Heo

This patch removes set_freezable_with_signal() along with PF_FREEZER_NOSIG

Great. I never understood PF_FREEZER_NOSIG logic ;)

One question,

kernel/freezer.c bool __refrigerator(bool check_kthr_stop) · line 78

Quoted 8 lines

kernel/freezer.c +0 -4

@@ -72,10 +72,6 @@ bool __refrigerator(bool check_kthr_stop)
 		schedule();
 	}
 
-	spin_lock_irq(&current->sighand->siglock);
-	recalc_sigpending(); /* We sent fake signal, clean it up */
-	spin_unlock_irq(&current->sighand->siglock);
-

Why? This recalc_sigpending() makes sense imho. Otherwise the user-space tasks (almost) always return with TIF_SIGPENDING. May be we can do this under "if (PF_KTRHREAD)".

For example. Suppose the user-space task does wait_event_freezable()...

Hmm. OTOH, wait_event_freezable() looks wrong anyway... So probably this doesn't matter. ptrace_stop/get_signal_to_deliver doesn't need this, probably we do not care about the other callers.

It seems, a lot of get_signal_to_deliver() calles also call try_to_freeze() for no reason.

So, yes, I am starting to think this change is fine too ;)

Oleg.

Oleg Nesterov

2011-09-04 18:46 UTC #28 lore raw

Re: [PATCH 6/6] freezer: kill unused set_freezable_with_signal()

Quoted 3 lines · Tejun Heo

This patch removes set_freezable_with_signal() along with PF_FREEZER_NOSIG

Great. I never understood PF_FREEZER_NOSIG logic ;)

One question,

kernel/freezer.c bool __refrigerator(bool check_kthr_stop) · line 78

Quoted 8 lines

kernel/freezer.c +0 -4

@@ -72,10 +72,6 @@ bool __refrigerator(bool check_kthr_stop)
 		schedule();
 	}
 
-	spin_lock_irq(&current->sighand->siglock);
-	recalc_sigpending(); /* We sent fake signal, clean it up */
-	spin_unlock_irq(&current->sighand->siglock);
-

Why? This recalc_sigpending() makes sense imho. Otherwise the user-space tasks (almost) always return with TIF_SIGPENDING. May be we can do this under "if (PF_KTRHREAD)".

For example. Suppose the user-space task does wait_event_freezable()...

Hmm. OTOH, wait_event_freezable() looks wrong anyway... So probably this doesn't matter. ptrace_stop/get_signal_to_deliver doesn't need this, probably we do not care about the other callers.

It seems, a lot of get_signal_to_deliver() calles also call try_to_freeze() for no reason.

So, yes, I am starting to think this change is fine too ;)

Oleg.

Oleg Nesterov

2011-09-04 18:49 UTC #29 lore raw

Re: [PATCH 6/6] freezer: kill unused set_freezable_with_signal()

Quoted 3 lines · Tejun Heo

This patch removes set_freezable_with_signal() along with PF_FREEZER_NOSIG

Great. I never understood PF_FREEZER_NOSIG logic ;)

One question,

kernel/freezer.c bool __refrigerator(bool check_kthr_stop) · line 78

Quoted 8 lines

kernel/freezer.c +0 -4

@@ -72,10 +72,6 @@ bool __refrigerator(bool check_kthr_stop)
 		schedule();
 	}
 
-	spin_lock_irq(&current->sighand->siglock);
-	recalc_sigpending(); /* We sent fake signal, clean it up */
-	spin_unlock_irq(&current->sighand->siglock);
-

Why? This recalc_sigpending() makes sense imho. Otherwise the user-space tasks (almost) always return with TIF_SIGPENDING. May be we can do this under "if (PF_KTRHREAD)".

For example. Suppose the user-space task does wait_event_freezable()...

Hmm. OTOH, wait_event_freezable() looks wrong anyway... So probably this doesn't matter. ptrace_stop/get_signal_to_deliver doesn't need this, probably we do not care about the other callers.

It seems, a lot of get_signal_to_deliver() calles also call try_to_freeze() for no reason.

So, yes, I am starting to think this change is fine too ;)

Oleg.

A message that is not in the archive (sent off-list or not yet received): [email protected]

Tejun Heo

2011-09-05 02:33 UTC #31 lore raw

Re: [PATCH 6/6] freezer: kill unused set_freezable_with_signal()

Hello, Oleg.

Quoted 4 lines · Oleg Nesterov

Quoted 4 lines

kernel/freezer.c +0 -0

@@ -72,10 +72,6 @@ bool __refrigerator(bool check_kthr_stop)
 		schedule();
 	}

Quoted 8 lines

kernel/freezer.c bool __refrigerator(bool check_kthr_stop) · line 78
Quoted 4 lines
kernel/freezer.c +0 -4
-	spin_lock_irq(&current->sighand->siglock);
-	recalc_sigpending(); /* We sent fake signal, clean it up */
-	spin_unlock_irq(&current->sighand->siglock);
-
Why? This recalc_sigpending() makes sense imho. Otherwise the user-space tasks (almost) always return with TIF_SIGPENDING. May be we can do this under "if (PF_KTRHREAD)".

PF_KTHREAD no longer gets TIF_SIGPENDING set, so...

Quoted 5 lines

For example. Suppose the user-space task does wait_event_freezable()...
Hmm. OTOH, wait_event_freezable() looks wrong anyway... So probably this doesn't matter. ptrace_stop/get_signal_to_deliver doesn't need this, probably we do not care about the other callers.

Can you elaborate on it being wrong? Do you mean the possibility of leaking spurious TIF_SIGPENDING?

Quoted 2 lines

It seems, a lot of get_signal_to_deliver() calles also call try_to_freeze() for no reason.

Yeap, it doesn't really matter. In most cases userland tasks would get parked in the signal delivery path anyway and if a task is deep in the kernel, it should and usually does hit syscall restart path. Except for few special cases (maybe some of unfailable NFS calls), there'userland tasks shouldn't take try_to_freeze() path outside of signal delivery / job control path.

Also, in general, I don't think it's a good idea to add non-trivial optimization like above to code path which is as cold as it gets without any backing data. The PM freezer used to busy loop until all tasks enter refrigerator. Nobody cares whether some tasks enter signal delivery path one more time.

Quoted 1 line

So, yes, I am starting to think this change is fine too ;)

Yeay. :)

Thanks.

Signature · 1 line

tejun

Tejun Heo

author

2011-09-05 02:33 UTC #32 lore raw

Re: [PATCH 6/6] freezer: kill unused set_freezable_with_signal()

Hello, Oleg.

Quoted 4 lines · Oleg Nesterov

Quoted 4 lines

kernel/freezer.c +0 -0

@@ -72,10 +72,6 @@ bool __refrigerator(bool check_kthr_stop)
 		schedule();
 	}

Quoted 8 lines

kernel/freezer.c bool __refrigerator(bool check_kthr_stop) · line 78
Quoted 4 lines
kernel/freezer.c +0 -4
-	spin_lock_irq(&current->sighand->siglock);
-	recalc_sigpending(); /* We sent fake signal, clean it up */
-	spin_unlock_irq(&current->sighand->siglock);
-
Why? This recalc_sigpending() makes sense imho. Otherwise the user-space tasks (almost) always return with TIF_SIGPENDING. May be we can do this under "if (PF_KTRHREAD)".

PF_KTHREAD no longer gets TIF_SIGPENDING set, so...

Quoted 5 lines

For example. Suppose the user-space task does wait_event_freezable()...
Hmm. OTOH, wait_event_freezable() looks wrong anyway... So probably this doesn't matter. ptrace_stop/get_signal_to_deliver doesn't need this, probably we do not care about the other callers.

Can you elaborate on it being wrong? Do you mean the possibility of leaking spurious TIF_SIGPENDING?

Quoted 2 lines

It seems, a lot of get_signal_to_deliver() calles also call try_to_freeze() for no reason.

Quoted 1 line

So, yes, I am starting to think this change is fine too ;)

Yeay. :)

Thanks.

Signature · 1 line

tejun

Tejun Heo

author

2011-09-05 02:33 UTC #33 lore raw

Re: [PATCH 6/6] freezer: kill unused set_freezable_with_signal()

Hello, Oleg.

Quoted 4 lines · Oleg Nesterov

Quoted 4 lines

kernel/freezer.c +0 -0

@@ -72,10 +72,6 @@ bool __refrigerator(bool check_kthr_stop)
 		schedule();
 	}

Quoted 8 lines

kernel/freezer.c bool __refrigerator(bool check_kthr_stop) · line 78
Quoted 4 lines
kernel/freezer.c +0 -4
-	spin_lock_irq(&current->sighand->siglock);
-	recalc_sigpending(); /* We sent fake signal, clean it up */
-	spin_unlock_irq(&current->sighand->siglock);
-
Why? This recalc_sigpending() makes sense imho. Otherwise the user-space tasks (almost) always return with TIF_SIGPENDING. May be we can do this under "if (PF_KTRHREAD)".

PF_KTHREAD no longer gets TIF_SIGPENDING set, so...

Quoted 5 lines

For example. Suppose the user-space task does wait_event_freezable()...
Hmm. OTOH, wait_event_freezable() looks wrong anyway... So probably this doesn't matter. ptrace_stop/get_signal_to_deliver doesn't need this, probably we do not care about the other callers.

Can you elaborate on it being wrong? Do you mean the possibility of leaking spurious TIF_SIGPENDING?

Quoted 2 lines

It seems, a lot of get_signal_to_deliver() calles also call try_to_freeze() for no reason.

Quoted 1 line

So, yes, I am starting to think this change is fine too ;)

Yeay. :)

Thanks.

Signature · 1 line

tejun

A message that is not in the archive (sent off-list or not yet received): 20110905023315.GB9807-Gd/HAXX7CRxy/[email protected]

Tejun Heo

2011-09-05 02:35 UTC #35 lore raw

Re: [PATCH 6/6] freezer: kill unused set_freezable_with_signal()

Quoted 3 lines · Tejun Heo

Quoted 1 line
So, yes, I am starting to think this change is fine too ;)
Yeay. :)

Ooh, can I add your Acked-by before sending pull request to Rafael?

Thanks.

Signature · 1 line

tejun

Oleg Nesterov

2011-09-05 16:20 UTC #36 lore raw

Re: [PATCH 6/6] freezer: kill unused set_freezable_with_signal()

Quoted 17 lines · Tejun Heo

Hello, Oleg.
Quoted 4 lines · Oleg Nesterov
Quoted 4 lines
kernel/freezer.c +0 -0
@@ -72,10 +72,6 @@ bool __refrigerator(bool check_kthr_stop)
 		schedule();
 	}
 
Quoted 8 lines
kernel/freezer.c bool __refrigerator(bool check_kthr_stop) · line 78
Quoted 4 lines
kernel/freezer.c +0 -4
-	spin_lock_irq(&current->sighand->siglock);
-	recalc_sigpending(); /* We sent fake signal, clean it up */
-	spin_unlock_irq(&current->sighand->siglock);
-
Why? This recalc_sigpending() makes sense imho. Otherwise the user-space tasks (almost) always return with TIF_SIGPENDING. May be we can do this under "if (PF_KTRHREAD)".

Quoted 1 line

PF_KTHREAD no longer gets TIF_SIGPENDING set, so...

Yes,

Quoted 8 lines

Quoted 5 lines
For example. Suppose the user-space task does wait_event_freezable()...
Hmm. OTOH, wait_event_freezable() looks wrong anyway... So probably this doesn't matter. ptrace_stop/get_signal_to_deliver doesn't need this, probably we do not care about the other callers.
Can you elaborate on it being wrong? Do you mean the possibility of leaking spurious TIF_SIGPENDING?

Perhaps it is correct... Just I do not understand what it should do. I thought it is "wait_for_event && do_not_block_freezer". And at first glance the code looks as if it tries to do this. Say, in the "likely" case we restart wait_event_interruptible() after refrigerator().

But this looks racy. Suppose that freezing() is already false when try_to_freeze() or __refrigerator() is called. Say, cgroup_freezer does freeze_task() + __thaw_task(). Why it returns -ERESTARTSYS in this case?

And if it can be used by the userspace thread, then we should probably do recalc_sigpending() somewhere, otherwise wait_event_freezable() will always return -ERESTARTSYS after __refrigerator().

Oleg.

Tejun Heo

author

2011-09-05 02:35 UTC #37 lore raw

Re: [PATCH 6/6] freezer: kill unused set_freezable_with_signal()

Quoted 3 lines · Tejun Heo

Quoted 1 line
So, yes, I am starting to think this change is fine too ;)
Yeay. :)

Ooh, can I add your Acked-by before sending pull request to Rafael?

Thanks.

Signature · 1 line

tejun

Tejun Heo

author

2011-09-05 02:35 UTC #38 lore raw

Re: [PATCH 6/6] freezer: kill unused set_freezable_with_signal()

Quoted 3 lines · Tejun Heo

Quoted 1 line
So, yes, I am starting to think this change is fine too ;)
Yeay. :)

Ooh, can I add your Acked-by before sending pull request to Rafael?

Thanks.

Signature · 1 line

tejun

A message that is not in the archive (sent off-list or not yet received): 20110905023505.GC9807-Gd/HAXX7CRxy/[email protected]

Oleg Nesterov

2011-09-05 16:21 UTC #40 lore raw

Re: [PATCH 6/6] freezer: kill unused set_freezable_with_signal()

Quoted 2 lines · Tejun Heo

Ooh, can I add your Acked-by before sending pull request to Rafael?

Yes, thanks, please feel free.

Oleg.

Oleg Nesterov

2011-09-05 16:21 UTC #41 lore raw

Re: [PATCH 6/6] freezer: kill unused set_freezable_with_signal()

Quoted 2 lines · Tejun Heo

Ooh, can I add your Acked-by before sending pull request to Rafael?

Yes, thanks, please feel free.

Oleg.

Oleg Nesterov

2011-09-05 16:24 UTC #42 lore raw

Re: [PATCH 6/6] freezer: kill unused set_freezable_with_signal()

Quoted 2 lines · Tejun Heo

Ooh, can I add your Acked-by before sending pull request to Rafael?

Yes, thanks, please feel free.

Oleg.

Oleg Nesterov

2011-09-05 16:20 UTC #43 lore raw

Re: [PATCH 6/6] freezer: kill unused set_freezable_with_signal()

Quoted 17 lines · Tejun Heo

Hello, Oleg.
Quoted 4 lines · Oleg Nesterov
Quoted 4 lines
kernel/freezer.c +0 -0
@@ -72,10 +72,6 @@ bool __refrigerator(bool check_kthr_stop)
 		schedule();
 	}
 
Quoted 8 lines
kernel/freezer.c bool __refrigerator(bool check_kthr_stop) · line 78
Quoted 4 lines
kernel/freezer.c +0 -4
-	spin_lock_irq(&current->sighand->siglock);
-	recalc_sigpending(); /* We sent fake signal, clean it up */
-	spin_unlock_irq(&current->sighand->siglock);
-
Why? This recalc_sigpending() makes sense imho. Otherwise the user-space tasks (almost) always return with TIF_SIGPENDING. May be we can do this under "if (PF_KTRHREAD)".

Quoted 1 line

PF_KTHREAD no longer gets TIF_SIGPENDING set, so...

Yes,

Quoted 8 lines

Quoted 5 lines
For example. Suppose the user-space task does wait_event_freezable()...
Hmm. OTOH, wait_event_freezable() looks wrong anyway... So probably this doesn't matter. ptrace_stop/get_signal_to_deliver doesn't need this, probably we do not care about the other callers.
Can you elaborate on it being wrong? Do you mean the possibility of leaking spurious TIF_SIGPENDING?

Oleg.

Oleg Nesterov

2011-09-05 16:23 UTC #44 lore raw

Re: [PATCH 6/6] freezer: kill unused set_freezable_with_signal()

Quoted 17 lines · Tejun Heo

Hello, Oleg.
Quoted 4 lines · Oleg Nesterov
Quoted 4 lines
kernel/freezer.c +0 -0
@@ -72,10 +72,6 @@ bool __refrigerator(bool check_kthr_stop)
 		schedule();
 	}
 
Quoted 8 lines
kernel/freezer.c bool __refrigerator(bool check_kthr_stop) · line 78
Quoted 4 lines
kernel/freezer.c +0 -4
-	spin_lock_irq(&current->sighand->siglock);
-	recalc_sigpending(); /* We sent fake signal, clean it up */
-	spin_unlock_irq(&current->sighand->siglock);
-
Why? This recalc_sigpending() makes sense imho. Otherwise the user-space tasks (almost) always return with TIF_SIGPENDING. May be we can do this under "if (PF_KTRHREAD)".

Quoted 1 line

PF_KTHREAD no longer gets TIF_SIGPENDING set, so...

Yes,

Quoted 8 lines

Quoted 5 lines
For example. Suppose the user-space task does wait_event_freezable()...
Hmm. OTOH, wait_event_freezable() looks wrong anyway... So probably this doesn't matter. ptrace_stop/get_signal_to_deliver doesn't need this, probably we do not care about the other callers.
Can you elaborate on it being wrong? Do you mean the possibility of leaking spurious TIF_SIGPENDING?

Oleg.

A message that is not in the archive (sent off-list or not yet received): [email protected]

Tejun Heo

2011-09-06 03:28 UTC #46 lore raw

Re: [PATCH 6/6] freezer: kill unused set_freezable_with_signal()

Hello,

Quoted 8 lines · Oleg Nesterov

Perhaps it is correct... Just I do not understand what it should do. I thought it is "wait_for_event && do_not_block_freezer". And at first glance the code looks as if it tries to do this. Say, in the "likely" case we restart wait_event_interruptible() after refrigerator().
But this looks racy. Suppose that freezing() is already false when try_to_freeze() or __refrigerator() is called. Say, cgroup_freezer does freeze_task() + __thaw_task(). Why it returns -ERESTARTSYS in this case?

It may return -ERESTARTSYS when not strictly necessary but given that it's supposed to trigger restart anyway I don't think it's actually broken (it doesn't break the contract of the wait). Another thing to note is that kthread freezing via cgroup is almost fundamentally broken. With the removal of freezable_with_signal, this shouldn't be an issue anymore although cgroup_freezer still needs to be fixed to account for kthreads for xstate transitions (it currently triggers BUG_ON).

Quoted 3 lines

And if it can be used by the userspace thread, then we should probably do recalc_sigpending() somewhere, otherwise wait_event_freezable() will always return -ERESTARTSYS after __refrigerator().

Thankfully, currently, all the few users are kthreads. I don't think it makes sense for userland tasks at all. Interruptible sleeps for userland tasks necessiate the ability to return to signal delivery path and restart or abort the current operation. There's no point in using wait_event_freezable() instead of wait_event_interruptible().

Thanks.

Signature · 1 line

tejun

Tejun Heo

author

2011-09-06 03:28 UTC #47 lore raw

Re: [PATCH 6/6] freezer: kill unused set_freezable_with_signal()

Hello,

Quoted 8 lines · Oleg Nesterov

Perhaps it is correct... Just I do not understand what it should do. I thought it is "wait_for_event && do_not_block_freezer". And at first glance the code looks as if it tries to do this. Say, in the "likely" case we restart wait_event_interruptible() after refrigerator().
But this looks racy. Suppose that freezing() is already false when try_to_freeze() or __refrigerator() is called. Say, cgroup_freezer does freeze_task() + __thaw_task(). Why it returns -ERESTARTSYS in this case?

Quoted 3 lines

And if it can be used by the userspace thread, then we should probably do recalc_sigpending() somewhere, otherwise wait_event_freezable() will always return -ERESTARTSYS after __refrigerator().

Thanks.

Signature · 1 line

tejun

Tejun Heo

author

2011-09-06 03:29 UTC #48 lore raw

Re: [PATCH 6/6] freezer: kill unused set_freezable_with_signal()

Hello,

Quoted 8 lines · Oleg Nesterov

Perhaps it is correct... Just I do not understand what it should do. I thought it is "wait_for_event && do_not_block_freezer". And at first glance the code looks as if it tries to do this. Say, in the "likely" case we restart wait_event_interruptible() after refrigerator().
But this looks racy. Suppose that freezing() is already false when try_to_freeze() or __refrigerator() is called. Say, cgroup_freezer does freeze_task() + __thaw_task(). Why it returns -ERESTARTSYS in this case?

Quoted 3 lines

And if it can be used by the userspace thread, then we should probably do recalc_sigpending() somewhere, otherwise wait_event_freezable() will always return -ERESTARTSYS after __refrigerator().

Thanks.

Signature · 1 line

tejun

A message that is not in the archive (sent off-list or not yet received): 20110906032846.GA18425-9pTldWuhBndy/[email protected]

Oleg Nesterov

2011-09-06 15:18 UTC #50 lore raw

Re: [PATCH 6/6] freezer: kill unused set_freezable_with_signal()

Hi,

Quoted 9 lines · Tejun Heo

Hello,
Quoted 5 lines · Oleg Nesterov
Perhaps it is correct... Just I do not understand what it should do. I thought it is "wait_for_event && do_not_block_freezer". And at first glance the code looks as if it tries to do this. Say, in the "likely" case we restart wait_event_interruptible() after refrigerator().

Quoted 7 lines

Quoted 3 lines
But this looks racy. Suppose that freezing() is already false when try_to_freeze() or __refrigerator() is called. Say, cgroup_freezer does freeze_task() + __thaw_task(). Why it returns -ERESTARTSYS in this case?
It may return -ERESTARTSYS when not strictly necessary but given that it's supposed to trigger restart anyway I don't think it's actually broken (it doesn't break the contract of the wait).

OK, but still this doesn't look right.

Quoted 3 lines

Another thing to
note is that kthread freezing via cgroup is almost fundamentally broken.

OK, let it be freeze_processes()+thaw_tasks(). Of course this is mostly theoretical.

Quoted 6 lines

Quoted 3 lines
And if it can be used by the userspace thread, then we should probably do recalc_sigpending() somewhere, otherwise wait_event_freezable() will always return -ERESTARTSYS after __refrigerator().
Thankfully, currently, all the few users are kthreads. I don't think it makes sense for userland tasks at all.

Yes, agreed. In this case I think it should be

	#define wait_event_freezable(wq, condition)				\
	({									\
		int __retval;							\
		for (;;) {							\
			__retval = wait_event_interruptible(wq, 		\
					(condition) || freezing(current));	\
			if (__retval || (condition))				\
				break;						\
			try_to_freeze();					\
		}								\
		__retval;							\
	})

__retval/ERESTARTSYS is only needed for kthreads which play with allow_signal(), probably nobody should do this.

Oleg.

Oleg Nesterov

2011-09-06 15:18 UTC #51 lore raw

Re: [PATCH 6/6] freezer: kill unused set_freezable_with_signal()

Hi,

Quoted 9 lines · Tejun Heo

Hello,
Quoted 5 lines · Oleg Nesterov
Perhaps it is correct... Just I do not understand what it should do. I thought it is "wait_for_event && do_not_block_freezer". And at first glance the code looks as if it tries to do this. Say, in the "likely" case we restart wait_event_interruptible() after refrigerator().

Quoted 7 lines

Quoted 3 lines
But this looks racy. Suppose that freezing() is already false when try_to_freeze() or __refrigerator() is called. Say, cgroup_freezer does freeze_task() + __thaw_task(). Why it returns -ERESTARTSYS in this case?
It may return -ERESTARTSYS when not strictly necessary but given that it's supposed to trigger restart anyway I don't think it's actually broken (it doesn't break the contract of the wait).

OK, but still this doesn't look right.

Quoted 3 lines

Another thing to
note is that kthread freezing via cgroup is almost fundamentally broken.

OK, let it be freeze_processes()+thaw_tasks(). Of course this is mostly theoretical.

Quoted 6 lines

Quoted 3 lines
And if it can be used by the userspace thread, then we should probably do recalc_sigpending() somewhere, otherwise wait_event_freezable() will always return -ERESTARTSYS after __refrigerator().
Thankfully, currently, all the few users are kthreads. I don't think it makes sense for userland tasks at all.

Yes, agreed. In this case I think it should be

	#define wait_event_freezable(wq, condition)				\
	({									\
		int __retval;							\
		for (;;) {							\
			__retval = wait_event_interruptible(wq, 		\
					(condition) || freezing(current));	\
			if (__retval || (condition))				\
				break;						\
			try_to_freeze();					\
		}								\
		__retval;							\
	})

__retval/ERESTARTSYS is only needed for kthreads which play with allow_signal(), probably nobody should do this.

Oleg.

Oleg Nesterov

2011-09-06 15:22 UTC #52 lore raw

Re: [PATCH 6/6] freezer: kill unused set_freezable_with_signal()

Hi,

Quoted 9 lines · Tejun Heo

Hello,
Quoted 5 lines · Oleg Nesterov
Perhaps it is correct... Just I do not understand what it should do. I thought it is "wait_for_event && do_not_block_freezer". And at first glance the code looks as if it tries to do this. Say, in the "likely" case we restart wait_event_interruptible() after refrigerator().

Quoted 7 lines

Quoted 3 lines
But this looks racy. Suppose that freezing() is already false when try_to_freeze() or __refrigerator() is called. Say, cgroup_freezer does freeze_task() + __thaw_task(). Why it returns -ERESTARTSYS in this case?
It may return -ERESTARTSYS when not strictly necessary but given that it's supposed to trigger restart anyway I don't think it's actually broken (it doesn't break the contract of the wait).

OK, but still this doesn't look right.

Quoted 3 lines

Another thing to
note is that kthread freezing via cgroup is almost fundamentally broken.

OK, let it be freeze_processes()+thaw_tasks(). Of course this is mostly theoretical.

Quoted 6 lines

Quoted 3 lines
And if it can be used by the userspace thread, then we should probably do recalc_sigpending() somewhere, otherwise wait_event_freezable() will always return -ERESTARTSYS after __refrigerator().
Thankfully, currently, all the few users are kthreads. I don't think it makes sense for userland tasks at all.

Yes, agreed. In this case I think it should be

	#define wait_event_freezable(wq, condition)				\
	({									\
		int __retval;							\
		for (;;) {							\
			__retval = wait_event_interruptible(wq, 		\
					(condition) || freezing(current));	\
			if (__retval || (condition))				\
				break;						\
			try_to_freeze();					\
		}								\
		__retval;							\
	})

__retval/ERESTARTSYS is only needed for kthreads which play with allow_signal(), probably nobody should do this.

Oleg.

A message that is not in the archive (sent off-list or not yet received): [email protected]

Oleg Nesterov

2011-09-06 15:25 UTC #54 lore raw

Re: [PATCH 6/6] freezer: kill unused set_freezable_with_signal()

Quoted 16 lines · Oleg Nesterov

Yes, agreed. In this case I think it should be

	#define wait_event_freezable(wq, condition)				\
	({									\
		int __retval;							\
		for (;;) {							\
			__retval = wait_event_interruptible(wq, 		\
					(condition) || freezing(current));	\
			if (__retval || (condition))				\
				break;						\
			try_to_freeze();					\
		}								\
		__retval;							\
	})

Quoted 2 lines

__retval/ERESTARTSYS is only needed for kthreads which play with allow_signal(), probably nobody should do this.

I meant, unless the caller plays with allow_signal(), it has all rights to do

	if (wait_event_freezable(...))
		BUG();

This becomes correct with the code above.

Oleg.

Oleg Nesterov

2011-09-06 15:25 UTC #55 lore raw

Re: [PATCH 6/6] freezer: kill unused set_freezable_with_signal()

Quoted 16 lines · Oleg Nesterov

Yes, agreed. In this case I think it should be

	#define wait_event_freezable(wq, condition)				\
	({									\
		int __retval;							\
		for (;;) {							\
			__retval = wait_event_interruptible(wq, 		\
					(condition) || freezing(current));	\
			if (__retval || (condition))				\
				break;						\
			try_to_freeze();					\
		}								\
		__retval;							\
	})

Quoted 2 lines

__retval/ERESTARTSYS is only needed for kthreads which play with allow_signal(), probably nobody should do this.

I meant, unless the caller plays with allow_signal(), it has all rights to do

	if (wait_event_freezable(...))
		BUG();

This becomes correct with the code above.

Oleg.

Oleg Nesterov

2011-09-06 15:29 UTC #56 lore raw

Re: [PATCH 6/6] freezer: kill unused set_freezable_with_signal()

Quoted 16 lines · Oleg Nesterov

Yes, agreed. In this case I think it should be

	#define wait_event_freezable(wq, condition)				\
	({									\
		int __retval;							\
		for (;;) {							\
			__retval = wait_event_interruptible(wq, 		\
					(condition) || freezing(current));	\
			if (__retval || (condition))				\
				break;						\
			try_to_freeze();					\
		}								\
		__retval;							\
	})

Quoted 2 lines

__retval/ERESTARTSYS is only needed for kthreads which play with allow_signal(), probably nobody should do this.

I meant, unless the caller plays with allow_signal(), it has all rights to do

	if (wait_event_freezable(...))
		BUG();

This becomes correct with the code above.

Oleg.

A message that is not in the archive (sent off-list or not yet received): [email protected]

Tejun Heo

2011-09-06 15:53 UTC #58 lore raw

Re: [PATCH 6/6] freezer: kill unused set_freezable_with_signal()

Hello,

Quoted 20 lines · Oleg Nesterov

Quoted 16 lines · Oleg Nesterov
Yes, agreed. In this case I think it should be
	#define wait_event_freezable(wq, condition)				\
	({									\
		int __retval;							\
		for (;;) {							\
			__retval = wait_event_interruptible(wq, 		\
					(condition) || freezing(current));	\
			if (__retval || (condition))				\
				break;						\
			try_to_freeze();					\
		}								\
		__retval;							\
	})
Quoted 2 lines
__retval/ERESTARTSYS is only needed for kthreads which play with allow_signal(), probably nobody should do this.

Quoted 6 lines

I meant, unless the caller plays with allow_signal(), it has all rights to do
	if (wait_event_freezable(...))
		BUG();
This becomes correct with the code above.

Yeap, sure, w/ freezable_with_signal gone, the above should work fine. Care to create a patch?

Thanks.

Signature · 1 line

tejun

Tejun Heo

author

2011-09-06 15:53 UTC #59 lore raw

Re: [PATCH 6/6] freezer: kill unused set_freezable_with_signal()

Hello,

Quoted 20 lines · Oleg Nesterov

Quoted 16 lines · Oleg Nesterov
Yes, agreed. In this case I think it should be
	#define wait_event_freezable(wq, condition)				\
	({									\
		int __retval;							\
		for (;;) {							\
			__retval = wait_event_interruptible(wq, 		\
					(condition) || freezing(current));	\
			if (__retval || (condition))				\
				break;						\
			try_to_freeze();					\
		}								\
		__retval;							\
	})
Quoted 2 lines
__retval/ERESTARTSYS is only needed for kthreads which play with allow_signal(), probably nobody should do this.

Quoted 6 lines

I meant, unless the caller plays with allow_signal(), it has all rights to do
	if (wait_event_freezable(...))
		BUG();
This becomes correct with the code above.

Yeap, sure, w/ freezable_with_signal gone, the above should work fine. Care to create a patch?

Thanks.

Signature · 1 line

tejun

Tejun Heo

author

2011-09-06 15:53 UTC #60 lore raw

Re: [PATCH 6/6] freezer: kill unused set_freezable_with_signal()

Hello,

Quoted 20 lines · Oleg Nesterov

Quoted 16 lines · Oleg Nesterov
Yes, agreed. In this case I think it should be
	#define wait_event_freezable(wq, condition)				\
	({									\
		int __retval;							\
		for (;;) {							\
			__retval = wait_event_interruptible(wq, 		\
					(condition) || freezing(current));	\
			if (__retval || (condition))				\
				break;						\
			try_to_freeze();					\
		}								\
		__retval;							\
	})
Quoted 2 lines
__retval/ERESTARTSYS is only needed for kthreads which play with allow_signal(), probably nobody should do this.

Quoted 6 lines

I meant, unless the caller plays with allow_signal(), it has all rights to do
	if (wait_event_freezable(...))
		BUG();
This becomes correct with the code above.

Yeap, sure, w/ freezable_with_signal gone, the above should work fine. Care to create a patch?

Thanks.

Signature · 1 line

tejun

A message that is not in the archive (sent off-list or not yet received): 20110906155332.GF18425-9pTldWuhBndy/[email protected]

Oleg Nesterov

2011-09-07 18:21 UTC #62 lore raw

cover freezer: fix wait_event_freezable/__thaw_task races

Hi,

Quoted 10 lines · Tejun Heo

Quoted 7 lines
I meant, unless the caller plays with allow_signal(), it has all rights to do
	if (wait_event_freezable(...))
		BUG();
This becomes correct with the code above.
Yeap, sure, w/ freezable_with_signal gone, the above should work fine. Care to create a patch?

Sure.

But. Tejun, Rafael, I have to rely on your review. I have no idea how can I test this change. Hopfully it is simple enough, though.

And. wait_event_freezable_timeout() obviously has another problem, although I hope this is fine. The caller can spend a lot of time in refrigerator, shouldn't we update "timeout" in this case? I hope we shouldn't.

Oleg.

Oleg Nesterov

2011-09-07 18:21 UTC #63 lore raw

cover freezer: fix wait_event_freezable/__thaw_task races

Hi,

Quoted 10 lines · Tejun Heo

Quoted 7 lines
I meant, unless the caller plays with allow_signal(), it has all rights to do
	if (wait_event_freezable(...))
		BUG();
This becomes correct with the code above.
Yeap, sure, w/ freezable_with_signal gone, the above should work fine. Care to create a patch?

Sure.

But. Tejun, Rafael, I have to rely on your review. I have no idea how can I test this change. Hopfully it is simple enough, though.

Oleg.

Oleg Nesterov

2011-09-07 18:26 UTC #64 lore raw

cover freezer: fix wait_event_freezable/__thaw_task races

Hi,

Quoted 10 lines · Tejun Heo

Quoted 7 lines
I meant, unless the caller plays with allow_signal(), it has all rights to do
	if (wait_event_freezable(...))
		BUG();
This becomes correct with the code above.
Yeap, sure, w/ freezable_with_signal gone, the above should work fine. Care to create a patch?

Sure.

But. Tejun, Rafael, I have to rely on your review. I have no idea how can I test this change. Hopfully it is simple enough, though.

Oleg.

A message that is not in the archive (sent off-list or not yet received): [email protected]

Oleg Nesterov

2011-09-07 18:22 UTC #66 lore raw

PATCH 1/1 freezer: fix wait_event_freezable/__thaw_task races

wait_event_freezable() and wait_event_freezable_timeout() stop the waiting if try_to_freeze() fails. This is not right, we can race with __thaw_task() and in this case

- wait_event_freezable() returns the wrong ERESTARTSYS

- wait_event_freezable_timeout() can return the positive value while condition == F

Change the code to always check __retval/condition before return.

Note: with or without this patch the timeout logic looks strange, probably we should recalc timeout if try_to_freeze() returns T.

Signed-off-by Oleg Nesterov <[email protected]>

---

 include/linux/freezer.h |   19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

include/linux/freezer.h renamed from include/linux/freezer.h~w_e_f +10 -9

@@ -107,32 +107,33 @@ static inline int freezer_should_skip(st
  * Freezer-friendly wrappers around wait_event_interruptible() and
  * wait_event_interruptible_timeout(), originally defined in <linux/wait.h>
  */
-
 #define wait_event_freezable(wq, condition)				\
 ({									\
 	int __retval;							\
-	do {								\
+	for (;;) {							\
 		__retval = wait_event_interruptible(wq, 		\
 				(condition) || freezing(current));	\
-		if (__retval && !freezing(current))			\
+		if (__retval || (condition))				\
 			break;						\
-		else if (!(condition))					\
-			__retval = -ERESTARTSYS;			\
-	} while (try_to_freeze());					\
+		try_to_freeze();					\
+	}								\
 	__retval;							\
 })
 
-
 #define wait_event_freezable_timeout(wq, condition, timeout)		\
 ({									\
 	long __retval = timeout;					\
-	do {								\
+	for (;;) {							\
 		__retval = wait_event_interruptible_timeout(wq,		\
 				(condition) || freezing(current),	\
 				__retval); 				\
-	} while (try_to_freeze());					\
+		if (__retval <= 0 || (condition))			\
+			break;						\
+		try_to_freeze();					\
+	}								\
 	__retval;							\
 })
+
 #else /* !CONFIG_FREEZER */
 static inline bool frozen(struct task_struct *p) { return false; }
 static inline bool freezing(struct task_struct *p) { return false; }

Oleg Nesterov

2011-09-07 18:22 UTC #67 lore raw

PATCH 1/1 freezer: fix wait_event_freezable/__thaw_task races

wait_event_freezable() and wait_event_freezable_timeout() stop the waiting if try_to_freeze() fails. This is not right, we can race with __thaw_task() and in this case

- wait_event_freezable() returns the wrong ERESTARTSYS

- wait_event_freezable_timeout() can return the positive value while condition == F

Change the code to always check __retval/condition before return.

Note: with or without this patch the timeout logic looks strange, probably we should recalc timeout if try_to_freeze() returns T.

Signed-off-by Oleg Nesterov <[email protected]>

---

 include/linux/freezer.h |   19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

include/linux/freezer.h renamed from include/linux/freezer.h~w_e_f +10 -9

@@ -107,32 +107,33 @@ static inline int freezer_should_skip(st
  * Freezer-friendly wrappers around wait_event_interruptible() and
  * wait_event_interruptible_timeout(), originally defined in <linux/wait.h>
  */
-
 #define wait_event_freezable(wq, condition)				\
 ({									\
 	int __retval;							\
-	do {								\
+	for (;;) {							\
 		__retval = wait_event_interruptible(wq, 		\
 				(condition) || freezing(current));	\
-		if (__retval && !freezing(current))			\
+		if (__retval || (condition))				\
 			break;						\
-		else if (!(condition))					\
-			__retval = -ERESTARTSYS;			\
-	} while (try_to_freeze());					\
+		try_to_freeze();					\
+	}								\
 	__retval;							\
 })
 
-
 #define wait_event_freezable_timeout(wq, condition, timeout)		\
 ({									\
 	long __retval = timeout;					\
-	do {								\
+	for (;;) {							\
 		__retval = wait_event_interruptible_timeout(wq,		\
 				(condition) || freezing(current),	\
 				__retval); 				\
-	} while (try_to_freeze());					\
+		if (__retval <= 0 || (condition))			\
+			break;						\
+		try_to_freeze();					\
+	}								\
 	__retval;							\
 })
+
 #else /* !CONFIG_FREEZER */
 static inline bool frozen(struct task_struct *p) { return false; }
 static inline bool freezing(struct task_struct *p) { return false; }

Oleg Nesterov

2011-09-07 18:27 UTC #68 lore raw

PATCH 1/1 freezer: fix wait_event_freezable/__thaw_task races

wait_event_freezable() and wait_event_freezable_timeout() stop the waiting if try_to_freeze() fails. This is not right, we can race with __thaw_task() and in this case

- wait_event_freezable() returns the wrong ERESTARTSYS

- wait_event_freezable_timeout() can return the positive value while condition == F

Change the code to always check __retval/condition before return.

Note: with or without this patch the timeout logic looks strange, probably we should recalc timeout if try_to_freeze() returns T.

Signed-off-by Oleg Nesterov <[email protected]>

---

 include/linux/freezer.h |   19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

include/linux/freezer.h renamed from include/linux/freezer.h~w_e_f +10 -9

@@ -107,32 +107,33 @@ static inline int freezer_should_skip(st
  * Freezer-friendly wrappers around wait_event_interruptible() and
  * wait_event_interruptible_timeout(), originally defined in <linux/wait.h>
  */
-
 #define wait_event_freezable(wq, condition)				\
 ({									\
 	int __retval;							\
-	do {								\
+	for (;;) {							\
 		__retval = wait_event_interruptible(wq, 		\
 				(condition) || freezing(current));	\
-		if (__retval && !freezing(current))			\
+		if (__retval || (condition))				\
 			break;						\
-		else if (!(condition))					\
-			__retval = -ERESTARTSYS;			\
-	} while (try_to_freeze());					\
+		try_to_freeze();					\
+	}								\
 	__retval;							\
 })
 
-
 #define wait_event_freezable_timeout(wq, condition, timeout)		\
 ({									\
 	long __retval = timeout;					\
-	do {								\
+	for (;;) {							\
 		__retval = wait_event_interruptible_timeout(wq,		\
 				(condition) || freezing(current),	\
 				__retval); 				\
-	} while (try_to_freeze());					\
+		if (__retval <= 0 || (condition))			\
+			break;						\
+		try_to_freeze();					\
+	}								\
 	__retval;							\
 })
+
 #else /* !CONFIG_FREEZER */
 static inline bool frozen(struct task_struct *p) { return false; }
 static inline bool freezing(struct task_struct *p) { return false; }

Tejun Heo

author

2011-09-08 01:05 UTC #69 lore raw

Re: [PATCH 1/1] freezer: fix wait_event_freezable/__thaw_task races

Hello,

Quoted 8 lines · Oleg Nesterov

wait_event_freezable() and wait_event_freezable_timeout() stop the waiting if try_to_freeze() fails. This is not right, we can race with __thaw_task() and in this case
- wait_event_freezable() returns the wrong ERESTARTSYS
- wait_event_freezable_timeout() can return the positive value while condition == F

Indeed, nice catch. This one actually is pretty dangerous. We probably want to make a separate fix for this and backport it to -stable?

Quoted 6 lines

Change the code to always check __retval/condition before return.
Note: with or without this patch the timeout logic looks strange, probably we should recalc timeout if try_to_freeze() returns T.
Signed-off-by Oleg Nesterov <[email protected]>

Yeap, with freezable_with_signal gone, this looks correct & simpler to me but it would be nice if you can sprinkle some documentation while at it. :)

Thanks.

Signature · 1 line

tejun

Oleg Nesterov

2011-09-08 17:59 UTC #70 lore raw

Re: [PATCH 1/1] freezer: fix wait_event_freezable/__thaw_task races

Hi,

Quoted 8 lines · Tejun Heo

Hello,
Quoted 4 lines · Oleg Nesterov
wait_event_freezable() and wait_event_freezable_timeout() stop the waiting if try_to_freeze() fails. This is not right, we can race with __thaw_task() and in this case

Quoted 8 lines

Quoted 4 lines
- wait_event_freezable() returns the wrong ERESTARTSYS
- wait_event_freezable_timeout() can return the positive value while condition == F
Indeed, nice catch. This one actually is pretty dangerous. We probably want to make a separate fix for this and backport it to -stable?

And I forgot to mention that wait_event_freezable_timeout() doesn't handle -ERESTARTSYS at all.

But I don't think -stable needs this fix. According to grep, nobody check the returned value, and none of the callers plays with signals.

Quoted 9 lines

Quoted 6 lines
Change the code to always check __retval/condition before return.
Note: with or without this patch the timeout logic looks strange, probably we should recalc timeout if try_to_freeze() returns T.
Signed-off-by Oleg Nesterov <[email protected]>
Yeap, with freezable_with_signal gone, this looks correct & simpler to me

I don't really understand this... set_freezable_with_signal() has a lot of problems, yes... But even if it wasn't removed this fix makes sense anyway, afaics.

If freezable_with_signal caller does wait_event_freezable_timeout(), __retval becomes -ERESTARTSYS after freeze_task(). The next iteration will return 0 with the KERN_ERR message from schedule_timeout().

Quoted 2 lines

but it would be nice if you can sprinkle some documentation while at it. :)

But they already have the comment ;) What can I add?

Oleg.

A message that is not in the archive (sent off-list or not yet received): [email protected]

Tejun Heo

2011-09-11 01:54 UTC #72 lore raw

Re: [PATCH 1/1] freezer: fix wait_event_freezable/__thaw_task races

Hello,

Quoted 9 lines · Oleg Nesterov

Quoted 3 lines
Indeed, nice catch. This one actually is pretty dangerous. We probably want to make a separate fix for this and backport it to -stable?
And I forgot to mention that wait_event_freezable_timeout() doesn't handle -ERESTARTSYS at all.
But I don't think -stable needs this fix. According to grep, nobody check the returned value, and none of the callers plays with signals.

Ah, no user, okay.

Quoted 10 lines

Quoted 2 lines
Yeap, with freezable_with_signal gone, this looks correct & simpler to me
I don't really understand this... set_freezable_with_signal() has a lot of problems, yes... But even if it wasn't removed this fix makes sense anyway, afaics.
If freezable_with_signal caller does wait_event_freezable_timeout(), __retval becomes -ERESTARTSYS after freeze_task(). The next iteration will return 0 with the KERN_ERR message from schedule_timeout().

Hmmm... but with the change, if a kthread gets TIF_SIGPENDING from freezer and then thawed, it would not enter try_to_freeze() and thus won't clear TIF_SIGPENDING. The original code was racy too but the window would be much larger afterwards. Anyways, this doesn't matter anymore.

Quoted 4 lines

Quoted 2 lines
but it would be nice if you can sprinkle some documentation while at it. :)
But they already have the comment ;) What can I add?

Proper /** - */ comment w/ return value documentation? :P

Thanks.

Signature · 1 line

tejun

Tejun Heo

author

2011-09-11 01:54 UTC #73 lore raw

Re: [PATCH 1/1] freezer: fix wait_event_freezable/__thaw_task races

Hello,

Quoted 9 lines · Oleg Nesterov

Quoted 3 lines
Indeed, nice catch. This one actually is pretty dangerous. We probably want to make a separate fix for this and backport it to -stable?
And I forgot to mention that wait_event_freezable_timeout() doesn't handle -ERESTARTSYS at all.
But I don't think -stable needs this fix. According to grep, nobody check the returned value, and none of the callers plays with signals.

Ah, no user, okay.

Quoted 10 lines

Quoted 2 lines
Yeap, with freezable_with_signal gone, this looks correct & simpler to me
I don't really understand this... set_freezable_with_signal() has a lot of problems, yes... But even if it wasn't removed this fix makes sense anyway, afaics.
If freezable_with_signal caller does wait_event_freezable_timeout(), __retval becomes -ERESTARTSYS after freeze_task(). The next iteration will return 0 with the KERN_ERR message from schedule_timeout().

Quoted 4 lines

Quoted 2 lines
but it would be nice if you can sprinkle some documentation while at it. :)
But they already have the comment ;) What can I add?

Proper /** - */ comment w/ return value documentation? :P

Thanks.

Signature · 1 line

tejun

Tejun Heo

author

2011-09-11 01:54 UTC #74 lore raw

Re: [PATCH 1/1] freezer: fix wait_event_freezable/__thaw_task races

Hello,

Quoted 9 lines · Oleg Nesterov

Quoted 3 lines
Indeed, nice catch. This one actually is pretty dangerous. We probably want to make a separate fix for this and backport it to -stable?
And I forgot to mention that wait_event_freezable_timeout() doesn't handle -ERESTARTSYS at all.
But I don't think -stable needs this fix. According to grep, nobody check the returned value, and none of the callers plays with signals.

Ah, no user, okay.

Quoted 10 lines

Quoted 2 lines
Yeap, with freezable_with_signal gone, this looks correct & simpler to me
I don't really understand this... set_freezable_with_signal() has a lot of problems, yes... But even if it wasn't removed this fix makes sense anyway, afaics.
If freezable_with_signal caller does wait_event_freezable_timeout(), __retval becomes -ERESTARTSYS after freeze_task(). The next iteration will return 0 with the KERN_ERR message from schedule_timeout().

Quoted 4 lines

Quoted 2 lines
but it would be nice if you can sprinkle some documentation while at it. :)
But they already have the comment ;) What can I add?

Proper /** - */ comment w/ return value documentation? :P

Thanks.

Signature · 1 line

tejun

Oleg Nesterov

2011-09-11 18:33 UTC #75 lore raw

Re: [PATCH 1/1] freezer: fix wait_event_freezable/__thaw_task races

Hello,

Quoted 10 lines · Tejun Heo

Quoted 7 lines · Oleg Nesterov
Quoted 2 lines
Yeap, with freezable_with_signal gone, this looks correct & simpler to me
I don't really understand this... set_freezable_with_signal() has a lot of problems, yes... But even if it wasn't removed this fix makes sense anyway, afaics.

Quoted 7 lines

Quoted 3 lines
If freezable_with_signal caller does wait_event_freezable_timeout(), __retval becomes -ERESTARTSYS after freeze_task(). The next iteration will return 0 with the KERN_ERR message from schedule_timeout().
Hmmm... but with the change, if a kthread gets TIF_SIGPENDING from freezer and then thawed, it would not enter try_to_freeze() and thus won't clear TIF_SIGPENDING.

Ah yes, you are right, thanks. But once again, please note that in this case wait_event_freezable_timeout() is broken anyway.

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
but it would be nice if you can sprinkle some documentation while at it. :)
But they already have the comment ;) What can I add?
Proper /** - */ comment w/ return value documentation? :P

I already noticed a long ago you like the comments too much ;)

OK, will do.

Oleg.

Oleg Nesterov

2011-09-11 18:44 UTC #76 lore raw

Re: [PATCH 1/1] freezer: fix wait_event_freezable/__thaw_task races

Quoted 16 lines · Oleg Nesterov

Quoted 14 lines · Tejun Heo
Quoted 10 lines · Oleg Nesterov
Quoted 2 lines
Yeap, with freezable_with_signal gone, this looks correct & simpler to me
I don't really understand this... set_freezable_with_signal() has a lot of problems, yes... But even if it wasn't removed this fix makes sense anyway, afaics.
If freezable_with_signal caller does wait_event_freezable_timeout(), __retval becomes -ERESTARTSYS after freeze_task(). The next iteration will return 0 with the KERN_ERR message from schedule_timeout().

Quoted 6 lines

Quoted 3 lines
Hmmm... but with the change, if a kthread gets TIF_SIGPENDING from freezer and then thawed, it would not enter try_to_freeze() and thus won't clear TIF_SIGPENDING.
Ah yes, you are right, thanks. But once again, please note that in this case wait_event_freezable_timeout() is broken anyway.

OTOH, I forgot about wait_event_freezable(). It was racy, but mostly worked. So yes, you are right, without "kill freezable_with_signal" this patch is wrong.

I missed this, thanks.

Oleg.

Oleg Nesterov

2011-09-08 17:59 UTC #77 lore raw

Re: [PATCH 1/1] freezer: fix wait_event_freezable/__thaw_task races

Hi,

Quoted 8 lines · Tejun Heo

Hello,
Quoted 4 lines · Oleg Nesterov
wait_event_freezable() and wait_event_freezable_timeout() stop the waiting if try_to_freeze() fails. This is not right, we can race with __thaw_task() and in this case

Quoted 8 lines

Quoted 4 lines
- wait_event_freezable() returns the wrong ERESTARTSYS
- wait_event_freezable_timeout() can return the positive value while condition == F
Indeed, nice catch. This one actually is pretty dangerous. We probably want to make a separate fix for this and backport it to -stable?

And I forgot to mention that wait_event_freezable_timeout() doesn't handle -ERESTARTSYS at all.

But I don't think -stable needs this fix. According to grep, nobody check the returned value, and none of the callers plays with signals.

Quoted 9 lines

Quoted 6 lines
Change the code to always check __retval/condition before return.
Note: with or without this patch the timeout logic looks strange, probably we should recalc timeout if try_to_freeze() returns T.
Signed-off-by Oleg Nesterov <[email protected]>
Yeap, with freezable_with_signal gone, this looks correct & simpler to me

I don't really understand this... set_freezable_with_signal() has a lot of problems, yes... But even if it wasn't removed this fix makes sense anyway, afaics.

Quoted 2 lines

but it would be nice if you can sprinkle some documentation while at it. :)

But they already have the comment ;) What can I add?

Oleg.

Tejun Heo

author

2011-09-08 01:05 UTC #78 lore raw

Re: [PATCH 1/1] freezer: fix wait_event_freezable/__thaw_task races

Hello,

Quoted 8 lines · Oleg Nesterov

wait_event_freezable() and wait_event_freezable_timeout() stop the waiting if try_to_freeze() fails. This is not right, we can race with __thaw_task() and in this case
- wait_event_freezable() returns the wrong ERESTARTSYS
- wait_event_freezable_timeout() can return the positive value while condition == F

Indeed, nice catch. This one actually is pretty dangerous. We probably want to make a separate fix for this and backport it to -stable?

Quoted 6 lines

Change the code to always check __retval/condition before return.
Note: with or without this patch the timeout logic looks strange, probably we should recalc timeout if try_to_freeze() returns T.
Signed-off-by Oleg Nesterov <[email protected]>

Yeap, with freezable_with_signal gone, this looks correct & simpler to me but it would be nice if you can sprinkle some documentation while at it. :)

Thanks.

Signature · 1 line

tejun

Matt Helsley

2011-09-08 18:01 UTC #79 lore raw

Re: [PATCH 6/6] freezer: kill unused set_freezable_with_signal()

Quoted 12 lines · Tejun Heo

Hello,
Quoted 8 lines · Oleg Nesterov
Perhaps it is correct... Just I do not understand what it should do. I thought it is "wait_for_event && do_not_block_freezer". And at first glance the code looks as if it tries to do this. Say, in the "likely" case we restart wait_event_interruptible() after refrigerator().
But this looks racy. Suppose that freezing() is already false when try_to_freeze() or __refrigerator() is called. Say, cgroup_freezer does freeze_task() + __thaw_task(). Why it returns -ERESTARTSYS in this case?

Quoted 8 lines

It may return -ERESTARTSYS when not strictly necessary but given that it's supposed to trigger restart anyway I don't think it's actually broken (it doesn't break the contract of the wait). Another thing to note is that kthread freezing via cgroup is almost fundamentally broken. With the removal of freezable_with_signal, this shouldn't be an issue anymore although cgroup_freezer still needs to be fixed to account for kthreads for xstate transitions (it currently triggers BUG_ON).

Looking at the code and docs I realize I didn't explicitly mention that kthreads could not be frozen by the cgroup freezer. However, the code did not allow it. When freezing tasks the cgroup freezer always did:

freeze_task(task, true)

which would only freeze tasks without PF_FREEZER_NOSIG due to the second "sig_only" parameter. I believe this means it could not be used to freeze kthreads.

My feeling is kthreads should not be "managed" directly by userspace. Especially if that includes the ability to arbitrarily stop or freeze them. So it seems more appropriate to explicitly disallow freezing of kthreads via the cgroup freezer.

Cheers,
-Matt Helsley

A message that is not in the archive (sent off-list or not yet received): 20110908180159.GA4197-52DBMbEzqgQ/wnmkkaCWp/[email protected]

Tejun Heo

2011-09-11 01:37 UTC #81 lore raw

Re: [PATCH 6/6] freezer: kill unused set_freezable_with_signal()

Hello, Matt.

Quoted 6 lines · Matt Helsley

Looking at the code and docs I realize I didn't explicitly mention that kthreads could not be frozen by the cgroup freezer. However, the code did not allow it. When freezing tasks the cgroup freezer always did:
freeze_task(task, true)

Quoted 8 lines

which would only freeze tasks without PF_FREEZER_NOSIG due to the second "sig_only" parameter. I believe this means it could not be used to freeze kthreads.
My feeling is kthreads should not be "managed" directly by userspace. Especially if that includes the ability to arbitrarily stop or freeze them. So it seems more appropriate to explicitly disallow freezing of kthreads via the cgroup freezer.

Oh yeah, that's what we should do and maybe that's what the current code intends to achieve too but currently it ends up triggering BUG_ON().

Thanks.

Signature · 1 line

tejun

Tejun Heo

author

2011-09-11 01:37 UTC #82 lore raw

Re: [PATCH 6/6] freezer: kill unused set_freezable_with_signal()

In reply to Matt Helsley

Hello, Matt.

Quoted 6 lines · Matt Helsley

Looking at the code and docs I realize I didn't explicitly mention that kthreads could not be frozen by the cgroup freezer. However, the code did not allow it. When freezing tasks the cgroup freezer always did:
freeze_task(task, true)

Quoted 8 lines

which would only freeze tasks without PF_FREEZER_NOSIG due to the second "sig_only" parameter. I believe this means it could not be used to freeze kthreads.
My feeling is kthreads should not be "managed" directly by userspace. Especially if that includes the ability to arbitrarily stop or freeze them. So it seems more appropriate to explicitly disallow freezing of kthreads via the cgroup freezer.

Oh yeah, that's what we should do and maybe that's what the current code intends to achieve too but currently it ends up triggering BUG_ON().

Thanks.

Signature · 1 line

tejun

Tejun Heo

author

2011-09-11 01:37 UTC #83 lore raw

Re: [PATCH 6/6] freezer: kill unused set_freezable_with_signal()

In reply to Matt Helsley

Hello, Matt.

Quoted 6 lines · Matt Helsley

Looking at the code and docs I realize I didn't explicitly mention that kthreads could not be frozen by the cgroup freezer. However, the code did not allow it. When freezing tasks the cgroup freezer always did:
freeze_task(task, true)

Quoted 8 lines

which would only freeze tasks without PF_FREEZER_NOSIG due to the second "sig_only" parameter. I believe this means it could not be used to freeze kthreads.
My feeling is kthreads should not be "managed" directly by userspace. Especially if that includes the ability to arbitrarily stop or freeze them. So it seems more appropriate to explicitly disallow freezing of kthreads via the cgroup freezer.

Oh yeah, that's what we should do and maybe that's what the current code intends to achieve too but currently it ends up triggering BUG_ON().

Thanks.

Signature · 1 line

tejun

Matt Helsley

2011-09-08 18:01 UTC #84 lore raw

Re: [PATCH 6/6] freezer: kill unused set_freezable_with_signal()

Quoted 12 lines · Tejun Heo

Hello,
Quoted 8 lines · Oleg Nesterov
Perhaps it is correct... Just I do not understand what it should do. I thought it is "wait_for_event && do_not_block_freezer". And at first glance the code looks as if it tries to do this. Say, in the "likely" case we restart wait_event_interruptible() after refrigerator().
But this looks racy. Suppose that freezing() is already false when try_to_freeze() or __refrigerator() is called. Say, cgroup_freezer does freeze_task() + __thaw_task(). Why it returns -ERESTARTSYS in this case?

Quoted 8 lines

It may return -ERESTARTSYS when not strictly necessary but given that it's supposed to trigger restart anyway I don't think it's actually broken (it doesn't break the contract of the wait). Another thing to note is that kthread freezing via cgroup is almost fundamentally broken. With the removal of freezable_with_signal, this shouldn't be an issue anymore although cgroup_freezer still needs to be fixed to account for kthreads for xstate transitions (it currently triggers BUG_ON).

freeze_task(task, true)

which would only freeze tasks without PF_FREEZER_NOSIG due to the second "sig_only" parameter. I believe this means it could not be used to freeze kthreads.

Cheers,
-Matt Helsley

Oleg Nesterov

2011-09-04 18:48 UTC #85 lore raw

Quoted 8 lines · Tejun Heo

These six patches are fixes and simplifications for the recent freezer changes. The first four have been posted twice. Changes since the last posting[L] are
* freezer->state setting bug fix updated per Matt Helsley so that the actual per-task freeze/thaw operations are always performed.

Quoted 7 lines

* fixed a bug caused by forgetting to unlock freezer_lock in "freezer:
restructure __refrigerator()".
* Two patches added. The fifth one is mostly trivial. The sixth a bit more involved but still shouldn't cause any noticeable functional difference. This removes the buggy and unused freezable_with_signal.

Afaics, everything is fine.

Oleg.

Oleg Nesterov

2011-09-04 18:48 UTC #86 lore raw