sched,livepatch: Untangle cond_resched() and live-patching

RFC PATCH In review Peter Zijlstra 3 participants 7 messages Mar 24, 2025 – Mar 26, 2025

Subsystems (2)

Scheduler Live patching

lore

Share & tools

lore

Peter Zijlstra

author maintainer

2025-03-24 13:49 UTC #0 lore raw

RFC PATCH 1/1 sched,livepatch: Untangle cond_resched() and live-patching

With the goal of deprecating / removing VOLUNTARY preempt, live-patch needs to stop relying on cond_resched() to make forward progress.

Instead, rely on schedule() with TASK_FREEZABLE set. Just like live-patching, the freezer needs to be able to stop tasks in a safe / known state.

Compile tested only.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 include/linux/livepatch_sched.h | 15 +++++--------
 include/linux/sched.h           |  6 -----
 kernel/livepatch/transition.c   | 30 ++++++-------------------
 kernel/sched/core.c             | 50 +++++++----------------------------------
 4 files changed, 21 insertions(+), 80 deletions(-)

include/linux/livepatch_sched.h +6 -9

index 013794fb5da0..7e8171226dd7 100644
@@ -3,27 +3,24 @@
 #define _LINUX_LIVEPATCH_SCHED_H_
 
 #include <linux/jump_label.h>
-#include <linux/static_call_types.h>
+#include <linux/sched.h>
+
 
 #ifdef CONFIG_LIVEPATCH
 
 void __klp_sched_try_switch(void);
 
-#if !defined(CONFIG_PREEMPT_DYNAMIC) || !defined(CONFIG_HAVE_PREEMPT_DYNAMIC_CALL)
-
 DECLARE_STATIC_KEY_FALSE(klp_sched_try_switch_key);
 
-static __always_inline void klp_sched_try_switch(void)
+static __always_inline void klp_sched_try_switch(struct task_struct *curr)
 {
-	if (static_branch_unlikely(&klp_sched_try_switch_key))
+	if (static_branch_unlikely(&klp_sched_try_switch_key) &&
+	    READ_ONCE(curr->__state) & TASK_FREEZABLE)
 		__klp_sched_try_switch();
 }
 
-#endif /* !CONFIG_PREEMPT_DYNAMIC || !CONFIG_HAVE_PREEMPT_DYNAMIC_CALL */
-
 #else /* !CONFIG_LIVEPATCH */
-static inline void klp_sched_try_switch(void) {}
-static inline void __klp_sched_try_switch(void) {}
+static inline void klp_sched_try_switch(struct task_struct *curr) {}
 #endif /* CONFIG_LIVEPATCH */
 
 #endif /* _LINUX_LIVEPATCH_SCHED_H_ */

include/linux/sched.h +0 -6

index 6e5c38718ff5..b988e1ae9bd9 100644
@@ -44,7 +44,6 @@
 #include <linux/seqlock_types.h>
 #include <linux/kcsan.h>
 #include <linux/rv.h>
-#include <linux/livepatch_sched.h>
 #include <linux/uidgid_types.h>
 #include <asm/kmap_size.h>

@@ -2069,9 +2068,6 @@ extern int __cond_resched(void);
 
 #if defined(CONFIG_PREEMPT_DYNAMIC) && defined(CONFIG_HAVE_PREEMPT_DYNAMIC_CALL)
 
-void sched_dynamic_klp_enable(void);
-void sched_dynamic_klp_disable(void);
-
 DECLARE_STATIC_CALL(cond_resched, __cond_resched);
 
 static __always_inline int _cond_resched(void)

@@ -2092,7 +2088,6 @@ static __always_inline int _cond_resched(void)
 
 static inline int _cond_resched(void)
 {
-	klp_sched_try_switch();
 	return __cond_resched();
 }

@@ -2102,7 +2097,6 @@ static inline int _cond_resched(void)
 
 static inline int _cond_resched(void)
 {
-	klp_sched_try_switch();
 	return 0;
 }

kernel/livepatch/transition.c +7 ~~-23~~

