sched,livepatch: call klp_try_switch_task in __cond_resched

Busy kernel threads may block the transition of livepatch. Call klp_try_switch_task from __cond_resched to make the transition easier.

Quoted 4 lines · Song Liu

Busy kernel threads may block the transition of livepatch. Call klp_try_switch_task from __cond_resched to make the transition easier.

That seems like a useful idea given what we're seeing on some systems, but I do have a nitpick with your patch :)

kernel/sched/core.c SYSCALL_DEFINE0(sched_yield) · line 6998

Quoted 11 lines

+++ b/kernel/sched/core.c

kernel/sched/core.c +3 -0

@@ -6990,6 +6990,9 @@ SYSCALL_DEFINE0(sched_yield)
 #if !defined(CONFIG_PREEMPTION) || defined(CONFIG_PREEMPT_DYNAMIC)
 int __sched __cond_resched(void)
 {
+       if (unlikely(klp_patch_pending(current)))
+               klp_try_switch_task(current);
+
        if (should_resched(0)) {
                preempt_schedule_common();
                return 1;

While should_resched and klp_patch_pending check the same cache line (task->flags), now there are two separate conditionals on this.

Would it make sense to fold the tests for TIF_NEED_RESCHED and TIF_PATCH_PENDING int should_resched(), and then re-do the test for TIF_PATCH_PENDING only if should_resched() returns true?

Quoted 27 lines · Rik van Riel

Quoted 4 lines · Song Liu

Busy kernel threads may block the transition of livepatch. Call klp_try_switch_task from __cond_resched to make the transition easier.

That seems like a useful idea given what we're seeing on some systems, but I do have a nitpick with your patch :)

Quoted 11 lines

+++ b/kernel/sched/core.c

kernel/sched/core.c +3 -0

@@ -6990,6 +6990,9 @@ SYSCALL_DEFINE0(sched_yield)
  #if !defined(CONFIG_PREEMPTION) ||
 defined(CONFIG_PREEMPT_DYNAMIC)
  int __sched __cond_resched(void)
  {
+       if (unlikely(klp_patch_pending(current)))
+               klp_try_switch_task(current);
+
         if (should_resched(0)) {
                 preempt_schedule_common();

Quoted 1 line

                return 1;

While should_resched and klp_patch_pending check the same cache line (task->flags), now there are two separate conditionals on this.

Quoted 4 lines

Would it make sense to fold the tests for TIF_NEED_RESCHED and TIF_PATCH_PENDING int should_resched(), and then re-do the test for TIF_PATCH_PENDING only if should_resched() returns true?

x86 has a different version of should_resched(),

so I am not
quite sure what’s the right way o modify shhould_resched(). OTOH, we can probably see should_resched() as-is and just move klp_patch_pending, like

int __sched __cond_resched(void)
{
        if (should_resched(0)) {
                if (unlikely(klp_patch_pending(current))) klp_try_switch_task(current);

                preempt_schedule_common();
                return 1;
        }
#ifndef CONFIG_PREEMPT_RCU
        rcu_all_qs();
#endif
        return 0;
}

Given live patch user space usually waits for many seconds, I guess this should work?

Huh, I just looked at that, and the x86 should_resched() only seems to check that we _can_ resched, not whether we should...

/*
* Returns true when we need to resched and can (barring IRQ state). */
static __always_inline bool should_resched(int preempt_offset)

{
        return unlikely(raw_cpu_read_4(__preempt_count) ==

preempt_offset);
}

I wonder if that was intended, and why, or whether the x86 should_resched should also be checking for TIF_NEED_RESCHED?

No, it does what you think it should do, you're just getting confused by
the inverted PREEMPT_NEED_RESCHED bit :-)

Busy kernel threads may block the transition of livepatch. Call klp_try_switch_task from __cond_resched to make the transition easier.

Quoted 2 lines · Song Liu

Busy kernel threads may block the transition of livepatch. Call klp_try_switch_task from __cond_resched to make the transition easier.

What will a PREEMPT=y kernel do? How is it not a problem there, and if it is, this will not help that.

That is; I don't think this can be right.

Quoted 8 lines · Peter Zijlstra

Quoted 2 lines · Song Liu

Busy kernel threads may block the transition of livepatch. Call klp_try_switch_task from __cond_resched to make the transition easier.

Quoted 4 lines

What will a PREEMPT=y kernel do? How is it not a problem there, and if it is, this will not help that.

That is; I don't think this can be right.

I guess on PREEMPT=y kernel, we can simply preempt the long running kernel thread and check the transition?

In this case (PREEMPT=n), we see a long running kernel thread could not finish the transition. It calls cond_resched() and gets rescheduled (moves among different cores). However, it never finishes the transition, because live patch doesn’t get a chance to check the stack.

Does this answer the question?

Quoted 5 lines · Song Liu

Quoted 5 lines

On May 9, 2022, at 12:04 AM, Peter Zijlstra
<[email protected]> wrote:

Quoted 8 lines

Quoted 8 lines · Song Liu

Quoted 3 lines

Busy kernel threads may block the transition of livepatch. Call klp_try_switch_task from __cond_resched to make the transition easier.

What will a PREEMPT=y kernel do? How is it not a problem there, and if
it is, this will not help that.

Not really. There is no difference between an explicit preemption point
(cond_resched) or an involuntary preemption point (PREEMPT=y).

So unless you can *exactly* say why it isn't a problem on PREEMPT=y, none of this makes any sense.

Quoted 15 lines · Peter Zijlstra

Quoted 5 lines · Song Liu

Quoted 5 lines

On May 9, 2022, at 12:04 AM, Peter Zijlstra
<[email protected]> wrote:

Quoted 8 lines

Quoted 8 lines · Song Liu

Quoted 3 lines

Busy kernel threads may block the transition of livepatch. Call klp_try_switch_task from __cond_resched to make the transition easier.

What will a PREEMPT=y kernel do? How is it not a problem there, and if
it is, this will not help that.

Quoted 6 lines

Not really. There is no difference between an explicit preemption point
(cond_resched) or an involuntary preemption point (PREEMPT=y).

So unless you can *exactly* say why it isn't a problem on PREEMPT=y, none of this makes any sense.

I suspect it is a problem on PREEMPT=y too, but is there some sort of fairly light weight (in terms of stuff we need to add to the kernel) solution that could solve both?

Do we have some real time per-CPU kernel threads we could just issue a NOOP call to, which would preempt long-running kernel threads (like a kworker with oodles of work to do)?

Could the stopper workqueue be a suitable tool for this job?

Busy kernel threads may block the transition of livepatch. Call klp_try_switch_task from __cond_resched to make the transition easier.

Quoted 2 lines · Song Liu

Busy kernel threads may block the transition of livepatch. Call klp_try_switch_task from __cond_resched to make the transition easier.

Do you have some numbers how this speeds up the transition and how it slows down the scheduler, please?

cond_resched() is typically called in cycles with many interactions where the task might spend a lot of time. There are two possibilities. cond_resched() is called in:

+ livepatched function

In this case, klp_try_switch_task(current) will always fail. And it will non-necessarily slow down every iteration by checking the very same stack.

+ non-livepatched function

In this case, the transition will succeed on the first attempt. OK, but it would succeed also without that patch. The task would most likely sleep in this cond_resched() so that it might be successfully transitioned on the next occasion.

From my POV this patch this patch brings more harm than good.

Note that scheduling is a fast path. It is repeated zillion-times on any system. But livepatch transition is a slow path. It does not matter if it takes 1 second or 1 hour.

Best Regards,
Petr

Quoted 8 lines · Petr Mladek

Quoted 2 lines · Song Liu

Busy kernel threads may block the transition of livepatch. Call klp_try_switch_task from __cond_resched to make the transition easier.

Quoted 2 lines

Do you have some numbers how this speeds up the transition and how it slows down the scheduler, please?

We don’t have number on how much this would slow down the scheduler. For the transition, we see cases where the transition cannot finish with in 60 seconds (how much "kpatch load" waits by default).

Quoted 11 lines

cond_resched() is typically called in cycles with many interactions where the task might spend a lot of time. There are two possibilities. cond_resched() is called in:

+ livepatched function

In this case, klp_try_switch_task(current) will always fail. And it will non-necessarily slow down every iteration by checking the very same stack.

Quoted 6 lines

+ non-livepatched function

In this case, the transition will succeed on the first attempt. OK, but it would succeed also without that patch. The task would most likely sleep in this cond_resched() so that it might be successfully transitioned on the next occasion.

We are in the non-live patched case. But the transition didn’t happen in time, because the kernel thread doesn’t go to sleep. While there is clearly something weird with this thread, we think live patch should work because the thread does call cond_resched from time to time.

IMHO, the problem is that klp_transition_work_fn() tries the transition "only" once per second, see

void klp_try_complete_transition(void)
{
[...]
                schedule_delayed_work(&klp_transition_work, round_jiffies_relative(HZ)); [...]
}

It means that there are "only" 60 attempts to migrate the busy process.
It fails when the process is in the running state or sleeping in a livepatched function. There is a _non-zero_ chance of a bad luck.

Anyway, the limit 60s looks like a bad idea to me. It is too low.

Quoted 12 lines · Petr Mladek

IMHO, the problem is that klp_transition_work_fn() tries the transition "only" once per second, see

void klp_try_complete_transition(void)
{
[...]
                schedule_delayed_work(&klp_transition_work, round_jiffies_relative(HZ)); [...]
}

Quoted 5 lines

It means that there are "only" 60 attempts to migrate the busy process.
It fails when the process is in the running state or sleeping in a livepatched function. There is a _non-zero_ chance of a bad luck.

We are definitely hitting that non-zero chance :)

Quoted 1 line

Anyway, the limit 60s looks like a bad idea to me. It is too low.

That has its own issues, though. System management software tracks whether kpatch succeeds, and a run of the system management software will not complete until all of the commands it has run have completed.

One reason for this is that allowing system management software to just fork more and more things that might potentially get stuck is that you never want your system management software to come even close to resembling a fork bomb :)

Rollout of the next config change to a system should not be blocked on KLP completion.

I think the best approach for us might be to just track what is causing the transition failures, and send in trivial patches to make the outer loop in such kernel threads do the same KLP transition the idle task already does.

Quoted 2 lines · Rik van Riel

On Tue, 2022-05-10 at 09:56 +0200, Petr Mladek wrote:

Quoted 4 lines

I think the best approach for us might be to just track what is causing the transition failures, and send in trivial patches to make the outer loop in such kernel threads do the same KLP transition the idle task already does.

I am afraid that is a way to hell. We might end up in doing really crazy things if we want to complete the transition in one minute.

Quoted 4 lines · Petr Mladek

Quoted 2 lines · Rik van Riel

On Tue, 2022-05-10 at 09:56 +0200, Petr Mladek wrote:

Quoted 9 lines

Quoted 4 lines

I think the best approach for us might be to just track what is causing the transition failures, and send in trivial patches to make the outer loop in such kernel threads do the same KLP transition the idle task already does.

I am afraid that is a way to hell. We might end up in doing really crazy things if we want to complete the transition in one minute.

Now I wonder if we could just hook up a preempt notifier for kernel live patches. All the distro kernels already need the preempt notifier for KVM, anyway...

Is it crazy? Maybe a little.

Is it self contained, and something that could be done without inserting any extra code into a hot path while not in the middle of a KLP transition? Yes.

I'd be happy to come up with a patch that does that, unless anybody has good reasons I should not :)

Quoted 5 lines · Rik van Riel

Quoted 1 line

Now I wonder if we could just hook up a preempt notifier for kernel live patches. All the distro kernels already need the preempt notifier for KVM, anyway...

I wouldn't be opposed to that, but how does it solve this problem? If
as Peter said cond_resched() can be a NOP, then preemption would have to
be from an interrupt, in which case frame pointers aren't reliable.

Quoted 7 lines · Josh Poimboeuf

Quoted 5 lines · Rik van Riel

Quoted 1 line

Now I wonder if we could just hook up a preempt notifier for kernel live patches. All the distro kernels already need the preempt notifier for KVM, anyway...

Quoted 6 lines

I wouldn't be opposed to that, but how does it solve this problem? If
as Peter said cond_resched() can be a NOP, then preemption would have to
be from an interrupt, in which case frame pointers aren't reliable.

The systems where we are seeing problems do not, as far as I know, throw softlockup errors, so the kworker threads that fail to transition to the new KLP version are sleeping and getting scheduled out at times.

A KLP transition preempt notifier would help those kernel threads transition to the new KLP version at any time they reschedule.

How much it will help is hard to predict, but I should be able to get results from a fairly large sample size of systems within a few weeks :)

Quoted 14 lines · Rik van Riel

Quoted 13 lines · Josh Poimboeuf

Quoted 5 lines · Rik van Riel

Quoted 1 line

Now I wonder if we could just hook up a preempt notifier for kernel live patches. All the distro kernels already need the preempt notifier for KVM, anyway...

I wouldn't be opposed to that, but how does it solve this problem? If
as Peter said cond_resched() can be a NOP, then preemption would have to
be from an interrupt, in which case frame pointers aren't reliable.

Quoted 4 lines

The systems where we are seeing problems do not, as far as I know, throw softlockup errors, so the kworker threads that fail to transition to the new KLP version are sleeping and getting scheduled out at times.

Are they sleeping due to an explicit call to cond_resched()?

Quoted 4 lines

A KLP transition preempt notifier would help those kernel threads transition to the new KLP version at any time they reschedule.

... unless cond_resched() is a no-op due to CONFIG_PREEMPT?

Quoted 4 lines

How much it will help is hard to predict, but I should be able to get results from a fairly large sample size of systems within a few weeks :)

As Peter said, keep in mind that we will need to fix other cases beyond Facebook, i.e., CONFIG_PREEMPT combined with non-x86 arches which don't have ORC so they can't reliably unwind from an IRQ.

Quoted 4 lines

