Re: [Xen-devel] [PATCH v4 2/7] add current_time function to time manager

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

On Tue, Apr 10, 2018 at 09:16:56PM +0200, Paul Semel wrote:
> this function returns the "epoch" time
> 
> Signed-off-by: Paul Semel <phentex@xxxxxxxxx>
> ---
> 
> 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 <arch/barrier.h>
>  #include <arch/lib.h>
> +#include <arch/div.h>

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