[PATCH 0/2] x86/entry: Tiny idtentry_exit_cond_resched() cleanup

[PATCH 0/2] x86/entry: Tiny idtentry_exit_cond_resched() cleanup

[Sebastian Andrzej Siewior]

Tiny two patch series against idtentry_exit_cond_resched() after I
noticed it.

Sebastian

[PATCH 1/2] x86/entry: Use should_resched() in idtentry_exit_cond_resched()

[Sebastian Andrzej Siewior]

The TIF_NEED_RESCHED bit is inlined on x86 into the preemption counter.
So instead checking the preemption counter against zero via
preempt_count() and later checking the TIF_NEED_RESCHED bit via
need_resched() we could use should_resched() which does both checks in
one go.
The functional difference is that we don't enter the if statement with
preempt_count == 0 and TIF_NEED_RESCHED not set.

Use should_resched() instead need_resched() + preempt_count().

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 arch/x86/entry/common.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
index bd3f14175193c..212382f61b8e4 100644
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c
@@ -612,13 +612,12 @@ bool noinstr idtentry_enter_cond_rcu(struct pt_regs *regs)
 
 static void idtentry_exit_cond_resched(struct pt_regs *regs, bool may_sched)
 {
-	if (may_sched && !preempt_count()) {
+	if (may_sched && should_resched(0)) {
 		/* Sanity check RCU and thread stack */
 		rcu_irq_exit_check_preempt();
 		if (IS_ENABLED(CONFIG_DEBUG_ENTRY))
 			WARN_ON_ONCE(!on_thread_stack());
-		if (need_resched())
-			preempt_schedule_irq();
+		preempt_schedule_irq();
 	}
 	/* Covers both tracing and lockdep */
 	trace_hardirqs_on();
-- 
2.27.0

Re: [PATCH 1/2] x86/entry: Use should_resched() in idtentry_exit_cond_resched()

[Peter Zijlstra]

On Tue, Jun 30, 2020 at 12:22:08PM +0200, Sebastian Andrzej Siewior wrote:
> The TIF_NEED_RESCHED bit is inlined on x86 into the preemption counter.
> So instead checking the preemption counter against zero via
> preempt_count() and later checking the TIF_NEED_RESCHED bit via
> need_resched() we could use should_resched() which does both checks in
> one go.
> The functional difference is that we don't enter the if statement with
> preempt_count == 0 and TIF_NEED_RESCHED not set.
> 
> Use should_resched() instead need_resched() + preempt_count().
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  arch/x86/entry/common.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
> index bd3f14175193c..212382f61b8e4 100644
> --- a/arch/x86/entry/common.c
> +++ b/arch/x86/entry/common.c
> @@ -612,13 +612,12 @@ bool noinstr idtentry_enter_cond_rcu(struct pt_regs *regs)
>  
>  static void idtentry_exit_cond_resched(struct pt_regs *regs, bool may_sched)
>  {
> -	if (may_sched && !preempt_count()) {
> +	if (may_sched && should_resched(0)) {
>  		/* Sanity check RCU and thread stack */
>  		rcu_irq_exit_check_preempt();
>  		if (IS_ENABLED(CONFIG_DEBUG_ENTRY))
>  			WARN_ON_ONCE(!on_thread_stack());

This was done on purpose, your change avoids hitting this WARN.

The thing is, if we could preempt (but not nessecarily have to) we want
to validate we're on the thread stack.

> -		if (need_resched())
> -			preempt_schedule_irq();
> +		preempt_schedule_irq();
>  	}
>  	/* Covers both tracing and lockdep */
>  	trace_hardirqs_on();
> -- 
> 2.27.0
> 

Re: [PATCH 1/2] x86/entry: Use should_resched() in idtentry_exit_cond_resched()

[Sebastian Andrzej Siewior]

On 2020-06-30 13:10:12 [+0200], Peter Zijlstra wrote:
> > --- a/arch/x86/entry/common.c
> > +++ b/arch/x86/entry/common.c
> > @@ -612,13 +612,12 @@ bool noinstr idtentry_enter_cond_rcu(struct pt_regs *regs)
> >  
> >  static void idtentry_exit_cond_resched(struct pt_regs *regs, bool may_sched)
> >  {
> > -	if (may_sched && !preempt_count()) {
> > +	if (may_sched && should_resched(0)) {
> >  		/* Sanity check RCU and thread stack */
> >  		rcu_irq_exit_check_preempt();
> >  		if (IS_ENABLED(CONFIG_DEBUG_ENTRY))
> >  			WARN_ON_ONCE(!on_thread_stack());
> 
> This was done on purpose, your change avoids hitting this WARN.
> 
> The thing is, if we could preempt (but not nessecarily have to) we want
> to validate we're on the thread stack.

Okay then.

Sebastian

[PATCH v2] x86/entry: Use should_resched() in idtentry_exit_cond_resched()

[Sebastian Andrzej Siewior]

The TIF_NEED_RESCHED bit is inlined on x86 into the preemption counter.
By using should_resched(0) instead of need_resched() the same check can
be performed which uses the same variable as 'preempt_count()` which was
issued before.

Use should_resched(0) instead need_resched().

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v1…v2: Use should_resched() but keep the !preempt_count() check.

 arch/x86/entry/common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
index bd3f14175193c..0bb6966f62738 100644
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c
@@ -615,11 +615,11 @@ static void idtentry_exit_cond_resched(struct pt_regs *regs, bool may_sched)
 	if (may_sched && !preempt_count()) {
 		/* Sanity check RCU and thread stack */
 		rcu_irq_exit_check_preempt();
 		if (IS_ENABLED(CONFIG_DEBUG_ENTRY))
 			WARN_ON_ONCE(!on_thread_stack());
-		if (need_resched())
+		if (should_resched(0))
 			preempt_schedule_irq();
 	}
 	/* Covers both tracing and lockdep */
 	trace_hardirqs_on();
 }
-- 
2.27.0

[PATCH 2/2] x86/entry: Remove `regs' parameter from idtentry_exit_cond_resched()

[Sebastian Andrzej Siewior]

The `regs' parameter is not used in idtentry_exit_cond_resched() and can
be removed.

Remove the `regs' parameter.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 arch/x86/entry/common.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
index 212382f61b8e4..652dce37643b4 100644
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c
@@ -610,7 +610,7 @@ bool noinstr idtentry_enter_cond_rcu(struct pt_regs *regs)
 	return false;
 }
 
-static void idtentry_exit_cond_resched(struct pt_regs *regs, bool may_sched)
+static void idtentry_exit_cond_resched(bool may_sched)
 {
 	if (may_sched && should_resched(0)) {
 		/* Sanity check RCU and thread stack */
@@ -664,7 +664,7 @@ void noinstr idtentry_exit_cond_rcu(struct pt_regs *regs, bool rcu_exit)
 		}
 
 		instrumentation_begin();
-		idtentry_exit_cond_resched(regs, IS_ENABLED(CONFIG_PREEMPTION));
+		idtentry_exit_cond_resched(IS_ENABLED(CONFIG_PREEMPTION));
 		instrumentation_end();
 	} else {
 		/*
@@ -770,7 +770,7 @@ __visible noinstr void xen_pv_evtchn_do_upcall(struct pt_regs *regs)
 	inhcall = get_and_clear_inhcall();
 	if (inhcall && !WARN_ON_ONCE(rcu_exit)) {
 		instrumentation_begin();
-		idtentry_exit_cond_resched(regs, true);
+		idtentry_exit_cond_resched(true);
 		instrumentation_end();
 		restore_inhcall(inhcall);
 	} else {
-- 
2.27.0

Re: [PATCH 2/2] x86/entry: Remove `regs' parameter from idtentry_exit_cond_resched()

[Peter Zijlstra]

On Tue, Jun 30, 2020 at 12:22:09PM +0200, Sebastian Andrzej Siewior wrote:
> The `regs' parameter is not used in idtentry_exit_cond_resched() and can
> be removed.
> 
> Remove the `regs' parameter.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  arch/x86/entry/common.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
> index 212382f61b8e4..652dce37643b4 100644
> --- a/arch/x86/entry/common.c
> +++ b/arch/x86/entry/common.c
> @@ -610,7 +610,7 @@ bool noinstr idtentry_enter_cond_rcu(struct pt_regs *regs)
>  	return false;
>  }
>  
> -static void idtentry_exit_cond_resched(struct pt_regs *regs, bool may_sched)
> +static void idtentry_exit_cond_resched(bool may_sched)

This is a style question; so far all idtentry_*() functions have regs
passed. In the lockdep-vs-nmi series I introduce
idtentry_{enter,exit}_nmi() both having an unused regs argument.

Not sure which way around I feel on this.

Re: [PATCH 2/2] x86/entry: Remove `regs' parameter from idtentry_exit_cond_resched()

[Sebastian Andrzej Siewior]

On 2020-06-30 13:14:30 [+0200], Peter Zijlstra wrote:
> > -static void idtentry_exit_cond_resched(struct pt_regs *regs, bool may_sched)
> > +static void idtentry_exit_cond_resched(bool may_sched)
> 
> This is a style question; so far all idtentry_*() functions have regs
> passed. In the lockdep-vs-nmi series I introduce
> idtentry_{enter,exit}_nmi() both having an unused regs argument.
> 
> Not sure which way around I feel on this.

Do as you are pleased. I was more attached to the previous patch that to
this one :)

Sebastian