Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1531850 > unrolled thread

RE: [PATCH][RFC] ACPI throttling: Save/restore tstate for each CPUs across suspend/resume

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  RE: [PATCH][RFC] ACPI throttling: Save/restore tstate for each CPUs  across suspend/resume "Chen, Yu C" <yu.c.chen@intel.com> - 2016-11-29 02:30 +0100
    Re: [PATCH][RFC] ACPI throttling: Save/restore tstate for each CPUs  across suspend/resume "Rafael J. Wysocki" <rafael@kernel.org> - 2016-11-29 03:00 +0100

#1531850 — RE: [PATCH][RFC] ACPI throttling: Save/restore tstate for each CPUs across suspend/resume

Hi,
> -----Original Message-----
> From: rjwysocki@gmail.com [mailto:rjwysocki@gmail.com] On Behalf Of
> Rafael J. Wysocki
> Sent: Wednesday, November 23, 2016 7:03 AM
> To: Chen, Yu C
> Cc: ACPI Devel Maling List; Rui Wang; Linux Kernel Mailing List; Len Brown;
> Rafael J. Wysocki; Pavel Machek; Matthew Garrett; Zhang, Rui; Linux PM
> Subject: Re: [PATCH][RFC] ACPI throttling: Save/restore tstate for each CPUs
> across suspend/resume
> 
> On Mon, Nov 14, 2016 at 6:44 PM, Chen Yu <yu.c.chen@intel.com> wrote:
> > This is a trial version and any comments are appreciated.
> >
> > Previously a bug was reported that on certain Broadwell platforms,
> > after resuming from S3, the CPU is running at an anomalously low
> > speed, due to BIOS has enabled the throttling across S3. The solution
> > to this is to introduce a quirk framework to save/restore tstate MSR
> > register around suspend/resume, in Commit 7a9c2dd08ead ("x86/pm:
> > Introduce quirk framework to save/restore extra MSR registers around
> > suspend/resume").
> >
> > However more and more reports show that other platforms also
> > experienced the same issue, because some BIOSes would like to adjust
> > the tstate if he thinks the temperature is too high.
> > To deal with this situation, the Linux uses a compensation strategy
> > that, the thermal management leverages thermal_pm_notify() upon resume
> > to check if the Processors inside the thermal zone should be throttled
> > or not, thus tstate would be re-evaluated. Unfortunately on these
> > bogus platforms, none of the Processors are inside any thermal zones
> > due to BIOS's implementation. Thus tstate for Processors never has a
> > chance to be brought back to normal.
> >
> > This patch tries to save/restore tstate on receiving the
> > PM_SUSPEND_PREPARE and PM_POST_SUSPEND, to be more specific, the
> > tstate is saved after thermal_pm_notify(PM_SUSPEND_PREPARE)
> > is called, while it's restored before
> > thermal_pm_notify(PM_POST_SUSPEND),
> > in this way the thermal zone would adjust the tstate eventually and
> > also help adjust the tstate for Processors which do not have thermal
> > zone bound. Thus it does not imapct the old semantics.
> >
> > Another concern is that, each CPU should take care of the save/restore
> > operation, thus this patch uses percpu workqueue to achieve this.
> >
> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=90041
> > Reported-by: Matthew Garrett <mjg59@srcf.ucam.org>
> > Reported-by: Kadir <kadir@colakoglu.nl>
> > Cc: Len Brown <lenb@kernel.org>
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > Cc: Pavel Machek <pavel@ucw.cz>
> > Cc: Matthew Garrett <mjg59@srcf.ucam.org>
> > Cc: Rui Zhang <rui.zhang@intel.com>
> > Cc: linux-pm@vger.kernel.org
> > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> > ---
> >  drivers/acpi/processor_throttling.c | 70
> > +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 70 insertions(+)
> >
> > diff --git a/drivers/acpi/processor_throttling.c
> > b/drivers/acpi/processor_throttling.c
> > index d51ca1c..8ddc7d6 100644
> > --- a/drivers/acpi/processor_throttling.c
> > +++ b/drivers/acpi/processor_throttling.c
> > @@ -29,6 +29,7 @@
> >  #include <linux/sched.h>
> >  #include <linux/cpufreq.h>
> >  #include <linux/acpi.h>
> > +#include <linux/suspend.h>
> >  #include <acpi/processor.h>
> >  #include <asm/io.h>
> >  #include <asm/uaccess.h>
> > @@ -758,6 +759,75 @@ static int acpi_throttling_wrmsr(u64 value)
> >         }
> >         return ret;
> >  }
> > +
> > +#ifdef CONFIG_PM_SLEEP
> > +static DEFINE_PER_CPU(u64, tstate_msr);
> 
> Call it saved_tstate_msr maybe?
OK.
> 
> > +
> > +static long tstate_pm_fn(void *data)
> > +{
> > +       u64 value;
> > +       bool save = *(bool *)data;
> > +
> > +       if (save) {
> > +               acpi_throttling_rdmsr(&value);
> > +               this_cpu_write(tstate_msr, value);
> > +       } else {
> > +               value = this_cpu_read(tstate_msr);
> > +               if (value)
> > +                       acpi_throttling_wrmsr(value);
> > +       }
> > +       return 0;
> > +}
> 
> I would split the above into two functions, one for saving and one for restoring ->
> 
OK.
> > +
> > +static void tstate_check(unsigned long mode, bool suspend) {
> > +       int cpu;
> > +       bool save;
> > +
> > +       if (suspend && mode == PM_SUSPEND_PREPARE)
> > +               save = true;
> > +       else if (!suspend && mode == PM_POST_SUSPEND)
> > +               save = false;
> > +       else
> > +               return;
> > +
> > +       get_online_cpus();
> > +       for_each_online_cpu(cpu)
> 
> -> and decide here which one to invoke.
OK.
> 
> > +               work_on_cpu(cpu, tstate_pm_fn, &save);
> 
> Does work_on_cpu() wait for the work to complete?
> 
Yes, it might increase the suspend/resume time, a 'queue_work_on' might be better? 
> > +       put_online_cpus();
> > +}
> > +
> > +static int tstate_suspend(struct notifier_block *nb,
> > +                               unsigned long mode, void *_unused) {
> > +       tstate_check(mode, true);
> > +       return 0;
> > +}
> > +
> > +static int tstate_resume(struct notifier_block *nb,
> > +                               unsigned long mode, void *_unused) {
> > +       tstate_check(mode, false);
> > +       return 0;
> > +}
> > +
> > +static int __init tstate_pm_init(void) {
> > +       /*
> > +        * tstate_suspend should save tstate after
> > +        * thermal zone's update in thermal_pm_notify,
> > +        * vice versa tstate_resume restore tstate before
> > +        * thermal_pm_notify, thus the thermal framework
> > +        * has a chance to re-adjust tstate according to the
> > +        * temperature trend.
> > +        */
> > +       pm_notifier(tstate_suspend, -1);
> > +       pm_notifier(tstate_resume, 1);
> 
> I don't think this is going to do what you really want.
> 
> Each of these notifiers is going to be invoked during both suspend and resume,
Yes,
> so I guess you only need one notifier?
Here's my original thought:  tstate_suspend needs to be invoked after
 thermal_pm_notify, which has a priority of '0',
so the notifier of tstate_suspend should be lower than '0', 
thus '-1'. And the same for tstate_resume,
it should be invoked before thermal_pm_notify, 
thus priority is '1' ?
> 
> > +       return 0;
> > +}
> > +
> > +core_initcall(tstate_pm_init);
> > +#endif
> >  #else
> >  static int acpi_throttling_rdmsr(u64 *value)  {
> > --
> 
> Thanks,
> Rafael

Thanks,
Yu

[toc] | [next] | [standalone]

#1531856

On Tue, Nov 29, 2016 at 2:27 AM, Chen, Yu C <yu.c.chen@intel.com> wrote:
> Hi,
>> -----Original Message-----
>> From: rjwysocki@gmail.com [mailto:rjwysocki@gmail.com] On Behalf Of
>> Rafael J. Wysocki
>> Sent: Wednesday, November 23, 2016 7:03 AM

[cut]

>>
>> > +               work_on_cpu(cpu, tstate_pm_fn, &save);
>>
>> Does work_on_cpu() wait for the work to complete?
>>
> Yes, it might increase the suspend/resume time, a 'queue_work_on' might be better?

Not really, because you'd need to wait before doing the
put_online_cpus() anyway.

>> > +       put_online_cpus();
>> > +}
>> > +
>> > +static int tstate_suspend(struct notifier_block *nb,
>> > +                               unsigned long mode, void *_unused) {
>> > +       tstate_check(mode, true);
>> > +       return 0;
>> > +}
>> > +
>> > +static int tstate_resume(struct notifier_block *nb,
>> > +                               unsigned long mode, void *_unused) {
>> > +       tstate_check(mode, false);
>> > +       return 0;
>> > +}
>> > +
>> > +static int __init tstate_pm_init(void) {
>> > +       /*
>> > +        * tstate_suspend should save tstate after
>> > +        * thermal zone's update in thermal_pm_notify,
>> > +        * vice versa tstate_resume restore tstate before
>> > +        * thermal_pm_notify, thus the thermal framework
>> > +        * has a chance to re-adjust tstate according to the
>> > +        * temperature trend.
>> > +        */
>> > +       pm_notifier(tstate_suspend, -1);
>> > +       pm_notifier(tstate_resume, 1);
>>
>> I don't think this is going to do what you really want.
>>
>> Each of these notifiers is going to be invoked during both suspend and resume,
> Yes,
>
>> so I guess you only need one notifier?
>
> Here's my original thought:  tstate_suspend needs to be invoked after
>  thermal_pm_notify, which has a priority of '0',
> so the notifier of tstate_suspend should be lower than '0',
> thus '-1'. And the same for tstate_resume,
> it should be invoked before thermal_pm_notify,
> thus priority is '1' ?

If there's a dependency like that, it needs to be explicit.  That is,
thermal_pm_notify() needs to invoke your new code in the right order
with respect to what it does already.

Thanks,
Rafael

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web