sched,rcu,context_tracking,livepatch: Improve livepatch task transitions for idle and NOHZ_FULL cpus

Instead of frobbing around with scheduler internals, use the shiny new task_try_func() interface.

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

---
 kernel/livepatch/transition.c |   84 ++++++++++++++++++------------------------
 1 file changed, 37 insertions(+), 47 deletions(-)

kernel/livepatch/transition.c +19 -7

+{
+	int ret;
+
+	if (task_curr(task))
+		return -EBUSY;
+
+	ret = klp_check_stack(task, arg);
+	if (ret)
+		return ret;
+
+	clear_tsk_thread_flag(task, TIF_PATCH_PENDING);
+	task->patch_state = klp_target_state;
+	return 0;
+}
+
 /*
  * Try to safely switch a task to the target patch state.  If it's currently
  * running, or it's sleeping on a to-be-patched or to-be-unpatched function, or

@@ -305,36 +308,23 @@ static bool klp_try_switch_task(struct t
 	 * functions.  If all goes well, switch the task to the target patch
 	 * state.
 	 */
-	rq = task_rq_lock(task, &flags);
-
-	if (task_running(rq, task) && task != current) {
-		snprintf(err_buf, STACK_ERR_BUF_SIZE,
-			 "%s: %s:%d is running\n", __func__, task->comm,
-			 task->pid);
-		goto done;
+	ret = task_try_func(task, klp_check_task, &old_name);
+	switch (ret) {
+	case -EBUSY:
+		pr_debug("%s: %s:%d is running\n",

kernel/livepatch/transition.c +0 -13

 	}
 
-	ret = klp_check_stack(task, err_buf);
-	if (ret)
-		goto done;
-
-	success = true;
-
-	clear_tsk_thread_flag(task, TIF_PATCH_PENDING);
-	task->patch_state = klp_target_state;
-
-done:
-	task_rq_unlock(rq, task, &flags);
-
-	/*

Quoted 1 line · Peter Zijlstra

+static int klp_check_task(struct task_struct *task, void *arg)

Please, call this klp_check_and_switch_task() to make it clear that it actually does the switch.

Quoted 4 lines

kernel/livepatch/transition.c +4 -0

+	ret = task_try_func(task, klp_check_task, &old_name);
+	switch (ret) {
+	case -EBUSY:
+		pr_debug("%s: %s:%d is running\n",

kernel/livepatch/transition.c static bool klp_try_switch_task(struct t · line 324

Quoted 10 lines

kernel/livepatch/transition.c +10 -0

+			 __func__, task->comm, task->pid);
+		break;
+	case -EINVAL:
+		pr_debug("%s: %s:%d has an unreliable stack\n",
+			 __func__, task->comm, task->pid);
+		break;
+	case -EADDRINUSE:
+		pr_debug("%s: %s:%d is sleeping on function %s\n",
+			 __func__, task->comm, task->pid, old_name);
+		break;

I would prefer to be on the safe side and catch error codes that might eventually appear in the future.

	case 0:
		/* success */
		break;

	default:
		pr_debug("%s: Unknown error code (%d) when trying to switch %s:%d\n",
			 __func__, ret, task->comm, task->pid);

Quoted 1 line

}

Quoted 6 lines · Petr Mladek

I would prefer to be on the safe side and catch error codes that might eventually appear in the future.

	case 0:
		/* success */
		break;

	case -EAGAIN:
		/* task_try_func() raced */
		break;

Quoted 5 lines

kernel/livepatch/transition.c +0 -1

@@ -405,8 +405,10 @@ void klp_try_complete_transition(void)
 	for_each_possible_cpu(cpu) {
 		task = idle_task(cpu);
 		if (cpu_online(cpu)) {
-			if (!klp_try_switch_task(task))

kernel/livepatch/transition.c void klp_try_complete_transition(void) · line 411

Quoted 5 lines

kernel/livepatch/transition.c +4 -1

-				complete = false;
+			int ret = klp_try_switch_task(task);
+			if (ret == -EBUSY)
+				wake_up_if_idle(cpu);
+			complete = !ret;

This is broken. You can basically change "complete" only to false (when it applies). This could leave some tasks in the old patching state.

Use rcu_user_{enter,exit}() calls to provide SMP ordering on context tracking state stores:

__context_tracking_exit()
__this_cpu_write(context_tracking.state, CONTEXT_KERNEL) rcu_user_exit()

    rcu_eqs_exit()
      rcu_dynticks_eqs_eit()
        rcu_dynticks_inc()
          atomic_add_return() /* smp_mb */

__context_tracking_enter()
rcu_user_enter()

    rcu_eqs_enter()
      rcu_dynticks_eqs_enter()
        rcu_dynticks_inc()
	  atomic_add_return() /* smp_mb */

__this_cpu_write(context_tracking.state, state)

This separates USER/KERNEL state with an smp_mb() on each side, therefore, a user of context_tracking_state_cpu() can say the CPU must pass through an smp_mb() before changing.

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

---
 include/linux/context_tracking_state.h |   12 ++++++++++++
 kernel/context_tracking.c              |    7 ++++---
 2 files changed, 16 insertions(+), 3 deletions(-)

include/linux/context_tracking_state.h +12 -0

@@ -45,11 +45,23 @@ static __always_inline bool context_trac
 {
 	return __this_cpu_read(context_tracking.state) == CONTEXT_USER;
 }
+
+static __always_inline bool context_tracking_state_cpu(int cpu)
+{
+	struct context_tracking *ct = per_cpu_ptr(&context_tracking);
+
+	if (!context_tracking_enabled() || !ct->active)
+		return CONTEXT_DISABLED;
+
+	return ct->state;
+}
+
 #else
 static inline bool context_tracking_in_user(void) { return false; }
 static inline bool context_tracking_enabled(void) { return false; }
 static inline bool context_tracking_enabled_cpu(int cpu) { return false; }
 static inline bool context_tracking_enabled_this_cpu(void) { return false; }
+static inline bool context_tracking_state_cpu(int cpu) { return CONTEXT_DISABLED; }
 #endif /* CONFIG_CONTEXT_TRACKING */
 
 #endif

kernel/context_tracking.c +4 -3

@@ -82,7 +82,7 @@ void noinstr __context_tracking_enter(en
 				vtime_user_enter(current);
 				instrumentation_end();
 			}
-			rcu_user_enter();
+			rcu_user_enter(); /* smp_mb */
 		}
 		/*
 		 * Even if context tracking is disabled on this CPU, because it's outside

@@ -149,12 +149,14 @@ void noinstr __context_tracking_exit(enu
 		return;
 
 	if (__this_cpu_read(context_tracking.state) == state) {
+		__this_cpu_write(context_tracking.state, CONTEXT_KERNEL);
+
 		if (__this_cpu_read(context_tracking.active)) {
 			/*
 			 * We are going to run code that may use RCU. Inform
 			 * RCU core about that (ie: we may need the tick again).
 			 */
-			rcu_user_exit();
+			rcu_user_exit(); /* smp_mb */
 			if (state == CONTEXT_USER) {
 				instrumentation_begin();
 				vtime_user_exit(current);

@@ -162,7 +164,6 @@ void noinstr __context_tracking_exit(enu
 				instrumentation_end();
 			}
 		}
-		__this_cpu_write(context_tracking.state, CONTEXT_KERNEL);
 	}
 	context_tracking_recursion_exit();
 }

