linux-pm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC][PATCH 0/4] Fix up ACPI processor idle vs RCU
@ 2020-09-15 10:31 Peter Zijlstra
  2020-09-15 10:31 ` [RFC][PATCH 1/4] acpi: Use CPUIDLE_FLAG_TIMER_STOP Peter Zijlstra
                   ` (4 more replies)
  0 siblings, 5 replies; 22+ messages in thread
From: Peter Zijlstra @ 2020-09-15 10:31 UTC (permalink / raw)
  To: rjw, bp
  Cc: x86, tony.luck, lenb, daniel.lezcano, linux-kernel, linux-acpi,
	linux-pm, ulf.hansson, paulmck, tglx, naresh.kamboju, peterz

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.


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [RFC][PATCH 1/4] acpi: Use CPUIDLE_FLAG_TIMER_STOP
  2020-09-15 10:31 [RFC][PATCH 0/4] Fix up ACPI processor idle vs RCU Peter Zijlstra
@ 2020-09-15 10:31 ` Peter Zijlstra
  2020-09-15 16:26   ` Rafael J. Wysocki
  2020-09-22  3:26   ` Guenter Roeck
  2020-09-15 10:31 ` [RFC][PATCH 2/4] acpi: Use CPUIDLE_FLAG_TLB_FLUSHED Peter Zijlstra
                   ` (3 subsequent siblings)
  4 siblings, 2 replies; 22+ messages in thread
From: Peter Zijlstra @ 2020-09-15 10:31 UTC (permalink / raw)
  To: rjw, bp
  Cc: x86, tony.luck, lenb, daniel.lezcano, linux-kernel, linux-acpi,
	linux-pm, ulf.hansson, paulmck, tglx, naresh.kamboju, peterz

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;



^ permalink raw reply	[flat|nested] 22+ messages in thread

* [RFC][PATCH 2/4] acpi: Use CPUIDLE_FLAG_TLB_FLUSHED
  2020-09-15 10:31 [RFC][PATCH 0/4] Fix up ACPI processor idle vs RCU Peter Zijlstra
  2020-09-15 10:31 ` [RFC][PATCH 1/4] acpi: Use CPUIDLE_FLAG_TIMER_STOP Peter Zijlstra
@ 2020-09-15 10:31 ` Peter Zijlstra
  2020-09-15 10:32 ` [RFC][PATCH 3/4] cpuidle: Allow cpuidle drivers to take over RCU-idle Peter Zijlstra
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 22+ messages in thread
From: Peter Zijlstra @ 2020-09-15 10:31 UTC (permalink / raw)
  To: rjw, bp
  Cc: x86, tony.luck, lenb, daniel.lezcano, linux-kernel, linux-acpi,
	linux-pm, ulf.hansson, paulmck, tglx, naresh.kamboju, peterz

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)



^ permalink raw reply	[flat|nested] 22+ messages in thread

* [RFC][PATCH 3/4] cpuidle: Allow cpuidle drivers to take over RCU-idle
  2020-09-15 10:31 [RFC][PATCH 0/4] Fix up ACPI processor idle vs RCU Peter Zijlstra
  2020-09-15 10:31 ` [RFC][PATCH 1/4] acpi: Use CPUIDLE_FLAG_TIMER_STOP Peter Zijlstra
  2020-09-15 10:31 ` [RFC][PATCH 2/4] acpi: Use CPUIDLE_FLAG_TLB_FLUSHED Peter Zijlstra
@ 2020-09-15 10:32 ` Peter Zijlstra
  2020-09-15 13:20   ` Ulf Hansson
  2020-09-15 10:32 ` [RFC][PATCH 4/4] acpi: Take over RCU-idle for C3-BM idle Peter Zijlstra
  2020-09-15 18:31 ` [RFC][PATCH 0/4] Fix up ACPI processor idle vs RCU Borislav Petkov
  4 siblings, 1 reply; 22+ messages in thread
From: Peter Zijlstra @ 2020-09-15 10:32 UTC (permalink / raw)
  To: rjw, bp
  Cc: x86, tony.luck, lenb, daniel.lezcano, linux-kernel, linux-acpi,
	linux-pm, ulf.hansson, paulmck, tglx, naresh.kamboju, peterz

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;



^ permalink raw reply	[flat|nested] 22+ messages in thread

* [RFC][PATCH 4/4] acpi: Take over RCU-idle for C3-BM idle
  2020-09-15 10:31 [RFC][PATCH 0/4] Fix up ACPI processor idle vs RCU Peter Zijlstra
                   ` (2 preceding siblings ...)
  2020-09-15 10:32 ` [RFC][PATCH 3/4] cpuidle: Allow cpuidle drivers to take over RCU-idle Peter Zijlstra
@ 2020-09-15 10:32 ` Peter Zijlstra
  2020-09-21  9:12   ` Sven Joachim
  2020-09-25 15:20   ` [RFC][PATCH 4/4] acpi: Take over RCU-idle for C3-BM idle Guenter Roeck
  2020-09-15 18:31 ` [RFC][PATCH 0/4] Fix up ACPI processor idle vs RCU Borislav Petkov
  4 siblings, 2 replies; 22+ messages in thread
From: Peter Zijlstra @ 2020-09-15 10:32 UTC (permalink / raw)
  To: rjw, bp
  Cc: x86, tony.luck, lenb, daniel.lezcano, linux-kernel, linux-acpi,
	linux-pm, ulf.hansson, paulmck, tglx, naresh.kamboju, peterz

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)



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC][PATCH 3/4] cpuidle: Allow cpuidle drivers to take over RCU-idle
  2020-09-15 10:32 ` [RFC][PATCH 3/4] cpuidle: Allow cpuidle drivers to take over RCU-idle Peter Zijlstra
@ 2020-09-15 13:20   ` Ulf Hansson
  0 siblings, 0 replies; 22+ messages in thread
From: Ulf Hansson @ 2020-09-15 13:20 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Rafael J. Wysocki, Borislav Petkov, x86, Tony Luck, Len Brown,
	Daniel Lezcano, Linux Kernel Mailing List,
	ACPI Devel Maling List, Linux PM, Paul E. McKenney,
	Thomas Gleixner, Naresh Kamboju

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC][PATCH 1/4] acpi: Use CPUIDLE_FLAG_TIMER_STOP
  2020-09-15 10:31 ` [RFC][PATCH 1/4] acpi: Use CPUIDLE_FLAG_TIMER_STOP Peter Zijlstra
@ 2020-09-15 16:26   ` Rafael J. Wysocki
  2020-09-16 15:42     ` peterz
  2020-09-22  3:26   ` Guenter Roeck
  1 sibling, 1 reply; 22+ messages in thread
From: Rafael J. Wysocki @ 2020-09-15 16:26 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Rafael J. Wysocki, Borislav Petkov, the arch/x86 maintainers,
	Tony Luck, Len Brown, Daniel Lezcano, Linux Kernel Mailing List,
	ACPI Devel Maling List, Linux PM, Ulf Hansson, Paul E. McKenney,
	Thomas Gleixner, Naresh Kamboju

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC][PATCH 0/4] Fix up ACPI processor idle vs RCU
  2020-09-15 10:31 [RFC][PATCH 0/4] Fix up ACPI processor idle vs RCU Peter Zijlstra
                   ` (3 preceding siblings ...)
  2020-09-15 10:32 ` [RFC][PATCH 4/4] acpi: Take over RCU-idle for C3-BM idle Peter Zijlstra
@ 2020-09-15 18:31 ` Borislav Petkov
  4 siblings, 0 replies; 22+ messages in thread
From: Borislav Petkov @ 2020-09-15 18:31 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: rjw, x86, tony.luck, lenb, daniel.lezcano, linux-kernel,
	linux-acpi, linux-pm, ulf.hansson, paulmck, tglx, naresh.kamboju

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC][PATCH 1/4] acpi: Use CPUIDLE_FLAG_TIMER_STOP
  2020-09-15 16:26   ` Rafael J. Wysocki
@ 2020-09-16 15:42     ` peterz
  2020-09-16 16:01       ` Borislav Petkov
  2020-09-16 17:38       ` Rafael J. Wysocki
  0 siblings, 2 replies; 22+ messages in thread
From: peterz @ 2020-09-16 15:42 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Rafael J. Wysocki, Borislav Petkov, the arch/x86 maintainers,
	Tony Luck, Len Brown, Daniel Lezcano, Linux Kernel Mailing List,
	ACPI Devel Maling List, Linux PM, Ulf Hansson, Paul E. McKenney,
	Thomas Gleixner, Naresh Kamboju

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.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC][PATCH 1/4] acpi: Use CPUIDLE_FLAG_TIMER_STOP
  2020-09-16 15:42     ` peterz
@ 2020-09-16 16:01       ` Borislav Petkov
  2020-09-16 17:38         ` Rafael J. Wysocki
  2020-09-16 17:38       ` Rafael J. Wysocki
  1 sibling, 1 reply; 22+ messages in thread
From: Borislav Petkov @ 2020-09-16 16:01 UTC (permalink / raw)
  To: peterz
  Cc: Rafael J. Wysocki, Rafael J. Wysocki, the arch/x86 maintainers,
	Tony Luck, Len Brown, Daniel Lezcano, Linux Kernel Mailing List,
	ACPI Devel Maling List, Linux PM, Ulf Hansson, Paul E. McKenney,
	Thomas Gleixner, Naresh Kamboju

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC][PATCH 1/4] acpi: Use CPUIDLE_FLAG_TIMER_STOP
  2020-09-16 15:42     ` peterz
  2020-09-16 16:01       ` Borislav Petkov
@ 2020-09-16 17:38       ` Rafael J. Wysocki
  1 sibling, 0 replies; 22+ messages in thread
From: Rafael J. Wysocki @ 2020-09-16 17:38 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Rafael J. Wysocki, Rafael J. Wysocki, Borislav Petkov,
	the arch/x86 maintainers, Tony Luck, Len Brown, Daniel Lezcano,
	Linux Kernel Mailing List, ACPI Devel Maling List, Linux PM,
	Ulf Hansson, Paul E. McKenney, Thomas Gleixner, Naresh Kamboju

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!

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC][PATCH 1/4] acpi: Use CPUIDLE_FLAG_TIMER_STOP
  2020-09-16 16:01       ` Borislav Petkov
@ 2020-09-16 17:38         ` Rafael J. Wysocki
  0 siblings, 0 replies; 22+ messages in thread
From: Rafael J. Wysocki @ 2020-09-16 17:38 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Peter Zijlstra, Rafael J. Wysocki, Rafael J. Wysocki,
	the arch/x86 maintainers, Tony Luck, Len Brown, Daniel Lezcano,
	Linux Kernel Mailing List, ACPI Devel Maling List, Linux PM,
	Ulf Hansson, Paul E. McKenney, Thomas Gleixner, Naresh Kamboju

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.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC][PATCH 4/4] acpi: Take over RCU-idle for C3-BM idle
  2020-09-15 10:32 ` [RFC][PATCH 4/4] acpi: Take over RCU-idle for C3-BM idle Peter Zijlstra
@ 2020-09-21  9:12   ` Sven Joachim
  2020-09-21 10:37     ` [PATCH] rcu/tree: Export rcu_idle_{enter,exit} to module Borislav Petkov
  2020-09-25 15:20   ` [RFC][PATCH 4/4] acpi: Take over RCU-idle for C3-BM idle Guenter Roeck
  1 sibling, 1 reply; 22+ messages in thread
From: Sven Joachim @ 2020-09-21  9:12 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: rjw, bp, x86, tony.luck, lenb, daniel.lezcano, linux-kernel,
	linux-acpi, linux-pm, ulf.hansson, paulmck, tglx, naresh.kamboju

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)

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH] rcu/tree: Export rcu_idle_{enter,exit} to module
  2020-09-21  9:12   ` Sven Joachim
@ 2020-09-21 10:37     ` Borislav Petkov
  2020-09-21 12:15       ` Rafael J. Wysocki
  2020-09-21 13:32       ` Paul E. McKenney
  0 siblings, 2 replies; 22+ messages in thread
From: Borislav Petkov @ 2020-09-21 10:37 UTC (permalink / raw)
  To: Sven Joachim
  Cc: Peter Zijlstra, rjw, x86, tony.luck, lenb, daniel.lezcano,
	linux-kernel, linux-acpi, linux-pm, ulf.hansson, paulmck, tglx,
	naresh.kamboju, Josh Triplett, Steven Rostedt, Mathieu Desnoyers,
	Lai Jiangshan, Joel Fernandes, rcu

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

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH] rcu/tree: Export rcu_idle_{enter,exit} to module
  2020-09-21 10:37     ` [PATCH] rcu/tree: Export rcu_idle_{enter,exit} to module Borislav Petkov
@ 2020-09-21 12:15       ` Rafael J. Wysocki
  2020-09-21 13:32       ` Paul E. McKenney
  1 sibling, 0 replies; 22+ messages in thread
From: Rafael J. Wysocki @ 2020-09-21 12:15 UTC (permalink / raw)
  To: Borislav Petkov, Paul E. McKenney
  Cc: Sven Joachim, Peter Zijlstra, Rafael J. Wysocki,
	the arch/x86 maintainers, Tony Luck, Len Brown, Daniel Lezcano,
	Linux Kernel Mailing List, ACPI Devel Maling List, Linux PM,
	Ulf Hansson, Thomas Gleixner, Naresh Kamboju, Josh Triplett,
	Steven Rostedt, Mathieu Desnoyers, Lai Jiangshan, Joel Fernandes,
	rcu

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] rcu/tree: Export rcu_idle_{enter,exit} to module
  2020-09-21 10:37     ` [PATCH] rcu/tree: Export rcu_idle_{enter,exit} to module Borislav Petkov
  2020-09-21 12:15       ` Rafael J. Wysocki
@ 2020-09-21 13:32       ` Paul E. McKenney
  2020-09-21 13:38         ` Rafael J. Wysocki
  1 sibling, 1 reply; 22+ messages in thread
From: Paul E. McKenney @ 2020-09-21 13:32 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Sven Joachim, Peter Zijlstra, rjw, x86, tony.luck, lenb,
	daniel.lezcano, linux-kernel, linux-acpi, linux-pm, ulf.hansson,
	tglx, naresh.kamboju, Josh Triplett, Steven Rostedt,
	Mathieu Desnoyers, Lai Jiangshan, Joel Fernandes, rcu

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] rcu/tree: Export rcu_idle_{enter,exit} to module
  2020-09-21 13:32       ` Paul E. McKenney
@ 2020-09-21 13:38         ` Rafael J. Wysocki
  0 siblings, 0 replies; 22+ messages in thread
From: Rafael J. Wysocki @ 2020-09-21 13:38 UTC (permalink / raw)
  To: Paul E. McKenney, Borislav Petkov
  Cc: Sven Joachim, Peter Zijlstra, Rafael J. Wysocki,
	the arch/x86 maintainers, Tony Luck, Len Brown, Daniel Lezcano,
	Linux Kernel Mailing List, ACPI Devel Maling List, Linux PM,
	Ulf Hansson, Thomas Gleixner, Naresh Kamboju, Josh Triplett,
	Steven Rostedt, Mathieu Desnoyers, Lai Jiangshan, Joel Fernandes,
	rcu

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC][PATCH 1/4] acpi: Use CPUIDLE_FLAG_TIMER_STOP
  2020-09-15 10:31 ` [RFC][PATCH 1/4] acpi: Use CPUIDLE_FLAG_TIMER_STOP Peter Zijlstra
  2020-09-15 16:26   ` Rafael J. Wysocki
@ 2020-09-22  3:26   ` Guenter Roeck
  2020-09-22 19:03     ` Rafael J. Wysocki
  1 sibling, 1 reply; 22+ messages in thread
From: Guenter Roeck @ 2020-09-22  3:26 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: rjw, bp, x86, tony.luck, lenb, daniel.lezcano, linux-kernel,
	linux-acpi, linux-pm, ulf.hansson, paulmck, tglx, naresh.kamboju

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;

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC][PATCH 1/4] acpi: Use CPUIDLE_FLAG_TIMER_STOP
  2020-09-22  3:26   ` Guenter Roeck
@ 2020-09-22 19:03     ` Rafael J. Wysocki
  0 siblings, 0 replies; 22+ messages in thread
From: Rafael J. Wysocki @ 2020-09-22 19:03 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Peter Zijlstra, bp, x86, tony.luck, lenb, daniel.lezcano,
	linux-kernel, linux-acpi, linux-pm, ulf.hansson, paulmck, tglx,
	naresh.kamboju

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




^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC][PATCH 4/4] acpi: Take over RCU-idle for C3-BM idle
  2020-09-15 10:32 ` [RFC][PATCH 4/4] acpi: Take over RCU-idle for C3-BM idle Peter Zijlstra
  2020-09-21  9:12   ` Sven Joachim
@ 2020-09-25 15:20   ` Guenter Roeck
  2020-09-25 15:24     ` Rafael J. Wysocki
  2020-09-25 15:29     ` Paul E. McKenney
  1 sibling, 2 replies; 22+ messages in thread
