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

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

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.

> +
> +    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:

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

Roger.

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