Fix up ACPI processor idle vs RCU

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) <[email protected]>

---
 drivers/acpi/processor_idle.c |   49 +++++++++++++-----------------------------
 1 file changed, 16 insertions(+), 33 deletions(-)

drivers/acpi/processor_idle.c +16 -33

@@ -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;

Quoted 6 lines · 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) <[email protected]>

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

Acked-by Rafael J. Wysocki <[email protected]>

to all of the four patches.

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

Quoted 10 lines · Rafael J. Wysocki

Quoted 6 lines · 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) <[email protected]>

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

Quoted 5 lines

Acked-by Rafael J. Wysocki <[email protected]>

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.

Quoted 11 lines · On Wed, Sep 16, 2020 at 05:42:12PM +0200, [email protected] wrote:

Quoted 10 lines · Rafael J. Wysocki

Quoted 6 lines · 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) <[email protected]>

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

Quoted 8 lines

Quoted 5 lines

Acked-by Rafael J. Wysocki <[email protected]>

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 <[email protected]>

for this one and for this whole series:

Tested-by Borislav Petkov <[email protected]>

Quoted 10 lines · Rafael J. Wysocki

Quoted 6 lines · 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) <[email protected]>

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

Quoted 5 lines

Acked-by Rafael J. Wysocki <[email protected]>

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.

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) <[email protected]>

Acked-by Rafael J. Wysocki <[email protected]>

Reported-by Borislav Petkov <[email protected]>

Tested-by Borislav Petkov <[email protected]>

---
 drivers/acpi/processor_idle.c |   49 +++++++++++++-----------------------------
 1 file changed, 16 insertions(+), 33 deletions(-)

drivers/acpi/processor_idle.c +4 -12

@@ -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,

drivers/acpi/processor_idle.c +9 -18

@@ -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;

Quoted 39 lines · 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) <[email protected]>

Acked-by Rafael J. Wysocki <[email protected]>

Reported-by Borislav Petkov <[email protected]>

Tested-by Borislav Petkov <[email protected]>

---
 drivers/acpi/processor_idle.c |   49 +++++++++++++-----------------------------
 1 file changed, 16 insertions(+), 33 deletions(-)

drivers/acpi/processor_idle.c +4 -12

@@ -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,

drivers/acpi/processor_idle.c static void lapic_timer_state_broadcast( · line 179

Quoted 10 lines

drivers/acpi/processor_idle.c +3 -3

 				   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 ?

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) <[email protected]>

---
 drivers/cpuidle/cpuidle.c |   15 ++++++++++-----
 include/linux/cpuidle.h   |    1 +
 2 files changed, 11 insertions(+), 5 deletions(-)

drivers/cpuidle/cpuidle.c +10 -5

@@ -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();

include/linux/cpuidle.h +1 -0

@@ -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;

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) <[email protected]>

---
 drivers/acpi/processor_idle.c |   69 +++++++++++++++++++++++++++++-------------
 1 file changed, 49 insertions(+), 20 deletions(-)

drivers/acpi/processor_idle.c +49 -20

@@ -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)

Quoted 10 lines · 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) <[email protected]>

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.

Lemme add whatever get_maintainer spits, to Cc.

Quoted 13 lines · Sven Joachim

Quoted 10 lines · 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) <[email protected]>

Quoted 8 lines

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 <[email protected]>
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 <[email protected]>

Suggested-by Peter Zijlstra <[email protected]>

Signed-off-by Borislav Petkov <[email protected]>

---
 kernel/rcu/tree.c | 2 ++
 1 file changed, 2 insertions(+)

kernel/rcu/tree.c +2 -1

index 8ce77d9ac716..f78ee759af9c 100644
@@ -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

Lemme add whatever get_maintainer spits, to Cc.

Quoted 13 lines · Sven Joachim

Quoted 10 lines · 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) <[email protected]>

Quoted 8 lines

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 <[email protected]>
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 <[email protected]>

Suggested-by Peter Zijlstra <[email protected]>

Signed-off-by Borislav Petkov <[email protected]>

---
 kernel/rcu/tree.c | 2 ++
 1 file changed, 2 insertions(+)

kernel/rcu/tree.c +2 -0

index 8ce77d9ac716..f78ee759af9c 100644
@@ -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
 /**

Signature · 7 lines

2.21.0

-- 
Regards/Gruss,
    Boris.

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

Quoted 40 lines · Borislav Petkov

Lemme add whatever get_maintainer spits, to Cc.

Quoted 13 lines · Sven Joachim

Quoted 10 lines · 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) <[email protected]>

Quoted 8 lines

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 <[email protected]>
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.

Quoted 4 lines

Fixes 1fecfdbb7acc ("ACPI: processor: Take over RCU-idle for C3-BM idle")

Reported-by Sven Joachim <[email protected]>

Suggested-by Peter Zijlstra <[email protected]>

Signed-off-by Borislav Petkov <[email protected]>

Reviewed-by Paul E. McKenney <[email protected]>

Quoted 32 lines

---
 kernel/rcu/tree.c | 2 ++
 1 file changed, 2 insertions(+)

kernel/rcu/tree.c +2 -1

index 8ce77d9ac716..f78ee759af9c 100644
@@ -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

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) <[email protected]>

Quoted 10 lines · 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) <[email protected]>

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.

Quoted 10 lines · 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) <[email protected]>

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 ?

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