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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Apr 09, 2018 at 05:12:46PM +0200, Paul Semel wrote:
> 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@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.
> 
> 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 );

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.

Hope this clarifies the changes.

Roger.

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