Quoted 23 lines · Peter Zijlstra

Use rcu_user_{enter,exit}() calls to provide SMP ordering on context tracking state stores:

__context_tracking_exit()
__this_cpu_write(context_tracking.state, CONTEXT_KERNEL) rcu_user_exit()

    rcu_eqs_exit()
      rcu_dynticks_eqs_eit()
        rcu_dynticks_inc()
          atomic_add_return() /* smp_mb */

__context_tracking_enter()
rcu_user_enter()

    rcu_eqs_enter()
      rcu_dynticks_eqs_enter()
        rcu_dynticks_inc()
	  atomic_add_return() /* smp_mb */

__this_cpu_write(context_tracking.state, state)

This separates USER/KERNEL state with an smp_mb() on each side, therefore, a user of context_tracking_state_cpu() can say the CPU must pass through an smp_mb() before changing.

Quoted 1 line

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

For the transformation to negative errno return value and name change from an RCU perspective:

Acked-by Paul E. McKenney <[email protected]>

For the sampling of nohz_full userspace state:

Another approach is for the rcu_data structure's ->dynticks variable to use the lower two bits to differentiate between idle, nohz_full userspace and kernel. In theory, inlining should make this zero cost for idle transition, and should allow you to safely sample nohz_full userspace state with a load and a couple of memory barriers instead of an IPI.

