Xen devel - Apr 2011 - Re: Re: system freeze when processor.ko is loaded

Hi Jan,
> Date: Wed, 06 Apr 2011 10:58:59 +0100
> From: "Jan Beulich" <JBeulich@novell.com>
> Subject: Re: [Xen-devel] Re: system freeze when processor.ko is loaded
> 	during boot
> To: "Keir Fraser" <keir@xen.org>
> Cc: Jinsong Liu <jinsong.liu@intel.com>,
> 	"xen-devel@lists.xensource.com"
<xen-devel@lists.xensource.com>,
> 	Yunhong Jiang <yunhong.jiang@intel.com>,	Donald D Dugger
> 	<donald.d.dugger@intel.com>, Xin Li <xin.li@intel.com>,
> 	Haitao Shan
> 	<maillists.shan@gmail.com>,	Gang Wei <gang.wei@intel.com>,
> Martin
> 	Wilck <mwilck@arcor.de>
> Message-ID: <4D9C5583020000780003A268@vpn.id2.novell.com>
> Content-Type: text/plain; charset=US-ASCII
> 
> >>> On 04.04.11 at 11:22, "Jan Beulich"
<JBeulich@novell.com> wrote:
> > Haitao, while it is quite clear that with the current
> > implementation we just can''t use C states above C1 on CPUs
> > that may halt the TSC in C2 or C3 *and* that don''t allow
> > writing the full TSC, this family/model based determination
> > clearly isn''t nice (and since it is a white list, it
can''t possibly
> be
> > complete). An alternative would seem to be to probe for how
> > TSC writes behave (thus at once covering eventual other
> > vendors'' CPUs that may have similar shortcomings). That of
> > course would need to be done early, so that resetting the
> > upper bits to zero wouldn''t have any adverse effect. What
> > do you think?
> 
> The probing itself seems to work fine. I''m confused by something
> else though: synchronize_tsc_{master,slave}() execute their
> loops (at boot or during hotplug) on any CPU that doesn''t have
> X86_FEATURE_TSC_RELIABLE, including such where TSC writes
> don''t really work (luckily I still haven''t thrown out one
that is
> affected by this). What is the point of doing this synchronization
> if we can happily live with it actually not working (Xen runs fine
> on that box afaict)? c/s 21468:26c2922da53c is also not very
> verbose about why this got (re-)added... Should the body
> perhaps really only be run for X86_FEATURE_CONSTANT_TSC but
> !X86_FEATURE_NONSTOP_TSC CPUs?
> 
> Jan
> 
Thanks for the great effort for root cause the issue!
I think restore only the lower 32bit TSC is good enough, why do we need to touch
upper 32 bit TSC?
1. I would not think if any processor''s deep c-state wakeup from idle
can more than 100 ms.
2. For 3GHZ processor lower 32bit TSC wrapper around time is ~2.83 Sec.
3. The platform timer (22bit ACPI timer) wrapper around is 2.34 sec (this is
used for counting the delta before enter deep_cstate and before wakeup)
Just need to change cstate_restore_tsc() and only patch back the delta time to
the lower 32 bit TSC to make that simple?

Winston,

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> On 12.04.11 at 00:35, "Wang, Winston L"
<winston.l.wang@intel.com> wrote:
> I think restore only the lower 32bit TSC is good enough, why do we need to 
> touch upper 32 bit TSC?
You just can''t: A wrmsr to this MSR always updates the full 64-bits,
just that on some CPUs this update implies the upper 32 bits getting
zeroed.

And even if you could, it wouldn''t be easy to deal with the situation
where the update would carry into the upper 32 bits.

