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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 31.08.16 at 15:42, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 03/08/16 10:43, Jan Beulich wrote:
>>>>> On 02.08.16 at 21:08, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 04/07/16 16:53, Jan Beulich wrote:
>>>>>>> On 04.07.16 at 17:39, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>>> On 20/06/16 16:20, Jan Beulich wrote:
>>>>>>>>> On 20.06.16 at 16:32, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>>>>> On 15/06/16 11:28, Jan Beulich wrote:
>>>>>>>> --- a/xen/arch/x86/time.c
>>>>>>>> +++ b/xen/arch/x86/time.c
>>>>>>>> @@ -1358,6 +1358,24 @@ static void time_calibration(void *unuse
>>>>>>>>                       &r, 1);
>>>>>>>>  }
>>>>>>>>  
>>>>>>>> +void __init clear_tsc_cap(unsigned int feature)
>>>>>>>> +{
>>>>>>>> +    void (*rendezvous_fn)(void *) = time_calibration_std_rendezvous;
>>>>>>> This should read time_calibration_rendezvous_fn rather than assuming
>>>>>>> time_calibration_std_rendezvous.
>>>>>>>
>>>>>>> Otherwise, there is a risk that it could be reset back to
>>>>>>> time_calibration_std_rendezvous.
>>>>>> But that's the purpose: We may need to switch back.
>>>>> Under what circumstances could we ever move from re-syncing back to not
>>>>> re-syncing?
>>>> verify_tsc_reliability() may result in X86_FEATURE_TSC_RELIABLE
>>>> getting cleared. That's an initcall, which means it runs after
>>>> init_xen_time(), and hence after the rendezvous function got
>>>> established initially.
>>> Right, but that isn't important.
>>>
>>> There will never be a case where, once TSC_RELIABLE is cleared, it is
>>> safe to revert back to std_rendezvous, even if TSC_RELIABLE is
>>> subsequently re-set.
>> You've got this backwards: TSC_RELIABLE may get _cleared_ late.
> 
> Quite - I haven't got this backwards.
> 
>> Nothing can ever set it late, due to the use of setup_clear_cpu_cap().
>> Reverting back to time_calibration_std_rendezvous() would only be
>> possible if CONSTANT_TSC got cleared late, ...
> 
> time_calibration_rendezvous_fn defaults to
> time_calibration_std_rendezvous(), i.e. defaults to the assumption that
> the TSCs are invariant.
> 
> We then later call clear_caps(TSC_RELIABLE), and the default changes to
> time_calibration_tsc_rendezvous().
> 
> We then later call clear_tsc_cap(CONSTANT_TSC), or indeed that
> CONSTANT_TSC was never set in the first place, and the default switches
> back to time_calibration_std_rendezvous() because of the aformentioned bug.
> 
> Once the switch to time_calibration_tsc_rendezvous() is made, it is
> never safe to switch back.

You still don't explain why - I don't see what's wrong with doing
so namely when there wasn't a whole lot of skew gained yet. Plus
I don't see why running with tsc_rendezvous is fine when one of
the two pre-conditions for switching to it isn't met, but we find
out only after having brought up APs.

Jan

> Therefore, your function must read time_calibration_rendezvous_fn and
> not assume time_calibration_std_rendezvous().
> 
> ~Andrew




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