To make this work nicely, the low-order bits have to be 00 for kernel, and (say) 01 for idle and 10 for nohz_full userspace. 11 would be an error.

The trick would be for rcu_user_enter() and rcu_user_exit() to atomically increment ->dynticks by 2, for rcu_nmi_exit() to increment by 1 and rcu_nmi_enter() to increment by 3. The state sampling would need to change accordingly.

Does this make sense, or am I missing something?

Anyway, lemme see if I get your proposal; lets say the counter starts at 0 and is in kernel space.

0x00(0) - kernel
0x02(2) - user
0x04(0) - kernel

So far so simple, then NMI on top of that goes:

0x00(0) - kernel
0x03(3) - kernel + nmi
0x04(0) - kernel
0x06(2) - user
0x09(1) - user + nmi
0x0a(2) - user

Which then gives us:

(0) := kernel
(1) := nmi-from-user
(2) := user
(3) := nmi-from-kernel

Which should work I suppose. But like I said above, I'd be happier if this counter would live in context_tracking rather than RCU.

Quoted 18 lines · Peter Zijlstra

Anyway, lemme see if I get your proposal; lets say the counter starts at 0 and is in kernel space.

0x00(0) - kernel
0x02(2) - user
0x04(0) - kernel

So far so simple, then NMI on top of that goes:

0x00(0) - kernel
0x03(3) - kernel + nmi
0x04(0) - kernel
0x06(2) - user
0x09(1) - user + nmi
0x0a(2) - user

Which then gives us:

Quoted 7 lines

(0) := kernel
(1) := nmi-from-user
(2) := user
(3) := nmi-from-kernel

Which should work I suppose. But like I said above, I'd be happier if this counter would live in context_tracking rather than RCU.

Furthermore, if we have this counter, the we can also do things like:

  seq = context_tracking_seq_cpu(that_cpu);
  if ((seq & 3) != USER)
    // nohz_fail, do something
  set_tsk_thread_flag(curr_task(that_cpu), TIF_DO_SOME_WORK);
  if (seq == context_tracking_seq_cpu(that_cpu))
    // success!!

To remotely set pending state. Allowing yet more NOHZ_FULL fixes, like, for example, eliding the text_poke IPIs.

Quoted 24 lines · Paul E. McKenney

Quoted 23 lines · Peter Zijlstra

Use rcu_user_{enter,exit}() calls to provide SMP ordering on context tracking state stores:

__context_tracking_exit()
__this_cpu_write(context_tracking.state, CONTEXT_KERNEL) rcu_user_exit()

    rcu_eqs_exit()
      rcu_dynticks_eqs_eit()
        rcu_dynticks_inc()
          atomic_add_return() /* smp_mb */

__context_tracking_enter()
rcu_user_enter()

    rcu_eqs_enter()
      rcu_dynticks_eqs_enter()
        rcu_dynticks_inc()
	  atomic_add_return() /* smp_mb */

__this_cpu_write(context_tracking.state, state)

This separates USER/KERNEL state with an smp_mb() on each side, therefore, a user of context_tracking_state_cpu() can say the CPU must pass through an smp_mb() before changing.

Quoted 6 lines

Quoted 1 line

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

For the transformation to negative errno return value and name change from an RCU perspective:

Acked-by Paul E. McKenney <[email protected]>

Thanks!

Quoted 7 lines

For the sampling of nohz_full userspace state:

Another approach is for the rcu_data structure's ->dynticks variable to use the lower two bits to differentiate between idle, nohz_full userspace and kernel. In theory, inlining should make this zero cost for idle transition, and should allow you to safely sample nohz_full userspace state with a load and a couple of memory barriers instead of an IPI.

That's what I do now, it's like:

<user code>

state = KERNEL
smp_mb()

<kernel code>