index ba069459c101..2676c43642ff 100644
@@ -29,22 +29,13 @@ static unsigned int klp_signals_cnt;
 
 /*
  * When a livepatch is in progress, enable klp stack checking in
- * cond_resched().  This helps CPU-bound kthreads get patched.
+ * schedule().  This helps CPU-bound kthreads get patched.
  */
-#if defined(CONFIG_PREEMPT_DYNAMIC) && defined(CONFIG_HAVE_PREEMPT_DYNAMIC_CALL)
-
-#define klp_cond_resched_enable() sched_dynamic_klp_enable()
-#define klp_cond_resched_disable() sched_dynamic_klp_disable()
-
-#else /* !CONFIG_PREEMPT_DYNAMIC || !CONFIG_HAVE_PREEMPT_DYNAMIC_CALL */
 
 DEFINE_STATIC_KEY_FALSE(klp_sched_try_switch_key);
-EXPORT_SYMBOL(klp_sched_try_switch_key);
 
-#define klp_cond_resched_enable() static_branch_enable(&klp_sched_try_switch_key)
-#define klp_cond_resched_disable() static_branch_disable(&klp_sched_try_switch_key)
-
-#endif /* CONFIG_PREEMPT_DYNAMIC && CONFIG_HAVE_PREEMPT_DYNAMIC_CALL */
+#define klp_resched_enable() static_branch_enable(&klp_sched_try_switch_key)
+#define klp_resched_disable() static_branch_disable(&klp_sched_try_switch_key)
 
 /*
  * This work can be performed periodically to finish patching or unpatching any

@@ -365,9 +356,6 @@ static bool klp_try_switch_task(struct task_struct *task)
 
 void __klp_sched_try_switch(void)
 {
-	if (likely(!klp_patch_pending(current)))
-		return;
-
 	/*
 	 * This function is called from cond_resched() which is called in many
 	 * places throughout the kernel.  Using the klp_mutex here might

@@ -377,14 +365,14 @@ void __klp_sched_try_switch(void)
 	 * klp_try_switch_task().  Thanks to task_call_func() they won't be
 	 * able to switch this task while it's running.
 	 */
-	preempt_disable();
+	lockdep_assert_preemption_disabled();
 
 	/*
 	 * Make sure current didn't get patched between the above check and
 	 * preempt_disable().
 	 */
 	if (unlikely(!klp_patch_pending(current)))
-		goto out;
+		return;
 
 	/*
 	 * Enforce the order of the TIF_PATCH_PENDING read above and the

@@ -395,11 +383,7 @@ void __klp_sched_try_switch(void)
 	smp_rmb();
 
 	klp_try_switch_task(current);
-
-out:
-	preempt_enable();
 }
-EXPORT_SYMBOL(__klp_sched_try_switch);
 
 /*
  * Sends a fake signal to all non-kthread tasks with TIF_PATCH_PENDING set.

@@ -508,7 +492,7 @@ void klp_try_complete_transition(void)
 	}
 
 	/* Done!  Now cleanup the data structures. */
-	klp_cond_resched_disable();
+	klp_resched_disable();
 	patch = klp_transition_patch;
 	klp_complete_transition();

@@ -560,7 +544,7 @@ void klp_start_transition(void)
 			set_tsk_thread_flag(task, TIF_PATCH_PENDING);
 	}
 
-	klp_cond_resched_enable();
+	klp_resched_enable();
 
 	klp_signals_cnt = 0;
 }

kernel/sched/core.c +8 ~~-42~~

110 lines not on this page · raw message

Petr Mladek

maintainer

2025-03-26 09:49 UTC #1 lore raw

Quoted 36 lines · Peter Zijlstra

With the goal of deprecating / removing VOLUNTARY preempt, live-patch needs to stop relying on cond_resched() to make forward progress.
Instead, rely on schedule() with TASK_FREEZABLE set. Just like live-patching, the freezer needs to be able to stop tasks in a safe / known state.
Compile tested only.
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
---
 include/linux/livepatch_sched.h | 15 +++++--------
 include/linux/sched.h           |  6 -----
 kernel/livepatch/transition.c   | 30 ++++++-------------------
 kernel/sched/core.c             | 50 +++++++----------------------------------
 4 files changed, 21 insertions(+), 80 deletions(-)
include/linux/livepatch_sched.h +2 -3
index 013794fb5da0..7e8171226dd7 100644
@@ -3,27 +3,24 @@
 #define _LINUX_LIVEPATCH_SCHED_H_
 
 #include <linux/jump_label.h>
-#include <linux/static_call_types.h>
+#include <linux/sched.h>
+
 
 #ifdef CONFIG_LIVEPATCH
 
 void __klp_sched_try_switch(void);
 
-#if !defined(CONFIG_PREEMPT_DYNAMIC) || !defined(CONFIG_HAVE_PREEMPT_DYNAMIC_CALL)
-

include/linux/livepatch_sched.h line 20

Quoted 10 lines

include/linux/livepatch_sched.h +3 -2

 DECLARE_STATIC_KEY_FALSE(klp_sched_try_switch_key);
 
-static __always_inline void klp_sched_try_switch(void)
+static __always_inline void klp_sched_try_switch(struct task_struct *curr)
 {
-	if (static_branch_unlikely(&klp_sched_try_switch_key))
+	if (static_branch_unlikely(&klp_sched_try_switch_key) &&
+	    READ_ONCE(curr->__state) & TASK_FREEZABLE)
 		__klp_sched_try_switch();
 }

Do we really need to check the TASK_FREEZABLE state, please?

My understanding is that TASK_FREEZABLE is set when kernel kthreads go into a "freezable" sleep, e.g. wait_event_freezable().

But __klp_sched_try_switch() should be safe when the task is not running and the stack is reliable. IMHO, it should be safe anytime it is being scheduled out.

Note that wait_event_freezable() is a good location. It is usually called in the main loop of the kthread where the stack is small. So that the chance that it is not running a livepatched function is higher than on another random schedulable location.

But we actually wanted to have it in cond_resched() because it might take a long time to reach the main loop, and sleep there.

Best Regards,
Petr

Peter Zijlstra

author maintainer

2025-03-26 10:38 UTC #2 lore raw

Quoted 50 lines · Petr Mladek

Quoted 36 lines · Peter Zijlstra
With the goal of deprecating / removing VOLUNTARY preempt, live-patch needs to stop relying on cond_resched() to make forward progress.
Instead, rely on schedule() with TASK_FREEZABLE set. Just like live-patching, the freezer needs to be able to stop tasks in a safe / known state.
Compile tested only.
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
---
 include/linux/livepatch_sched.h | 15 +++++--------
 include/linux/sched.h           |  6 -----
 kernel/livepatch/transition.c   | 30 ++++++-------------------
 kernel/sched/core.c             | 50 +++++++----------------------------------
 4 files changed, 21 insertions(+), 80 deletions(-)
include/linux/livepatch_sched.h +2 -3
index 013794fb5da0..7e8171226dd7 100644
@@ -3,27 +3,24 @@
 #define _LINUX_LIVEPATCH_SCHED_H_
 
 #include <linux/jump_label.h>
-#include <linux/static_call_types.h>
+#include <linux/sched.h>
+
 
 #ifdef CONFIG_LIVEPATCH
 
 void __klp_sched_try_switch(void);
 
-#if !defined(CONFIG_PREEMPT_DYNAMIC) || !defined(CONFIG_HAVE_PREEMPT_DYNAMIC_CALL)
-
include/linux/livepatch_sched.h line 20
Quoted 10 lines
include/linux/livepatch_sched.h +3 -2
 DECLARE_STATIC_KEY_FALSE(klp_sched_try_switch_key);
 
-static __always_inline void klp_sched_try_switch(void)
+static __always_inline void klp_sched_try_switch(struct task_struct *curr)
 {
-	if (static_branch_unlikely(&klp_sched_try_switch_key))
+	if (static_branch_unlikely(&klp_sched_try_switch_key) &&
+	    READ_ONCE(curr->__state) & TASK_FREEZABLE)
 		__klp_sched_try_switch();
 }
Do we really need to check the TASK_FREEZABLE state, please?

Quoted 2 lines

