[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <53E0A3EE.50708@intel.com>
Date: Tue, 05 Aug 2014 17:29:18 +0800
From: Lan Tianyu <tianyu.lan@...el.com>
To: Borislav Petkov <bp@...en8.de>
CC: tglx@...utronix.de, mingo@...hat.com, hpa@...or.com,
x86@...nel.org, toshi.kani@...com, imammedo@...hat.com,
jan.kiszka@...mens.com, mingo@...nel.org, huawei.libin@...wei.com,
prarit@...hat.com, linux-kernel@...r.kernel.org,
"Brandt, Todd E" <todd.e.brandt@...el.com>
Subject: Re: [PATCH] X86/CPU: Avoid 100ms sleep for cpu offline during S3
On 2014年08月05日 15:54, Borislav Petkov wrote:
> Hi Tianyu,
>
> On Tue, Aug 05, 2014 at 10:41:27AM +0800, Lan Tianyu wrote:
>> Thanks for your review. We are doing S2RAM optimization.
>> We usually use AnalyzeSuspend tool from Brandt, Todd E to observe
>> the time consume during S2RAM.(https://github.com/01org/suspendresume.git)
>>
>> I attached the original result from the tool which showed cpu offline consumes
>> more than 100ms. This is due to msleep in the native_cpu_die(). The
>> improvement.html shows the test result with the patch. You can't find cpu
>> offline at first glance and need to click zoom-in button because the consume
>> time of cpu offline is reduced to less than 5ms.
>
> The fact that I can't find it is a good thing, right? :-)
Yes, you can say that. :)
>
> Ok, this looks nice, it actually shows an improvement from cpus going
> offline for about 100ish msec and that duration shrinking down to 1.5
> msec on average which is a ~100x improv in my book.
>
> And the total S/R time diff looks really good too.
>
> Can people do those measurements outside of your lab? Because if they
> can, I could do some on my boxes here too :)
I think you can pull Tod's tool from git hub and do test from your
laptop. I attached a configure file come from ubuntu which is easier
to produce the issue. You can use it to reproduce the issue.
>
> Now, what you could do is run this on a couple of systems, if possible,
> write down in the commit message how exactly you did it and add some
> relevant numbers to show the speedup. Because this completion thing is
> definitely worth pursuing further, provided it doesn't break suspend in
> some weird ways.
Ok. BTW, I have tested it on the Ivbridge and Haswell machines.
>
> Btw, the original patches which added the 100ms msleep are:
>
> ef6e525393db ("[PATCH] x86_64: Use msleep in smpboot.c")
> aeb8397b6a28 ("[PATCH] i386/smpboot: use msleep() instead of schedule_timeout()")
>
> and its commit message is, of course, :-( not very telling. WTF does
> "task delays as expected" mean? I have to go poke peterz for an idea
> what it might've meant...
>
> So do you see what I mean with writing a good, verbose commit message,
> explaining the situation?
>
> It needs to explain not what we did but why we did it years from now so
> that we know exactly if we have to go touch that code again.
Ok.
>
> HTH.
>
--
Best regards
Tianyu Lan
View attachment "config_file" of type "text/plain" (170503 bytes)
Powered by blists - more mailing lists