Re: [Xen-devel] [PATCH v3 1/7] introduce time managment in xtf

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 09/04/18 15:35, Roger Pau Monné wrote:
> On Mon, Apr 09, 2018 at 04:35:37PM +0200, Paul Semel wrote:
>> From: Paul Semel <phentex@xxxxxxxxx>
>>
>> this file is introduce to be able to implement an inter domain
>> communication protocol over xenstore. For synchronization purpose, we do
>> really want to be able to "control" time
>>
>> common/time.c: since_boot_time gets the time in nanoseconds from the
>> moment the VM has booted
>>
>> Signed-off-by: Paul Semel <phentex@xxxxxxxxx>
>> ---
> This seems to be missing a list of changes between v2 and v3. Please
> add such a list when posting new versions.
>
>> +uint64_t since_boot_time(void)
>> +{
>> +    uint64_t tsc;
>> +    uint32_t ver1, ver2;
>> +    uint64_t system_time;
>> +    uint64_t old_tsc;
>> +
>> +    do
>> +    {
>> +        do
>> +        {
>> +            ver1 = ACCESS_ONCE(shared_info.vcpu_info[0].time.version);
>> +            smp_rmb();
>> +        } while ( (ver1 & 1) == 1 );
>> +
>> +        system_time = 
>> ACCESS_ONCE(shared_info.vcpu_info[0].time.system_time);
>> +        old_tsc = ACCESS_ONCE(shared_info.vcpu_info[0].time.tsc_timestamp);
>> +        smp_rmb();
>> +        ver2 = ACCESS_ONCE(shared_info.vcpu_info[0].time.version);
>> +        smp_rmb();
>> +    } while ( ver1 != ver2 );
> This is still overly complicated IMO, and you have not replied to my
> question of whether doing the scale_delta below is OK.
>
> AFAICT uou _cannot_ access any of the vcpu_time_info fields without
> checking for the version (in order to avoid reading inconsistent data
> during an update), yet below you read tsc_to_system_mul and
> tsc_shift.
>
> I've already pointed out the code at:
>
> https://github.com/freebsd/freebsd/blob/master/sys/x86/x86/pvclock.c#L141
>
> As a simpler reference implementation.

There are real bugs in that implementation (wrong barriers, although
they are overly strong rather than overly week), and an inefficiency, in
that you don't want any extraneous calculation in between the two reads
of version.  The call to pvclock_get_nsec_offset(ti) wants to be outside
of the critical region, to reduce the chance of intersecting an update
and having repeat the loop again.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel