livepatch,sched: Add livepatch task switching to cond_resched()

Fix patching stalls caused by busy kthreads.

Josh Poimboeuf (3):
livepatch: Skip task_call_func() for current task livepatch,sched: Add livepatch task switching to cond_resched()
vhost: Fix livepatch timeouts in vhost_worker()

Fix patching stalls caused by busy kthreads.

Josh Poimboeuf (3):
livepatch: Skip task_call_func() for current task livepatch,sched: Add livepatch task switching to cond_resched()
vhost: Fix livepatch timeouts in vhost_worker()

Quoted 6 lines · Josh Poimboeuf

Fix patching stalls caused by busy kthreads.

Josh Poimboeuf (3):
livepatch: Skip task_call_func() for current task livepatch,sched: Add livepatch task switching to cond_resched()
vhost: Fix livepatch timeouts in vhost_worker()

Seems reasonable, you want me to take them through the sched tree?

The current task doesn't need the scheduler's protection to unwind its own stack.

Signed-off-by Josh Poimboeuf <[email protected]>

There have been reports [1][2] of live patches failing to complete within a reasonable amount of time due to CPU-bound kthreads.

Fix it by patching tasks in cond_resched().

There are four different flavors of cond_resched(), depending on the kernel configuration. Hook into all of them.

A more elegant solution might be to use a preempt notifier. However, non-ORC unwinders can't unwind a preempted task reliably.

Livepatch timeouts were reported due to busy vhost_worker() kthreads. Now that cond_resched() can do livepatch task switching, use cond_resched() in vhost_worker(). That's the better way to conditionally call schedule() anyway.

Reported-by Seth Forshee (DigitalOcean) <[email protected]>

Link https://lkml.kernel.org/lkml/[email protected] open here

Signed-off-by Josh Poimboeuf <[email protected]>

Review

Quoted 4 lines

static inline int _cond_resched(void)
{

(quoted diff) +1 -0

+	klp_sched_try_switch();
 	return __cond_resched();

My only concern is if it might cause any performance problems.

On one hand, cond_resched() is used in code paths that are slow on its own. Also it will do nothing most of the time.

On the other hand, cond_resched() is typically used in cycles. One cycle might be fast. The code might be slow because there are too many cycles. Repeating the same failing test might prolong the time significantly.

An idea is to try the switch only when it was not done during a real schedule. Something like:

static inline int _cond_resched(void)

{
	int scheduled;

	scheduled = __cond_resched();
	if (scheduled)
		klp_sched_try_switch();

	return scheduled();
}

But it would make it less reliable/predictable. Also it won't work in configurations when cond_resched() is always a nop.

I am probably too careful. We might keep it simple until any real life problems are reported.

Quoted 6 lines

kernel/livepatch/transition.c +0 -0

@@ -76,6 +96,8 @@ static void klp_complete_transition(void)
 		 klp_transition_patch->mod->name,
 		 klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
 

kernel/livepatch/transition.c static void klp_complete_transition(void) · line 100

Quoted 2 lines

kernel/livepatch/transition.c +2 -0

+	klp_cond_resched_disable();
+

Nit: Strictly speaking, this is not needed when klp_complete_transition()
is called from klp_cancel_transition(). In this case, klp_cond_resched_enable() was not called. So it might be moved into klp_try_complete_transition().

More important thing, thinking loudly:

We need to make sure that no task is in the middle klp_cond_resched_disable() when we modify anything that is used there.

We seem to be on the safe side in klp_complete_transition(). We are here only when all tasks have TIF_PATCH_PENDING cleared. In this case, __klp_sched_try_switch() just returns. Also it calls klp_synchronize_transition() so that all tasks finish the critical part in __klp_sched_try_switch() before any new transition starts.

But it is not the case in klp_reverse_transition(). It modifies klp_target_state() when __klp_sched_try_switch might be in the middle of klp_check_stack() and it might give wrong result.

klp_reverse_transition() already solves similar race with klp_update_patch_state() by clearing all TIF_PATCH_PENDING flags and calling klp_synchronize_transition(). We just need to do it earlier. Something like:

Quoted 10 lines · Petr Mladek

Review

Quoted 4 lines

static inline int _cond_resched(void)
{

(quoted diff) +1 -0

+	klp_sched_try_switch();
 	return __cond_resched();

My only concern is if it might cause any performance problems.

On one hand, cond_resched() is used in code paths that are slow on its own. Also it will do nothing most of the time.

Quoted 4 lines

On the other hand, cond_resched() is typically used in cycles. One cycle might be fast. The code might be slow because there are too many cycles. Repeating the same failing test might prolong the time significantly.

Yes, but it should hopefully be very rare to patch a function in the call stack of a kthread loop. In general it's a good idea for the patch author to avoid that.

Quoted 17 lines

An idea is to try the switch only when it was not done during a real schedule. Something like:

static inline int _cond_resched(void)

{
	int scheduled;

	scheduled = __cond_resched();
	if (scheduled)
		klp_sched_try_switch();

	return scheduled();
}

But it would make it less reliable/predictable. Also it won't work in configurations when cond_resched() is always a nop.

Quoted 2 lines

I am probably too careful. We might keep it simple until any real life problems are reported.

If we can get away with it, I much prefer the simple unconditional klp_sched_try_switch() because of the predictability and quickness with which the kthread gets patched.