[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 3/3] x86/time: don't move TSC backwards in time_calibration_tsc_rendezvous()

  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Mon, 8 Feb 2021 14:19:26 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=tV/26+qgpw3p+QQ995YyObvTlk8eaguLbR203PlOHrw=; b=ZABxf0UxZrsYIHYUlezTheiuMYAfHJfNjoZDk80KLNrNUtYfJHtua5iJQt5DU5RcKv9Yd+e5psDrNgd3OGLimFMFPTORL5Lt97PPvRMjN4JZ9vqk5K2f+eXiP/hjeAhNg17zV8V1PVuzGUR7/Z/Nk9ymrvfI81IFBWqVnzn2arhQK8lAB/ICKbJqe4hbXZO7V0W7C+/CZS4j9UbQOXcMwfOM2qTZ5YHanQ9LIFGENS6OJBsgjWLtSmGMatpn7Cgor1NorO2z1qHZpWSU5CLUdsoO9AyWL1eN6qRKU0eXVsatTrpIARtjrgjHql/WM5WMSZNpCV5Yo/j4OCWaDxL/iQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=DVz6PznaE3RTymjN6t2GZYjw//5j6yTM3bIFfdjcP9/fBIF8atc+0r+HiLOH84hEB0ik3py7mJ4dQMLhrgSuR6J4fiCbMJftAbEuTJJGNB2ye0MVZviiqThlDgu54mFh0t1Mqa9WT/hszXMSWpBcu2s5AasIHnJ7Z9CfwuxTiDlwGdLDTpMCSBAWiBk2WdEuUnjj1ZMsXX4am16vqBMiKeQbaLp+Q7VrdfaH0nBVfSS0Bv4H2AZpsjb9aL1mCZzG6ZUJTwsy6UJkO7ohqA4yoidjO2emQFFIEi/mKIG6ZgOGv3Owl5FfnYIhvm81yjlrT2RgVEy9H4bPFD/4wHp3mg==
  • Authentication-results: esa3.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, "Andrew Cooper" <andrew.cooper3@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Claudemir Todo Bom <claudemir@xxxxxxxxxxx>
  • Delivery-date: Mon, 08 Feb 2021 13:19:47 +0000
  • Ironport-sdr: CzNSBcy1REGjVGrIsJAxSyz1R8VXG9ExyALxSNh7SQavjoIJlTx0XggtyIfzXrbaidRyBBjX5f PUNGjHKkEPi4aTIU3Fs5Cy3VQoaJjTf0ariEpAEYH+oW68GujNVId18paHLGj57XnjE1wMhqGv 45ztj8yk6T+tvDoM70D4Nqay3y9MT7ffiLGIRQkpeOeBwGpWmA4j9M3mXfFpW3wx/Y3ZJIgK9C 45LZDH2V71Hs8aRcRvjuCDJY2Z57Mr5RIQwOJQZyVDRu7oVChHBrQXAdLC0l6Z6109Hz012sG7 NoU=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Mon, Feb 08, 2021 at 12:22:25PM +0100, Jan Beulich wrote:
> On 08.02.2021 10:38, Roger Pau Monné wrote:
> > On Mon, Feb 01, 2021 at 01:43:28PM +0100, Jan Beulich wrote:
> >> ---
> >> Since CPU0 reads its TSC last on the first iteration, if TSCs were
> >> perfectly sync-ed there shouldn't ever be a need to update. However,
> >> even on the TSC-reliable system I first tested this on (using
> >> "tsc=skewed" to get this rendezvous function into use in the first
> >> place) updates by up to several thousand clocks did happen. I wonder
> >> whether this points at some problem with the approach that I'm not (yet)
> >> seeing.
> > 
> > I'm confused by this, so on a system that had reliable TSCs, which
> > you forced to remove the reliable flag, and then you saw big
> > differences when doing the rendezvous?
> > 
> > That would seem to indicate that such system doesn't really have
> > reliable TSCs?
> I don't think so, no. This can easily be a timing effect from the
> heavy cache line bouncing involved here.
> What I'm worried here seeing these updates is that I might still
> be moving TSCs backwards in ways observable to the rest of the
> system (i.e. beyond the inherent property of the approach), and
> this then getting corrected by a subsequent rendezvous. But as
> said - I can't see what this could result from, and hence I'm
> inclined to assume these are merely effects I've not found a
> good explanation for so far.

I'm slightly worried by this, maybe because I'm misunderstanding part
of the TSC sync stuff.

So you forced a system that Xen would otherwise consider to have a
reliable TSC (one that doesn't need a calibration rendezvous) into
doing the calibration rendezvous, and then the skew seen is quite big.
I would expect such skew to be minimal? As we would otherwise consider
the system to not need calibration at all.

This makes me wonder if the system does indeed need such calibration
(which I don't think so), or if the calibration that we actually try
to do is quite bogus?

> >> @@ -1719,9 +1737,12 @@ static void time_calibration_tsc_rendezv
> >>              while ( atomic_read(&r->semaphore) > total_cpus )
> >>                  cpu_relax();
> >>          }
> >> +
> >> +        /* Just in case a read above ended up reading zero. */
> >> +        tsc += !tsc;
> > 
> > Won't that be worthy of an ASSERT_UNREACHABLE? I'm not sure I see how
> > tsc could be 0 on a healthy system after the loop above.
> It's not forbidden for the firmware to set the TSCs to some
> huge negative value. Considering the effect TSC_ADJUST has on
> the actual value read by RDTSC, I think I did actually observe
> a system coming up this way, because of (not very helpful)
> TSC_ADJUST setting by firmware. So no, no ASSERT_UNREACHABLE()
> here.

But then the code here will loop 5 times, and it's not possible for
those 5 loops to read a TSC value of 0? I could see it reading 0 on
one of the iterations but not in all of them.

Thanks, Roger.



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.