* Re: [PATCH v4 1/7] introduce time managment in xtf
[not found] <20180410191701.17203-1-semelpaul@gmail.com>
@ 2018-04-13 12:05 ` Roger Pau Monné
2018-04-16 10:48 ` Paul Semel
[not found] ` <20180410191701.17203-2-semelpaul@gmail.com>
` (5 subsequent siblings)
6 siblings, 1 reply; 13+ messages in thread
From: Roger Pau Monné @ 2018-04-13 12:05 UTC (permalink / raw)
To: Paul Semel; +Cc: xen-devel, Paul Semel, andrew.cooper3, wipawel
On Tue, Apr 10, 2018 at 09:16:55PM +0200, Paul Semel 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@amazon.de>
> ---
>
> Notes:
> v4:
> - moved rdtsc to arch/x86/include/arch/lib.h
> - added a rdtsc_ordered implementation to serialize rdtsc
> - simplified since_boot_time function
> - still need to have Andrew's scale_delta version
>
> arch/x86/include/arch/lib.h | 18 ++++++++++
> build/files.mk | 1 +
> common/time.c | 81 +++++++++++++++++++++++++++++++++++++++++++++
> include/xtf/time.h | 24 ++++++++++++++
> 4 files changed, 124 insertions(+)
> create mode 100644 common/time.c
> create mode 100644 include/xtf/time.h
>
> diff --git a/arch/x86/include/arch/lib.h b/arch/x86/include/arch/lib.h
> index 0045902..510cdb1 100644
> --- a/arch/x86/include/arch/lib.h
> +++ b/arch/x86/include/arch/lib.h
> @@ -6,6 +6,7 @@
> #include <xen/arch-x86/xen.h>
> #include <arch/desc.h>
> #include <arch/msr.h>
> +#include <arch/barrier.h>
This include list is sorted alphabetically.
>
> static inline void cpuid(uint32_t leaf,
> uint32_t *eax, uint32_t *ebx,
> @@ -374,6 +375,23 @@ static inline void write_xcr0(uint64_t xcr0)
> xsetbv(0, xcr0);
> }
>
> +static inline uint64_t rdtsc(void)
> +{
> + uint32_t lo, hi;
> +
> + asm volatile("rdtsc": "=a"(lo), "=d"(hi));
> +
> + return ((uint64_t)hi << 32) | lo;
> +}
> +
> +static inline uint64_t rdtsc_ordered(void)
> +{
> + rmb();
> + mb();
> +
> + return rdtsc();
> +}
I would likely just add a single function like:
static inline uint64_t rdtsc_ordered(void)
{
uint32_t lo, hi;
asm volatile("lfence; mfence; rdtsc" : "=a" (lo), "=d" (hi));
return ((uint64_t)hi << 32) | lo;
}
Because there's no external caller of rdtsc, but I will leave that to
Andrew to decide. In any case this should work fine.
> +
> #endif /* XTF_X86_LIB_H */
>
> /*
> diff --git a/build/files.mk b/build/files.mk
> index 46b42d6..55ed1ca 100644
> --- a/build/files.mk
> +++ b/build/files.mk
> @@ -16,6 +16,7 @@ obj-perarch += $(ROOT)/common/libc/vsnprintf.o
> obj-perarch += $(ROOT)/common/report.o
> obj-perarch += $(ROOT)/common/setup.o
> obj-perarch += $(ROOT)/common/xenbus.o
> +obj-perarch += $(ROOT)/common/time.o
>
> obj-perenv += $(ROOT)/arch/x86/decode.o
> obj-perenv += $(ROOT)/arch/x86/desc.o
> diff --git a/common/time.c b/common/time.c
> new file mode 100644
> index 0000000..79abc7e
> --- /dev/null
> +++ b/common/time.c
> @@ -0,0 +1,81 @@
> +#include <xtf/types.h>
> +#include <xtf/traps.h>
> +#include <xtf/time.h>
Alphabetic order please.
> +#include <arch/barrier.h>
> +#include <arch/lib.h>
> +
> +/* This function was taken from mini-os source code */
> +/* It returns ((delta << shift) * mul_frac) >> 32 */
Comment has wrong style, please check CODING_STYLE.
> +static inline uint64_t scale_delta(uint64_t delta, uint32_t mul_frac, int shift)
> +{
> + uint64_t product;
> +#ifdef __i386__
> + uint32_t tmp1, tmp2;
> +#endif
> +
> + if ( shift < 0 )
> + delta >>= -shift;
> + else
> + delta <<= shift;
> +
> +#ifdef __i386__
> + __asm__ (
> + "mul %5 ; "
> + "mov %4,%%eax ; "
> + "mov %%edx,%4 ; "
> + "mul %5 ; "
> + "add %4,%%eax ; "
> + "xor %5,%5 ; "
> + "adc %5,%%edx ; "
> + : "=A" (product), "=r" (tmp1), "=r" (tmp2)
> + : "a" ((uint32_t)delta), "1" ((uint32_t)(delta >> 32)), "2" (mul_frac) );
This line is too long.
> +#else
> + __asm__ (
> + "mul %%rdx ; shrd $32,%%rdx,%%rax"
> + : "=a" (product) : "0" (delta), "d" ((uint64_t)mul_frac) );
Not sure whether you need to add a ': "d"' clobber here, since the d
register is used but it's not in the list of output operands.
> +#endif
> +
> + return product;
> +}
> +
> +
> +uint64_t since_boot_time(void)
> +{
> + uint32_t ver1, ver2;
> + uint64_t tsc_timestamp, system_time, tsc;
> + uint32_t tsc_to_system_mul;
> + int8_t tsc_shift;
> +
> + do
> + {
> + ver1 = ACCESS_ONCE(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();
> + smp_rmb();
I don't think you need the barrier here if you use rdtsc_ordered, but
I would like confirmation from someone else.
> +
> + 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,
> + 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..8180e07
> --- /dev/null
> +++ b/include/xtf/time.h
> @@ -0,0 +1,24 @@
> +/**
> + * @file include/xtf/time.h
> + *
> + * Time management
> + */
> +#ifndef XTF_TIME_H
> +# define XTF_TIME_H
> +
> +#include <xtf/types.h>
> +
> +/* Time from boot in nanoseconds */
> +uint64_t since_boot_time(void);
since_boot_time looks like a weird name, I would probably name this
hypervisor_uptime or system_time, but I'm not specially thrilled
anyway.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 2/7] add current_time function to time manager
[not found] ` <20180410191701.17203-2-semelpaul@gmail.com>
@ 2018-04-13 13:37 ` Roger Pau Monné
0 siblings, 0 replies; 13+ messages in thread
From: Roger Pau Monné @ 2018-04-13 13:37 UTC (permalink / raw)
To: Paul Semel; +Cc: xen-devel, Paul Semel, andrew.cooper3, wipawel
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@amazon.de>
> ---
>
> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 3/7] add gettimeofday function to time managment
[not found] ` <20180410191701.17203-3-semelpaul@gmail.com>
@ 2018-04-13 13:39 ` Roger Pau Monné
2018-04-16 10:16 ` Paul Semel
0 siblings, 1 reply; 13+ messages in thread
From: Roger Pau Monné @ 2018-04-13 13:39 UTC (permalink / raw)
To: Paul Semel; +Cc: xen-devel, Paul Semel, andrew.cooper3, wipawel
On Tue, Apr 10, 2018 at 09:16:57PM +0200, Paul Semel wrote:
> this function acts as the POSIX gettimeofday function
>
> Signed-off-by: Paul Semel <phentex@amazon.de>
> ---
>
> Notes:
> v4:
> - new patch version
>
> common/time.c | 30 ++++++++++++++++++++++++++++++
> include/xtf/time.h | 8 ++++++++
> 2 files changed, 38 insertions(+)
>
> diff --git a/common/time.c b/common/time.c
> index c1b7cd1..8489f3b 100644
> --- a/common/time.c
> +++ b/common/time.c
> @@ -1,6 +1,7 @@
> #include <xtf/types.h>
> #include <xtf/traps.h>
> #include <xtf/time.h>
> +#include <xen/errno.h>
Sorting.
>
> #include <arch/barrier.h>
> #include <arch/lib.h>
> @@ -109,6 +110,35 @@ uint64_t current_time(void)
> return sec + boot_time;
> }
>
> +/* The POSIX gettimeofday syscall normally takes a second argument, which is
> + * the timezone (struct timezone). However, it sould be NULL because linux
> + * doesn't use it anymore. So we need for us to add it in this function
> + */
> +int gettimeofday(struct timeval *tp, void *restrict tzp)
> +{
> + uint64_t boot_time, sec;
> + uint32_t mod, nsec;
> +
> + if ( tzp != NULL )
> + return -EOPNOTSUPP;
> +
> + if ( tp == NULL )
> + return -EINVAL;
> +
> + get_time_info(&boot_time, &sec, &nsec);
Why are you using get_time_info here? Shouldn't you use the
current_time function introduced in the previous patch?
Or else I don't see the need to introduce current_time in the previous
patch.
> +#if defined(__i386__)
> + mod = divmod64(&boot_time, SEC_TO_NSEC(1));
> +#else
> + mod = boot_time % SEC_TO_NSEC(1);
> + boot_time /= SEC_TO_NSEC(1);
> +#endif
Please use divmod64 unconditionally.
> +
> + tp->sec = sec + boot_time;
> + tp->nsec = nsec + mod;
> + return 0;
> +}
> +
> /*
> * Local variables:
> * mode: C
> diff --git a/include/xtf/time.h b/include/xtf/time.h
> index e33dc8a..ce4d6db 100644
> --- a/include/xtf/time.h
> +++ b/include/xtf/time.h
> @@ -8,6 +8,12 @@
>
> #include <xtf/types.h>
>
> +struct timeval {
> + uint64_t sec;
> + uint64_t nsec;
> +};
> +
> +
Extra newline.
Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 4/7] add nspin_sleep function to time manager
[not found] ` <20180410191701.17203-4-semelpaul@gmail.com>
@ 2018-04-13 13:51 ` Roger Pau Monné
0 siblings, 0 replies; 13+ messages in thread
From: Roger Pau Monné @ 2018-04-13 13:51 UTC (permalink / raw)
To: Paul Semel; +Cc: xen-devel, Paul Semel, andrew.cooper3, wipawel
On Tue, Apr 10, 2018 at 09:16:58PM +0200, Paul Semel wrote:
> this function spin sleeps for t nanoseconds
>
> Signed-off-by: Paul Semel <phentex@amazon.de>
> ---
>
> Notes:
> v4:
> - new patch version
>
> common/time.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/common/time.c b/common/time.c
> index 8489f3b..232e134 100644
> --- a/common/time.c
> +++ b/common/time.c
> @@ -139,6 +139,18 @@ int gettimeofday(struct timeval *tp, void *restrict tzp)
> return 0;
> }
>
> +static inline void nspin_sleep(uint64_t t)
> +{
> + uint64_t curr = since_boot_time();
> + uint64_t end = curr + t;
> +
> + if ( end < curr )
> + panic("end value overflows counter\n");
> +
> + while ( since_boot_time() < end )
> + asm volatile ("pause");
You likely want to add a pause helper to arch/x86/include/arch/lib.h
in order to avoid open-coding the asm instruction in common code.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 5/7] add spin_sleep function to time manager
[not found] ` <20180410191701.17203-5-semelpaul@gmail.com>
@ 2018-04-13 13:52 ` Roger Pau Monné
0 siblings, 0 replies; 13+ messages in thread
From: Roger Pau Monné @ 2018-04-13 13:52 UTC (permalink / raw)
To: Paul Semel; +Cc: xen-devel, Paul Semel, andrew.cooper3, wipawel
On Tue, Apr 10, 2018 at 09:16:59PM +0200, Paul Semel wrote:
> this function uses nspin_sleep to spin sleep for t seconds
IMO you can squash this into the previous patch.
> Signed-off-by: Paul Semel <phentex@amazon.de>
> ---
>
> Notes:
> v4:
> - new patch version
>
> common/time.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/common/time.c b/common/time.c
> index 232e134..87db124 100644
> --- a/common/time.c
> +++ b/common/time.c
> @@ -151,6 +151,12 @@ static inline void nspin_sleep(uint64_t t)
> asm volatile ("pause");
> }
>
> +static inline void spin_sleep(uint64_t t)
> +{
> + uint64_t nsec = SEC_TO_NSEC(t);
Newline.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 6/7] add mspin_sleep function to time manager
[not found] ` <20180410191701.17203-6-semelpaul@gmail.com>
@ 2018-04-13 13:53 ` Roger Pau Monné
0 siblings, 0 replies; 13+ messages in thread
From: Roger Pau Monné @ 2018-04-13 13:53 UTC (permalink / raw)
To: Paul Semel; +Cc: xen-devel, Paul Semel, andrew.cooper3, wipawel
On Tue, Apr 10, 2018 at 09:17:00PM +0200, Paul Semel wrote:
> this function uses mspin_sleep to spin sleep for t milliseconds
>
> Signed-off-by: Paul Semel <phentex@amazon.de>
> ---
>
> Notes:
> v4:
> - new patch version
>
> common/time.c | 6 ++++++
> include/xtf/time.h | 1 +
> 2 files changed, 7 insertions(+)
>
> diff --git a/common/time.c b/common/time.c
> index 87db124..7515eb0 100644
> --- a/common/time.c
> +++ b/common/time.c
> @@ -157,6 +157,12 @@ static inline void spin_sleep(uint64_t t)
> nspin_sleep(nsec);
> }
>
> +static inline void mspin_sleep(uint64_t t)
> +{
> + uint64_t nsec = MSEC_TO_NSEC(t);
Newline.
And IMO can be squashed into the previous patch.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 7/7] add sleep, msleep and NOW() macros to time manager
[not found] ` <20180410191701.17203-7-semelpaul@gmail.com>
@ 2018-04-13 13:55 ` Roger Pau Monné
2018-04-16 10:45 ` Paul Semel
0 siblings, 1 reply; 13+ messages in thread
From: Roger Pau Monné @ 2018-04-13 13:55 UTC (permalink / raw)
To: Paul Semel; +Cc: xen-devel, Paul Semel, andrew.cooper3, wipawel
On Tue, Apr 10, 2018 at 09:17:01PM +0200, Paul Semel wrote:
> those are helpful macro to use the time manager correctly
>
> Signed-off-by: Paul Semel <phentex@amazon.de>
> ---
>
> Notes:
> v4:
> - new patch version
>
> common/time.c | 10 ++++++++++
> include/xtf/time.h | 12 ++++++++++++
> 2 files changed, 22 insertions(+)
>
> diff --git a/common/time.c b/common/time.c
> index 7515eb0..e2779b9 100644
> --- a/common/time.c
> +++ b/common/time.c
> @@ -163,6 +163,16 @@ static inline void mspin_sleep(uint64_t t)
> nspin_sleep(nsec);
> }
>
> +void sleep(uint64_t t)
> +{
> + spin_sleep(t);
> +}
> +
> +void msleep(uint64_t t)
> +{
> + mspin_sleep(t);
Why can you just call mspin_sleep msleep directly?
The same applies to spin_sleep.
Also I was expecting to see some kind of test appear at the end of the
series. You are basically adding a bunch of dead code, since there's
no user of any of the newly introduced functions.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 3/7] add gettimeofday function to time managment
2018-04-13 13:39 ` [PATCH v4 3/7] add gettimeofday function to time managment Roger Pau Monné
@ 2018-04-16 10:16 ` Paul Semel
2018-04-17 7:41 ` Roger Pau Monné
0 siblings, 1 reply; 13+ messages in thread
From: Paul Semel @ 2018-04-16 10:16 UTC (permalink / raw)
To: Roger Pau Monné; +Cc: xen-devel, andrew.cooper3, wipawel
Hi !
Thanks a lot for reviewing !
On 04/13/2018 03:39 PM, Roger Pau Monné wrote:
> On Tue, Apr 10, 2018 at 09:16:57PM +0200, Paul Semel wrote:
>> this function acts as the POSIX gettimeofday function
>>
>> Signed-off-by: Paul Semel <phentex@amazon.de>
>> ---
>>
>> Notes:
>> v4:
>> - new patch version
>>
>> common/time.c | 30 ++++++++++++++++++++++++++++++
>> include/xtf/time.h | 8 ++++++++
>> 2 files changed, 38 insertions(+)
>>
>> diff --git a/common/time.c b/common/time.c
>> index c1b7cd1..8489f3b 100644
>> --- a/common/time.c
>> +++ b/common/time.c
>> @@ -1,6 +1,7 @@
>> #include <xtf/types.h>
>> #include <xtf/traps.h>
>> #include <xtf/time.h>
>> +#include <xen/errno.h>
>
> Sorting.
>
>>
>> #include <arch/barrier.h>
>> #include <arch/lib.h>
>> @@ -109,6 +110,35 @@ uint64_t current_time(void)
>> return sec + boot_time;
>> }
>>
>> +/* The POSIX gettimeofday syscall normally takes a second argument, which is
>> + * the timezone (struct timezone). However, it sould be NULL because linux
>> + * doesn't use it anymore. So we need for us to add it in this function
>> + */
>> +int gettimeofday(struct timeval *tp, void *restrict tzp)
>> +{
>> + uint64_t boot_time, sec;
>> + uint32_t mod, nsec;
>> +
>> + if ( tzp != NULL )
>> + return -EOPNOTSUPP;
>> +
>> + if ( tp == NULL )
>> + return -EINVAL;
>> +
>> + get_time_info(&boot_time, &sec, &nsec);
>
> Why are you using get_time_info here? Shouldn't you use the
> current_time function introduced in the previous patch?
>
> Or else I don't see the need to introduce current_time in the previous
> patch.
>
Actually, I can't use *only* the current_time function here, because I won't be
able to get the nanoseconds if so.
Anyway, in the last patch, I am using current_time function for the NOW() macro,
which I think is really helpful.
Do you think I should drop all of those ?
>> +#if defined(__i386__)
>> + mod = divmod64(&boot_time, SEC_TO_NSEC(1));
>> +#else
>> + mod = boot_time % SEC_TO_NSEC(1);
>> + boot_time /= SEC_TO_NSEC(1);
>> +#endif
>
> Please use divmod64 unconditionally.
>
>> +
>> + tp->sec = sec + boot_time;
>> + tp->nsec = nsec + mod;
>> + return 0;
>> +}
>> +
>> /*
>> * Local variables:
>> * mode: C
>> diff --git a/include/xtf/time.h b/include/xtf/time.h
>> index e33dc8a..ce4d6db 100644
>> --- a/include/xtf/time.h
>> +++ b/include/xtf/time.h
>> @@ -8,6 +8,12 @@
>>
>> #include <xtf/types.h>
>>
>> +struct timeval {
>> + uint64_t sec;
>> + uint64_t nsec;
>> +};
>> +
>> +
>
> Extra newline.
Thanks,
--
Paul
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 7/7] add sleep, msleep and NOW() macros to time manager
2018-04-13 13:55 ` [PATCH v4 7/7] add sleep, msleep and NOW() macros " Roger Pau Monné
@ 2018-04-16 10:45 ` Paul Semel
2018-04-17 7:42 ` Roger Pau Monné
0 siblings, 1 reply; 13+ messages in thread
From: Paul Semel @ 2018-04-16 10:45 UTC (permalink / raw)
To: Roger Pau Monné; +Cc: xen-devel, andrew.cooper3, wipawel
On 04/13/2018 03:55 PM, Roger Pau Monné wrote:
>> those are helpful macro to use the time manager correctly
>>
>> Signed-off-by: Paul Semel <phentex@amazon.de>
>> ---
>>
>> Notes:
>> v4:
>> - new patch version
>>
>> common/time.c | 10 ++++++++++
>> include/xtf/time.h | 12 ++++++++++++
>> 2 files changed, 22 insertions(+)
>>
>> diff --git a/common/time.c b/common/time.c
>> index 7515eb0..e2779b9 100644
>> --- a/common/time.c
>> +++ b/common/time.c
>> @@ -163,6 +163,16 @@ static inline void mspin_sleep(uint64_t t)
>> nspin_sleep(nsec);
>> }
>>
>> +void sleep(uint64_t t)
>> +{
>> + spin_sleep(t);
>> +}
>> +
>> +void msleep(uint64_t t)
>> +{
>> + mspin_sleep(t);
>
> Why can you just call mspin_sleep msleep directly?
>
> The same applies to spin_sleep.
>
> Also I was expecting to see some kind of test appear at the end of the
> series. You are basically adding a bunch of dead code, since there's
> no user of any of the newly introduced functions.
Actually, I won't be able to add a real XSA test using this feature. Anyway, I
do think that having the sleep functions will be really useful in the future.
Anyway, I was thinking about adding a test that is calling the gettimeofday
function or something similar.
What do you think about it ?
--
Paul Semel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 1/7] introduce time managment in xtf
2018-04-13 12:05 ` [PATCH v4 1/7] introduce time managment in xtf Roger Pau Monné
@ 2018-04-16 10:48 ` Paul Semel
2018-04-17 7:45 ` Roger Pau Monné
0 siblings, 1 reply; 13+ messages in thread
From: Paul Semel @ 2018-04-16 10:48 UTC (permalink / raw)
To: Roger Pau Monné; +Cc: xen-devel, andrew.cooper3, wipawel
On 04/13/2018 02:05 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@amazon.de>
>> ---
>>
>> Notes:
>> v4:
>> - moved rdtsc to arch/x86/include/arch/lib.h
>> - added a rdtsc_ordered implementation to serialize rdtsc
>> - simplified since_boot_time function
>> - still need to have Andrew's scale_delta version
>>
>> arch/x86/include/arch/lib.h | 18 ++++++++++
>> build/files.mk | 1 +
>> common/time.c | 81 +++++++++++++++++++++++++++++++++++++++++++++
>> include/xtf/time.h | 24 ++++++++++++++
>> 4 files changed, 124 insertions(+)
>> create mode 100644 common/time.c
>> create mode 100644 include/xtf/time.h
>>
>> diff --git a/arch/x86/include/arch/lib.h b/arch/x86/include/arch/lib.h
>> index 0045902..510cdb1 100644
>> --- a/arch/x86/include/arch/lib.h
>> +++ b/arch/x86/include/arch/lib.h
>> @@ -6,6 +6,7 @@
>> #include <xen/arch-x86/xen.h>
>> #include <arch/desc.h>
>> #include <arch/msr.h>
>> +#include <arch/barrier.h>
>
> This include list is sorted alphabetically.
>
>>
>> static inline void cpuid(uint32_t leaf,
>> uint32_t *eax, uint32_t *ebx,
>> @@ -374,6 +375,23 @@ static inline void write_xcr0(uint64_t xcr0)
>> xsetbv(0, xcr0);
>> }
>>
>> +static inline uint64_t rdtsc(void)
>> +{
>> + uint32_t lo, hi;
>> +
>> + asm volatile("rdtsc": "=a"(lo), "=d"(hi));
>> +
>> + return ((uint64_t)hi << 32) | lo;
>> +}
>> +
>> +static inline uint64_t rdtsc_ordered(void)
>> +{
>> + rmb();
>> + mb();
>> +
>> + return rdtsc();
>> +}
>
> I would likely just add a single function like:
>
> static inline uint64_t rdtsc_ordered(void)
> {
> uint32_t lo, hi;
>
> asm volatile("lfence; mfence; rdtsc" : "=a" (lo), "=d" (hi));
>
> return ((uint64_t)hi << 32) | lo;
> }
>
> Because there's no external caller of rdtsc, but I will leave that to
> Andrew to decide. In any case this should work fine.
>
I think you're right for this one. What do you think about it @Andrew ?
>> +
>> #endif /* XTF_X86_LIB_H */
>>
>> /*
>> diff --git a/build/files.mk b/build/files.mk
>> index 46b42d6..55ed1ca 100644
>> --- a/build/files.mk
>> +++ b/build/files.mk
>> @@ -16,6 +16,7 @@ obj-perarch += $(ROOT)/common/libc/vsnprintf.o
>> obj-perarch += $(ROOT)/common/report.o
>> obj-perarch += $(ROOT)/common/setup.o
>> obj-perarch += $(ROOT)/common/xenbus.o
>> +obj-perarch += $(ROOT)/common/time.o
>>
>> obj-perenv += $(ROOT)/arch/x86/decode.o
>> obj-perenv += $(ROOT)/arch/x86/desc.o
>> diff --git a/common/time.c b/common/time.c
>> new file mode 100644
>> index 0000000..79abc7e
>> --- /dev/null
>> +++ b/common/time.c
>> @@ -0,0 +1,81 @@
>> +#include <xtf/types.h>
>> +#include <xtf/traps.h>
>> +#include <xtf/time.h>
>
> Alphabetic order please.
>
>> +#include <arch/barrier.h>
>> +#include <arch/lib.h>
>> +
>> +/* This function was taken from mini-os source code */
>> +/* It returns ((delta << shift) * mul_frac) >> 32 */
>
> Comment has wrong style, please check CODING_STYLE.
>
>> +static inline uint64_t scale_delta(uint64_t delta, uint32_t mul_frac, int shift)
>> +{
>> + uint64_t product;
>> +#ifdef __i386__
>> + uint32_t tmp1, tmp2;
>> +#endif
>> +
>> + if ( shift < 0 )
>> + delta >>= -shift;
>> + else
>> + delta <<= shift;
>> +
>> +#ifdef __i386__
>> + __asm__ (
>> + "mul %5 ; "
>> + "mov %4,%%eax ; "
>> + "mov %%edx,%4 ; "
>> + "mul %5 ; "
>> + "add %4,%%eax ; "
>> + "xor %5,%5 ; "
>> + "adc %5,%%edx ; "
>> + : "=A" (product), "=r" (tmp1), "=r" (tmp2)
>> + : "a" ((uint32_t)delta), "1" ((uint32_t)(delta >> 32)), "2" (mul_frac) );
>
> This line is too long.
>
>> +#else
>> + __asm__ (
>> + "mul %%rdx ; shrd $32,%%rdx,%%rax"
>> + : "=a" (product) : "0" (delta), "d" ((uint64_t)mul_frac) );
>
> Not sure whether you need to add a ': "d"' clobber here, since the d
> register is used but it's not in the list of output operands.
>
>> +#endif
>> +
>> + return product;
>> +}
>> +
Actually, I'm not sure that I have to make that much changes to this function,
as @Andrew wanted to use another version of it as far as I recall.
>> +
>> +uint64_t since_boot_time(void)
>> +{
>> + uint32_t ver1, ver2;
>> + uint64_t tsc_timestamp, system_time, tsc;
>> + uint32_t tsc_to_system_mul;
>> + int8_t tsc_shift;
>> +
>> + do
>> + {
>> + ver1 = ACCESS_ONCE(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();
>> + smp_rmb();
>
> I don't think you need the barrier here if you use rdtsc_ordered, but
> I would like confirmation from someone else.
>
>> +
>> + 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,
>> + 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..8180e07
>> --- /dev/null
>> +++ b/include/xtf/time.h
>> @@ -0,0 +1,24 @@
>> +/**
>> + * @file include/xtf/time.h
>> + *
>> + * Time management
>> + */
>> +#ifndef XTF_TIME_H
>> +# define XTF_TIME_H
>> +
>> +#include <xtf/types.h>
>> +
>> +/* Time from boot in nanoseconds */
>> +uint64_t since_boot_time(void);
>
> since_boot_time looks like a weird name, I would probably name this
> hypervisor_uptime or system_time, but I'm not specially thrilled
> anyway.
Sure. since_boot_time was really awful..
Thanks,
--
Paul Semel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 3/7] add gettimeofday function to time managment
2018-04-16 10:16 ` Paul Semel
@ 2018-04-17 7:41 ` Roger Pau Monné
0 siblings, 0 replies; 13+ messages in thread
From: Roger Pau Monné @ 2018-04-17 7:41 UTC (permalink / raw)
To: Paul Semel; +Cc: xen-devel, andrew.cooper3, wipawel
On Mon, Apr 16, 2018 at 12:16:18PM +0200, Paul Semel wrote:
> Hi !
>
> Thanks a lot for reviewing !
>
> On 04/13/2018 03:39 PM, Roger Pau Monné wrote:
> > On Tue, Apr 10, 2018 at 09:16:57PM +0200, Paul Semel wrote:
> > > this function acts as the POSIX gettimeofday function
> > >
> > > Signed-off-by: Paul Semel <phentex@amazon.de>
> > > ---
> > >
> > > Notes:
> > > v4:
> > > - new patch version
> > >
> > > common/time.c | 30 ++++++++++++++++++++++++++++++
> > > include/xtf/time.h | 8 ++++++++
> > > 2 files changed, 38 insertions(+)
> > >
> > > diff --git a/common/time.c b/common/time.c
> > > index c1b7cd1..8489f3b 100644
> > > --- a/common/time.c
> > > +++ b/common/time.c
> > > @@ -1,6 +1,7 @@
> > > #include <xtf/types.h>
> > > #include <xtf/traps.h>
> > > #include <xtf/time.h>
> > > +#include <xen/errno.h>
> >
> > Sorting.
> >
> > > #include <arch/barrier.h>
> > > #include <arch/lib.h>
> > > @@ -109,6 +110,35 @@ uint64_t current_time(void)
> > > return sec + boot_time;
> > > }
> > > +/* The POSIX gettimeofday syscall normally takes a second argument, which is
> > > + * the timezone (struct timezone). However, it sould be NULL because linux
> > > + * doesn't use it anymore. So we need for us to add it in this function
> > > + */
> > > +int gettimeofday(struct timeval *tp, void *restrict tzp)
> > > +{
> > > + uint64_t boot_time, sec;
> > > + uint32_t mod, nsec;
> > > +
> > > + if ( tzp != NULL )
> > > + return -EOPNOTSUPP;
> > > +
> > > + if ( tp == NULL )
> > > + return -EINVAL;
> > > +
> > > + get_time_info(&boot_time, &sec, &nsec);
> >
> > Why are you using get_time_info here? Shouldn't you use the
> > current_time function introduced in the previous patch?
> >
> > Or else I don't see the need to introduce current_time in the previous
> > patch.
> >
>
> Actually, I can't use *only* the current_time function here, because I won't
> be able to get the nanoseconds if so.
>
> Anyway, in the last patch, I am using current_time function for the NOW()
> macro, which I think is really helpful.
>
> Do you think I should drop all of those ?
Without having any users of those functions it's hard to tell which
ones we should keep and which ones should be removed.
IMO, I would keep gettimeofday in order to get the current time.
Then I would also keep the m/u/sleep functions and add a small selftest in
test/selftest/main.c that makes use of those functions, for example
something as simple as:
s = gettimeofday
msleep(1)
if ( s + 1ms < gettimeofday )
xtf_failure("Fail: error in time keeping functions\n")
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 7/7] add sleep, msleep and NOW() macros to time manager
2018-04-16 10:45 ` Paul Semel
@ 2018-04-17 7:42 ` Roger Pau Monné
0 siblings, 0 replies; 13+ messages in thread
From: Roger Pau Monné @ 2018-04-17 7:42 UTC (permalink / raw)
To: Paul Semel; +Cc: xen-devel, andrew.cooper3, wipawel
On Mon, Apr 16, 2018 at 12:45:17PM +0200, Paul Semel wrote:
> On 04/13/2018 03:55 PM, Roger Pau Monné wrote:
> > > those are helpful macro to use the time manager correctly
> > >
> > > Signed-off-by: Paul Semel <phentex@amazon.de>
> > > ---
> > >
> > > Notes:
> > > v4:
> > > - new patch version
> > >
> > > common/time.c | 10 ++++++++++
> > > include/xtf/time.h | 12 ++++++++++++
> > > 2 files changed, 22 insertions(+)
> > >
> > > diff --git a/common/time.c b/common/time.c
> > > index 7515eb0..e2779b9 100644
> > > --- a/common/time.c
> > > +++ b/common/time.c
> > > @@ -163,6 +163,16 @@ static inline void mspin_sleep(uint64_t t)
> > > nspin_sleep(nsec);
> > > }
> > > +void sleep(uint64_t t)
> > > +{
> > > + spin_sleep(t);
> > > +}
> > > +
> > > +void msleep(uint64_t t)
> > > +{
> > > + mspin_sleep(t);
> >
> > Why can you just call mspin_sleep msleep directly?
> >
> > The same applies to spin_sleep.
> >
> > Also I was expecting to see some kind of test appear at the end of the
> > series. You are basically adding a bunch of dead code, since there's
> > no user of any of the newly introduced functions.
>
> Actually, I won't be able to add a real XSA test using this feature. Anyway,
> I do think that having the sleep functions will be really useful in the
> future.
> Anyway, I was thinking about adding a test that is calling the gettimeofday
> function or something similar.
>
> What do you think about it ?
Yes, see my last reply to 3/7.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 1/7] introduce time managment in xtf
2018-04-16 10:48 ` Paul Semel
@ 2018-04-17 7:45 ` Roger Pau Monné
0 siblings, 0 replies; 13+ messages in thread
From: Roger Pau Monné @ 2018-04-17 7:45 UTC (permalink / raw)
To: Paul Semel; +Cc: xen-devel, andrew.cooper3, wipawel
On Mon, Apr 16, 2018 at 12:48:49PM +0200, Paul Semel wrote:
> On 04/13/2018 02:05 PM, Roger Pau Monné wrote:
> > > +static inline uint64_t scale_delta(uint64_t delta, uint32_t mul_frac, int shift)
> > > +{
> > > + uint64_t product;
> > > +#ifdef __i386__
> > > + uint32_t tmp1, tmp2;
> > > +#endif
> > > +
> > > + if ( shift < 0 )
> > > + delta >>= -shift;
> > > + else
> > > + delta <<= shift;
> > > +
> > > +#ifdef __i386__
> > > + __asm__ (
> > > + "mul %5 ; "
> > > + "mov %4,%%eax ; "
> > > + "mov %%edx,%4 ; "
> > > + "mul %5 ; "
> > > + "add %4,%%eax ; "
> > > + "xor %5,%5 ; "
> > > + "adc %5,%%edx ; "
> > > + : "=A" (product), "=r" (tmp1), "=r" (tmp2)
> > > + : "a" ((uint32_t)delta), "1" ((uint32_t)(delta >> 32)), "2" (mul_frac) );
> >
> > This line is too long.
> >
> > > +#else
> > > + __asm__ (
> > > + "mul %%rdx ; shrd $32,%%rdx,%%rax"
> > > + : "=a" (product) : "0" (delta), "d" ((uint64_t)mul_frac) );
> >
> > Not sure whether you need to add a ': "d"' clobber here, since the d
> > register is used but it's not in the list of output operands.
> >
> > > +#endif
> > > +
> > > + return product;
> > > +}
> > > +
>
> Actually, I'm not sure that I have to make that much changes to this
> function, as @Andrew wanted to use another version of it as far as I recall.
IMO if there are known issues with this function they need to be
sorted out before committing.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2018-04-17 7:45 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20180410191701.17203-1-semelpaul@gmail.com>
2018-04-13 12:05 ` [PATCH v4 1/7] introduce time managment in xtf Roger Pau Monné
2018-04-16 10:48 ` Paul Semel
2018-04-17 7:45 ` Roger Pau Monné
[not found] ` <20180410191701.17203-2-semelpaul@gmail.com>
2018-04-13 13:37 ` [PATCH v4 2/7] add current_time function to time manager Roger Pau Monné
[not found] ` <20180410191701.17203-3-semelpaul@gmail.com>
2018-04-13 13:39 ` [PATCH v4 3/7] add gettimeofday function to time managment Roger Pau Monné
2018-04-16 10:16 ` Paul Semel
2018-04-17 7:41 ` Roger Pau Monné
[not found] ` <20180410191701.17203-4-semelpaul@gmail.com>
2018-04-13 13:51 ` [PATCH v4 4/7] add nspin_sleep function to time manager Roger Pau Monné
[not found] ` <20180410191701.17203-5-semelpaul@gmail.com>
2018-04-13 13:52 ` [PATCH v4 5/7] add spin_sleep " Roger Pau Monné
[not found] ` <20180410191701.17203-6-semelpaul@gmail.com>
2018-04-13 13:53 ` [PATCH v4 6/7] add mspin_sleep " Roger Pau Monné
[not found] ` <20180410191701.17203-7-semelpaul@gmail.com>
2018-04-13 13:55 ` [PATCH v4 7/7] add sleep, msleep and NOW() macros " Roger Pau Monné
2018-04-16 10:45 ` Paul Semel
2018-04-17 7:42 ` Roger Pau Monné
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.