Message ID | 20191023135644.46847-1-roger.pau@citrix.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | x86/tsc: limit the usage of synchronization rendezvous | expand |
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
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
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.
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;
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(-)