Jan
> 1. I would not think if any processor''s deep c-state wakeup from
idle can
> more than 100 ms.
> 2. For 3GHZ processor lower 32bit TSC wrapper around time is ~2.83 Sec.
> 3. The platform timer (22bit ACPI timer) wrapper around is 2.34 sec (this
is
> used for counting the delta before enter deep_cstate and before wakeup)
> Just need to change cstate_restore_tsc() and only patch back the delta time
> to the lower 32 bit TSC to make that simple?
> 
> Winston,



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> >>> On 12.04.11 at 00:35, "Wang, Winston L"
<winston.l.wang@intel.com>
> wrote:
> > I think restore only the lower 32bit TSC is good enough, why do we
> need to touch upper 32 bit TSC?
> 
> You just can''t: A wrmsr to this MSR always updates the full
64-bits,
> just that on some CPUs this update implies the upper 32 bits getting
> zeroed.
You are right, just can''t only write lower 32 bit without updating 32
bit since TSC is always 64 bits:(
So we have to give up the deep C state for those old processors since we still
need to maintain TSC all the time for SMP support.
The solution should go with your patch: re-validate the processor boot with
" X86_FEATURE_TSC_RELIABLE" to see upper 32 bit TSC MSR is writeable
at init_xen_time or not , then decide if allow deep_cstate. This is a better
idea than only allow the processor going to the deep c state with
"x86_FEATURE_CONSTANT_TSC but !X86_FEATURE_NONSTOP_TSC CPUs".

Would you double test patch before pulling in? we need to check both the old
failed processor and the current one. And what''s the power idle power
impact.
> 
> And even if you could, it wouldn''t be easy to deal with the
situation
> where the update would carry into the upper 32 bits.
> 
> Jan
> 
> > 1. I would not think if any processor''s deep c-state wakeup
from idle
> can
> > more than 100 ms.
> > 2. For 3GHZ processor lower 32bit TSC wrapper around time is ~2.83
> Sec.
> > 3. The platform timer (22bit ACPI timer) wrapper around is 2.34 sec
> (this is
> > used for counting the delta before enter deep_cstate and before
> wakeup)
> > Just need to change cstate_restore_tsc() and only patch back the
> delta time
> > to the lower 32 bit TSC to make that simple?
> >
> > Winston,
> 
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 12/04/2011 18:27, "Wang, Winston L"
<winston.l.wang@intel.com> wrote:
>>>>> On 12.04.11 at 00:35, "Wang, Winston L"
<winston.l.wang@intel.com>
>> wrote:
>>> I think restore only the lower 32bit TSC is good enough, why do we
>> need to touch upper 32 bit TSC?
>> 
>> You just can''t: A wrmsr to this MSR always updates the full
64-bits,
>> just that on some CPUs this update implies the upper 32 bits getting
>> zeroed.
> You are right, just can''t only write lower 32 bit without updating
32 bit
> since TSC is always 64 bits:(
> So we have to give up the deep C state for those old processors since we
still
> need to maintain TSC all the time for SMP support.
> The solution should go with your patch: re-validate the processor boot with
"
> X86_FEATURE_TSC_RELIABLE" to see upper 32 bit TSC MSR is writeable at
> init_xen_time or not , then decide if allow deep_cstate. This is a better
idea
> than only allow the processor going to the deep c state with
> "x86_FEATURE_CONSTANT_TSC but !X86_FEATURE_NONSTOP_TSC CPUs".
> 
> Would you double test patch before pulling in? we need to check both the
old
> failed processor and the current one. And what''s the power idle
power impact.
Do you want a chance to Ack the patch before I apply it to xen-unstable?

 -- Keir
>> And even if you could, it wouldn''t be easy to deal with the
situation
>> where the update would carry into the upper 32 bits.
>> 
>> Jan
>> 
>>> 1. I would not think if any processor''s deep c-state
wakeup from idle
>> can
>>> more than 100 ms.
>>> 2. For 3GHZ processor lower 32bit TSC wrapper around time is ~2.83
>> Sec.
>>> 3. The platform timer (22bit ACPI timer) wrapper around is 2.34 sec
>> (this is
>>> used for counting the delta before enter deep_cstate and before
>> wakeup)
>>> Just need to change cstate_restore_tsc() and only patch back the
>> delta time
>>> to the lower 32 bit TSC to make that simple?
>>> 
>>> Winston,
>> 
>> 
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> On 12.04.11 at 19:27, "Wang, Winston L"
<winston.l.wang@intel.com> wrote:
> Would you double test patch before pulling in? we need to check both the
old
> failed processor and the current one. And what''s the power idle
power impact.
I''ll surely test the patch on the systems I have available, but for
the moment I''m still waiting to see whether it actually fixes the
problem at hand.

As to the power idle impact - I don''t think that matters here.
There''s
not going to be a change on unaffected CPUs, and on affected ones
we want them to be able to boot at all - caring of power savings is
clearly secondary (and honestly I don''t think anyone wants to waste
cycles on improving this for the few old processors that are still out
there and that Xen is being run on).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@novell.com]
> >>> On 12.04.11 at 19:27, "Wang, Winston L"
<winston.l.wang@intel.com>
> wrote:
> > Would you double test patch before pulling in? we need to check both
> the old
> > failed processor and the current one. And what''s the power
idle power
> impact.
> 
> I''ll surely test the patch on the systems I have available, but
for
> the moment I''m still waiting to see whether it actually fixes the
> problem at hand.
Sounds good, I don''t have the failed the processors
Martin, does Jan''s patch fix the problem of your failed
platform?
> 
> As to the power idle impact - I don''t think that matters here.
There''s
> not going to be a change on unaffected CPUs, and on affected ones
> we want them to be able to boot at all - caring of power savings is
> clearly secondary (and honestly I don''t think anyone wants to
waste
> cycles on improving this for the few old processors that are still out
> there and that Xen is being run on).
Agree.

Winston,

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Hi,
> Sounds good, I don''t have the failed the processors
> Martin, does Jan''s patch fix the problem of your failed platform?
Yes, Jan''s patch (on top of the OpenSUSE Xen release) worked well for
me.
>> clearly secondary (and honestly I don''t think anyone wants to
waste
>> cycles on improving this for the few old processors that are still out
>> there and that Xen is being run on).
> Agree.
As you may imagine, I am not too happy about that, but I understand that
the lead Xen developers have different priorities.

Martin

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel