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

[Paul Semel <semelpaul@gmail.com>]

On 04/09/2018 04:35 PM, Roger Pau Monné wrote:
>> 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@amazon.de>
>> ---
> 
> 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.

About this scale_delta, we discussed with Andrew, and we are going to use 
another version of the function as far as I remember. That's why I am not taking 
care of it for the moment.
> 
> 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'm sorry, I am probably not getting your point here, because I am already 
checking for the version. I was actually checking for the wc_version too in the 
first version of those patches, but after chatting with Andrew, It appeared that 
it was not necessary..

> 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.
> 
>> +
>> +    rdtsc(tsc);
>> +
>> +    system_time += scale_delta(tsc - old_tsc,
>> +                   ACCESS_ONCE(shared_info.vcpu_info[0].time.tsc_to_system_mul),
>> +                   ACCESS_ONCE(shared_info.vcpu_info[0].time.tsc_shift));
>> +
>> +    return system_time;
>> +}
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * tab-width: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/include/xtf/time.h b/include/xtf/time.h
>> new file mode 100644
>> index 0000000..b88da63
>> --- /dev/null
>> +++ b/include/xtf/time.h
>> @@ -0,0 +1,31 @@
>> +/**
>> + * @file include/xtf/time.h
>> + *
>> + * Time management
>> + */
>> +#ifndef XTF_TIME_H
>> +# define XTF_TIME_H
>> +
>> +#include <xtf/types.h>
>> +
>> +#define rdtsc(tsc) {\
>> +    uint32_t lo, hi;\
>> +    __asm__ volatile("rdtsc": "=a"(lo), "=d"(hi));\
> 
> Please make sure you only send a new version after having fixed all
> the comments, this is still missing the serialization requirements
> mentioned in the review, and it's also the wrong file to place this
> helper:
> 

I am sorry, I was really convinced that this version didn't need revision 
anymore (and I still don't see what I should change).

> https://lists.xenproject.org/archives/html/xen-devel/2018-04/msg00546.html
> 
> Roger.
> 

Thanks,

-- 
Paul

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