smp_mb()
state = USER

<user core>

vs

<patch kernel code>
smp_mb()
if (state == USER)

    // then we're guaranteed any subsequent kernel code execution
    // will see the modified kernel code

more-or-less

Quoted 9 lines

To make this work nicely, the low-order bits have to be 00 for kernel, and (say) 01 for idle and 10 for nohz_full userspace. 11 would be an error.

The trick would be for rcu_user_enter() and rcu_user_exit() to atomically increment ->dynticks by 2, for rcu_nmi_exit() to increment by 1 and rcu_nmi_enter() to increment by 3. The state sampling would need to change accordingly.

Quoted 1 line

Does this make sense, or am I missing something?

Why doesn't the proposed patch work? Also, ISTR sampling of remote context state coming up before. And as is, it's a weird mix between context_tracking and rcu.

AFAICT there is very little useful in context_tracking as is, but it's also very weird to have to ask RCU about this. Is there any way to slice this this code differently? Perhaps move some of the state RCU now keeps into context_tracking ?

Anyway, lemme see if I get your proposal; lets say the counter starts at 0 and is in kernel space.

0x00(0) - kernel
0x02(2) - user
0x04(0) - kernel

So far so simple, then NMI on top of that goes:

0x00(0) - kernel
0x03(3) - kernel + nmi

0x04(0) - kernel

0x06(2) - user

0x09(1) - user + nmi

0x0a(2) - user

Which then gives us:

(0) := kernel
(1) := nmi-from-user
(2) := user
(3) := nmi-from-kernel

Which should work I suppose. But like I said above, I'd be happier if this counter would live in context_tracking rather than RCU.

I wasn't saying that the patch doesn't work. But doesn't it add an IPI? Or was I looking at it too early this morning?

Use rcu_user_{enter,exit}() calls to provide SMP ordering on context tracking state stores:

__context_tracking_exit()
__this_cpu_write(context_tracking.state, CONTEXT_KERNEL) rcu_user_exit()

    rcu_eqs_exit()
      rcu_dynticks_eqs_eit()
        rcu_dynticks_inc()
          atomic_add_return() /* smp_mb */

__context_tracking_enter()
rcu_user_enter()

    rcu_eqs_enter()
      rcu_dynticks_eqs_enter()
        rcu_dynticks_inc()
	  atomic_add_return() /* smp_mb */

__this_cpu_write(context_tracking.state, state)

This separates USER/KERNEL state with an smp_mb() on each side, therefore, a user of context_tracking_state_cpu() can say the CPU must pass through an smp_mb() before changing.

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

---
 include/linux/context_tracking_state.h |   12 ++++++++++++
 kernel/context_tracking.c              |    7 ++++---
 2 files changed, 16 insertions(+), 3 deletions(-)

---
 include/linux/context_tracking_state.h |   12 ++++++++++++
 kernel/context_tracking.c              |    7 ++++---
 2 files changed, 16 insertions(+), 3 deletions(-)

include/linux/context_tracking_state.h +10 -0

@@ -45,11 +45,23 @@ static __always_inline bool context_trac
 {
 	return __this_cpu_read(context_tracking.state) == CONTEXT_USER;
 }
+
+static __always_inline bool context_tracking_state_cpu(int cpu)
+{
+	struct context_tracking *ct = per_cpu_ptr(&context_tracking);
+
+	if (!context_tracking_enabled() || !ct->active)
+		return CONTEXT_DISABLED;
+
+	return ct->state;
+}

kernel/context_tracking.c +4 -3

@@ -82,7 +82,7 @@ void noinstr __context_tracking_enter(en
 				vtime_user_enter(current);
 				instrumentation_end();
 			}
-			rcu_user_enter();
+			rcu_user_enter(); /* smp_mb */
 		}
 		/*
 		 * Even if context tracking is disabled on this CPU, because it's outside

@@ -149,12 +149,14 @@ void noinstr __context_tracking_exit(enu
 		return;
 
 	if (__this_cpu_read(context_tracking.state) == state) {
+		__this_cpu_write(context_tracking.state, CONTEXT_KERNEL);
+
 		if (__this_cpu_read(context_tracking.active)) {
 			/*
 			 * We are going to run code that may use RCU. Inform
 			 * RCU core about that (ie: we may need the tick again).
 			 */
