RE: [PATCH] x86: don't write_tsc() non-zero values on CPUs updating only the lower 32 bits

From: Dan Magenheimer <dan.magenheimer@oracle.com>
To: Keir Fraser <keir.xen@gmail.com>, Jan Beulich <JBeulich@novell.com>
Cc: xen-devel@lists.xensource.com,
	"winston.l.wang" <winston.l.wang@intel.com>
Subject: RE: [PATCH] x86: don't write_tsc() non-zero values on CPUs updating only the lower 32 bits
Date: Thu, 14 Apr 2011 15:41:20 -0700 (PDT)	[thread overview]
Message-ID: <7a5514b0-b3d6-478a-9434-b80758a7445d@default> (raw)
In-Reply-To: <C9CC7A6A.16558%keir.xen@gmail.com>

> -----Original Message-----
> From: Keir Fraser [mailto:keir.xen@gmail.com]
> Sent: Thursday, April 14, 2011 3:18 AM
> To: Jan Beulich
> Cc: winston.l.wang; xen-devel@lists.xensource.com; Dan Magenheimer
> Subject: Re: [Xen-devel] [PATCH] x86: don't write_tsc() non-zero values
> on CPUs updating only the lower 32 bits
> 
> On 14/04/2011 09:06, "Jan Beulich" <JBeulich@novell.com> wrote:
> 
> >> For physically-added CPUs only. Kind of unavoidable, that one: we
> can only
> >> try to do our best in that case. And let's face it, that probably
> affects
> >> exactly zero production users of Xen/x86 right now.
> >
> > That latter part I agree to.
> >
> > But what are you afraid of? Probing the TSC write shouldn't do any
> > harm.
> 
> You will end up with a BSP TSC value different than what it would have
> been
> if you had not probed. Since you write back a (slightly) stale TSC
> value.
> Which would increase cross-CPU TSC skew if the platform has done a good
> sync
> job at power-on.
> 
> Now, do I personally think this much matters? Not really, since I
> believe
> that direct guest TSC access is a huge non-issue. But it is something
> that
> Dan disagreed on, he did a bunch of work on time management, and the
> code
> as-is does try quite hard to avoid writing TSC if at all possible. I
> don't
> think it's a good idea to change this without feedback from Dan, at
> least.

My feedback is don't break what is fixed ;-)

The CPU vendors are trying REALLY hard to make TSC a very fast
synchronized-across-all-CPUs clocksource and Linux now does use
it that way if TSC_RELIABLE is set.

While it's not perfect, it's close enough as long as your recent
vintage machine doesn't have hot-plug CPUs or buggy firmware.

> > Additionally, did you read the comment immediately preceding
> > the probing code? AMD doesn't guarantee the TSC to be writable at
> > all.
> >
> >>> cstate_restore_tsc() also has no such gating afaics.
> >>
> >> It is gated on NONSTOP_TSC which is implied by TSC_RELIABLE.
> >
> > Ah, yes. But (I think) not architecturally, only by virtue of how
> > code is currently structured. If that changes, we'd be back at a
> > latent (and quite non-obvious) bug.
> 
> Yeah, if we want to continue to try avoiding write_tsc() on
> TSC_RELIABLE
> then we should assert !TSC_RELIABLE on the write_tsc() path in
> cstate_tsc_restore().

Agreed.  In fact, maybe it should be asserted in write_tsc?

Dan