[Xen-devel] [PATCH] x86/tsc: limit the usage of synchronization rendezvous

[Xen-devel] [PATCH] x86/tsc: limit the usage of synchronization rendezvous

[Roger Pau Monne]

If Xen detects the TSC is unreliable it will set a rendezvous helper
that tries to synchronize the different CPUs TSC value by propagating
the one from CPU#0 and writing it into the IA32_TSC MSR on each CPU.

When the system has a single thread and there are no hotplugable CPUs
doing the above just results in reading the TSC from CPU#0 and writing
it into the IA32_TSC MSR of CPU#0, which is pointless, so limit the
usage of the synchronization rendezvous to systems that have more than
one CPU, even if those CPUs are yet to be hotplugged.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
I'm not sure whether this is suitable for 4.13, being a performance
improvement but not fixing a functional bug.
---
 xen/arch/x86/time.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index d8242295ef..dddbb60f58 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1821,7 +1821,8 @@ static int __init verify_tsc_reliability(void)
      * are not marked as 'reliable', re-sync during rendezvous.
      */
     if ( boot_cpu_has(X86_FEATURE_CONSTANT_TSC) &&
-         !boot_cpu_has(X86_FEATURE_TSC_RELIABLE) )
+         !boot_cpu_has(X86_FEATURE_TSC_RELIABLE) &&
+         num_present_cpus() > 1 )
         time_calibration_rendezvous_fn = time_calibration_tsc_rendezvous;
 
     return 0;
-- 
2.23.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/tsc: limit the usage of synchronization rendezvous

[Jan Beulich]

On 23.10.2019 15:56, Roger Pau Monne wrote:
> If Xen detects the TSC is unreliable it will set a rendezvous helper
> that tries to synchronize the different CPUs TSC value by propagating
> the one from CPU#0 and writing it into the IA32_TSC MSR on each CPU.
> 
> When the system has a single thread and there are no hotplugable CPUs
> doing the above just results in reading the TSC from CPU#0 and writing
> it into the IA32_TSC MSR of CPU#0, which is pointless, so limit the
> usage of the synchronization rendezvous to systems that have more than
> one CPU, even if those CPUs are yet to be hotplugged.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> I'm not sure whether this is suitable for 4.13, being a performance
> improvement but not fixing a functional bug.

Unless we know there's a lot of use of Xen in UP mode, I'd say
rather not. _If_ there was a lot of such use, then I think we
ought to do more work towards performance there (like re-
introducing SMP alternatives patching).

Of course in the end the decision is to be taken by Jürgen,
whom you didn't even Cc.

> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -1821,7 +1821,8 @@ static int __init verify_tsc_reliability(void)
>       * are not marked as 'reliable', re-sync during rendezvous.
>       */
>      if ( boot_cpu_has(X86_FEATURE_CONSTANT_TSC) &&
> -         !boot_cpu_has(X86_FEATURE_TSC_RELIABLE) )
> +         !boot_cpu_has(X86_FEATURE_TSC_RELIABLE) &&
> +         num_present_cpus() > 1 )
>          time_calibration_rendezvous_fn = time_calibration_tsc_rendezvous;

Did you consider the alternative of switching the rendezvous
function when going from UP to MP mode (or back)?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/tsc: limit the usage of synchronization rendezvous

[Jürgen Groß]

On 23.10.19 16:17, Jan Beulich wrote:
> On 23.10.2019 15:56, Roger Pau Monne wrote:
>> If Xen detects the TSC is unreliable it will set a rendezvous helper
>> that tries to synchronize the different CPUs TSC value by propagating
>> the one from CPU#0 and writing it into the IA32_TSC MSR on each CPU.
>>
>> When the system has a single thread and there are no hotplugable CPUs
>> doing the above just results in reading the TSC from CPU#0 and writing
>> it into the IA32_TSC MSR of CPU#0, which is pointless, so limit the
>> usage of the synchronization rendezvous to systems that have more than
>> one CPU, even if those CPUs are yet to be hotplugged.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> ---
>> I'm not sure whether this is suitable for 4.13, being a performance
>> improvement but not fixing a functional bug.
> 
> Unless we know there's a lot of use of Xen in UP mode, I'd say
> rather not. _If_ there was a lot of such use, then I think we
> ought to do more work towards performance there (like re-
> introducing SMP alternatives patching).

I believe the main use case is pv-shim.

> Of course in the end the decision is to be taken by Jürgen,
> whom you didn't even Cc.

I'm inclined not to Release-ack it.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/tsc: limit the usage of synchronization rendezvous

[Roger Pau Monné]

On Wed, Oct 23, 2019 at 04:17:11PM +0200, Jan Beulich wrote:
> On 23.10.2019 15:56, Roger Pau Monne wrote:
> > If Xen detects the TSC is unreliable it will set a rendezvous helper
> > that tries to synchronize the different CPUs TSC value by propagating
> > the one from CPU#0 and writing it into the IA32_TSC MSR on each CPU.
> > 
> > When the system has a single thread and there are no hotplugable CPUs
> > doing the above just results in reading the TSC from CPU#0 and writing
> > it into the IA32_TSC MSR of CPU#0, which is pointless, so limit the
> > usage of the synchronization rendezvous to systems that have more than
> > one CPU, even if those CPUs are yet to be hotplugged.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > I'm not sure whether this is suitable for 4.13, being a performance
> > improvement but not fixing a functional bug.
> 
> Unless we know there's a lot of use of Xen in UP mode, I'd say
> rather not. _If_ there was a lot of such use, then I think we
> ought to do more work towards performance there (like re-
> introducing SMP alternatives patching).
> 
> Of course in the end the decision is to be taken by Jürgen,
> whom you didn't even Cc.

Since this is a performance fix I felt like it wasn't really
appropriate for 4.13, hence I didn't Cc Jürgen but added this note.

> > --- a/xen/arch/x86/time.c
> > +++ b/xen/arch/x86/time.c
> > @@ -1821,7 +1821,8 @@ static int __init verify_tsc_reliability(void)
> >       * are not marked as 'reliable', re-sync during rendezvous.
> >       */
> >      if ( boot_cpu_has(X86_FEATURE_CONSTANT_TSC) &&
> > -         !boot_cpu_has(X86_FEATURE_TSC_RELIABLE) )
> > +         !boot_cpu_has(X86_FEATURE_TSC_RELIABLE) &&
> > +         num_present_cpus() > 1 )
> >          time_calibration_rendezvous_fn = time_calibration_tsc_rendezvous;
> 
> Did you consider the alternative of switching the rendezvous
> function when going from UP to MP mode (or back)?

I will look into that. The usefulness of this fix is quite limited,
given it only applies to UP, so I didn't want to add a lot of
complexity for a not very common scenario IMO.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel