lkml.org 
[lkml]   [2016]   [Nov]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: [PATCH][RFC] ACPI throttling: Save/restore tstate for each CPUs across suspend/resume
    Date
    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
    \
     
     \ /
      Last update: 2016-11-29 02:28    [W:2.234 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site