On Tue, Apr 10, 2018 at 09:16:56PM +0200, Paul Semel wrote:
> this function returns the "epoch" time
>
> Signed-off-by: Paul Semel
> ---
>
> Notes:
> v4:
> - new patch version
>
> common/time.c | 39 +++
> include/xtf/time.h | 5 +
> 2 files changed, 44 insertions(+)
>
> diff --git a/common/time.c b/common/time.c
> index 79abc7e..c1b7cd1 100644
> --- a/common/time.c
> +++ b/common/time.c
> @@ -4,6 +4,7 @@
>
> #include
> #include
> +#include
Sorting
>
> /* This function was taken from mini-os source code */
> /* It returns ((delta << shift) * mul_frac) >> 32 */
> @@ -70,6 +71,44 @@ uint64_t since_boot_time(void)
> return system_time;
> }
>
> +static void get_time_info(uint64_t *boot_time, uint64_t *sec, uint32_t *nsec)
> +{
> +uint32_t ver1, ver2;
> +do {
> +ver1 = ACCESS_ONCE(shared_info.wc_version);
> +smp_rmb();
> +*boot_time = since_boot_time();
Why are you reading the uptime in the middle of the loop? You can
read this out of the loop, or in a different function.
> +#if defined(__i386__)
> +*sec = (uint64_t)ACCESS_ONCE(shared_info.wc_sec);
> +#else
> +*sec = ((uint64_t)ACCESS_ONCE(shared_info.wc_sec_hi) << 32)
> +| ACCESS_ONCE(shared_info.wc_sec);
> +#endif
I think this should be:
*sec = shared_info.wc_sec;
#ifdef __i386__
*sec |= shared_info.arch.wc_sec_hi << 32;
#else
*sec |= shared_info.wc_sec_hi << 32;
#endif
> +*nsec = (uint64_t)ACCESS_ONCE(shared_info.wc_nsec);
> +smp_rmb();
> +ver2 = ACCESS_ONCE(shared_info.wc_version);
> +smp_rmb();
This AFAICT this has the same issues as the code used in patch 1 to
access the vcpu_time_info data.
You only need the ACCESS_ONCE in order to read ver2, the rest can be
dropped.
> +} while ( (ver1 & 1) != 0 && ver1 != ver2 );
> +}
> +
> +/* This function return the epoch time (number of seconds elapsed
> + * since Juanary 1, 1970) */
> +uint64_t current_time(void)
> +{
> +uint32_t nsec;
> +uint64_t boot_time, sec;
> +
> +get_time_info(&boot_time, &sec, &nsec);
I think it would make more sense to:
1. Call since_boot_time hypervisor_uptime.
2. Call get_time_info hypervisor_wallclock.
In order to get the current time here you call both functions and then
add the result, like you do below.
> +
> +#if defined(__i386__)
> +divmod64(&boot_time, SEC_TO_NSEC(1));
> +#else
> +boot_time /= SEC_TO_NSEC(1);
> +#endif
Please use devmod64 unconditionally.
Thanks, Roger.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