My understanding is that TASK_FREEZABLE is set when kernel kthreads go into a "freezable" sleep, e.g. wait_event_freezable().

Right.

Quoted 3 lines

But __klp_sched_try_switch() should be safe when the task is not running and the stack is reliable. IMHO, it should be safe anytime it is being scheduled out.

So for the reasons you touched upon in the next paragraph, FREEZABLE seemed like a more suitable location.

Quoted 4 lines

Note that wait_event_freezable() is a good location. It is usually called in the main loop of the kthread where the stack is small. So that the chance that it is not running a livepatched function is higher than on another random schedulable location.

Right, it is the natural quiescent state of the kthread, it holds no resources.

Quoted 2 lines

But we actually wanted to have it in cond_resched() because it might take a long time to reach the main loop, and sleep there.

Well, cond_resched() is going to get deleted, so we need to find something else. And I was thinking that the suspend people want reasonable timeliness too -- you don't want your laptop to continue running for many seconds after you close the lid and stuff it in your bag, now do you.

So per that reasoning I figured FREEZABLE should be good enough.

Sharing the pain with suspend can only lead to improving both -- faster patching progress leads to faster suspend and vice-versa.

Miroslav Benes

maintainer

2025-03-26 14:37 UTC #3 lore raw

Hi,

Quoted 80 lines · Peter Zijlstra

Quoted 50 lines · Petr Mladek
Quoted 36 lines · Peter Zijlstra
With the goal of deprecating / removing VOLUNTARY preempt, live-patch needs to stop relying on cond_resched() to make forward progress.
Instead, rely on schedule() with TASK_FREEZABLE set. Just like live-patching, the freezer needs to be able to stop tasks in a safe / known state.
Compile tested only.
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
---
 include/linux/livepatch_sched.h | 15 +++++--------
 include/linux/sched.h           |  6 -----
 kernel/livepatch/transition.c   | 30 ++++++-------------------
 kernel/sched/core.c             | 50 +++++++----------------------------------
 4 files changed, 21 insertions(+), 80 deletions(-)
include/linux/livepatch_sched.h +2 -3
index 013794fb5da0..7e8171226dd7 100644
@@ -3,27 +3,24 @@
 #define _LINUX_LIVEPATCH_SCHED_H_
 
 #include <linux/jump_label.h>
-#include <linux/static_call_types.h>
+#include <linux/sched.h>
+
 
 #ifdef CONFIG_LIVEPATCH
 
 void __klp_sched_try_switch(void);
 
-#if !defined(CONFIG_PREEMPT_DYNAMIC) || !defined(CONFIG_HAVE_PREEMPT_DYNAMIC_CALL)
-
include/linux/livepatch_sched.h line 20
Quoted 10 lines
include/linux/livepatch_sched.h +3 -2
 DECLARE_STATIC_KEY_FALSE(klp_sched_try_switch_key);
 
-static __always_inline void klp_sched_try_switch(void)
+static __always_inline void klp_sched_try_switch(struct task_struct *curr)
 {
-	if (static_branch_unlikely(&klp_sched_try_switch_key))
+	if (static_branch_unlikely(&klp_sched_try_switch_key) &&
+	    READ_ONCE(curr->__state) & TASK_FREEZABLE)
 		__klp_sched_try_switch();
 }
Do we really need to check the TASK_FREEZABLE state, please?
Quoted 2 lines
My understanding is that TASK_FREEZABLE is set when kernel kthreads go into a "freezable" sleep, e.g. wait_event_freezable().
Right.
Quoted 3 lines
But __klp_sched_try_switch() should be safe when the task is not running and the stack is reliable. IMHO, it should be safe anytime it is being scheduled out.
So for the reasons you touched upon in the next paragraph, FREEZABLE seemed like a more suitable location.
Quoted 4 lines
Note that wait_event_freezable() is a good location. It is usually called in the main loop of the kthread where the stack is small. So that the chance that it is not running a livepatched function is higher than on another random schedulable location.
Right, it is the natural quiescent state of the kthread, it holds no resources.
Quoted 2 lines
But we actually wanted to have it in cond_resched() because it might take a long time to reach the main loop, and sleep there.
Well, cond_resched() is going to get deleted, so we need to find something else. And I was thinking that the suspend people want reasonable timeliness too -- you don't want your laptop to continue running for many seconds after you close the lid and stuff it in your bag, now do you.

