diff mbox series

x86/alternatives: Add cond_resched() to text_poke_bp_batch()

Message ID 20230528084652.5f3b48f0@rorschach.local.home
State New
Headers
Series x86/alternatives: Add cond_resched() to text_poke_bp_batch() |

Commit Message

Steven Rostedt May 28, 2023, 12:46 p.m. UTC 
  From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

Debugging in the kernel has started slowing down the kernel by a
noticeable amount. The ftrace start up tests are triggering the softlockup
watchdog on some boxes. This is caused by the start up tests that enable
function and function graph tracing several times. Sprinkling
cond_resched() just in the start up test code was not enough to stop the
softlockup from triggering. It would sometimes trigger in the
text_poke_bp_batch() code.

The text_poke_bp_batch() is run in schedulable context. Add
cond_resched() between each phase (adding the int3, updating the code, and
removing the int3). This keeps the softlockup from triggering in the start
up tests.

Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 arch/x86/kernel/alternative.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

Comments

Masami Hiramatsu (Google) May 29, 2023, 2:52 a.m. UTC | #1 
On Sun, 28 May 2023 08:46:52 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> Debugging in the kernel has started slowing down the kernel by a
> noticeable amount. The ftrace start up tests are triggering the softlockup
> watchdog on some boxes. This is caused by the start up tests that enable
> function and function graph tracing several times. Sprinkling
> cond_resched() just in the start up test code was not enough to stop the
> softlockup from triggering. It would sometimes trigger in the
> text_poke_bp_batch() code.
> 
> The text_poke_bp_batch() is run in schedulable context. Add
> cond_resched() between each phase (adding the int3, updating the code, and
> removing the int3). This keeps the softlockup from triggering in the start
> up tests.
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
>  arch/x86/kernel/alternative.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> index f615e0cb6d93..e024eddd457f 100644
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -1953,6 +1953,14 @@ static void text_poke_bp_batch(struct text_poke_loc *tp, unsigned int nr_entries
>  	 */
>  	atomic_set_release(&bp_desc.refs, 1);
>  
> +	/*
> +	 * Function tracing can enable thousands of places that need to be
> +	 * updated. This can take quite some time, and with full kernel debugging
> +	 * enabled, this could cause the softlockup watchdog to trigger.
> +	 * Add cond_resched() calls to each phase.
> +	 */
> +	cond_resched();

Hmm, why don't you put this between the first step (put int3) and the
second step (put other bytes)? I guess those would takes more time.

Thank you,

> +
>  	/*
>  	 * Corresponding read barrier in int3 notifier for making sure the
>  	 * nr_entries and handler are correctly ordered wrt. patching.
> @@ -2030,6 +2038,7 @@ static void text_poke_bp_batch(struct text_poke_loc *tp, unsigned int nr_entries
>  		 * better safe than sorry (plus there's not only Intel).
>  		 */
>  		text_poke_sync();
> +		cond_resched();
>  	}
>  
>  	/*
> @@ -2049,8 +2058,10 @@ static void text_poke_bp_batch(struct text_poke_loc *tp, unsigned int nr_entries
>  		do_sync++;
>  	}
>  
> -	if (do_sync)
> +	if (do_sync) {
>  		text_poke_sync();
> +		cond_resched();
> +	}
>  
>  	/*
>  	 * Remove and wait for refs to be zero.
> -- 
> 2.39.2
>
Steven Rostedt May 29, 2023, 3:21 a.m. UTC | #2 
On Mon, 29 May 2023 11:52:46 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> > +	/*
> > +	 * Function tracing can enable thousands of places that need to be
> > +	 * updated. This can take quite some time, and with full kernel debugging
> > +	 * enabled, this could cause the softlockup watchdog to trigger.
> > +	 * Add cond_resched() calls to each phase.
> > +	 */
> > +	cond_resched();  
> 
> Hmm, why don't you put this between the first step (put int3) and the
> second step (put other bytes)? I guess those would takes more time.

Ah you're right. I still want this here to clear the 'need resched'
flag before we start the process, but I did miss one after the first
loop.

Thanks for the review!

-- Steve
Peter Zijlstra May 30, 2023, 12:01 p.m. UTC | #3 
On Sun, May 28, 2023 at 08:46:52AM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> Debugging in the kernel has started slowing down the kernel by a
> noticeable amount. The ftrace start up tests are triggering the softlockup
> watchdog on some boxes. This is caused by the start up tests that enable
> function and function graph tracing several times. Sprinkling
> cond_resched() just in the start up test code was not enough to stop the
> softlockup from triggering. It would sometimes trigger in the
> text_poke_bp_batch() code.
> 
> The text_poke_bp_batch() is run in schedulable context. Add
> cond_resched() between each phase (adding the int3, updating the code, and
> removing the int3). This keeps the softlockup from triggering in the start
> up tests.
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
>  arch/x86/kernel/alternative.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> index f615e0cb6d93..e024eddd457f 100644
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -1953,6 +1953,14 @@ static void text_poke_bp_batch(struct text_poke_loc *tp, unsigned int nr_entries
>  	 */
>  	atomic_set_release(&bp_desc.refs, 1);
>  
> +	/*
> +	 * Function tracing can enable thousands of places that need to be
> +	 * updated. This can take quite some time, and with full kernel debugging
> +	 * enabled, this could cause the softlockup watchdog to trigger.
> +	 * Add cond_resched() calls to each phase.
> +	 */
> +	cond_resched();

But but but... you can only have TP_VEC_MAX pokes queued, which is 256
on normal setups.

Please explain how this leads to problems and why you need _3_
reschedule points here.
Steven Rostedt May 30, 2023, 12:35 p.m. UTC | #4 
On Tue, 30 May 2023 14:01:48 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Sun, May 28, 2023 at 08:46:52AM -0400, Steven Rostedt wrote:
> > diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> > index f615e0cb6d93..e024eddd457f 100644
> > --- a/arch/x86/kernel/alternative.c
> > +++ b/arch/x86/kernel/alternative.c
> > @@ -1953,6 +1953,14 @@ static void text_poke_bp_batch(struct text_poke_loc *tp, unsigned int nr_entries
> >  	 */
> >  	atomic_set_release(&bp_desc.refs, 1);
> >  
> > +	/*
> > +	 * Function tracing can enable thousands of places that need to be
> > +	 * updated. This can take quite some time, and with full kernel debugging
> > +	 * enabled, this could cause the softlockup watchdog to trigger.
> > +	 * Add cond_resched() calls to each phase.
> > +	 */
> > +	cond_resched();  
> 
> But but but... you can only have TP_VEC_MAX pokes queued, which is 256
> on normal setups.
> 
> Please explain how this leads to problems and why you need _3_
> reschedule points here.

Maybe this was me being overly paranoid (and thinking, it doesn't hurt).

I could try it with just adding one (to make sure it happens every
flush), and see if it doesn't trigger the softlock up.

-- Steve
Steven Rostedt May 31, 2023, 9:08 a.m. UTC | #5 
On Tue, 30 May 2023 14:01:48 +0200
Peter Zijlstra <peterz@infradead.org> wrote:


> > diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> > index f615e0cb6d93..e024eddd457f 100644
> > --- a/arch/x86/kernel/alternative.c
> > +++ b/arch/x86/kernel/alternative.c
> > @@ -1953,6 +1953,14 @@ static void text_poke_bp_batch(struct text_poke_loc *tp, unsigned int nr_entries
> >  	 */
> >  	atomic_set_release(&bp_desc.refs, 1);
> >  
> > +	/*
> > +	 * Function tracing can enable thousands of places that need to be
> > +	 * updated. This can take quite some time, and with full kernel debugging
> > +	 * enabled, this could cause the softlockup watchdog to trigger.
> > +	 * Add cond_resched() calls to each phase.
> > +	 */
> > +	cond_resched();  
> 
> But but but... you can only have TP_VEC_MAX pokes queued, which is 256
> on normal setups.
> 
> Please explain how this leads to problems and why you need _3_
> reschedule points here.

I removed all but this first one and it works fine.

I'll resend with the update.

-- Steve
diff mbox series

Patch

diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index f615e0cb6d93..e024eddd457f 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -1953,6 +1953,14 @@  static void text_poke_bp_batch(struct text_poke_loc *tp, unsigned int nr_entries
 	 */
 	atomic_set_release(&bp_desc.refs, 1);
 
+	/*
+	 * Function tracing can enable thousands of places that need to be
+	 * updated. This can take quite some time, and with full kernel debugging
+	 * enabled, this could cause the softlockup watchdog to trigger.
+	 * Add cond_resched() calls to each phase.
+	 */
+	cond_resched();
+
 	/*
 	 * Corresponding read barrier in int3 notifier for making sure the
 	 * nr_entries and handler are correctly ordered wrt. patching.
@@ -2030,6 +2038,7 @@  static void text_poke_bp_batch(struct text_poke_loc *tp, unsigned int nr_entries
 		 * better safe than sorry (plus there's not only Intel).
 		 */
 		text_poke_sync();
+		cond_resched();
 	}
 
 	/*
@@ -2049,8 +2058,10 @@  static void text_poke_bp_batch(struct text_poke_loc *tp, unsigned int nr_entries
 		do_sync++;
 	}
 
-	if (do_sync)
+	if (do_sync) {
 		text_poke_sync();
+		cond_resched();
+	}
 
 	/*
 	 * Remove and wait for refs to be zero.