Quoted 3 lines

A KLP transition preempt notifier would help those kernel threads transition to the new KLP version at any time they reschedule.

Quoted 1 line

... unless cond_resched() is a no-op due to CONFIG_PREEMPT?

Based on my understanding (and a few other folks we chatted with), a kernel thread can legally run for extended time, as long as it calls cond_resched() at a reasonable frequency. Therefore, I think we should be able to patch such thread easily, unless it calls cond_resched() with being-patched function in the stack, of course.

OTOH, Petr's mindset of allowing many minutes for the patch transition is new to me. I need to think more about it. Josh, what’s you opinion on this? IIUC, kpatch is designed to only wait up to 60 seconds (no option to overwrite the time).

Quoted 7 lines

Quoted 3 lines

How much it will help is hard to predict, but I should be able to get results from a fairly large sample size of systems within a few weeks :)

As Peter said, keep in mind that we will need to fix other cases beyond Facebook, i.e., CONFIG_PREEMPT combined with non-x86 arches which don't have ORC so they can't reliably unwind from an IRQ.

I think livepatch transition may fail in different cases, and we don't need to address all of them in one shoot. Fixing some cases is an improvement as long as we don't slow down other cases. I understand that adding tiny overhead to __cond_resched() may end up as a visible regression. But maybe adding it to preempt_schedule_common() is light enough?

