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

[Paul Semel <semelpaul@xxxxxxxxx>]

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.
> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel