Re: [PATCH V2 03/11] perf/x86: Add support for TSC in nanoseconds as a perf event clock

Re: [PATCH V2 03/11] perf/x86: Add support for TSC in nanoseconds as a perf event clock

[Peter Zijlstra]

On Fri, Mar 04, 2022 at 08:27:45PM +0200, Adrian Hunter wrote:
> On 04/03/2022 15:41, Peter Zijlstra wrote:
> > On Mon, Feb 14, 2022 at 01:09:06PM +0200, Adrian Hunter wrote:
> >> Currently, when Intel PT is used within a VM guest, it is not possible to
> >> make use of TSC because perf clock is subject to paravirtualization.
> > 
> > Yeah, so how much of that still makes sense, or ever did? AFAIK the
> > whole pv_clock thing is utter crazy. Should we not fix that instead?
> 
> Presumably pv_clock must work with different host operating systems.
> Similarly, KVM must work with different guest operating systems.
> Perhaps I'm wrong, but I imagine re-engineering time virtualization
> might be a pretty big deal,  far exceeding the scope of these patches.

I think not; on both counts. That is, I don't think it's going to be
hard, and even it if were, it would still be the right thing to do.

We're not going to add interface just to work around a known broken
piece of crap just because we don't want to fix it.

So I'm thinking we should do the below and simply ignore any paravirt
sched clock offered when there's ART on.

---
diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
index 4420499f7bb4..a1f179ed39bf 100644
--- a/arch/x86/kernel/paravirt.c
+++ b/arch/x86/kernel/paravirt.c
@@ -145,6 +145,15 @@ DEFINE_STATIC_CALL(pv_sched_clock, native_sched_clock);
 
 void paravirt_set_sched_clock(u64 (*func)(void))
 {
+	/*
+	 * Anything with ART on promises to have sane TSC, otherwise the whole
+	 * ART thing is useless. In order to make ART useful for guests, we
+	 * should continue to use the TSC. As such, ignore any paravirt
+	 * muckery.
+	 */
+	if (cpu_feature_enabled(X86_FEATURE_ART))
+		return;
+
 	static_call_update(pv_sched_clock, func);
 }
 
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V2 03/11] perf/x86: Add support for TSC in nanoseconds as a perf event clock

[Juergen Gross via Virtualization]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 2054 bytes --]

On 07.03.22 10:50, Peter Zijlstra wrote:
> On Fri, Mar 04, 2022 at 08:27:45PM +0200, Adrian Hunter wrote:
>> On 04/03/2022 15:41, Peter Zijlstra wrote:
>>> On Mon, Feb 14, 2022 at 01:09:06PM +0200, Adrian Hunter wrote:
>>>> Currently, when Intel PT is used within a VM guest, it is not possible to
>>>> make use of TSC because perf clock is subject to paravirtualization.
>>>
>>> Yeah, so how much of that still makes sense, or ever did? AFAIK the
>>> whole pv_clock thing is utter crazy. Should we not fix that instead?
>>
>> Presumably pv_clock must work with different host operating systems.
>> Similarly, KVM must work with different guest operating systems.
>> Perhaps I'm wrong, but I imagine re-engineering time virtualization
>> might be a pretty big deal,  far exceeding the scope of these patches.
> 
> I think not; on both counts. That is, I don't think it's going to be
> hard, and even it if were, it would still be the right thing to do.
> 
> We're not going to add interface just to work around a known broken
> piece of crap just because we don't want to fix it.
> 
> So I'm thinking we should do the below and simply ignore any paravirt
> sched clock offered when there's ART on.
> 
> ---
> diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
> index 4420499f7bb4..a1f179ed39bf 100644
> --- a/arch/x86/kernel/paravirt.c
> +++ b/arch/x86/kernel/paravirt.c
> @@ -145,6 +145,15 @@ DEFINE_STATIC_CALL(pv_sched_clock, native_sched_clock);
>   
>   void paravirt_set_sched_clock(u64 (*func)(void))
>   {
> +	/*
> +	 * Anything with ART on promises to have sane TSC, otherwise the whole
> +	 * ART thing is useless. In order to make ART useful for guests, we
> +	 * should continue to use the TSC. As such, ignore any paravirt
> +	 * muckery.
> +	 */
> +	if (cpu_feature_enabled(X86_FEATURE_ART))
> +		return;
> +
>   	static_call_update(pv_sched_clock, func);
>   }
>   
> 

NAK, this will break live migration of a guest coming from a host
without this feature.


Juergen

[-- Attachment #1.1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V2 03/11] perf/x86: Add support for TSC in nanoseconds as a perf event clock

[Peter Zijlstra]

[-- Attachment #1.1: Type: text/plain, Size: 1040 bytes --]

On Mon, Mar 07, 2022 at 11:06:46AM +0100, Juergen Gross wrote:

> > diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
> > index 4420499f7bb4..a1f179ed39bf 100644
> > --- a/arch/x86/kernel/paravirt.c
> > +++ b/arch/x86/kernel/paravirt.c
> > @@ -145,6 +145,15 @@ DEFINE_STATIC_CALL(pv_sched_clock, native_sched_clock);
> >   void paravirt_set_sched_clock(u64 (*func)(void))
> >   {
> > +	/*
> > +	 * Anything with ART on promises to have sane TSC, otherwise the whole
> > +	 * ART thing is useless. In order to make ART useful for guests, we
> > +	 * should continue to use the TSC. As such, ignore any paravirt
> > +	 * muckery.
> > +	 */
> > +	if (cpu_feature_enabled(X86_FEATURE_ART))
> > +		return;
> > +
> >   	static_call_update(pv_sched_clock, func);
> >   }
> > 
> 
> NAK, this will break live migration of a guest coming from a host
> without this feature.

I thought the whole live-migration nonsense made sure to equalize crud
like that. That is, then don't expose ART to the guest.


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V2 03/11] perf/x86: Add support for TSC in nanoseconds as a perf event clock

[Juergen Gross via Virtualization]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 1199 bytes --]

On 07.03.22 11:38, Peter Zijlstra wrote:
> On Mon, Mar 07, 2022 at 11:06:46AM +0100, Juergen Gross wrote:
> 
>>> diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
>>> index 4420499f7bb4..a1f179ed39bf 100644
>>> --- a/arch/x86/kernel/paravirt.c
>>> +++ b/arch/x86/kernel/paravirt.c
>>> @@ -145,6 +145,15 @@ DEFINE_STATIC_CALL(pv_sched_clock, native_sched_clock);
>>>    void paravirt_set_sched_clock(u64 (*func)(void))
>>>    {
>>> +	/*
>>> +	 * Anything with ART on promises to have sane TSC, otherwise the whole
>>> +	 * ART thing is useless. In order to make ART useful for guests, we
>>> +	 * should continue to use the TSC. As such, ignore any paravirt
>>> +	 * muckery.
>>> +	 */
>>> +	if (cpu_feature_enabled(X86_FEATURE_ART))
>>> +		return;
>>> +
>>>    	static_call_update(pv_sched_clock, func);
>>>    }
>>>
>>
>> NAK, this will break live migration of a guest coming from a host
>> without this feature.
> 
> I thought the whole live-migration nonsense made sure to equalize crud
> like that. That is, then don't expose ART to the guest.

Oh, right. I managed to confuse host-side and guest-side usage.

Sorry for the noise.


Juergen

[-- Attachment #1.1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V2 03/11] perf/x86: Add support for TSC in nanoseconds as a perf event clock

[Peter Zijlstra]

On Mon, Mar 07, 2022 at 02:36:03PM +0200, Adrian Hunter wrote:

> > diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
> > index 4420499f7bb4..a1f179ed39bf 100644
> > --- a/arch/x86/kernel/paravirt.c
> > +++ b/arch/x86/kernel/paravirt.c
> > @@ -145,6 +145,15 @@ DEFINE_STATIC_CALL(pv_sched_clock, native_sched_clock);
> >  
> >  void paravirt_set_sched_clock(u64 (*func)(void))
> >  {
> > +	/*
> > +	 * Anything with ART on promises to have sane TSC, otherwise the whole
> > +	 * ART thing is useless. In order to make ART useful for guests, we
> > +	 * should continue to use the TSC. As such, ignore any paravirt
> > +	 * muckery.
> > +	 */
> > +	if (cpu_feature_enabled(X86_FEATURE_ART))
> 
> Does not seem to work because the feature X86_FEATURE_ART does not seem to get set.
> Possibly because detect_art() excludes anything running on a hypervisor.

Simple enough to delete that clause I suppose. Christopher, what is
needed to make that go away? I suppose the guest needs to be aware of
the active TSC scaling parameters to make it work ?
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V2 03/11] perf/x86: Add support for TSC in nanoseconds as a perf event clock

[Thomas Gleixner]

On Mon, Apr 25 2022 at 08:30, Adrian Hunter wrote:
> On 14/03/22 13:50, Adrian Hunter wrote:
>>> TSC offsetting may also be a problem. The VMCS TSC offset must be discoverable by the
>>> guest. This can be done via TSC_ADJUST MSR. The offset in the VMCS and the guest
>>> TSC_ADJUST MSR must always be equivalent, i.e. a write to TSC_ADJUST in the guest
>>> must be reflected in the VMCS and any changes to the offset in the VMCS must be
>>> reflected in the TSC_ADJUST MSR. Otherwise a para-virtualized method must
>>> be invented to communicate an arbitrary VMCS TSC offset to the guest.
>>>
>> 
>> In my view it is reasonable for perf to support TSC as a perf clock in any case
>> because:
>> 	a) it allows users to work entirely with TSC if they wish
>> 	b) other kernel performance / debug facilities like ftrace already support TSC
>> 	c) the patches to add TSC support are relatively small and straight-forward
>> 
>> May we have support for TSC as a perf event clock?
>
> Any update on this?