Quoted 4 lines

So per that reasoning I figured FREEZABLE should be good enough.
Sharing the pain with suspend can only lead to improving both -- faster patching progress leads to faster suspend and vice-versa.

If I remember correctly, we had something like this in the old kGraft implementation of the live patching (SUSE way). We exactly had a hook somewhere in the kthread freezing code. This looks much cleaner and as far as I know the fridge went through improvements recently.

Peter, so that I understand it correctly... we would rely on all kthreads becoming freezable eventually so that both suspend and livepatch benefit. Is that what you meant by the above?

Miroslav

Peter Zijlstra

author maintainer

2025-03-26 14:42 UTC #4 lore raw

Quoted 4 lines · Miroslav Benes

If I remember correctly, we had something like this in the old kGraft implementation of the live patching (SUSE way). We exactly had a hook somewhere in the kthread freezing code. This looks much cleaner and as far as I know the fridge went through improvements recently.

Yeah, I rewrote it a while ago :-)

Quoted 3 lines

Peter, so that I understand it correctly... we would rely on all kthreads becoming freezable eventually so that both suspend and livepatch benefit. Is that what you meant by the above?

Well, IIRC (its been a while already) all kthreads should have a FREEZABLE already. Things like suspend-to-idle don't hit the hotplug path at all anymore and everything must freeze, otherwise they fail.

I was more meaning the time-to-freeze; if some kthreads take a long time to freeze/patch then this would want improving on both ends.

Miroslav Benes

maintainer

2025-03-26 14:52 UTC #5 lore raw

Quoted 8 lines · Peter Zijlstra

Quoted 4 lines · Miroslav Benes
If I remember correctly, we had something like this in the old kGraft implementation of the live patching (SUSE way). We exactly had a hook somewhere in the kthread freezing code. This looks much cleaner and as far as I know the fridge went through improvements recently.
Yeah, I rewrote it a while ago :-)

Right :)

Quoted 7 lines

Quoted 3 lines
Peter, so that I understand it correctly... we would rely on all kthreads becoming freezable eventually so that both suspend and livepatch benefit. Is that what you meant by the above?
Well, IIRC (its been a while already) all kthreads should have a FREEZABLE already. Things like suspend-to-idle don't hit the hotplug path at all anymore and everything must freeze, otherwise they fail.

Good.

Quoted 2 lines

I was more meaning the time-to-freeze; if some kthreads take a long time to freeze/patch then this would want improving on both ends.

Makes sense.

So I am all for the patch. See my comment elsewhere though.

Miroslav

Miroslav Benes

maintainer

2025-03-26 14:54 UTC #6 lore raw

In reply to Peter Zijlstra

Quoted 12 lines

void __klp_sched_try_switch(void)
{

kernel/livepatch/transition.c +0 -3

-	if (likely(!klp_patch_pending(current)))
-		return;
-
 	/*
 	 * This function is called from cond_resched() which is called in many
 	 * places throughout the kernel.  Using the klp_mutex here might

@@ -377,14 +365,14 @@ void __klp_sched_try_switch(void)
 	 * klp_try_switch_task().  Thanks to task_call_func() they won't be
 	 * able to switch this task while it's running.
 	 */

kernel/livepatch/transition.c void __klp_sched_try_switch(void) · line 375

Quoted 10 lines

kernel/livepatch/transition.c +2 -2

-	preempt_disable();
+	lockdep_assert_preemption_disabled();
 
 	/*
 	 * Make sure current didn't get patched between the above check and
 	 * preempt_disable().
 	 */
 	if (unlikely(!klp_patch_pending(current)))
-		goto out;
+		return;

It does not look correct. We just make sure that klp_patch_pending(current) did not change here. It would be highly unlikely. However, we should keep the likely way out (the first removed condition above). So let's also s/unlikely/likely/ here.

And the comments in the function should be updated as well.

Miroslav