Did I miss/misunderstand something?

Quoted 4 lines · Song Liu

Quoted 4 lines

Quoted 3 lines

A KLP transition preempt notifier would help those kernel threads transition to the new KLP version at any time they reschedule.

Quoted 8 lines

Quoted 1 line

... unless cond_resched() is a no-op due to CONFIG_PREEMPT?

Based on my understanding (and a few other folks we chatted with), a kernel thread can legally run for extended time, as long as it calls cond_resched() at a reasonable frequency. Therefore, I think we should be able to patch such thread easily, unless it calls cond_resched() with being-patched function in the stack, of course.

But again, with CONFIG_PREEMPT, that doesn't work.

Quoted 4 lines

OTOH, Petr's mindset of allowing many minutes for the patch transition is new to me. I need to think more about it. Josh, what’s you opinion on this? IIUC, kpatch is designed to only wait up to 60 seconds (no option to overwrite the time).

I wouldn't be necessarily opposed to changing the kpatch timeout to something bigger, or eliminating it altogether in favor of a WARN() after x minutes.

Quoted 15 lines

Quoted 7 lines

Quoted 3 lines

How much it will help is hard to predict, but I should be able to get results from a fairly large sample size of systems within a few weeks :)

As Peter said, keep in mind that we will need to fix other cases beyond Facebook, i.e., CONFIG_PREEMPT combined with non-x86 arches which don't have ORC so they can't reliably unwind from an IRQ.

I think livepatch transition may fail in different cases, and we don't need to address all of them in one shoot. Fixing some cases is an improvement as long as we don't slow down other cases. I understand that adding tiny overhead to __cond_resched() may end up as a visible regression. But maybe adding it to preempt_schedule_common() is light enough?

Quoted 1 line

Did I miss/misunderstand something?

If it's a real bug, we should fix it everywhere, not just for Facebook. Otherwise CONFIG_PREEMPT and/or non-x86 arches become second-class citizens.

Quoted 3 lines

If it's a real bug, we should fix it everywhere, not just for Facebook. Otherwise CONFIG_PREEMPT and/or non-x86 arches become second-class citizens.

I think "is it a real bug?" is the top question for me. So maybe we should take a step back.

The behavior we see is: A busy kernel thread blocks klp transition for more than a minute. But the transition eventually succeeded after < 10 retries on most systems. The kernel thread is well-behaved, as it calls cond_resched() at a reasonable frequency, so this is not a deadlock.

