Re: [Xen-devel] Ping: [PATCH v2 RESEND] x86/time: correctly honor late clearing of TSC related feature flags

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 15/12/2016 15:09, Jan Beulich wrote:
>>>> On 15.12.16 at 12:04, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 15/12/16 09:49, Jan Beulich wrote:
>>>>>> On 06.12.16 at 11:51, <JBeulich@xxxxxxxx> wrote:
>>>> As such clearing of flags may have an impact on the selected rendezvous
>>>> function, handle such in a central place.
>>>>
>>>> But don't allow such feature flags to be cleared during CPU hotplug:
>>>> Platform and local system times may have diverged significantly by
>>>> then, potentially causing noticably (even if only temporary) strange
>>>> behavior. As we're anyway expecting only sufficiently similar CPUs to
>>>> appear during hotplug, this shouldn't be introducing new limitations.
>>>>
>>>> Reported-by: Joao Martins <joao.m.martins@xxxxxxxxxx>
>>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>> Tested-by: Dario Faggioli <dario.faggioli@xxxxxxxxxx>
>>>> Tested-by: Joao Martins <joao.m.martins@xxxxxxxxxx>
>>>> ---
>>>> The resend is mainly to get the discussion going again on what the
>>>> alternatives are, if this patch is not acceptable.
>>> Even if you don't agree with the patch, can we at least revive
>>> the discussion of what alternatives there are?
>> Sorry - it slipped through the cracks.  I have no issue with the
>> principle of the patch.
>>
>> The only problem I have, which we didn't sort out last time, is the
>> initialisation of rendezvous_fn
>>
>> It is still my opinion that under no circumstance is it ok for
>> clear_tsc_cap() to modify time_calibration_rendezvous_fn from
>> time_calibration_tsc_rendezvous to time_calibration_std_rendezvous,
>> which can in principle happen because rendezvous_fn doesn't get
>> initialised from the current time_calibration_rendezvous_fn.
> I understand that this is your primary reservation against the patch,
> yet at the same time this is also the purpose of the patch. If we're
> not allowed to change the rendezvous function to what it is supposed
> to be given the final set of CPU features we determined, then the
> whole patch is pointless. At the time we first set the pointer, we
> simply don't know yet what we'll find once we brought up APs. If we
> knew, we'd set it differently. Since pre- and post-AP bringup
> knowledge will always have the potential to differ, we need to adjust
> the function pointer in one direction anyway: Either we set it to std
> early, and switch to tsc later, or we allow setting it to tsc early,
> reverting to std if need be.

Once we have ever had cause to use time_calibration_tsc_rendezvous,
there is no situation where it is safe to switch back to
time_calibration_std_rendezvous, as the choice to use
time_calibration_tsc_rendezvous means the TSCs aren't in sync, and Xen
has no practical mechanism to resync them.  (We could guarantee not to
ever invoke Cstates, including C1E, but this doesn't prevent an SMI
doing that for us.)

time_calibration_rendezvous_fn should start with the best case scenario,
and be gradually made worse by our AP discovery, but should never get
better.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel