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

[Paul Semel <semelpaul@gmail.com>]

On 04/10/2018 10:08 AM, 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.
> 
> You should send that version then :).
> 
>>>
>>> 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..
> 
> AFAICT the following should work:
> 
> do
> {
>      ver1 = shared_info.vcpu_info[0].time.version;
>      smp_rmb();
> 
>      system_time = shared_info.vcpu_info[0].time.system_time;
>      tsc_timestamp = shared_info.vcpu_info[0].time.tsc_timestamp;
>      tsc_to_system_mul = shared_info.vcpu_info[0].time.tsc_to_system_mul;
>      tsc_shift = shared_info.vcpu_info[0].time.tsc_shift;
>      tsc = rdtsc_ordered();
>      /* NB: this barrier is probably not needed if rdtsc is serializing. */
>      smp_rmb();
> 
>      ver2 = ACCESS_ONCE(shared_info.vcpu_info[0].time.version);
> } while ( ver2 & 1 || ver1 != ver2 );
> 

Just a (probably dumb) question. Why aren't you doing ACCESS_ONCE on 
every shared_info field accesses ?
As far as I understand, we need to do this as most as we can to avoid 
having completely broken data (or security issues). Am I missing something ?

> system_time += scale_delta(tsc - tsc_timestamp, tsc_to_system_mul,
>                             time.tsc_shift);
> 
> I'm not sure the second barrier is actually needed, since
> rdtsc_ordered should be serializing.
> 
>>> 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).
> 
> rdtsc is not a serializing instruction, and as such there's no
> guarantee it's not executed before or after any of it's preceding or
> following instructions. In order to make it serializing you need to
> add a lfence or mfence, or if you are not sure about the architecture
> you likely need to add both, see:
> 
> https://marc.info/?l=xen-devel&m=151983511212795&w=2
> 
> Also, as said in the previous review, the rdtsc helper should be
> placed in a x86 specific file because it's a x86 specific instruction.
> IMO it should be placed in arch/x86/include/arch/lib.h with the rest
> of the x86 specific instructions.
> 

Thanks for clarifying this point ! I will put the function in the 
correct place, and add an implementation of rdtsc_ordered with 
serialization 🙂

> Hope this clarifies the changes.

This definitely does, thanks !

-- 
Paul

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