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

Re: [PATCH linux-next v2] x86/xen/time: prefer tsc as clocksource when it is invariant


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Krister Johansen <kjlx@xxxxxxxxxxxxxxxxxx>
  • Date: Tue, 13 Dec 2022 10:58:42 -0800
  • Arc-authentication-results: i=1; rspamd-85f95c7974-5pdxx; auth=pass smtp.auth=dreamhost smtp.mailfrom=kjlx@xxxxxxxxxxxxxxxxxx
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=mailchannels.net; s=arc-2022; t=1670957925; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references:dkim-signature; bh=E0qUk9TTQJ05tyym3+Ykvnx/NywduTmX7ZoBw4ir51k=; b=NXSLQcRbT2saxtsgmEjKwf1sWyth/k73W2naGvnIqY+VdOZCauVYSIk9fsDLEISu7B4IDD Bmn+NlUxU3tKxi3Zc4Zp2h+XkogU3S08Irl8iIX/Q2EMeccsabaVKeLRWhli5tNNu/iQow LOeNWZgCMk0dwz1gCmzpwSbuRBlsknUviddq1Ipw0Vs6sO+1+Ti/VG7ShIHoRL83a3YEB3 HxNVU4HWt64YupEtazTQVbTlnGp+a500CZ8XtXaOssb6D0KbH2QuNgrCqBAME197j0LglT uPBWtz9jOZ6mdJO+jkyIyGCOExvVBPHV/IBjXk76Il1AJbyCNPZcVQz0cRNeXg==
  • Arc-seal: i=1; s=arc-2022; d=mailchannels.net; t=1670957925; a=rsa-sha256; cv=none; b=vbxmtodfxpAhP/S3TK48+NjWBYDR0OvBKkeJGv9vjJN66H8aXMUJXlj+07w2iX+OWz8cK0 sxapHBK2uMRITvYyeGGu1s5USkxwWEDOXb0Q1a1SxXNhVAIEja49ZfTFT6yiz7ytrn5x/C 1hTYZ1YCs61AT6PYZgXdD4H6JSZ0iczwAxA/35DCEtmBKIV230WtbSOALgT+qy32b2rHDn BL0xY+5qs4IDn9ADewDBuxdT7C44LFm3Unl/MDeHWGWCLcgC2ffp7hLYHUYll5NtE7urQy 5M79SgMpwBgXR8GqOBuhUXVQUfjzBUtu0DEq6PenP7Ocr+P3yG0z2lXqlW0wKg==
  • Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>, Ingo Molnar <mingo@xxxxxxxxxx>, Borislav Petkov <bp@xxxxxxxxx>, Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>, x86@xxxxxxxxxx, "H. Peter Anvin" <hpa@xxxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx, Marcelo Tosatti <mtosatti@xxxxxxxxxx>, Anthony Liguori <aliguori@xxxxxxxxxx>, David Reaver <me@xxxxxxxxxxxxxxx>, Brendan Gregg <brendan@xxxxxxxxx>, Juergen Gross <jgross@xxxxxxxx>, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
  • Delivery-date: Tue, 13 Dec 2022 18:58:58 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Tue, Dec 13, 2022 at 08:23:29AM +0100, Jan Beulich wrote:
> On 12.12.2022 23:05, Krister Johansen wrote:
> > On Mon, Dec 12, 2022 at 05:46:29PM +0100, Jan Beulich wrote:
> >> On 12.12.2022 17:05, Krister Johansen wrote:
> >>> Both the Intel SDM[4] and the Xen tsc documentation explain that marking
> >>> a tsc as invariant means that it should be considered stable by the OS
> >>> and is elibile to be used as a wall clock source.  The Xen documentation
> >>> further clarifies that this is only reliable on HVM and PVH because PV
> >>> cannot intercept a cpuid instruction.
> >>
> >> Without meaning to express a view on the argumentation as a whole, this
> >> PV aspect is suspicious. Unless you open-code a use of the CPUID insn
> >> in the kernel, all uses of CPUID are going to be processed by Xen by
> >> virtue of the respective pvops hook. Documentation says what it says
> >> for environments where this might not be the case.
> > 
> > Thanks, appreciate the clarification here. Just restating this for my
> > own understanding: your advice would be to drop this check below?
> 
> No, I'm unconvinced of if/where this transformation is really appropriate.

Ah, I see.  You're saying that you're not convinced that this code
should ever lower the priority of xen clocksource in favor of the tsc?
If so, are you willing to say a bit more about what you find to be
unconvincing?

In as much detail as I can muster: the impetus for the patch was that I
had a variety of different systems running as both kvm and xen guests.
Some of these guests had clocksource tunings in place as a result of
consulting the documentation linked in the patch.  But others didn't.
On kvm they had somehow done the "right" (?) thing.  Some systems had
tuning in place for xen, despite no longer being a xen guests.  Other
systems were running on xen and didn't have the recommended tuning
applied.  That's all sorted now, but it seemed like it might be nice to
eliminate the need for others to do this in future. With kvm doing
something similar, I thought there might be enough precedent to consider
this for xen guests.

> My comment was merely to indicate that the justification for ...
> 
> >>> + if (!(xen_hvm_domain() || xen_pvh_domain()))
> >>> +         return 0;
> 
> ... this isn't really correct.

The rationale for this bit of code was the justification that turns
out to be incorrect.  That sounds to me like I have unnessary code,
unless I was right by mistake?

> > And then update the commit message to dispense with the distinction
> > between HVM, PV, and PVH?
> > 
> >>> + cpuid(xen_cpuid_base() + 3, &eax, &ebx, &ecx, &edx);
> >>
> >> Xen leaf 3 has sub-leaves, so I think you need to set ecx to zero before
> >> this call.
> > 
> > The cpuid() inline in arch/x86/include/asm/processor.h assigns zero to
> > ecx prior to calling __cpuid.  In arch/x86/boot/cpuflags.c the macros
> > are a little different, but it looks like there too, the macro passes 0
> > as an input argument to cpuid_count which ends up being %ecx.  Happy to
> > fix this up if I'm looking at the wrong cpuid functions, though.
> 
> Oh, I didn't expect cpuid() to be more than a trivial wrapper around the
> the pvops hook, and I merely looked at native_cpuid() and xen_cpuid().
> I'm sorry for the noise then. Yet still, with there being sub-leaves, I'd
> recommend switching to cpuid_count() just for clarity.

No apology necessary.  I'm happy to modify this to use cpuid_count() for
clarity.

-K



 


Rackspace

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