If I understand Petr correctly, this behavior is expected, and thus is not a bug or issue for the livepatch subsystem. This is different to our original expectation, but if this is what we agree on, we will look into ways to incorporate long wait time for patch transition in our automations.

OTOH, if we think this is a suboptimal behavior (not necessarily a bug, IIUC), we should consider improve it. I personally don’t think shipping an improvement to one configuration makes other configurations second-class citizens. And, it is possible PREEMPT kernel does NOT even have such suboptimal behavior, right? (I really don't know.)

So, if we come back to the same question: is this a bug (or a suboptimal behavior that worth fixing)? If so, we are open to any solution that would also help PREEMPT and/or non-x86 arches.

Lastly, maybe a really naive question: does the following also helps PREEMPT=y configurations?

Quoted 13 lines · Song Liu

Quoted 3 lines

If it's a real bug, we should fix it everywhere, not just for Facebook. Otherwise CONFIG_PREEMPT and/or non-x86 arches become second-class citizens.

I think "is it a real bug?" is the top question for me. So maybe we should take a step back.

The behavior we see is: A busy kernel thread blocks klp transition for more than a minute. But the transition eventually succeeded after < 10 retries on most systems. The kernel thread is well-behaved, as it calls cond_resched() at a reasonable frequency, so this is not a deadlock.

Quoted 5 lines

If I understand Petr correctly, this behavior is expected, and thus is not a bug or issue for the livepatch subsystem. This is different to our original expectation, but if this is what we agree on, we will look into ways to incorporate long wait time for patch transition in our automations.

That's how we've traditionally looked at it, though apparently Red Hat and SUSE have implemented different ideas of what a long wait time is.

In practice, one minute has always been enough for all of kpatch's users -- AFAIK, everybody except SUSE -- up until now.

Though, these options might be considered workarounds, as it's theoretically possible for a kthread to be CPU-bound indefinitely, beyond any arbitrarily chosen timeout. But maybe that's not realistic beyond a certain timeout value of X and we don't care? I dunno.

As I have been trying to say, that won't work for PREEMPT+!ORC, because, when the kthread gets preempted, the stack trace will be attempted from an IRQ and will be reported as unreliable.

Ideally we'd have the ORC unwinder for all arches, that would make this much easier. But we're not there yet.

Quoted 20 lines · Josh Poimboeuf

Quoted 18 lines · Song Liu

Quoted 3 lines

If it's a real bug, we should fix it everywhere, not just for Facebook. Otherwise CONFIG_PREEMPT and/or non-x86 arches become second-class citizens.

I think "is it a real bug?" is the top question for me. So maybe we should take a step back.

The behavior we see is: A busy kernel thread blocks klp transition for more than a minute. But the transition eventually succeeded after < 10 retries on most systems. The kernel thread is well-behaved, as it calls cond_resched() at a reasonable frequency, so this is not a deadlock.

If I understand Petr correctly, this behavior is expected, and thus is not a bug or issue for the livepatch subsystem. This is different to our original expectation, but if this is what we agree on, we will look into ways to incorporate long wait time for patch transition in our automations.

Quoted 5 lines

That's how we've traditionally looked at it, though apparently Red Hat and SUSE have implemented different ideas of what a long wait time is.

In practice, one minute has always been enough for all of kpatch's users -- AFAIK, everybody except SUSE -- up until now.

I am actually surprised that nobody met the problem yet. There are "only" 60 attempts to transition the pending tasks.

Well, the problem is mainly with kthreads. User space processes are migrated also on the kernel boundary. And the fake signal is likely pretty effective here. And it probably is not that common that a kthread would occupy a single CPU all the time.

Quoted 4 lines

Though, these options might be considered workarounds, as it's theoretically possible for a kthread to be CPU-bound indefinitely, beyond any arbitrarily chosen timeout. But maybe that's not realistic beyond a certain timeout value of X and we don't care? I dunno.

I agree that it might happen theoretically. And it would be great to be prepared for this. My only concern is the complexity and risk. We should know that it is worth it.

Quoted 3 lines

As I have been trying to say, that won't work for PREEMPT+!ORC, because, when the kthread gets preempted, the stack trace will be attempted from an IRQ and will be reported as unreliable.

This limits the range of possible solutions quite a lot. But it is how it is.

Quoted 2 lines

Ideally we'd have the ORC unwinder for all arches, that would make this much easier. But we're not there yet.

The alternative solution is that the process has to migrate itself on some safe location.

One crazy idea. It still might be possible to find the called functions on the stack even when it is not reliable. Then it might be possible to add another ftrace handler on these found functions. This other ftrace handler might migrate the task when it calls this function again.

It assumes that the task will call the same functions again and again. Also it might require that the tasks checks its own stack from the ftrace handler. I am not sure if this is possible.

There might be other variants of this approach.

Quoted 17 lines

Quoted 2 lines

Ideally we'd have the ORC unwinder for all arches, that would make this much easier. But we're not there yet.

The alternative solution is that the process has to migrate itself on some safe location.

One crazy idea. It still might be possible to find the called functions on the stack even when it is not reliable. Then it might be possible to add another ftrace handler on these found functions. This other ftrace handler might migrate the task when it calls this function again.

It assumes that the task will call the same functions again and again. Also it might require that the tasks checks its own stack from the ftrace handler. I am not sure if this is possible.

Quoted 1 line

There might be other variants of this approach.

This might be the ultimate solution! As ftrace allows filtering based on pid (/sys/kernel/tracing/set_ftrace_pid), we can technically trigger klp_try_switch_task() on every function of the pending tasks. If this works, we should finish most of the transition in seconds. And the only failure there would be threads with being patched function at the very sottom of its stack. Am I too optimistic here?

Quoted 31 lines · Petr Mladek

Quoted 25 lines · Josh Poimboeuf

Quoted 13 lines · Song Liu

Quoted 3 lines

If it's a real bug, we should fix it everywhere, not just for Facebook. Otherwise CONFIG_PREEMPT and/or non-x86 arches become second-class citizens.

I think "is it a real bug?" is the top question for me. So maybe we should take a step back.

The behavior we see is: A busy kernel thread blocks klp transition for more than a minute. But the transition eventually succeeded after < 10 retries on most systems. The kernel thread is well-behaved, as it calls cond_resched() at a reasonable frequency, so this is not a deadlock.

Quoted 5 lines

If I understand Petr correctly, this behavior is expected, and thus is not a bug or issue for the livepatch subsystem. This is different to our original expectation, but if this is what we agree on, we will look into ways to incorporate long wait time for patch transition in our automations.

That's how we've traditionally looked at it, though apparently Red Hat and SUSE have implemented different ideas of what a long wait time is.

In practice, one minute has always been enough for all of kpatch's users -- AFAIK, everybody except SUSE -- up until now.

Quoted 2 lines

I am actually surprised that nobody met the problem yet. There are "only" 60 attempts to transition the pending tasks.

Maybe we should consider increase the frequency we try? Say to 10 times per second? I guess this will solve most of the failures we are seeing in current case.

My concern is that klp_try_complete_transition() checks all processes under read_lock(&tasklist_lock). It might create some contention on this lock. I am not sure if this lock is fair. It might slow down block writers (creating/deleting tasks).

So, if we come back to the same question: is this a bug (or a suboptimal
behavior that worth fixing)? If so, we are open to any solution that would also help PREEMPT and/or non-x86 arches.

Lastly, maybe a really naive question: does the following also helps PREEMPT=y configurations?

 static void __sched notrace preempt_schedule_common(void) {
+       if (unlikely(klp_patch_pending(current))) +               klp_try_switch_task(current);
+
        do {

Quoted 6 lines · Song Liu

So, if we come back to the same question: is this a bug (or a suboptimal
behavior that worth fixing)? If so, we are open to any solution that would also help PREEMPT and/or non-x86 arches.

Using the preempt notifiers during KLP transition should work equally well for PREEMPT and !PREEMPT. It also does not insert any additional code into the scheduler while there is no KLP transition going on.

Quoted 8 lines · Rik van Riel

Quoted 7 lines · Song Liu

So, if we come back to the same question: is this a bug (or a suboptimal
behavior that worth fixing)? If so, we are open to any solution that
would also help PREEMPT and/or non-x86 arches.

Quoted 4 lines

Using the preempt notifiers during KLP transition should work equally well for PREEMPT and !PREEMPT. It also does not insert any additional code into the scheduler while there is no KLP transition going on.

As I've been saying, this is not going to work for PREEMPT because, without ORC, we can't reliably unwind from an IRQ handler, so the kthread won't get patched.

Quoted 14 lines · Josh Poimboeuf

Quoted 12 lines · Rik van Riel

Quoted 7 lines · Song Liu

So, if we come back to the same question: is this a bug (or a suboptimal
behavior that worth fixing)? If so, we are open to any solution that
would also help PREEMPT and/or non-x86 arches.

Using the preempt notifiers during KLP transition should work equally well for PREEMPT and !PREEMPT. It also does not insert any additional code into the scheduler while there is no KLP transition going on.

Quoted 4 lines

As I've been saying, this is not going to work for PREEMPT because, without ORC, we can't reliably unwind from an IRQ handler, so the kthread won't get patched.

Isn't the sched_out preempt notifier always run in process context?

What am I missing?

Quoted 24 lines · Rik van Riel

Quoted 15 lines · Josh Poimboeuf

Quoted 9 lines · Rik van Riel

Quoted 8 lines · Song Liu

So, if we come back to the same question: is this a bug (or a suboptimal
behavior that worth fixing)? If so, we are open to any solution
that
would also help PREEMPT and/or non-x86 arches.

Quoted 4 lines

Using the preempt notifiers during KLP transition should work equally well for PREEMPT and !PREEMPT. It also does not insert any additional code into the scheduler while there is no KLP transition going on.

Quoted 5 lines

As I've been saying, this is not going to work for PREEMPT because,
without ORC, we can't reliably unwind from an IRQ handler, so the kthread won't get patched.

Isn't the sched_out preempt notifier always run in process context?

Quoted 1 line

What am I missing?

Maybe it's technically process context at that point.  But the important
point is that the call to the scheduler via preempt_schedule_irq() originates from the "return from interrupt" path.

Quoted 25 lines · Josh Poimboeuf

Quoted 24 lines · Rik van Riel

Quoted 15 lines · Josh Poimboeuf

Quoted 9 lines · Rik van Riel

Quoted 8 lines · Song Liu

So, if we come back to the same question: is this a bug (or a suboptimal
behavior that worth fixing)? If so, we are open to any solution
that
would also help PREEMPT and/or non-x86 arches.

Quoted 4 lines

Using the preempt notifiers during KLP transition should work equally well for PREEMPT and !PREEMPT. It also does not insert any additional code into the scheduler while there is no KLP transition going on.

Quoted 5 lines

As I've been saying, this is not going to work for PREEMPT because,
without ORC, we can't reliably unwind from an IRQ handler, so the kthread won't get patched.

Isn't the sched_out preempt notifier always run in process context?

Quoted 6 lines

Quoted 1 line

What am I missing?

Maybe it's technically process context at that point.  But the important
point is that the call to the scheduler via preempt_schedule_irq() originates from the "return from interrupt" path.

Ahhhh, I think I understand.

Does that mean if the scheduling of the kernel thread originated from an IRQ, the KLP transition will fail probably?

However, if the call to schedule came from a voluntary preemption, for example through a cond_resched() or due to the thread going to sleep a little bit, the stack walk will be reliable, and the KLP transition may succeed?

After talking with Peter, this seems like it might be a potential approach to fix the issue for kernels both with PREEMPT enabled and disabled.

If this looks like a reasonable approach to people, we can run experiments with this patch on a few thousand systems, and compare it with the kernel live patch transition latencies (and number of failures) on a kernel without that patch.

Does this look like an approach that could work?

---8<---
sched,livepatch: call stop_one_cpu in klp_check_and_switch_task

If a running task fails to transition to the new kernel live patch after the first attempt, use the stopper thread to preempt it during subsequent attempts at switching to the new kernel live patch.

<INSERT EXPERIMENTAL RESULTS HERE>

Signed-off-by Rik van Riel <[email protected]>

kernel/livepatch/transition.c +9 -0

index 5d03a2ad1066..26e9e5f09822 100644
@@ -9,6 +9,7 @@
 
 #include <linux/cpu.h>
 #include <linux/stacktrace.h>
+#include <linux/stop_machine.h>
 #include "core.h"
 #include "patch.h"
 #include "transition.h"

@@ -281,6 +282,11 @@ static int klp_check_and_switch_task(struct task_struct *task, void *arg)
 	return 0;
 }
 
+static int kpatch_dummy_fn(void *dummy)
+{
+	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

@@ -315,6 +321,9 @@ static bool klp_try_switch_task(struct task_struct *task)
 	case -EBUSY:	/* klp_check_and_switch_task() */
 		pr_debug("%s: %s:%d is running\n",
 			 __func__, task->comm, task->pid);
+		/* Preempt the task from the second KLP switch attempt. */
+		if (klp_signals_cnt)
+			stop_one_cpu(task_cpu(task), kpatch_dummy_fn, NULL);
 		break;
 	case -EINVAL:	/* klp_check_and_switch_task() */
 		pr_debug("%s: %s:%d has an unreliable stack\n",

Does this look like an approach that could work?

---8<---
sched,livepatch: call stop_one_cpu in klp_check_and_switch_task

If a running task fails to transition to the new kernel live patch after the first attempt, use the stopper thread to preempt it during subsequent attempts at switching to the new kernel live patch.

<INSERT EXPERIMENTAL RESULTS HERE>

+static int kpatch_dummy_fn(void *dummy)

kernel/livepatch/transition.c +4 -0

+{
+	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

Quoted 4 lines · Rik van Riel

Does this look like an approach that could work?

(quoted diff) +0 -0

@@ -315,6 +321,9 @@ static bool klp_try_switch_task(struct
 task_struct *task)

Quoted 8 lines

        case -EBUSY:    /* klp_check_and_switch_task() */ pr_debug("%s: %s:%d is running\n", __func__, task->comm, task->pid); +               /* Preempt the task from the second KLP switch attempt. */
+               if (klp_signals_cnt)
+                       stop_one_cpu(task_cpu(task), kpatch_dummy_fn, NULL);

I must be missing something, how is briefly preempting a kthread supposed to actually transition it?  Won't it likely go back to running
on the CPU before the next periodic klp_transition_work_fn() check?

Quoted 19 lines · Josh Poimboeuf

Quoted 4 lines · Rik van Riel

Does this look like an approach that could work?

(quoted diff) +0 -0

@@ -315,6 +321,9 @@ static bool klp_try_switch_task(struct
 task_struct *task)

Quoted 8 lines

        case -EBUSY:    /* klp_check_and_switch_task() */ pr_debug("%s: %s:%d is running\n", __func__, task->comm, task->pid); +               /* Preempt the task from the second KLP switch attempt. */
+               if (klp_signals_cnt)
+                       stop_one_cpu(task_cpu(task), kpatch_dummy_fn, NULL);

I must be missing something, how is briefly preempting a kthread supposed to actually transition it?  Won't it likely go back to running
on the CPU before the next periodic klp_transition_work_fn() check?

That's the kind of feedback I was hoping for ;)

I looked around the code a little bit, and it seems that only the idle tasks can transition to another KLP while they are running?

That makes me wonder how the kworker thread that runs the klp switching code transitions itself...

Should kernel threads that can use a lot of CPU have something in their outer loop to transition KLPs, just like the idle task does?

Quoted 3 lines · Rik van Riel

Should kernel threads that can use a lot of CPU have something in their outer loop to transition KLPs, just like the idle task does?

Maybe - I suppose this is the first time we've had an issue with CPU-bound kthreads.  I didn't know that was a thing ;-)

Quoted 7 lines · Josh Poimboeuf

Quoted 3 lines · Rik van Riel

Should kernel threads that can use a lot of CPU have something in their outer loop to transition KLPs, just like the idle task does?

Quoted 3 lines

Maybe - I suppose this is the first time we've had an issue with CPU-bound kthreads.  I didn't know that was a thing ;-)

Kworkers have as much work as you want them to do, and with things like btrfs compression that can be quite a bit.

Quoted 11 lines · Rik van Riel

Quoted 10 lines · Josh Poimboeuf

Quoted 3 lines · Rik van Riel

Should kernel threads that can use a lot of CPU have something in their outer loop to transition KLPs, just like the idle task does?

Maybe - I suppose this is the first time we've had an issue with CPU-bound kthreads. I didn't know that was a thing ;-)

Quoted 2 lines

Kworkers have as much work as you want them to do, and with things like btrfs compression that can be quite a bit.

To prevent patching, it would need to be some kind of sustained CPU activity, rather than a burst. I guess we haven't seen that show up as a real-world problem until now.

If you're able to identify which kthreads would be problematic, then yeah, defining a "transition point" in their outer loops could be an option.

We could look also at a more general approach, like stack checking from an irq handler. But as Petr alluded to, that would be problematic for CONFIG_FRAME_POINTER.

We could maybe deprecate frame pointers on x86 for live patching, but I think other arches would have a similar problem unless they were to do something like the ORC unwinder.

cond_sched() feels like a natural “transition point” to me, and it

To prevent patching, it would need to be some kind of sustained CPU activity, rather than a burst.  I guess we haven't seen that show up as
a real-world problem until now.

If you're able to identify which kthreads would be problematic, then yeah, defining a "transition point" in their outer loops could be an option.