-			rcu_user_exit();
+			rcu_user_exit(); /* smp_mb */
 			if (state == CONTEXT_USER) {
 				instrumentation_begin();
 				vtime_user_exit(current);

@@ -162,7 +164,6 @@ void noinstr __context_tracking_exit(enu
 				instrumentation_end();
 			}
 		}
-		__this_cpu_write(context_tracking.state, CONTEXT_KERNEL);
 	}
 	context_tracking_recursion_exit();
 }

Quoted 30 lines · Peter Zijlstra

---
 include/linux/context_tracking_state.h |   12 ++++++++++++
 kernel/context_tracking.c              |    7 ++++---
 2 files changed, 16 insertions(+), 3 deletions(-)

include/linux/context_tracking_state.h +12 -0

@@ -45,11 +45,23 @@ static __always_inline bool context_trac
 {
 	return __this_cpu_read(context_tracking.state) == CONTEXT_USER;
 }
+
+static __always_inline bool context_tracking_state_cpu(int cpu)
+{
+	struct context_tracking *ct = per_cpu_ptr(&context_tracking);
+
+	if (!context_tracking_enabled() || !ct->active)
+		return CONTEXT_DISABLED;
+
+	return ct->state;
+}
+
 #else
 static inline bool context_tracking_in_user(void) { return false; }
 static inline bool context_tracking_enabled(void) { return false; }
 static inline bool context_tracking_enabled_cpu(int cpu) { return false; }
 static inline bool context_tracking_enabled_this_cpu(void) { return false; }
+static inline bool context_tracking_state_cpu(int cpu) { return CONTEXT_DISABLED; }
 #endif /* CONFIG_CONTEXT_TRACKING */
 

include/linux/context_tracking_state.h static __always_inline bool context_trac · line 67

Quoted 1 line

include/linux/context_tracking_state.h +0 -0

 #endif

Should context_tracking_state_cpu return an enum ctx_state rather than a bool? It appears to be doing an implicit cast.

I don't know if it possible to run livepatch with
CONFIG_CONTEXT_TRACKING disabled,

modified by patch 7 will always consider the transition complete even if the current task is in kernel mode. Also in the general case, the CPU will consider the task complete if has ctx_state CONTEXT_GUEST though the condition does not make it explicit.

When a task is stuck in NOHZ_FULL usermode, we can simply mark the livepatch state complete.

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

---
 kernel/livepatch/transition.c |   13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

kernel/livepatch/transition.c +0 -0

@@ -270,13 +270,24 @@ static int klp_check_task(struct task_st
 {
 	int ret;
 

IMHO, this is not safe:

CPU0 CPU1

klp_check_task(A)
if (context_tracking_state_cpu(task_cpu(task)) == CONTEXT_USER)

     goto complete;

  clear_tsk_thread_flag(task, TIF_PATCH_PENDING);

				# task switching to kernel space
				klp_update_patch_state(A)
				       if (test_and_clear_tsk_thread_flag(task,	TIF_PATCH_PENDING))
				       //false

				# calling kernel code with old task->patch_state

	task->patch_state = klp_target_state;

BANG: CPU0 sets task->patch_state when task A is already running
kernel code on CPU1.

Quoted 17 lines · Petr Mladek

IMHO, this is not safe:

CPU0 CPU1

klp_check_task(A)
if (context_tracking_state_cpu(task_cpu(task)) == CONTEXT_USER)

     goto complete;

  clear_tsk_thread_flag(task, TIF_PATCH_PENDING);

				# task switching to kernel space
				klp_update_patch_state(A)
				       if (test_and_clear_tsk_thread_flag(task,	TIF_PATCH_PENDING))
				       //false

				# calling kernel code with old task->patch_state

Quoted 4 lines

	task->patch_state = klp_target_state;

BANG: CPU0 sets task->patch_state when task A is already running
kernel code on CPU1.

Why is that a problem? That is, who actually cares about task->patch_state ? I was under the impression that state was purely for klp itself, to track which task has observed the new state.

Hi,

Compile tested only, please consider carefully, esp. the last few patches that concern context_tracking and nohz_full.