From: Joao Martins <email@example.com> To: Jan Beulich <JBeulich@suse.com> Cc: Andrew Cooper <firstname.lastname@example.org>, Wei Liu <email@example.com>, Dario Faggioli <firstname.lastname@example.org>, xen-devel <email@example.com> Subject: Re: [PATCH] x86/time: use correct (local) time stamp in constant-TSC calibration fast path Date: Thu, 9 Jun 2016 16:00:20 +0100 [thread overview] Message-ID: <firstname.lastname@example.org> (raw) In-Reply-To: <575983BE02000078000F37BB@prv-mh.provo.novell.com> On 06/09/2016 01:57 PM, Jan Beulich wrote: >>>> On 09.06.16 at 14:11, <email@example.com> wrote: >> On 06/09/2016 01:01 PM, Jan Beulich wrote: >>> This looks like a copy and paste mistake in commit 1b6a99892d ("x86: >>> Simpler time handling when TSC is constant across all power saving >>> states"), responsible for occasional many-microsecond cross-CPU skew of >>> what NOW() returns. >>> >>> Also improve the correlation between local TSC and stime stamps >>> obtained at the end of the two calibration handlers: Compute the stime >>> one from the TSC one, instead of doing another rdtsc() for that >>> compuation. >>> >>> Signed-off-by: Jan Beulich <firstname.lastname@example.org> >>> --- >>> As to 4.7 inclusion: This of course looks like a pretty blatant mistake >>> that has been there for many years (and hence many releases). There's >>> certainly non-zero risk that I'm overlooking something here (despite >>> Joao apparently having come to the same conclusion), so I can't really >>> make up my mind on whether to request this patch to be put there right >>> away, or rather having linger in -unstable for a while. >>> >> Initially I thought of this as a fix too, but then wouldn't having >> t->stime_local_stamp be c->stime_local_stamp, render no use to the >> platform timer reads done on calibration? Unless we would change >> update_vcpu_system to use stime_master_stamp instead? >> stime_master_stamp field isn't used anywhere other than the dom0 injected >> cpu_frequency_change or when at boot seeding the cpu_time struct on >> init_percpu_time (and the already mentioned use on local_time_calibration) ? >> init_percpu_time also takes a different read of the platform timer per >> cpu and could probably be inherited by a read done on the boot processor >> and written on remaining CPUs, so that all would start from the same stamp. >> IOW - this sounds like time we are turning stime to be totally TSC except >> when initially seeding each cpu_time? > > Did you also look at the "slow" path in local_time_calibration()? That > does use the master stamp. Yeah I am aware of its usage there - my comment was only on the fast path. I think the latter is the most common case too. > So in effect for the fast path the patch > changes the situation from c->stime_local_stamp being effectively > unused to c->stime_master_stamp being so. In the former case, if > that really hadn't been a typo, deleting the write of that field from > time_calibration_std_rendezvous() would have made sense, as > get_s_time() certainly is more overhead than the simply memory > read and write needed for keeping c->stime_master_stamp up to > date (despite not being used). I agree, but what I meant previously was more of a concern meaning: CPU 0 is doing an expensive read_platform_time (e.g. HPET supposedly microseconds order, plus a non-contented lock) to set stime_master_stamp that doesn't get used at all - effectively not using the clocksource set initially at boot. What if verify_tsc_reliability clears out X86_FEATURE_TSC_RELIABLE when failing the warp test? The calibration function is set early on right after interrupts are enabled and the time warp check later on when all CPUs are up. So on problematic platforms it's possible that std_rendezvous is used with a constant TSC but still deemed unreliable. We still keep incrementing deltas at roughly about the same time, but in effect with this change the stime_local_stamp would be TSC-only based thus leading to warps with an unreliable TSC? And there's also the CPU hotplug/onlining case that we once discussed. Joao _______________________________________________ Xen-devel mailing list Xenemail@example.com http://lists.xen.org/xen-devel
next prev parent reply other threads:[~2016-06-09 14:59 UTC|newest] Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-06-09 12:01 Jan Beulich 2016-06-09 12:10 ` Andrew Cooper 2016-06-09 12:11 ` Joao Martins 2016-06-09 12:57 ` Jan Beulich 2016-06-09 15:00 ` Joao Martins [this message] 2016-06-09 15:52 ` Jan Beulich 2016-06-09 18:19 ` Joao Martins 2016-06-10 6:59 ` Jan Beulich 2016-06-10 9:29 ` Jan Beulich 2016-06-10 17:07 ` Joao Martins 2016-06-09 12:12 ` Wei Liu
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --firstname.lastname@example.org \ --email@example.com \ --cc=JBeulich@suse.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --subject='Re: [PATCH] x86/time: use correct (local) time stamp in constant-TSC calibration fast path' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).