If TSC is reliable on the host, then there is absolutely no reason not
to use it in the guest all over the place. And that is independent of
exposing ART to the guest.

So why do we need extra solutions for PT and perf, ftrace and whatever?

Can we just fix the underlying problem and make the hypervisor tell the
guest that TSC is stable, reliable and good to use?

Then everything else just falls into place and using TSC is a
substantial performance gain in general. Just look at the VDSO
implementation of __arch_get_hw_counter() -> vread_pvclock():

Instead of just reading the TSC, this needs to take a nested seqcount,
read TSC and do yet another mult/shift, which makes clock_gettime() ~20%
slower than necessary.

It's hillarious, that we still cling to this pvclock abomination, while
we happily expose TSC deadline timer to the guest. TSC virt scaling was
implemented in hardware for a reason.

Thanks,

        tglx
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V2 03/11] perf/x86: Add support for TSC in nanoseconds as a perf event clock

[Thomas Gleixner]

On Mon, Apr 25 2022 at 16:15, Adrian Hunter wrote:
> On 25/04/22 12:32, Thomas Gleixner wrote:
>> It's hillarious, that we still cling to this pvclock abomination, while
>> we happily expose TSC deadline timer to the guest. TSC virt scaling was
>> implemented in hardware for a reason.
>
> So you are talking about changing VMX TCS Offset on every VM-Entry to try to hide
> the time jumps when the VM is scheduled out?  Or neglect that and just let the time
> jumps happen?
>
> If changing VMX TCS Offset, how can TSC be kept consistent between each VCPU i.e.
> wouldn't that mean each VCPU has to have the same VMX TSC Offset?

Obviously so. That's the only thing which makes sense, no?

Thanks,

        tglx
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V2 03/11] perf/x86: Add support for TSC in nanoseconds as a perf event clock

[Thomas Gleixner]

On Tue, Apr 26 2022 at 09:51, Adrian Hunter wrote:
> On 25/04/22 20:05, Thomas Gleixner wrote:
>> On Mon, Apr 25 2022 at 16:15, Adrian Hunter wrote:
>>> On 25/04/22 12:32, Thomas Gleixner wrote:
>>>> It's hillarious, that we still cling to this pvclock abomination, while
>>>> we happily expose TSC deadline timer to the guest. TSC virt scaling was
>>>> implemented in hardware for a reason.
>>>
>>> So you are talking about changing VMX TCS Offset on every VM-Entry to try to hide
>>> the time jumps when the VM is scheduled out?  Or neglect that and just let the time
>>> jumps happen?
>>>
>>> If changing VMX TCS Offset, how can TSC be kept consistent between each VCPU i.e.
>>> wouldn't that mean each VCPU has to have the same VMX TSC Offset?
>> 
>> Obviously so. That's the only thing which makes sense, no?
>
> [ Sending this again, because I notice I messed up the email "From" ]
>
> But wouldn't that mean changing all the VCPUs VMX TSC Offset at the same time,
> which means when none are currently executing?  How could that be done?

Why would you change TSC offset after the point where a VM is started
and why would it be different per vCPU?

Time is global and time moves on when a vCPU is scheduled out. Anything
else is bonkers, really. If the hypervisor tries to screw with that then
how does the guest do timekeeping in a consistent way?

    CLOCK_REALTIME = CLOCK_MONOTONIC + offset

That offset changes when something sets the clock, i.e. clock_settime(),
settimeofday() or adjtimex() in case that NTP cannot compensate or for
the beloved leap seconds adjustment. At any other time the offset is
constant.

CLOCK_MONOTONIC is derived from the underlying clocksource which is
expected to increment with constant frequency and that has to be
consistent accross _all_ vCPUs of a particular VM.

So how would a hypervisor 'hide' scheduled out time w/o screwing up
timekeeping completely?

The guest TSC which is based on the host TSC is:

    guestTSC = offset + hostTSC * factor;

If you make offset different between guest vCPUs then timekeeping in the
guest is screwed.

The whole point of that paravirt clock was to handle migration between
hosts which did not have the VMCS TSC scaling/offset mechanism. The CPUs
which did not have that went EOL at least 10 years ago.

So what are you concerned about?

Thanks,

        tglx
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization