1. Patchew
  2. Xen
  3. [PATCH 0/2] Xen real-time x86
  4. View patch

[PATCH 1/2] xen/x86: don't send IPI to sync TSC when it is reliable

Stefano Stabellini posted 2 patches 3 months, 3 weeks ago
There is a newer version of this series
[PATCH 1/2] xen/x86: don't send IPI to sync TSC when it is reliable
Posted by Stefano Stabellini 3 months, 3 weeks ago 
On real time configuration with the null scheduler, we shouldn't
interrupt the guest execution unless strictly necessary: the guest could
be a real time guest (e.g. FreeRTOS) and interrupting its execution
could lead to a missed deadline.

The principal source of interruptions is IPIs. Remove the unnecessary
IPI on all physical CPUs to sync the TSC when the TSC is known to be
reliable.

Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
---
 xen/arch/x86/time.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index 59129f419d..bfd022174a 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -2303,6 +2303,10 @@ static void cf_check time_calibration(void *unused)
         local_irq_enable();
     }
 
+    if ( boot_cpu_has(X86_FEATURE_CONSTANT_TSC) &&
+         boot_cpu_has(X86_FEATURE_TSC_RELIABLE) )
+        return;
+
     cpumask_copy(&r.cpu_calibration_map, &cpu_online_map);
 
     /* @wait=1 because we must wait for all cpus before freeing @r. */
-- 
2.25.1
Re: [PATCH 1/2] xen/x86: don't send IPI to sync TSC when it is reliable
Posted by Jan Beulich 3 months, 3 weeks ago 
On 08.07.2025 02:07, Stefano Stabellini wrote:
> On real time configuration with the null scheduler, we shouldn't
> interrupt the guest execution unless strictly necessary: the guest could
> be a real time guest (e.g. FreeRTOS) and interrupting its execution
> could lead to a missed deadline.
> 
> The principal source of interruptions is IPIs. Remove the unnecessary
> IPI on all physical CPUs to sync the TSC when the TSC is known to be
> reliable.

If it had been truly unnecessary for all the time, I'm sure someone would
have suggested to get rid of the overhead. IOW I think there is more to be
said as to this being correct / safe, including in any corner cases.

> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> ---
>  xen/arch/x86/time.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
> index 59129f419d..bfd022174a 100644
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -2303,6 +2303,10 @@ static void cf_check time_calibration(void *unused)
>          local_irq_enable();
>      }
>  
> +    if ( boot_cpu_has(X86_FEATURE_CONSTANT_TSC) &&
> +         boot_cpu_has(X86_FEATURE_TSC_RELIABLE) )
> +        return;

This would render the (first of the two) invocation(s) of the function from
verify_tsc_reliability() (largely) dead; it would then be only r.master_stime
which gets updated (see also Alejandro's reply), which surely wouldn't have
required that call in the first place.

Jan
Re: [PATCH 1/2] xen/x86: don't send IPI to sync TSC when it is reliable
Posted by Stefano Stabellini 3 months, 3 weeks ago 
On Tue, 8 Jul 2025, Jan Beulich wrote:
> On 08.07.2025 02:07, Stefano Stabellini wrote:
> > On real time configuration with the null scheduler, we shouldn't
> > interrupt the guest execution unless strictly necessary: the guest could
> > be a real time guest (e.g. FreeRTOS) and interrupting its execution
> > could lead to a missed deadline.
> > 
> > The principal source of interruptions is IPIs. Remove the unnecessary
> > IPI on all physical CPUs to sync the TSC when the TSC is known to be
> > reliable.
> 
> If it had been truly unnecessary for all the time, I'm sure someone would
> have suggested to get rid of the overhead.

I am not so sure someone else would have suggested it given that Xen on
x86 has been mostly used on the datacenter where real-time is not a
requirement. 


> IOW I think there is more to be
> said as to this being correct / safe, including in any corner cases.
> 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> > ---
> >  xen/arch/x86/time.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
> > index 59129f419d..bfd022174a 100644
> > --- a/xen/arch/x86/time.c
> > +++ b/xen/arch/x86/time.c
> > @@ -2303,6 +2303,10 @@ static void cf_check time_calibration(void *unused)
> >          local_irq_enable();
> >      }
> >  
> > +    if ( boot_cpu_has(X86_FEATURE_CONSTANT_TSC) &&
> > +         boot_cpu_has(X86_FEATURE_TSC_RELIABLE) )
> > +        return;
> 
> This would render the (first of the two) invocation(s) of the function from
> verify_tsc_reliability() (largely) dead; it would then be only r.master_stime
> which gets updated (see also Alejandro's reply), which surely wouldn't have
> required that call in the first place.

I'll follow Alejandro's suggestions
Re: [PATCH 1/2] xen/x86: don't send IPI to sync TSC when it is reliable
Posted by Jan Beulich 3 months, 3 weeks ago 
On 08.07.2025 19:40, Stefano Stabellini wrote:
> On Tue, 8 Jul 2025, Jan Beulich wrote:
>> On 08.07.2025 02:07, Stefano Stabellini wrote:
>>> On real time configuration with the null scheduler, we shouldn't
>>> interrupt the guest execution unless strictly necessary: the guest could
>>> be a real time guest (e.g. FreeRTOS) and interrupting its execution
>>> could lead to a missed deadline.
>>>
>>> The principal source of interruptions is IPIs. Remove the unnecessary
>>> IPI on all physical CPUs to sync the TSC when the TSC is known to be
>>> reliable.
>>
>> If it had been truly unnecessary for all the time, I'm sure someone would
>> have suggested to get rid of the overhead.
> 
> I am not so sure someone else would have suggested it given that Xen on
> x86 has been mostly used on the datacenter where real-time is not a
> requirement. 

What I mean to indicate is that we're generally always on the hunt of
unnecessary overhead that can be eliminated.

Jan
Re: [PATCH 1/2] xen/x86: don't send IPI to sync TSC when it is reliable
Posted by Alejandro Vallejo 3 months, 3 weeks ago 
On Tue Jul 8, 2025 at 2:07 AM CEST, Stefano Stabellini wrote:
> On real time configuration with the null scheduler, we shouldn't
> interrupt the guest execution unless strictly necessary: the guest could
> be a real time guest (e.g. FreeRTOS) and interrupting its execution
> could lead to a missed deadline.
>
> The principal source of interruptions is IPIs. Remove the unnecessary
> IPI on all physical CPUs to sync the TSC when the TSC is known to be
> reliable.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> ---
>  xen/arch/x86/time.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
> index 59129f419d..bfd022174a 100644
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -2303,6 +2303,10 @@ static void cf_check time_calibration(void *unused)
>          local_irq_enable();
>      }
>  
> +    if ( boot_cpu_has(X86_FEATURE_CONSTANT_TSC) &&
> +         boot_cpu_has(X86_FEATURE_TSC_RELIABLE) )
> +        return;
> +

This should check "(tsc_flags & TSC_RELIABLE_SOCKET)" as well. The TSCs might
still be unsynchronized across sockets.

I'm still quite confused as to how Xen (mis)handles time, but wouldn't this need
to go inside the branch above? If the clocksource is not the TSC as well the TSC
can still drift with respect to the actual clocksource (PIT, HPET or ACPI timer).

If so, we could probably do an early return in the branch above ignoring the
conditions (they are required for picking the TSC clocksource already, including
synchronization across sockets).

Another matter is whether we could drop the "master_stime" write. Would we
care about it at all?

>      cpumask_copy(&r.cpu_calibration_map, &cpu_online_map);
>  
>      /* @wait=1 because we must wait for all cpus before freeing @r. */

Cheers,
Aljeandro
Re: [PATCH 1/2] xen/x86: don't send IPI to sync TSC when it is reliable
Posted by Stefano Stabellini 3 months, 3 weeks ago 
On Tue, 8 Jul 2025, Alejandro Vallejo wrote:
> On Tue Jul 8, 2025 at 2:07 AM CEST, Stefano Stabellini wrote:
> > On real time configuration with the null scheduler, we shouldn't
> > interrupt the guest execution unless strictly necessary: the guest could
> > be a real time guest (e.g. FreeRTOS) and interrupting its execution
> > could lead to a missed deadline.
> >
> > The principal source of interruptions is IPIs. Remove the unnecessary
> > IPI on all physical CPUs to sync the TSC when the TSC is known to be
> > reliable.
> >
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> > ---
> >  xen/arch/x86/time.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
> > index 59129f419d..bfd022174a 100644
> > --- a/xen/arch/x86/time.c
> > +++ b/xen/arch/x86/time.c
> > @@ -2303,6 +2303,10 @@ static void cf_check time_calibration(void *unused)
> >          local_irq_enable();
> >      }
> >  
> > +    if ( boot_cpu_has(X86_FEATURE_CONSTANT_TSC) &&
> > +         boot_cpu_has(X86_FEATURE_TSC_RELIABLE) )
> > +        return;
> > +
> 
> This should check "(tsc_flags & TSC_RELIABLE_SOCKET)" as well. The TSCs might
> still be unsynchronized across sockets.
>
> I'm still quite confused as to how Xen (mis)handles time, but wouldn't this need
> to go inside the branch above? If the clocksource is not the TSC as well the TSC
> can still drift with respect to the actual clocksource (PIT, HPET or ACPI timer).

I can move it inside the previous if


> If so, we could probably do an early return in the branch above ignoring the
> conditions (they are required for picking the TSC clocksource already, including
> synchronization across sockets).
> 
> Another matter is whether we could drop the "master_stime" write. Would we
> care about it at all?

I'll drop it.

Is this what you had in mind?


diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index 59129f419d..d72e640f72 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -2297,11 +2297,7 @@ static void cf_check time_calibration(void *unused)
     };
 
     if ( clocksource_is_tsc() )
-    {
-        local_irq_disable();
-        r.master_stime = read_platform_stime(&r.master_tsc_stamp);
-        local_irq_enable();
-    }
+        return;
 
     cpumask_copy(&r.cpu_calibration_map, &cpu_online_map);
Re: [PATCH 1/2] xen/x86: don't send IPI to sync TSC when it is reliable
Posted by Alejandro Vallejo 3 months, 3 weeks ago 
On Tue Jul 8, 2025 at 7:40 PM CEST, Stefano Stabellini wrote:
> On Tue, 8 Jul 2025, Alejandro Vallejo wrote:
>> On Tue Jul 8, 2025 at 2:07 AM CEST, Stefano Stabellini wrote:
>> > On real time configuration with the null scheduler, we shouldn't
>> > interrupt the guest execution unless strictly necessary: the guest could
>> > be a real time guest (e.g. FreeRTOS) and interrupting its execution
>> > could lead to a missed deadline.
>> >
>> > The principal source of interruptions is IPIs. Remove the unnecessary
>> > IPI on all physical CPUs to sync the TSC when the TSC is known to be
>> > reliable.
>> >
>> > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
>> > ---
>> >  xen/arch/x86/time.c | 4 ++++
>> >  1 file changed, 4 insertions(+)
>> >
>> > diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
>> > index 59129f419d..bfd022174a 100644
>> > --- a/xen/arch/x86/time.c
>> > +++ b/xen/arch/x86/time.c
>> > @@ -2303,6 +2303,10 @@ static void cf_check time_calibration(void *unused)
>> >          local_irq_enable();
>> >      }
>> >  
>> > +    if ( boot_cpu_has(X86_FEATURE_CONSTANT_TSC) &&
>> > +         boot_cpu_has(X86_FEATURE_TSC_RELIABLE) )
>> > +        return;
>> > +
>> 
>> This should check "(tsc_flags & TSC_RELIABLE_SOCKET)" as well. The TSCs might
>> still be unsynchronized across sockets.
>>
>> I'm still quite confused as to how Xen (mis)handles time, but wouldn't this need
>> to go inside the branch above? If the clocksource is not the TSC as well the TSC
>> can still drift with respect to the actual clocksource (PIT, HPET or ACPI timer).
>
> I can move it inside the previous if
>
>
>> If so, we could probably do an early return in the branch above ignoring the
>> conditions (they are required for picking the TSC clocksource already, including
>> synchronization across sockets).
>> 
>> Another matter is whether we could drop the "master_stime" write. Would we
>> care about it at all?
>
> I'll drop it.
>
> Is this what you had in mind?
>
>
> diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
> index 59129f419d..d72e640f72 100644
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -2297,11 +2297,7 @@ static void cf_check time_calibration(void *unused)
>      };
>  
>      if ( clocksource_is_tsc() )
> -    {
> -        local_irq_disable();
> -        r.master_stime = read_platform_stime(&r.master_tsc_stamp);
> -        local_irq_enable();
> -    }
> +        return;
>  
>      cpumask_copy(&r.cpu_calibration_map, &cpu_online_map);
>  

Yes, I think that would do.

Cheers,
Alejandro

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.