| Message ID | 20200915103806.280265587@infradead.org (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
| Series | Fix up ACPI processor idle vs RCU | expand |
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; > >
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.
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.
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!
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.
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;
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
--- 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;
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(-)