Re: [RFC][PATCH] freezer,sched: Rewrite core freezer logic

On Thu, Jun 03, 2021 at 12:35:22PM +0200, Peter Zijlstra wrote:

On Wed, Jun 02, 2021 at 01:54:53PM +0100, Will Deacon wrote:

quoted

There's also Documentation/power/freezing-of-tasks.rst to update. I'm not

Since it's .rst, the only update I'm willing to do is delete it
outright.

Hah! Well, don't do that.

quoted

sure if fs/proc/array.c should be updated to display frozen tasks; I
couldn't see how that was useful, but thought I'd mention it anyway.

Yeah, I considered it too, but I figured that if we're all frozen
there's noone left to observe us being frozen, so I didn't bother.

Agreed.

quoted

quoted

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 2982cfab1ae9..bfadc1dbcf24 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h

@@ -95,7 +95,12 @@ struct task_group;
 #define TASK_WAKING			0x0200
 #define TASK_NOLOAD			0x0400
 #define TASK_NEW			0x0800
-#define TASK_STATE_MAX			0x1000
+#define TASK_FREEZABLE			0x1000
+#define __TASK_FREEZABLE_UNSAFE		0x2000

Give that this is only needed to avoid lockdep checks, maybe we should avoid
allocating the bit if lockdep is not enabled? Otherwise, people might start
to use it for other things.

Something like

#define __TASK_FREEZABLE_UNSAFE			(0x2000 * IS_ENABLED(CONFIG_LOCKDEP))

?

Yup.

quoted

quoted

+#define TASK_FROZEN			0x4000
+#define TASK_STATE_MAX			0x8000
+
+#define TASK_FREEZABLE_UNSAFE		(TASK_FREEZABLE | __TASK_FREEZABLE_UNSAFE)

We probably want to preserve the "DO NOT ADD ANY NEW CALLERS OF THIS STATE"
comment for the unsafe stuff.

Done.

Thanks.

quoted

quoted

+/* Recursion relies on tail-call optimization to not blow away the stack */
+static bool __frozen(struct task_struct *p)
+{
+	if (p->state == TASK_FROZEN)
+		return true;

READ_ONCE()?

task_struct::state is volatile -- for now. I've got other patches to
deal with that.

Thanks, I missed that and have since reviewed your other series.

quoted

quoted

@@ -116,20 +173,8 @@ bool freeze_task(struct task_struct *p)
 {
 	unsigned long flags;
 
 	spin_lock_irqsave(&freezer_lock, flags);
+	if (!freezing(p) || frozen(p) || __freeze_task(p)) {
 		spin_unlock_irqrestore(&freezer_lock, flags);
 		return false;
 	}

I've been trying to figure out how this serialises with ttwu(), given that
frozen(p) will go and read p->state. I suppose it works out because only the
freezer can wake up tasks from the FROZEN state, but it feels a bit brittle.

p->pi_lock; both ttwu() and __freeze_task() (which is essentially a
variant of set_special_state()) take ->pi_lock. I'll put in a comment.

The part I struggled with was freeze_task(), which doesn't take ->pi_lock
yet calls frozen(p).

quoted

quoted

@@ -137,7 +182,7 @@ bool freeze_task(struct task_struct *p)
 	if (!(p->flags & PF_KTHREAD))
 		fake_signal_wake_up(p);
 	else
-		wake_up_state(p, TASK_INTERRUPTIBLE);
+		wake_up_state(p, TASK_INTERRUPTIBLE); // TASK_NORMAL ?!?
 
 	spin_unlock_irqrestore(&freezer_lock, flags);
 	return true;

@@ -148,8 +193,8 @@ void __thaw_task(struct task_struct *p)
 	unsigned long flags;
 
 	spin_lock_irqsave(&freezer_lock, flags);
-	if (frozen(p))
-		wake_up_process(p);
+	WARN_ON_ONCE(freezing(p));
+	wake_up_state(p, TASK_FROZEN | TASK_NORMAL);

Why do we need TASK_NORMAL here?

It's a left-over from hacking, but I left it in because anything
TASK_NORMAL should be able to deal with spuriuos wakeups, something
try_to_freeze() now also relies on.

I just worry that it might hide bugs if TASK_FROZEN is supposed to be
sufficient, as it would imply that we have some unfrozen tasks kicking
around. I dunno, maybe just a comment saying that everything _should_ be
FROZEN at this point?

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel