[RFC][PATCH 0/4] Fix up ACPI processor idle vs RCU

[RFC][PATCH 0/4] Fix up ACPI processor idle vs RCU

[Peter Zijlstra]

Hi,

A number of people have been tripping over the improved RCU-lockdep complaints
in idle.

These 4 patches attempt to cure ACPI processor idle. I've done my best to not
wreck things, but it's all magical code with very few comments, so who knows..

Boris tested an earlier version of these patches and they worked for his
32bit Atom board that was triggering complaints.

[RFC][PATCH 1/4] acpi: Use CPUIDLE_FLAG_TIMER_STOP

[Peter Zijlstra]

Make acpi_processor_idle use the common broadcast code, there's no
reason not to. This also removes some RCU usage after
rcu_idle_enter().

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 drivers/acpi/processor_idle.c |   49 +++++++++++++-----------------------------
 1 file changed, 16 insertions(+), 33 deletions(-)

--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -161,18 +161,10 @@ static void lapic_timer_propagate_broadc
 }
 
 /* Power(C) State timer broadcast control */
-static void lapic_timer_state_broadcast(struct acpi_processor *pr,
-				       struct acpi_processor_cx *cx,
-				       int broadcast)
-{
-	int state = cx - pr->power.states;
-
-	if (state >= pr->power.timer_broadcast_on_state) {
-		if (broadcast)
-			tick_broadcast_enter();
-		else
-			tick_broadcast_exit();
-	}
+static bool lapic_timer_needs_broadcast(struct acpi_processor *pr,
+					struct acpi_processor_cx *cx)
+{
+	return cx - pr->power.states >= pr->power.timer_broadcast_on_state;
 }
 
 #else
@@ -180,9 +172,9 @@ static void lapic_timer_state_broadcast(
 static void lapic_timer_check_state(int state, struct acpi_processor *pr,
 				   struct acpi_processor_cx *cstate) { }
 static void lapic_timer_propagate_broadcast(struct acpi_processor *pr) { }
-static void lapic_timer_state_broadcast(struct acpi_processor *pr,
-				       struct acpi_processor_cx *cx,
-				       int broadcast)
+
+static bool lapic_timer_needs_broadcast(struct acpi_processor *pr,
+					struct acpi_processor_cx *cx)
 {
 }
 
@@ -568,21 +560,13 @@ static DEFINE_RAW_SPINLOCK(c3_lock);
  * acpi_idle_enter_bm - enters C3 with proper BM handling
  * @pr: Target processor
  * @cx: Target state context
- * @timer_bc: Whether or not to change timer mode to broadcast
  */
 static void acpi_idle_enter_bm(struct acpi_processor *pr,
-			       struct acpi_processor_cx *cx, bool timer_bc)
+			       struct acpi_processor_cx *cx)
 {
 	acpi_unlazy_tlb(smp_processor_id());
 
 	/*
-	 * Must be done before busmaster disable as we might need to
-	 * access HPET !
-	 */
-	if (timer_bc)
-		lapic_timer_state_broadcast(pr, cx, 1);
-
-	/*
 	 * disable bus master
 	 * bm_check implies we need ARB_DIS
 	 * bm_control implies whether we can do ARB_DIS
@@ -609,9 +593,6 @@ static void acpi_idle_enter_bm(struct ac
 		c3_cpu_count--;
 		raw_spin_unlock(&c3_lock);
 	}
-
-	if (timer_bc)
-		lapic_timer_state_broadcast(pr, cx, 0);
 }
 
 static int acpi_idle_enter(struct cpuidle_device *dev,
@@ -630,7 +611,7 @@ static int acpi_idle_enter(struct cpuidl
 			cx = per_cpu(acpi_cstate[index], dev->cpu);
 		} else if (cx->type == ACPI_STATE_C3 && pr->flags.bm_check) {
 			if (cx->bm_sts_skip || !acpi_idle_bm_check()) {
-				acpi_idle_enter_bm(pr, cx, true);
+				acpi_idle_enter_bm(pr, cx);
 				return index;
 			} else if (drv->safe_state_index >= 0) {
 				index = drv->safe_state_index;
@@ -642,15 +623,11 @@ static int acpi_idle_enter(struct cpuidl
 		}
 	}
 
-	lapic_timer_state_broadcast(pr, cx, 1);
-
 	if (cx->type == ACPI_STATE_C3)
 		ACPI_FLUSH_CPU_CACHE();
 
 	acpi_idle_do_entry(cx);
 
-	lapic_timer_state_broadcast(pr, cx, 0);
-
 	return index;
 }
 
@@ -666,7 +643,7 @@ static int acpi_idle_enter_s2idle(struct
 			return 0;
 
 		if (pr->flags.bm_check) {
-			acpi_idle_enter_bm(pr, cx, false);
+			acpi_idle_enter_bm(pr, cx);
 			return 0;
 		} else {
 			ACPI_FLUSH_CPU_CACHE();
@@ -682,6 +659,7 @@ static int acpi_processor_setup_cpuidle_
 {
 	int i, count = ACPI_IDLE_STATE_START;
 	struct acpi_processor_cx *cx;
+	struct cpuidle_state *state;
 
 	if (max_cstate == 0)
 		max_cstate = 1;
@@ -694,6 +672,11 @@ static int acpi_processor_setup_cpuidle_
 
 		per_cpu(acpi_cstate[count], dev->cpu) = cx;
 
+		if (lapic_timer_needs_broadcast(pr, cx)) {
+			state = &acpi_idle_driver.states[count];
+			state->flags |= CPUIDLE_FLAG_TIMER_STOP;
+		}
+
 		count++;
 		if (count == CPUIDLE_STATE_MAX)
 			break;

Re: [RFC][PATCH 1/4] acpi: Use CPUIDLE_FLAG_TIMER_STOP

[Rafael J. Wysocki]

On Tue, Sep 15, 2020 at 12:44 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> Make acpi_processor_idle use the common broadcast code, there's no
> reason not to. This also removes some RCU usage after
> rcu_idle_enter().
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

The whole series looks good to me, so please feel free to add

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

to all of the four patches.

Alternatively, please let me know if you want me to take the patches.

> ---
>  drivers/acpi/processor_idle.c |   49 +++++++++++++-----------------------------
>  1 file changed, 16 insertions(+), 33 deletions(-)
>
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -161,18 +161,10 @@ static void lapic_timer_propagate_broadc
>  }
>
>  /* Power(C) State timer broadcast control */
> -static void lapic_timer_state_broadcast(struct acpi_processor *pr,
> -                                      struct acpi_processor_cx *cx,
> -                                      int broadcast)
> -{
> -       int state = cx - pr->power.states;
> -
> -       if (state >= pr->power.timer_broadcast_on_state) {
> -               if (broadcast)
> -                       tick_broadcast_enter();
> -               else
> -                       tick_broadcast_exit();
> -       }
> +static bool lapic_timer_needs_broadcast(struct acpi_processor *pr,
> +                                       struct acpi_processor_cx *cx)
> +{
> +       return cx - pr->power.states >= pr->power.timer_broadcast_on_state;
>  }
>
>  #else
> @@ -180,9 +172,9 @@ static void lapic_timer_state_broadcast(
>  static void lapic_timer_check_state(int state, struct acpi_processor *pr,
>                                    struct acpi_processor_cx *cstate) { }
>  static void lapic_timer_propagate_broadcast(struct acpi_processor *pr) { }
> -static void lapic_timer_state_broadcast(struct acpi_processor *pr,
> -                                      struct acpi_processor_cx *cx,
> -                                      int broadcast)
> +
> +static bool lapic_timer_needs_broadcast(struct acpi_processor *pr,
> +                                       struct acpi_processor_cx *cx)
>  {
>  }
>
> @@ -568,21 +560,13 @@ static DEFINE_RAW_SPINLOCK(c3_lock);
>   * acpi_idle_enter_bm - enters C3 with proper BM handling
>   * @pr: Target processor
>   * @cx: Target state context
> - * @timer_bc: Whether or not to change timer mode to broadcast
>   */
>  static void acpi_idle_enter_bm(struct acpi_processor *pr,
> -                              struct acpi_processor_cx *cx, bool timer_bc)
> +                              struct acpi_processor_cx *cx)
>  {
>         acpi_unlazy_tlb(smp_processor_id());
>
>         /*
> -        * Must be done before busmaster disable as we might need to
> -        * access HPET !
> -        */
> -       if (timer_bc)
> -               lapic_timer_state_broadcast(pr, cx, 1);
> -
> -       /*
>          * disable bus master
>          * bm_check implies we need ARB_DIS
>          * bm_control implies whether we can do ARB_DIS
> @@ -609,9 +593,6 @@ static void acpi_idle_enter_bm(struct ac
>                 c3_cpu_count--;
>                 raw_spin_unlock(&c3_lock);
>         }
> -
> -       if (timer_bc)
> -               lapic_timer_state_broadcast(pr, cx, 0);
>  }
>
>  static int acpi_idle_enter(struct cpuidle_device *dev,
> @@ -630,7 +611,7 @@ static int acpi_idle_enter(struct cpuidl
>                         cx = per_cpu(acpi_cstate[index], dev->cpu);
>                 } else if (cx->type == ACPI_STATE_C3 && pr->flags.bm_check) {
>                         if (cx->bm_sts_skip || !acpi_idle_bm_check()) {
> -                               acpi_idle_enter_bm(pr, cx, true);
> +                               acpi_idle_enter_bm(pr, cx);
>                                 return index;
>                         } else if (drv->safe_state_index >= 0) {
>                                 index = drv->safe_state_index;
> @@ -642,15 +623,11 @@ static int acpi_idle_enter(struct cpuidl
>                 }
>         }
>
> -       lapic_timer_state_broadcast(pr, cx, 1);
> -
>         if (cx->type == ACPI_STATE_C3)
>                 ACPI_FLUSH_CPU_CACHE();
>
>         acpi_idle_do_entry(cx);
>
> -       lapic_timer_state_broadcast(pr, cx, 0);
> -
>         return index;
>  }
>
> @@ -666,7 +643,7 @@ static int acpi_idle_enter_s2idle(struct
>                         return 0;
>
>                 if (pr->flags.bm_check) {
> -                       acpi_idle_enter_bm(pr, cx, false);
> +                       acpi_idle_enter_bm(pr, cx);
>                         return 0;
>                 } else {
>                         ACPI_FLUSH_CPU_CACHE();
> @@ -682,6 +659,7 @@ static int acpi_processor_setup_cpuidle_
>  {
>         int i, count = ACPI_IDLE_STATE_START;
>         struct acpi_processor_cx *cx;
> +       struct cpuidle_state *state;
>
>         if (max_cstate == 0)
>                 max_cstate = 1;
> @@ -694,6 +672,11 @@ static int acpi_processor_setup_cpuidle_
>
>                 per_cpu(acpi_cstate[count], dev->cpu) = cx;
>
> +               if (lapic_timer_needs_broadcast(pr, cx)) {
> +                       state = &acpi_idle_driver.states[count];
> +                       state->flags |= CPUIDLE_FLAG_TIMER_STOP;
> +               }
> +
>                 count++;
>                 if (count == CPUIDLE_STATE_MAX)
>                         break;
>
>

Re: [RFC][PATCH 1/4] acpi: Use CPUIDLE_FLAG_TIMER_STOP

[peterz]

On Tue, Sep 15, 2020 at 06:26:52PM +0200, Rafael J. Wysocki wrote:
> On Tue, Sep 15, 2020 at 12:44 PM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > Make acpi_processor_idle use the common broadcast code, there's no
> > reason not to. This also removes some RCU usage after
> > rcu_idle_enter().
> >
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> 
> The whole series looks good to me, so please feel free to add
> 
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> to all of the four patches.
> 
> Alternatively, please let me know if you want me to take the patches.

Feel free to take them. All the prerequisite borkage is in linus' tree
already.

Re: [RFC][PATCH 1/4] acpi: Use CPUIDLE_FLAG_TIMER_STOP

[Borislav Petkov]

On Wed, Sep 16, 2020 at 05:42:12PM +0200, peterz@infradead.org wrote:
> On Tue, Sep 15, 2020 at 06:26:52PM +0200, Rafael J. Wysocki wrote:
> > On Tue, Sep 15, 2020 at 12:44 PM Peter Zijlstra <peterz@infradead.org> wrote:
> > >
> > > Make acpi_processor_idle use the common broadcast code, there's no
> > > reason not to. This also removes some RCU usage after
> > > rcu_idle_enter().
> > >
> > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > 
> > The whole series looks good to me, so please feel free to add
> > 
> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > to all of the four patches.
> > 
> > Alternatively, please let me know if you want me to take the patches.
> 
> Feel free to take them. All the prerequisite borkage is in linus' tree
> already.

You can add:

Reported-by: Borislav Petkov <bp@suse.de>

for this one and for this whole series:

Tested-by: Borislav Petkov <bp@suse.de>

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [RFC][PATCH 1/4] acpi: Use CPUIDLE_FLAG_TIMER_STOP

[Rafael J. Wysocki]

On Wed, Sep 16, 2020 at 6:01 PM Borislav Petkov <bp@alien8.de> wrote:
>
> On Wed, Sep 16, 2020 at 05:42:12PM +0200, peterz@infradead.org wrote:
> > On Tue, Sep 15, 2020 at 06:26:52PM +0200, Rafael J. Wysocki wrote:
> > > On Tue, Sep 15, 2020 at 12:44 PM Peter Zijlstra <peterz@infradead.org> wrote:
> > > >
> > > > Make acpi_processor_idle use the common broadcast code, there's no
> > > > reason not to. This also removes some RCU usage after
> > > > rcu_idle_enter().
> > > >
> > > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > >
> > > The whole series looks good to me, so please feel free to add
> > >
> > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > to all of the four patches.
> > >
> > > Alternatively, please let me know if you want me to take the patches.
> >
> > Feel free to take them. All the prerequisite borkage is in linus' tree
> > already.
>
> You can add:
>
> Reported-by: Borislav Petkov <bp@suse.de>
>
> for this one and for this whole series:
>
> Tested-by: Borislav Petkov <bp@suse.de>

Done.

Re: [RFC][PATCH 1/4] acpi: Use CPUIDLE_FLAG_TIMER_STOP

[Rafael J. Wysocki]

On Wed, Sep 16, 2020 at 5:42 PM <peterz@infradead.org> wrote:
>
> On Tue, Sep 15, 2020 at 06:26:52PM +0200, Rafael J. Wysocki wrote:
> > On Tue, Sep 15, 2020 at 12:44 PM Peter Zijlstra <peterz@infradead.org> wrote:
> > >
> > > Make acpi_processor_idle use the common broadcast code, there's no
> > > reason not to. This also removes some RCU usage after
> > > rcu_idle_enter().
> > >
> > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> >
> > The whole series looks good to me, so please feel free to add
> >
> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > to all of the four patches.
> >
> > Alternatively, please let me know if you want me to take the patches.
>
> Feel free to take them. All the prerequisite borkage is in linus' tree
> already.

OK

All applied (with some minor edits in the subjects) for 5.9-rc6.

Thanks!

Re: [RFC][PATCH 1/4] acpi: Use CPUIDLE_FLAG_TIMER_STOP

[Guenter Roeck]

On Tue, Sep 15, 2020 at 12:31:58PM +0200, Peter Zijlstra wrote:
> Make acpi_processor_idle use the common broadcast code, there's no
> reason not to. This also removes some RCU usage after
> rcu_idle_enter().
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Reported-by: Borislav Petkov <bp@suse.de>
> Tested-by: Borislav Petkov <bp@suse.de>
> ---
>  drivers/acpi/processor_idle.c |   49 +++++++++++++-----------------------------
>  1 file changed, 16 insertions(+), 33 deletions(-)
> 
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -161,18 +161,10 @@ static void lapic_timer_propagate_broadc
>  }
>  
>  /* Power(C) State timer broadcast control */
> -static void lapic_timer_state_broadcast(struct acpi_processor *pr,
> -				       struct acpi_processor_cx *cx,
> -				       int broadcast)
> -{
> -	int state = cx - pr->power.states;
> -
> -	if (state >= pr->power.timer_broadcast_on_state) {
> -		if (broadcast)
> -			tick_broadcast_enter();
> -		else
> -			tick_broadcast_exit();
> -	}
> +static bool lapic_timer_needs_broadcast(struct acpi_processor *pr,
> +					struct acpi_processor_cx *cx)
> +{
> +	return cx - pr->power.states >= pr->power.timer_broadcast_on_state;
>  }
>  
>  #else
> @@ -180,9 +172,9 @@ static void lapic_timer_state_broadcast(
>  static void lapic_timer_check_state(int state, struct acpi_processor *pr,
>  				   struct acpi_processor_cx *cstate) { }
>  static void lapic_timer_propagate_broadcast(struct acpi_processor *pr) { }
> -static void lapic_timer_state_broadcast(struct acpi_processor *pr,
> -				       struct acpi_processor_cx *cx,
> -				       int broadcast)
> +
> +static bool lapic_timer_needs_broadcast(struct acpi_processor *pr,
> +					struct acpi_processor_cx *cx)
>  {
>  }

drivers/acpi/processor_idle.c: In function 'lapic_timer_needs_broadcast':
drivers/acpi/processor_idle.c:179:1: warning: no return statement in function returning non-void [-Wreturn-type]

Should this return true or false ?

Guenter

>  
> @@ -568,21 +560,13 @@ static DEFINE_RAW_SPINLOCK(c3_lock);
>   * acpi_idle_enter_bm - enters C3 with proper BM handling
>   * @pr: Target processor
>   * @cx: Target state context
> - * @timer_bc: Whether or not to change timer mode to broadcast
>   */
>  static void acpi_idle_enter_bm(struct acpi_processor *pr,
> -			       struct acpi_processor_cx *cx, bool timer_bc)
> +			       struct acpi_processor_cx *cx)
>  {
>  	acpi_unlazy_tlb(smp_processor_id());
>  
>  	/*
> -	 * Must be done before busmaster disable as we might need to
> -	 * access HPET !
> -	 */
> -	if (timer_bc)
> -		lapic_timer_state_broadcast(pr, cx, 1);
> -
> -	/*
>  	 * disable bus master
>  	 * bm_check implies we need ARB_DIS
>  	 * bm_control implies whether we can do ARB_DIS
> @@ -609,9 +593,6 @@ static void acpi_idle_enter_bm(struct ac
>  		c3_cpu_count--;
>  		raw_spin_unlock(&c3_lock);
>  	}
> -
> -	if (timer_bc)
> -		lapic_timer_state_broadcast(pr, cx, 0);
>  }
>  
>  static int acpi_idle_enter(struct cpuidle_device *dev,
> @@ -630,7 +611,7 @@ static int acpi_idle_enter(struct cpuidl
>  			cx = per_cpu(acpi_cstate[index], dev->cpu);
>  		} else if (cx->type == ACPI_STATE_C3 && pr->flags.bm_check) {
>  			if (cx->bm_sts_skip || !acpi_idle_bm_check()) {
> -				acpi_idle_enter_bm(pr, cx, true);
> +				acpi_idle_enter_bm(pr, cx);
>  				return index;
>  			} else if (drv->safe_state_index >= 0) {
>  				index = drv->safe_state_index;
> @@ -642,15 +623,11 @@ static int acpi_idle_enter(struct cpuidl
>  		}
>  	}
>  
> -	lapic_timer_state_broadcast(pr, cx, 1);
> -
>  	if (cx->type == ACPI_STATE_C3)
>  		ACPI_FLUSH_CPU_CACHE();
>  
>  	acpi_idle_do_entry(cx);
>  
> -	lapic_timer_state_broadcast(pr, cx, 0);
> -
>  	return index;
>  }
>  
> @@ -666,7 +643,7 @@ static int acpi_idle_enter_s2idle(struct
>  			return 0;
>  
>  		if (pr->flags.bm_check) {
> -			acpi_idle_enter_bm(pr, cx, false);
> +			acpi_idle_enter_bm(pr, cx);
>  			return 0;
>  		} else {
>  			ACPI_FLUSH_CPU_CACHE();
> @@ -682,6 +659,7 @@ static int acpi_processor_setup_cpuidle_
>  {
>  	int i, count = ACPI_IDLE_STATE_START;
>  	struct acpi_processor_cx *cx;
> +	struct cpuidle_state *state;
>  
>  	if (max_cstate == 0)
>  		max_cstate = 1;
> @@ -694,6 +672,11 @@ static int acpi_processor_setup_cpuidle_
>  
>  		per_cpu(acpi_cstate[count], dev->cpu) = cx;
>  
> +		if (lapic_timer_needs_broadcast(pr, cx)) {
> +			state = &acpi_idle_driver.states[count];
> +			state->flags |= CPUIDLE_FLAG_TIMER_STOP;
> +		}
> +
>  		count++;
>  		if (count == CPUIDLE_STATE_MAX)
>  			break;

Re: [RFC][PATCH 1/4] acpi: Use CPUIDLE_FLAG_TIMER_STOP

[Rafael J. Wysocki]

On Tuesday, September 22, 2020 5:26:51 AM CEST Guenter Roeck wrote:
> On Tue, Sep 15, 2020 at 12:31:58PM +0200, Peter Zijlstra wrote:
> > Make acpi_processor_idle use the common broadcast code, there's no
> > reason not to. This also removes some RCU usage after
> > rcu_idle_enter().
> > 
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Reported-by: Borislav Petkov <bp@suse.de>
> > Tested-by: Borislav Petkov <bp@suse.de>
> > ---
> >  drivers/acpi/processor_idle.c |   49 +++++++++++++-----------------------------
> >  1 file changed, 16 insertions(+), 33 deletions(-)
> > 
> > --- a/drivers/acpi/processor_idle.c
> > +++ b/drivers/acpi/processor_idle.c
> > @@ -161,18 +161,10 @@ static void lapic_timer_propagate_broadc
> >  }
> >  
> >  /* Power(C) State timer broadcast control */
> > -static void lapic_timer_state_broadcast(struct acpi_processor *pr,
> > -				       struct acpi_processor_cx *cx,
> > -				       int broadcast)
> > -{
> > -	int state = cx - pr->power.states;
> > -
> > -	if (state >= pr->power.timer_broadcast_on_state) {
> > -		if (broadcast)
> > -			tick_broadcast_enter();
> > -		else
> > -			tick_broadcast_exit();
> > -	}
> > +static bool lapic_timer_needs_broadcast(struct acpi_processor *pr,
> > +					struct acpi_processor_cx *cx)
> > +{
> > +	return cx - pr->power.states >= pr->power.timer_broadcast_on_state;
> >  }
> >  
> >  #else
> > @@ -180,9 +172,9 @@ static void lapic_timer_state_broadcast(
> >  static void lapic_timer_check_state(int state, struct acpi_processor *pr,
> >  				   struct acpi_processor_cx *cstate) { }
> >  static void lapic_timer_propagate_broadcast(struct acpi_processor *pr) { }
> > -static void lapic_timer_state_broadcast(struct acpi_processor *pr,
> > -				       struct acpi_processor_cx *cx,
> > -				       int broadcast)
> > +
> > +static bool lapic_timer_needs_broadcast(struct acpi_processor *pr,
> > +					struct acpi_processor_cx *cx)
> >  {
> >  }
> 
> drivers/acpi/processor_idle.c: In function 'lapic_timer_needs_broadcast':
> drivers/acpi/processor_idle.c:179:1: warning: no return statement in function returning non-void [-Wreturn-type]
> 
> Should this return true or false ?

false - if the lapic timer doesn't stop, it doesn't need broadcast.

FWIW, patch appended.

Cheers!

---
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: [PATCH] ACPI: processor: Fix build for ARCH_APICTIMER_STOPS_ON_C3 unset

Fix the lapic_timer_needs_broadcast() stub for
ARCH_APICTIMER_STOPS_ON_C3 unset to actually return
a value.

Fixes: aa6b43d57f99 ("ACPI: processor: Use CPUIDLE_FLAG_TIMER_STOP")
Reported-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/processor_idle.c |    1 +
 1 file changed, 1 insertion(+)

Index: linux-pm/drivers/acpi/processor_idle.c
===================================================================
--- linux-pm.orig/drivers/acpi/processor_idle.c
+++ linux-pm/drivers/acpi/processor_idle.c
@@ -176,6 +176,7 @@ static void lapic_timer_propagate_broadc
 static bool lapic_timer_needs_broadcast(struct acpi_processor *pr,
 					struct acpi_processor_cx *cx)
 {
+	return false;
 }
 
 #endif

[RFC][PATCH 2/4] acpi: Use CPUIDLE_FLAG_TLB_FLUSHED

[Peter Zijlstra]

Make acpi_processor_idle() use the generic TLB flushing code.
This again removes RCU usage after rcu_idle_enter().

(XXX make every C3 invalidate TLBs, not just C3-BM)

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/ia64/include/asm/acpi.h  |    2 --
 arch/x86/include/asm/acpi.h   |    2 --
 drivers/acpi/processor_idle.c |   10 +++++-----
 3 files changed, 5 insertions(+), 9 deletions(-)

--- a/arch/ia64/include/asm/acpi.h
+++ b/arch/ia64/include/asm/acpi.h
@@ -74,8 +74,6 @@ static inline void arch_acpi_set_pdc_bit
 	buf[2] |= ACPI_PDC_EST_CAPABILITY_SMP;
 }
 
-#define acpi_unlazy_tlb(x)
-
 #ifdef CONFIG_ACPI_NUMA
 extern cpumask_t early_cpu_possible_map;
 #define for_each_possible_early_cpu(cpu)  \
--- a/arch/x86/include/asm/acpi.h
+++ b/arch/x86/include/asm/acpi.h
@@ -159,8 +159,6 @@ static inline u64 x86_default_get_root_p
 extern int x86_acpi_numa_init(void);
 #endif /* CONFIG_ACPI_NUMA */
 
-#define acpi_unlazy_tlb(x)	leave_mm(x)
-
 #ifdef CONFIG_ACPI_APEI
 static inline pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr)
 {
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -565,8 +565,6 @@ static DEFINE_RAW_SPINLOCK(c3_lock);
 static void acpi_idle_enter_bm(struct acpi_processor *pr,
 			       struct acpi_processor_cx *cx)
 {
-	acpi_unlazy_tlb(smp_processor_id());
-
 	/*
 	 * disable bus master
 	 * bm_check implies we need ARB_DIS
@@ -666,6 +664,7 @@ static int acpi_processor_setup_cpuidle_
 		max_cstate = 1;
 
 	for (i = 1; i < ACPI_PROCESSOR_MAX_POWER && i <= max_cstate; i++) {
+		state = &acpi_idle_driver.states[count];
 		cx = &pr->power.states[i];
 
 		if (!cx->valid)
@@ -673,10 +672,11 @@ static int acpi_processor_setup_cpuidle_
 
 		per_cpu(acpi_cstate[count], dev->cpu) = cx;
 
-		if (lapic_timer_needs_broadcast(pr, cx)) {
-			state = &acpi_idle_driver.states[count];
+		if (lapic_timer_needs_broadcast(pr, cx))
 			state->flags |= CPUIDLE_FLAG_TIMER_STOP;
-		}
+
+		if (cx->type == ACPI_STATE_C3)
+			state->flags |= CPUIDLE_FLAG_TLB_FLUSHED;
 
 		count++;
 		if (count == CPUIDLE_STATE_MAX)

[RFC][PATCH 3/4] cpuidle: Allow cpuidle drivers to take over RCU-idle

[Peter Zijlstra]

Some drivers have to do significant work, some of which relies on RCU
still being active. Instead of using RCU_NONIDLE in the drivers and
flipping RCU back on, allow drivers to take over RCU-idle duty.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 drivers/cpuidle/cpuidle.c |   15 ++++++++++-----
 include/linux/cpuidle.h   |    1 +
 2 files changed, 11 insertions(+), 5 deletions(-)

--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -138,6 +138,7 @@ static void enter_s2idle_proper(struct c
 				struct cpuidle_device *dev, int index)
 {
 	ktime_t time_start, time_end;
+	struct cpuidle_state *target_state = &drv->states[index];
 
 	time_start = ns_to_ktime(local_clock());
 
@@ -153,8 +154,9 @@ static void enter_s2idle_proper(struct c
 	 * suspended is generally unsafe.
 	 */
 	stop_critical_timings();
-	rcu_idle_enter();
-	drv->states[index].enter_s2idle(dev, drv, index);
+	if (!(target_state->flags & CPUIDLE_FLAG_RCU_IDLE))
+		rcu_idle_enter();
+	target_state->enter_s2idle(dev, drv, index);
 	if (WARN_ON_ONCE(!irqs_disabled()))
 		local_irq_disable();
 	/*
@@ -162,7 +164,8 @@ static void enter_s2idle_proper(struct c
 	 * first CPU executing it calls functions containing RCU read-side
 	 * critical sections, so tell RCU about that.
 	 */
-	rcu_idle_exit();
+	if (!(target_state->flags & CPUIDLE_FLAG_RCU_IDLE))
+		rcu_idle_exit();
 	tick_unfreeze();
 	start_critical_timings();
 
@@ -239,9 +242,11 @@ int cpuidle_enter_state(struct cpuidle_d
 	time_start = ns_to_ktime(local_clock());
 
 	stop_critical_timings();
-	rcu_idle_enter();
+	if (!(target_state->flags & CPUIDLE_FLAG_RCU_IDLE))
+		rcu_idle_enter();
 	entered_state = target_state->enter(dev, drv, index);
-	rcu_idle_exit();
+	if (!(target_state->flags & CPUIDLE_FLAG_RCU_IDLE))
+		rcu_idle_exit();
 	start_critical_timings();
 
 	sched_clock_idle_wakeup_event();
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -82,6 +82,7 @@ struct cpuidle_state {
 #define CPUIDLE_FLAG_UNUSABLE		BIT(3) /* avoid using this state */
 #define CPUIDLE_FLAG_OFF		BIT(4) /* disable this state by default */
 #define CPUIDLE_FLAG_TLB_FLUSHED	BIT(5) /* idle-state flushes TLBs */
+#define CPUIDLE_FLAG_RCU_IDLE		BIT(6) /* idle-state takes care of RCU */
 
 struct cpuidle_device_kobj;
 struct cpuidle_state_kobj;

Re: [RFC][PATCH 3/4] cpuidle: Allow cpuidle drivers to take over RCU-idle

[Ulf Hansson]

On Tue, 15 Sep 2020 at 12:44, Peter Zijlstra <peterz@infradead.org> wrote:
>
> Some drivers have to do significant work, some of which relies on RCU
> still being active. Instead of using RCU_NONIDLE in the drivers and
> flipping RCU back on, allow drivers to take over RCU-idle duty.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

> ---
>  drivers/cpuidle/cpuidle.c |   15 ++++++++++-----
>  include/linux/cpuidle.h   |    1 +
>  2 files changed, 11 insertions(+), 5 deletions(-)
>
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -138,6 +138,7 @@ static void enter_s2idle_proper(struct c
>                                 struct cpuidle_device *dev, int index)
>  {
>         ktime_t time_start, time_end;
> +       struct cpuidle_state *target_state = &drv->states[index];
>
>         time_start = ns_to_ktime(local_clock());
>
> @@ -153,8 +154,9 @@ static void enter_s2idle_proper(struct c
>          * suspended is generally unsafe.
>          */
>         stop_critical_timings();
> -       rcu_idle_enter();
> -       drv->states[index].enter_s2idle(dev, drv, index);
> +       if (!(target_state->flags & CPUIDLE_FLAG_RCU_IDLE))
> +               rcu_idle_enter();
> +       target_state->enter_s2idle(dev, drv, index);
>         if (WARN_ON_ONCE(!irqs_disabled()))
>                 local_irq_disable();
>         /*
> @@ -162,7 +164,8 @@ static void enter_s2idle_proper(struct c
>          * first CPU executing it calls functions containing RCU read-side
>          * critical sections, so tell RCU about that.
>          */
> -       rcu_idle_exit();
> +       if (!(target_state->flags & CPUIDLE_FLAG_RCU_IDLE))
> +               rcu_idle_exit();
>         tick_unfreeze();
>         start_critical_timings();
>
> @@ -239,9 +242,11 @@ int cpuidle_enter_state(struct cpuidle_d
>         time_start = ns_to_ktime(local_clock());
>
>         stop_critical_timings();
> -       rcu_idle_enter();
> +       if (!(target_state->flags & CPUIDLE_FLAG_RCU_IDLE))
> +               rcu_idle_enter();
>         entered_state = target_state->enter(dev, drv, index);
> -       rcu_idle_exit();
> +       if (!(target_state->flags & CPUIDLE_FLAG_RCU_IDLE))
> +               rcu_idle_exit();
>         start_critical_timings();
>
>         sched_clock_idle_wakeup_event();
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -82,6 +82,7 @@ struct cpuidle_state {
>  #define CPUIDLE_FLAG_UNUSABLE          BIT(3) /* avoid using this state */
>  #define CPUIDLE_FLAG_OFF               BIT(4) /* disable this state by default */
>  #define CPUIDLE_FLAG_TLB_FLUSHED       BIT(5) /* idle-state flushes TLBs */
> +#define CPUIDLE_FLAG_RCU_IDLE          BIT(6) /* idle-state takes care of RCU */
>
>  struct cpuidle_device_kobj;
>  struct cpuidle_state_kobj;
>
>

[RFC][PATCH 4/4] acpi: Take over RCU-idle for C3-BM idle

[Peter Zijlstra]

The C3 BusMaster idle code takes lock in a number of places, some deep
inside the ACPI code. Instead of wrapping it all in RCU_NONIDLE, have
the driver take over RCU-idle duty and avoid flipping RCU state back
and forth a lot.

( by marking 'C3 && bm_check' as RCU_IDLE, we _must_ call enter_bm() for
  that combination, otherwise we'll loose RCU-idle, this requires
  shuffling some code around )

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 drivers/acpi/processor_idle.c |   69 +++++++++++++++++++++++++++++-------------
 1 file changed, 49 insertions(+), 20 deletions(-)

--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -558,22 +558,43 @@ static DEFINE_RAW_SPINLOCK(c3_lock);
 
 /**
  * acpi_idle_enter_bm - enters C3 with proper BM handling
+ * @drv: cpuidle driver
  * @pr: Target processor
  * @cx: Target state context
+ * @index: index of target state
  */
-static void acpi_idle_enter_bm(struct acpi_processor *pr,
-			       struct acpi_processor_cx *cx)
+static int acpi_idle_enter_bm(struct cpuidle_driver *drv,
+			       struct acpi_processor *pr,
+			       struct acpi_processor_cx *cx,
+			       int index)
 {
+	static struct acpi_processor_cx safe_cx = {
+		.entry_method = ACPI_CSTATE_HALT,
+	};
+
 	/*
 	 * disable bus master
 	 * bm_check implies we need ARB_DIS
 	 * bm_control implies whether we can do ARB_DIS
 	 *
-	 * That leaves a case where bm_check is set and bm_control is
-	 * not set. In that case we cannot do much, we enter C3
-	 * without doing anything.
+	 * That leaves a case where bm_check is set and bm_control is not set.
+	 * In that case we cannot do much, we enter C3 without doing anything.
 	 */
-	if (pr->flags.bm_control) {
+	bool dis_bm = pr->flags.bm_control;
+
+	/* If we can skip BM, demote to a safe state. */
+	if (!cx->bm_sts_skip && acpi_idle_bm_check()) {
+		dis_bm = false;
+		index = drv->safe_state_index;
+		if (index >= 0) {
+			cx = this_cpu_read(acpi_cstate[index]);
+		} else {
+			cx = &safe_cx;
+			index = -EBUSY;
+		}
+	}
+
+	if (dis_bm) {
 		raw_spin_lock(&c3_lock);
 		c3_cpu_count++;
 		/* Disable bus master arbitration when all CPUs are in C3 */
@@ -582,15 +603,21 @@ static void acpi_idle_enter_bm(struct ac
 		raw_spin_unlock(&c3_lock);
 	}
 
+	rcu_idle_enter();
+
 	acpi_idle_do_entry(cx);
 
+	rcu_idle_exit();
+
 	/* Re-enable bus master arbitration */
-	if (pr->flags.bm_control) {
+	if (dis_bm) {
 		raw_spin_lock(&c3_lock);
 		acpi_write_bit_register(ACPI_BITREG_ARB_DISABLE, 0);
 		c3_cpu_count--;
 		raw_spin_unlock(&c3_lock);
 	}
+
+	return index;
 }
 
 static int acpi_idle_enter(struct cpuidle_device *dev,
@@ -604,20 +631,13 @@ static int acpi_idle_enter(struct cpuidl
 		return -EINVAL;
 
 	if (cx->type != ACPI_STATE_C1) {
+		if (cx->type == ACPI_STATE_C3 && pr->flags.bm_check)
+			return acpi_idle_enter_bm(drv, pr, cx, index);
+
+		/* C2 to C1 demotion. */
 		if (acpi_idle_fallback_to_c1(pr) && num_online_cpus() > 1) {
 			index = ACPI_IDLE_STATE_START;
 			cx = per_cpu(acpi_cstate[index], dev->cpu);
-		} else if (cx->type == ACPI_STATE_C3 && pr->flags.bm_check) {
-			if (cx->bm_sts_skip || !acpi_idle_bm_check()) {
-				acpi_idle_enter_bm(pr, cx);
-				return index;
-			} else if (drv->safe_state_index >= 0) {
-				index = drv->safe_state_index;
-				cx = per_cpu(acpi_cstate[index], dev->cpu);
-			} else {
-				acpi_safe_halt();
-				return -EBUSY;
-			}
 		}
 	}
 
@@ -641,7 +661,13 @@ static int acpi_idle_enter_s2idle(struct
 			return 0;
 
 		if (pr->flags.bm_check) {
-			acpi_idle_enter_bm(pr, cx);
+			u8 bm_sts_skip = cx->bm_sts_skip;
+
+			/* Don't check BM_STS, do an unconditional ARB_DIS for S2IDLE */
+			cx->bm_sts_skip = 1;
+			acpi_idle_enter_bm(drv, pr, cx, index);
+			cx->bm_sts_skip = bm_sts_skip;
+
 			return 0;
 		} else {
 			ACPI_FLUSH_CPU_CACHE();
@@ -674,8 +700,11 @@ static int acpi_processor_setup_cpuidle_
 		if (lapic_timer_needs_broadcast(pr, cx))
 			state->flags |= CPUIDLE_FLAG_TIMER_STOP;
 
-		if (cx->type == ACPI_STATE_C3)
+		if (cx->type == ACPI_STATE_C3) {
 			state->flags |= CPUIDLE_FLAG_TLB_FLUSHED;
+			if (pr->flags.bm_check)
+				state->flags |= CPUIDLE_FLAG_RCU_IDLE;
+		}
 
 		count++;
 		if (count == CPUIDLE_STATE_MAX)

Re: [RFC][PATCH 4/4] acpi: Take over RCU-idle for C3-BM idle

[Sven Joachim]

On 2020-09-15 12:32 +0200, Peter Zijlstra wrote:

> The C3 BusMaster idle code takes lock in a number of places, some deep
> inside the ACPI code. Instead of wrapping it all in RCU_NONIDLE, have
> the driver take over RCU-idle duty and avoid flipping RCU state back
> and forth a lot.
>
> ( by marking 'C3 && bm_check' as RCU_IDLE, we _must_ call enter_bm() for
>   that combination, otherwise we'll loose RCU-idle, this requires
>   shuffling some code around )
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

I got modpost errors in 5.9-rc6 after this patch:

ERROR: modpost: "rcu_idle_enter" [drivers/acpi/processor.ko] undefined!
ERROR: modpost: "rcu_idle_exit" [drivers/acpi/processor.ko] undefined!

Reverting commit 1fecfdbb7acc made them go away.  Notably my
configuration had CONFIG_ACPI_PROCESSOR=m,  changing that
to CONFIG_ACPI_PROCESSOR=y let the build succeed as well.

Cheers,
       Sven

> ---
>  drivers/acpi/processor_idle.c |   69 +++++++++++++++++++++++++++++-------------
>  1 file changed, 49 insertions(+), 20 deletions(-)
>
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -558,22 +558,43 @@ static DEFINE_RAW_SPINLOCK(c3_lock);
>
>  /**
>   * acpi_idle_enter_bm - enters C3 with proper BM handling
> + * @drv: cpuidle driver
>   * @pr: Target processor
>   * @cx: Target state context
> + * @index: index of target state
>   */
> -static void acpi_idle_enter_bm(struct acpi_processor *pr,
> -			       struct acpi_processor_cx *cx)
> +static int acpi_idle_enter_bm(struct cpuidle_driver *drv,
> +			       struct acpi_processor *pr,
> +			       struct acpi_processor_cx *cx,
> +			       int index)
>  {
> +	static struct acpi_processor_cx safe_cx = {
> +		.entry_method = ACPI_CSTATE_HALT,
> +	};
> +
>  	/*
>  	 * disable bus master
>  	 * bm_check implies we need ARB_DIS
>  	 * bm_control implies whether we can do ARB_DIS
>  	 *
> -	 * That leaves a case where bm_check is set and bm_control is
> -	 * not set. In that case we cannot do much, we enter C3
> -	 * without doing anything.
> +	 * That leaves a case where bm_check is set and bm_control is not set.
> +	 * In that case we cannot do much, we enter C3 without doing anything.
>  	 */
> -	if (pr->flags.bm_control) {
> +	bool dis_bm = pr->flags.bm_control;
> +
> +	/* If we can skip BM, demote to a safe state. */
> +	if (!cx->bm_sts_skip && acpi_idle_bm_check()) {
> +		dis_bm = false;
> +		index = drv->safe_state_index;
> +		if (index >= 0) {
> +			cx = this_cpu_read(acpi_cstate[index]);
> +		} else {
> +			cx = &safe_cx;
> +			index = -EBUSY;
> +		}
> +	}
> +
> +	if (dis_bm) {
>  		raw_spin_lock(&c3_lock);
>  		c3_cpu_count++;
>  		/* Disable bus master arbitration when all CPUs are in C3 */
> @@ -582,15 +603,21 @@ static void acpi_idle_enter_bm(struct ac
>  		raw_spin_unlock(&c3_lock);
>  	}
>
> +	rcu_idle_enter();
> +
>  	acpi_idle_do_entry(cx);
>
> +	rcu_idle_exit();
> +
>  	/* Re-enable bus master arbitration */
> -	if (pr->flags.bm_control) {
> +	if (dis_bm) {
>  		raw_spin_lock(&c3_lock);
>  		acpi_write_bit_register(ACPI_BITREG_ARB_DISABLE, 0);
>  		c3_cpu_count--;
>  		raw_spin_unlock(&c3_lock);
>  	}
> +
> +	return index;
>  }
>
>  static int acpi_idle_enter(struct cpuidle_device *dev,
> @@ -604,20 +631,13 @@ static int acpi_idle_enter(struct cpuidl
>  		return -EINVAL;
>
>  	if (cx->type != ACPI_STATE_C1) {
> +		if (cx->type == ACPI_STATE_C3 && pr->flags.bm_check)
> +			return acpi_idle_enter_bm(drv, pr, cx, index);
> +
> +		/* C2 to C1 demotion. */
>  		if (acpi_idle_fallback_to_c1(pr) && num_online_cpus() > 1) {
>  			index = ACPI_IDLE_STATE_START;
>  			cx = per_cpu(acpi_cstate[index], dev->cpu);
> -		} else if (cx->type == ACPI_STATE_C3 && pr->flags.bm_check) {
> -			if (cx->bm_sts_skip || !acpi_idle_bm_check()) {
> -				acpi_idle_enter_bm(pr, cx);
> -				return index;
> -			} else if (drv->safe_state_index >= 0) {
> -				index = drv->safe_state_index;
> -				cx = per_cpu(acpi_cstate[index], dev->cpu);
> -			} else {
> -				acpi_safe_halt();
> -				return -EBUSY;
> -			}
>  		}
>  	}
>
> @@ -641,7 +661,13 @@ static int acpi_idle_enter_s2idle(struct
>  			return 0;
>
>  		if (pr->flags.bm_check) {
> -			acpi_idle_enter_bm(pr, cx);
> +			u8 bm_sts_skip = cx->bm_sts_skip;
> +
> +			/* Don't check BM_STS, do an unconditional ARB_DIS for S2IDLE */
> +			cx->bm_sts_skip = 1;
> +			acpi_idle_enter_bm(drv, pr, cx, index);
> +			cx->bm_sts_skip = bm_sts_skip;
> +
>  			return 0;
>  		} else {
>  			ACPI_FLUSH_CPU_CACHE();
> @@ -674,8 +700,11 @@ static int acpi_processor_setup_cpuidle_
>  		if (lapic_timer_needs_broadcast(pr, cx))
>  			state->flags |= CPUIDLE_FLAG_TIMER_STOP;
>
> -		if (cx->type == ACPI_STATE_C3)
> +		if (cx->type == ACPI_STATE_C3) {
>  			state->flags |= CPUIDLE_FLAG_TLB_FLUSHED;
> +			if (pr->flags.bm_check)
> +				state->flags |= CPUIDLE_FLAG_RCU_IDLE;
> +		}
>
>  		count++;
>  		if (count == CPUIDLE_STATE_MAX)

[PATCH] rcu/tree: Export rcu_idle_{enter,exit} to module

[Borislav Petkov]

Lemme add whatever get_maintainer spits, to Cc.

On Mon, Sep 21, 2020 at 11:12:33AM +0200, Sven Joachim wrote:
> On 2020-09-15 12:32 +0200, Peter Zijlstra wrote:
> 
> > The C3 BusMaster idle code takes lock in a number of places, some deep
> > inside the ACPI code. Instead of wrapping it all in RCU_NONIDLE, have
> > the driver take over RCU-idle duty and avoid flipping RCU state back
> > and forth a lot.
> >
> > ( by marking 'C3 && bm_check' as RCU_IDLE, we _must_ call enter_bm() for
> >   that combination, otherwise we'll loose RCU-idle, this requires
> >   shuffling some code around )
> >
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> 
> I got modpost errors in 5.9-rc6 after this patch:
> 
> ERROR: modpost: "rcu_idle_enter" [drivers/acpi/processor.ko] undefined!
> ERROR: modpost: "rcu_idle_exit" [drivers/acpi/processor.ko] undefined!
> 
> Reverting commit 1fecfdbb7acc made them go away.  Notably my
> configuration had CONFIG_ACPI_PROCESSOR=m,  changing that
> to CONFIG_ACPI_PROCESSOR=y let the build succeed as well.

I guess this. Running randconfigs on it for a while, to see what else
breaks.

---
From: Borislav Petkov <bp@suse.de>
Date: Mon, 21 Sep 2020 12:31:36 +0200

Fix this link error:

  ERROR: modpost: "rcu_idle_enter" [drivers/acpi/processor.ko] undefined!
  ERROR: modpost: "rcu_idle_exit" [drivers/acpi/processor.ko] undefined!

when CONFIG_ACPI_PROCESSOR is built as module. PeterZ says that in light
of ARM needing those soon too, they should simply be exported.

Fixes: 1fecfdbb7acc ("ACPI: processor: Take over RCU-idle for C3-BM idle")
Reported-by: Sven Joachim <svenjoac@gmx.de>
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Borislav Petkov <bp@suse.de>
---
 kernel/rcu/tree.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 8ce77d9ac716..f78ee759af9c 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -673,6 +673,7 @@ void rcu_idle_enter(void)
 	lockdep_assert_irqs_disabled();
 	rcu_eqs_enter(false);
 }
+EXPORT_SYMBOL_GPL(rcu_idle_enter);
 
 #ifdef CONFIG_NO_HZ_FULL
 /**
@@ -886,6 +887,7 @@ void rcu_idle_exit(void)
 	rcu_eqs_exit(false);
 	local_irq_restore(flags);
 }
+EXPORT_SYMBOL_GPL(rcu_idle_exit);
 
 #ifdef CONFIG_NO_HZ_FULL
 /**
-- 
2.21.0

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] rcu/tree: Export rcu_idle_{enter,exit} to module

[Rafael J. Wysocki]

On Mon, Sep 21, 2020 at 12:37 PM Borislav Petkov <bp@alien8.de> wrote:
>
> Lemme add whatever get_maintainer spits, to Cc.
>
> On Mon, Sep 21, 2020 at 11:12:33AM +0200, Sven Joachim wrote:
> > On 2020-09-15 12:32 +0200, Peter Zijlstra wrote:
> >
> > > The C3 BusMaster idle code takes lock in a number of places, some deep
> > > inside the ACPI code. Instead of wrapping it all in RCU_NONIDLE, have
> > > the driver take over RCU-idle duty and avoid flipping RCU state back
> > > and forth a lot.
> > >
> > > ( by marking 'C3 && bm_check' as RCU_IDLE, we _must_ call enter_bm() for
> > >   that combination, otherwise we'll loose RCU-idle, this requires
> > >   shuffling some code around )
> > >
> > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> >
> > I got modpost errors in 5.9-rc6 after this patch:
> >
> > ERROR: modpost: "rcu_idle_enter" [drivers/acpi/processor.ko] undefined!
> > ERROR: modpost: "rcu_idle_exit" [drivers/acpi/processor.ko] undefined!
> >
> > Reverting commit 1fecfdbb7acc made them go away.  Notably my
> > configuration had CONFIG_ACPI_PROCESSOR=m,  changing that
> > to CONFIG_ACPI_PROCESSOR=y let the build succeed as well.
>
> I guess this. Running randconfigs on it for a while, to see what else
> breaks.
>
> ---
> From: Borislav Petkov <bp@suse.de>
> Date: Mon, 21 Sep 2020 12:31:36 +0200
>
> Fix this link error:
>
>   ERROR: modpost: "rcu_idle_enter" [drivers/acpi/processor.ko] undefined!
>   ERROR: modpost: "rcu_idle_exit" [drivers/acpi/processor.ko] undefined!
>
> when CONFIG_ACPI_PROCESSOR is built as module. PeterZ says that in light
> of ARM needing those soon too, they should simply be exported.
>
> Fixes: 1fecfdbb7acc ("ACPI: processor: Take over RCU-idle for C3-BM idle")
> Reported-by: Sven Joachim <svenjoac@gmx.de>
> Suggested-by: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Borislav Petkov <bp@suse.de>

Well, I guess I should take this through cpuidle?

Any concerns about doing that?  Paul?

> ---
>  kernel/rcu/tree.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 8ce77d9ac716..f78ee759af9c 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -673,6 +673,7 @@ void rcu_idle_enter(void)
>         lockdep_assert_irqs_disabled();
>         rcu_eqs_enter(false);
>  }
> +EXPORT_SYMBOL_GPL(rcu_idle_enter);
>
>  #ifdef CONFIG_NO_HZ_FULL
>  /**
> @@ -886,6 +887,7 @@ void rcu_idle_exit(void)
>         rcu_eqs_exit(false);
>         local_irq_restore(flags);
>  }
> +EXPORT_SYMBOL_GPL(rcu_idle_exit);
>
>  #ifdef CONFIG_NO_HZ_FULL
>  /**
> --
> 2.21.0
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] rcu/tree: Export rcu_idle_{enter,exit} to module

[Paul E. McKenney]

On Mon, Sep 21, 2020 at 12:37:41PM +0200, Borislav Petkov wrote:
> Lemme add whatever get_maintainer spits, to Cc.
> 
> On Mon, Sep 21, 2020 at 11:12:33AM +0200, Sven Joachim wrote:
> > On 2020-09-15 12:32 +0200, Peter Zijlstra wrote:
> > 
> > > The C3 BusMaster idle code takes lock in a number of places, some deep
> > > inside the ACPI code. Instead of wrapping it all in RCU_NONIDLE, have
> > > the driver take over RCU-idle duty and avoid flipping RCU state back
> > > and forth a lot.
> > >
> > > ( by marking 'C3 && bm_check' as RCU_IDLE, we _must_ call enter_bm() for
> > >   that combination, otherwise we'll loose RCU-idle, this requires
> > >   shuffling some code around )
> > >
> > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > 
> > I got modpost errors in 5.9-rc6 after this patch:
> > 
> > ERROR: modpost: "rcu_idle_enter" [drivers/acpi/processor.ko] undefined!
> > ERROR: modpost: "rcu_idle_exit" [drivers/acpi/processor.ko] undefined!
> > 
> > Reverting commit 1fecfdbb7acc made them go away.  Notably my
> > configuration had CONFIG_ACPI_PROCESSOR=m,  changing that
> > to CONFIG_ACPI_PROCESSOR=y let the build succeed as well.
> 
> I guess this. Running randconfigs on it for a while, to see what else
> breaks.
> 
> ---
> From: Borislav Petkov <bp@suse.de>
> Date: Mon, 21 Sep 2020 12:31:36 +0200
> 
> Fix this link error:
> 
>   ERROR: modpost: "rcu_idle_enter" [drivers/acpi/processor.ko] undefined!
>   ERROR: modpost: "rcu_idle_exit" [drivers/acpi/processor.ko] undefined!
> 
> when CONFIG_ACPI_PROCESSOR is built as module. PeterZ says that in light
> of ARM needing those soon too, they should simply be exported.
> 
> Fixes: 1fecfdbb7acc ("ACPI: processor: Take over RCU-idle for C3-BM idle")
> Reported-by: Sven Joachim <svenjoac@gmx.de>
> Suggested-by: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Borislav Petkov <bp@suse.de>

Reviewed-by: Paul E. McKenney <paulmckrcu@kernel.org>

> ---
>  kernel/rcu/tree.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 8ce77d9ac716..f78ee759af9c 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -673,6 +673,7 @@ void rcu_idle_enter(void)
>  	lockdep_assert_irqs_disabled();
>  	rcu_eqs_enter(false);
>  }
> +EXPORT_SYMBOL_GPL(rcu_idle_enter);
>  
>  #ifdef CONFIG_NO_HZ_FULL
>  /**
> @@ -886,6 +887,7 @@ void rcu_idle_exit(void)
>  	rcu_eqs_exit(false);
>  	local_irq_restore(flags);
>  }
> +EXPORT_SYMBOL_GPL(rcu_idle_exit);
>  
>  #ifdef CONFIG_NO_HZ_FULL
>  /**
> -- 
> 2.21.0
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] rcu/tree: Export rcu_idle_{enter,exit} to module

[Rafael J. Wysocki]

On Mon, Sep 21, 2020 at 3:32 PM Paul E. McKenney <paulmck@kernel.org> wrote:
>
> On Mon, Sep 21, 2020 at 12:37:41PM +0200, Borislav Petkov wrote:
> > Lemme add whatever get_maintainer spits, to Cc.
> >
> > On Mon, Sep 21, 2020 at 11:12:33AM +0200, Sven Joachim wrote:
> > > On 2020-09-15 12:32 +0200, Peter Zijlstra wrote:
> > >
> > > > The C3 BusMaster idle code takes lock in a number of places, some deep
> > > > inside the ACPI code. Instead of wrapping it all in RCU_NONIDLE, have
> > > > the driver take over RCU-idle duty and avoid flipping RCU state back
> > > > and forth a lot.
> > > >
> > > > ( by marking 'C3 && bm_check' as RCU_IDLE, we _must_ call enter_bm() for
> > > >   that combination, otherwise we'll loose RCU-idle, this requires
> > > >   shuffling some code around )
> > > >
> > > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > >
> > > I got modpost errors in 5.9-rc6 after this patch:
> > >
> > > ERROR: modpost: "rcu_idle_enter" [drivers/acpi/processor.ko] undefined!
> > > ERROR: modpost: "rcu_idle_exit" [drivers/acpi/processor.ko] undefined!
> > >
> > > Reverting commit 1fecfdbb7acc made them go away.  Notably my
> > > configuration had CONFIG_ACPI_PROCESSOR=m,  changing that
> > > to CONFIG_ACPI_PROCESSOR=y let the build succeed as well.
> >
> > I guess this. Running randconfigs on it for a while, to see what else
> > breaks.
> >
> > ---
> > From: Borislav Petkov <bp@suse.de>
> > Date: Mon, 21 Sep 2020 12:31:36 +0200
> >
> > Fix this link error:
> >
> >   ERROR: modpost: "rcu_idle_enter" [drivers/acpi/processor.ko] undefined!
> >   ERROR: modpost: "rcu_idle_exit" [drivers/acpi/processor.ko] undefined!
> >
> > when CONFIG_ACPI_PROCESSOR is built as module. PeterZ says that in light
> > of ARM needing those soon too, they should simply be exported.
> >
> > Fixes: 1fecfdbb7acc ("ACPI: processor: Take over RCU-idle for C3-BM idle")
> > Reported-by: Sven Joachim <svenjoac@gmx.de>
> > Suggested-by: Peter Zijlstra <peterz@infradead.org>
> > Signed-off-by: Borislav Petkov <bp@suse.de>
>
> Reviewed-by: Paul E. McKenney <paulmckrcu@kernel.org>

OK

Applied as 5.9-rc7 material, thanks!

> > ---
> >  kernel/rcu/tree.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index 8ce77d9ac716..f78ee759af9c 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -673,6 +673,7 @@ void rcu_idle_enter(void)
> >       lockdep_assert_irqs_disabled();
> >       rcu_eqs_enter(false);
> >  }
> > +EXPORT_SYMBOL_GPL(rcu_idle_enter);
> >
> >  #ifdef CONFIG_NO_HZ_FULL
> >  /**
> > @@ -886,6 +887,7 @@ void rcu_idle_exit(void)
> >       rcu_eqs_exit(false);
> >       local_irq_restore(flags);
> >  }
> > +EXPORT_SYMBOL_GPL(rcu_idle_exit);
> >
> >  #ifdef CONFIG_NO_HZ_FULL
> >  /**
> > --
> > 2.21.0
> >
> > --
> > Regards/Gruss,
> >     Boris.
> >
> > https://people.kernel.org/tglx/notes-about-netiquette

Re: [RFC][PATCH 4/4] acpi: Take over RCU-idle for C3-BM idle

[Guenter Roeck]

On Tue, Sep 15, 2020 at 12:32:01PM +0200, Peter Zijlstra wrote:
> The C3 BusMaster idle code takes lock in a number of places, some deep
> inside the ACPI code. Instead of wrapping it all in RCU_NONIDLE, have
> the driver take over RCU-idle duty and avoid flipping RCU state back
> and forth a lot.
> 
> ( by marking 'C3 && bm_check' as RCU_IDLE, we _must_ call enter_bm() for
>   that combination, otherwise we'll loose RCU-idle, this requires
>   shuffling some code around )
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

ia64:defconfig:

ERROR: modpost: "rcu_idle_enter" [drivers/acpi/processor.ko] undefined!
ERROR: modpost: "rcu_idle_exit" [drivers/acpi/processor.ko] undefined!

I realize that this has already been reported more than a week ago, with
no visible reaction. Another problem introduced in the same file, resulting
in

drivers/acpi/processor_idle.c: In function 'lapic_timer_needs_broadcast':
drivers/acpi/processor_idle.c:179:1: warning:
	no return statement in function returning non-void

may cause ia64 boot problems since a non-zero return value will trigger
a function call. AFAICS that is not supposed to happen on ia64.

This makes me wonder - if no one cares about buiding (much less running)
ia64 images with the upstream kernel, is it possibly time to remove it ?

Guenter

Re: [RFC][PATCH 4/4] acpi: Take over RCU-idle for C3-BM idle

[Rafael J. Wysocki]

On Fri, Sep 25, 2020 at 5:20 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Tue, Sep 15, 2020 at 12:32:01PM +0200, Peter Zijlstra wrote:
> > The C3 BusMaster idle code takes lock in a number of places, some deep
> > inside the ACPI code. Instead of wrapping it all in RCU_NONIDLE, have
> > the driver take over RCU-idle duty and avoid flipping RCU state back
> > and forth a lot.
> >
> > ( by marking 'C3 && bm_check' as RCU_IDLE, we _must_ call enter_bm() for
> >   that combination, otherwise we'll loose RCU-idle, this requires
> >   shuffling some code around )
> >
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
>
> ia64:defconfig:
>
> ERROR: modpost: "rcu_idle_enter" [drivers/acpi/processor.ko] undefined!
> ERROR: modpost: "rcu_idle_exit" [drivers/acpi/processor.ko] undefined!
>
> I realize that this has already been reported more than a week ago, with
> no visible reaction. Another problem introduced in the same file, resulting
> in
>
> drivers/acpi/processor_idle.c: In function 'lapic_timer_needs_broadcast':
> drivers/acpi/processor_idle.c:179:1: warning:
>         no return statement in function returning non-void
>
> may cause ia64 boot problems since a non-zero return value will trigger
> a function call. AFAICS that is not supposed to happen on ia64.

There are fixes for the above in my tree, they will go to Linus shortly.

Thanks!

Re: [RFC][PATCH 4/4] acpi: Take over RCU-idle for C3-BM idle

[Paul E. McKenney]

On Fri, Sep 25, 2020 at 08:20:00AM -0700, Guenter Roeck wrote:
> On Tue, Sep 15, 2020 at 12:32:01PM +0200, Peter Zijlstra wrote:
> > The C3 BusMaster idle code takes lock in a number of places, some deep
> > inside the ACPI code. Instead of wrapping it all in RCU_NONIDLE, have
> > the driver take over RCU-idle duty and avoid flipping RCU state back
> > and forth a lot.
> > 
> > ( by marking 'C3 && bm_check' as RCU_IDLE, we _must_ call enter_bm() for
> >   that combination, otherwise we'll loose RCU-idle, this requires
> >   shuffling some code around )
> > 
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> 
> ia64:defconfig:
> 
> ERROR: modpost: "rcu_idle_enter" [drivers/acpi/processor.ko] undefined!
> ERROR: modpost: "rcu_idle_exit" [drivers/acpi/processor.ko] undefined!
> 
> I realize that this has already been reported more than a week ago, with
> no visible reaction. Another problem introduced in the same file, resulting
> in
> 
> drivers/acpi/processor_idle.c: In function 'lapic_timer_needs_broadcast':
> drivers/acpi/processor_idle.c:179:1: warning:
> 	no return statement in function returning non-void
> 
> may cause ia64 boot problems since a non-zero return value will trigger
> a function call. AFAICS that is not supposed to happen on ia64.
> 
> This makes me wonder - if no one cares about buiding (much less running)
> ia64 images with the upstream kernel, is it possibly time to remove it ?

Rafael is taking a fix up his cpuidle tree:

https://lkml.kernel.org/lkml/CAJZ5v0jVerU92WxL4qCoU6NC0KCyszmRNhpL3tu5LYtMqALd9A@mail.gmail.com/

							Thanx, Paul

Re: [RFC][PATCH 0/4] Fix up ACPI processor idle vs RCU

[Borislav Petkov]

On Tue, Sep 15, 2020 at 12:31:57PM +0200, Peter Zijlstra wrote:
> Boris tested an earlier version of these patches and they worked for his
> 32bit Atom board that was triggering complaints.

Want me to test them again?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette