Re: [PATCH v4 3/3] x86/time: avoid reading the platform timer in rendezvous functions

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Apr 01, 2021 at 11:55:10AM +0200, Jan Beulich wrote:
> Reading the platform timer isn't cheap, so we'd better avoid it when the
> resulting value is of no interest to anyone.
> 
> The consumer of master_stime, obtained by
> time_calibration_{std,tsc}_rendezvous() and propagated through
> this_cpu(cpu_calibration), is local_time_calibration(). With
> CONSTANT_TSC the latter function uses an early exit path, which doesn't
> explicitly use the field. While this_cpu(cpu_calibration) (including the
> master_stime field) gets propagated to this_cpu(cpu_time).stamp on that
> path, both structures' fields get consumed only by the !CONSTANT_TSC
> logic of the function.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Albeit as said on my other email I would prefer performance related
changes like this one to be accompanied with some proof that they
actually make a difference, or else we risk making the code more
complicated for no concrete benefit.

> ---
> v4: New.
> ---
> I realize there's some risk associated with potential new uses of the
> field down the road. What would people think about compiling time.c a
> 2nd time into a dummy object file, with a conditional enabled to force
> assuming CONSTANT_TSC, and with that conditional used to suppress
> presence of the field as well as all audited used of it (i.e. in
> particular that large part of local_time_calibration())? Unexpected new
> users of the field would then cause build time errors.
> 
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -52,6 +52,7 @@ unsigned long pit0_ticks;
>  struct cpu_time_stamp {
>      u64 local_tsc;
>      s_time_t local_stime;
> +    /* Next field unconditionally valid only when !CONSTANT_TSC. */

Could you also mention this is only true for the cpu_time_stamp that's
used in cpu_calibration?

For ap_bringup_ref master_stime is valid regardless of CONSTANT_TSC.

Thanks, Roger.