| Message ID | 20200915103806.280265587@infradead.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,1/4] acpi: Use CPUIDLE_FLAG_TIMER_STOP | 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 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 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(-)