From: Guenter Roeck @ 2020-09-25 15:20 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: rjw, bp, x86, tony.luck, lenb, daniel.lezcano, linux-kernel,
	linux-acpi, linux-pm, ulf.hansson, paulmck, tglx, naresh.kamboju

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC][PATCH 4/4] acpi: Take over RCU-idle for C3-BM idle
  2020-09-25 15:20   ` [RFC][PATCH 4/4] acpi: Take over RCU-idle for C3-BM idle Guenter Roeck
@ 2020-09-25 15:24     ` Rafael J. Wysocki
  2020-09-25 15:29     ` Paul E. McKenney
  1 sibling, 0 replies; 22+ messages in thread
From: Rafael J. Wysocki @ 2020-09-25 15:24 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Peter Zijlstra, Rafael J. Wysocki, Borislav Petkov,
	the arch/x86 maintainers, Tony Luck, Len Brown, Daniel Lezcano,
	Linux Kernel Mailing List, ACPI Devel Maling List, Linux PM,
	Ulf Hansson, Paul E. McKenney, Thomas Gleixner, Naresh Kamboju

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!

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC][PATCH 4/4] acpi: Take over RCU-idle for C3-BM idle
  2020-09-25 15:20   ` [RFC][PATCH 4/4] acpi: Take over RCU-idle for C3-BM idle Guenter Roeck
  2020-09-25 15:24     ` Rafael J. Wysocki
@ 2020-09-25 15:29     ` Paul E. McKenney
  1 sibling, 0 replies; 22+ messages in thread
From: Paul E. McKenney @ 2020-09-25 15:29 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Peter Zijlstra, rjw, bp, x86, tony.luck, lenb, daniel.lezcano,
	linux-kernel, linux-acpi, linux-pm, ulf.hansson, tglx,
	naresh.kamboju

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2020-09-25 15:29 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-15 10:31 [RFC][PATCH 0/4] Fix up ACPI processor idle vs RCU Peter Zijlstra
2020-09-15 10:31 ` [RFC][PATCH 1/4] acpi: Use CPUIDLE_FLAG_TIMER_STOP Peter Zijlstra
2020-09-15 16:26   ` Rafael J. Wysocki
2020-09-16 15:42     ` peterz
2020-09-16 16:01       ` Borislav Petkov
2020-09-16 17:38         ` Rafael J. Wysocki
2020-09-16 17:38       ` Rafael J. Wysocki
2020-09-22  3:26   ` Guenter Roeck
2020-09-22 19:03     ` Rafael J. Wysocki
2020-09-15 10:31 ` [RFC][PATCH 2/4] acpi: Use CPUIDLE_FLAG_TLB_FLUSHED Peter Zijlstra
2020-09-15 10:32 ` [RFC][PATCH 3/4] cpuidle: Allow cpuidle drivers to take over RCU-idle Peter Zijlstra
2020-09-15 13:20   ` Ulf Hansson
2020-09-15 10:32 ` [RFC][PATCH 4/4] acpi: Take over RCU-idle for C3-BM idle Peter Zijlstra
2020-09-21  9:12   ` Sven Joachim
2020-09-21 10:37     ` [PATCH] rcu/tree: Export rcu_idle_{enter,exit} to module Borislav Petkov
2020-09-21 12:15       ` Rafael J. Wysocki
2020-09-21 13:32       ` Paul E. McKenney
2020-09-21 13:38         ` Rafael J. Wysocki
2020-09-25 15:20   ` [RFC][PATCH 4/4] acpi: Take over RCU-idle for C3-BM idle Guenter Roeck
2020-09-25 15:24     ` Rafael J. Wysocki
2020-09-25 15:29     ` Paul E. McKenney
2020-09-15 18:31 ` [RFC][PATCH 0/4] Fix up ACPI processor idle vs RCU Borislav Petkov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).