introduce time managment in xtf
PATCH v3 1/7 introduce time managment in xtf
From: Paul Semel <[email protected]>
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
--- build/files.mk | 1 + common/time.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ include/xtf/time.h | 31 +++++++++++++++++++++ 3 files changed, 113 insertions(+) create mode 100644 common/time.c create mode 100644 include/xtf/time.h
build/files.mk +1 -0
@@ -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
common/time.c new file +81 -0
81 lines not on this page · raw message
include/xtf/time.h new file +31 -0
@@ -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));\ + tsc = ((uint64_t)hi << 32) | lo;\ +} + + +/* Time from boot in nanoseconds */ +uint64_t since_boot_time (void ); + +#endif /* XTF_TIME_H */ + +/* +* Local variables: +* mode: C +* c-file-style: "BSD" +* c-basic-offset: 4 +* tab-width: 4 +* indent-tabs-mode: nil +* End: +*/
Signature · 7 lines
2.16.1 _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
PATCH v3 2/7 add current_time function to time manager
From: Paul Semel <[email protected]>
this function returns the "epoch" time
--- common/time.c | 39 +++++++++++++++++++++++++++++++++++++++ include/xtf/time.h | 4 ++++ 2 files changed, 43 insertions(+)
common/time.c +39 -0
@@ -3,6 +3,7 @@#include <xtf/time.h> #include <arch/barrier.h> +#include <arch/div.h> /* 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 (); +#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 + *nsec = (uint64_t )ACCESS_ONCE (shared_info.wc_nsec); +smp_rmb (); + ver2 =ACCESS_ONCE (shared_info.wc_version); +smp_rmb (); + }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); + +#if defined(__i386__) +divmod64 (&boot_time,SEC_TO_NSEC (1 )); +#else + boot_time /=SEC_TO_NSEC (1 ); +#endif + +return sec + boot_time; +} +/* * Local variables: * mode: C
include/xtf/time.h +4 -0
@@ -14,10 +14,14 @@ tsc = ((uint64_t)hi << 32) | lo;\ } +#define SEC_TO_NSEC (x) ((x) * 1000000000ul) +/* Time from boot in nanoseconds */ uint64_t since_boot_time (void ); +uint64_t current_time (void ); +#endif /* XTF_TIME_H */ /*
Signature · 7 lines
2.16.1 _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
PATCH v3 3/7 add gettimeofday function to time managment
In reply to Paul SemelFrom: Paul Semel <[email protected]>
this function acts as the POSIX gettimeofday function
--- common/time.c | 30 ++++++++++++++++++++++++++++++ include/xtf/time.h | 7 +++++++ 2 files changed, 37 insertions(+)
common/time.c +30 -0
@@ -1,6 +1,7 @@#include <xtf/types.h> #include <xtf/traps.h> #include <xtf/time.h> +#include <xen/errno.h> #include <arch/barrier.h> #include <arch/div.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); + +#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 + + tp->sec = sec + boot_time; + tp->nsec = nsec + mod; +return 0 ; +} +/* * Local variables: * mode: C
include/xtf/time.h +7 -0
@@ -8,6 +8,11 @@#include <xtf/types.h> +struct timeval { +uint64_t sec; +uint64_t nsec; +}; +#define rdtsc (tsc) {\ uint32_t lo, hi;\ __asm__ volatile("rdtsc": "=a"(lo), "=d"(hi));\
@@ -22,6 +27,8 @@ uint64_t since_boot_time(void);uint64_t current_time (void ); +int gettimeofday (struct timeval *tp,void *restrict tzp); +#endif /* XTF_TIME_H */ /*
Signature · 7 lines
2.16.1 _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
PATCH v3 4/7 add nspin_sleep function to time manager
In reply to Paul SemelFrom: Paul Semel <[email protected]>
this function spin sleeps for t nanoseconds
--- common/time.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
common/time.c +12 -0
@@ -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 ) + asmvolatile ("pause" ); +} +/* * Local variables: * mode: C
Signature · 7 lines
2.16.1 _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
PATCH v3 5/7 add spin_sleep function to time manager
In reply to Paul SemelFrom: Paul Semel <[email protected]>
this function uses nspin_sleep to spin sleep for t seconds
--- common/time.c | 6 ++++++ 1 file changed, 6 insertions(+)
common/time.c +6 -0
@@ -151,6 +151,12 @@ static inline void nspin_sleep(uint64_t t) asmvolatile ("pause" ); } +static inline void spin_sleep (uint64_t t) +{ +uint64_t nsec =SEC_TO_NSEC (t); +nspin_sleep (nsec); +} +/* * Local variables: * mode: C
Signature · 7 lines
2.16.1 _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
PATCH v3 6/7 add mspin_sleep function to time manager
In reply to Paul SemelFrom: Paul Semel <[email protected]>
this function uses mspin_sleep to spin sleep for t milliseconds
--- common/time.c | 6 ++++++ include/xtf/time.h | 1 + 2 files changed, 7 insertions(+)
common/time.c +6 -0
@@ -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); +nspin_sleep (nsec); +} +/* * Local variables: * mode: C
include/xtf/time.h +1 -0
@@ -20,6 +20,7 @@ struct timeval { }#define SEC_TO_NSEC (x) ((x) * 1000000000ul) +#define MSEC_TO_NSEC (x) ((x) * 1000000ul)/* Time from boot in nanoseconds */
Signature · 7 lines
2.16.1 _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
PATCH v3 7/7 add sleep, msleep and NOW() macros to time manager
In reply to Paul SemelFrom: Paul Semel <[email protected]>
those are helpful macro to use the time manager correctly
--- common/time.c | 10 ++++++++++ include/xtf/time.h | 12 ++++++++++++ 2 files changed, 22 insertions(+)
common/time.c +10 -0
@@ -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); +} +/* * Local variables: * mode: C
include/xtf/time.h +12 -0
@@ -28,8 +28,20 @@ uint64_t since_boot_time(void);uint64_t current_time (void ); +/* This function takes seconds in parameter */ +void sleep (uint64_t f); + +/* Be careful, this function takes milliseconds in parameter, +* not microseconds ! +*/ +void msleep (uint64_t f); +int gettimeofday (struct timeval *tp,void *restrict tzp); + +/* This returns the current epoch time */ +#define NOW () current_time() +#endif /* XTF_TIME_H */ /*
Signature · 7 lines
2.16.1 _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
Quoted 11 lines · Paul Semel
From: Paul Semel <[email protected]>
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_timegets the time in nanoseconds from the moment the VM has bootedSigned-off-by Paul Semel <[email protected]>---
This seems to be missing a list of changes between v2 and v3. Please add such a list when posting new versions.
Quoted 11 lines
common/time.c new file +11
-0+uint64_t since_boot_time(void) +{ + uint64_t tsc; + uint32_t ver1, ver2; + uint64_t system_time; + uint64_t old_tsc; + + do + { + do + {
common/time.c line 62
Quoted 10 lines
common/time.c new file +10
-0+ 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.
Quoted 28 lines
common/time.c new file +19
-0+ +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: +*/ include/xtf/time.h new file +3
-0@@ -0,0 +1,31 @@ +/** +* @file include/xtf/time.h +*
include/xtf/time.h line 13
Quoted 10 lines
include/xtf/time.h new file +10
-0+* 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
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
Quoted 44 lines · Roger Pau Monné
Quoted 11 lines · Paul Semel
From: Paul Semel <[email protected]>
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_timegets the time in nanoseconds from the moment the VM has bootedSigned-off-by Paul Semel <[email protected]>---
This seems to be missing a list of changes between v2 and v3. Please add such a list when posting new versions.
Quoted 11 lines
common/time.c new file +11
-0+uint64_t since_boot_time(void) +{ + uint64_t tsc; + uint32_t ver1, ver2; + uint64_t system_time; + uint64_t old_tsc; + + do + { + do + {common/time.c line 62
Quoted 10 lines
common/time.c new file +10
-0+ 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_deltabelow is OK.AFAICT uou _cannot_ access any of the
vcpu_time_infofields without checking for the version (in order to avoid reading inconsistent data during an update), yet below you readtsc_to_system_mulandtsc_shift.
Quoted 5 lines
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.
There are real bugs in that implementation (wrong barriers, although they are overly strong rather than overly week), and an inefficiency, in that you don't want any extraneous calculation in between the two reads of version. The call to pvclock_get_nsec_offset(ti) wants to be outside of the critical region, to reduce the chance of intersecting an update and having repeat the loop again.
~Andrew
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
Quoted 29 lines · Roger Pau Monné
Quoted 9 lines
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_timegets the time in nanoseconds from the moment the VM has bootedSigned-off-by Paul Semel <[email protected]>---
This seems to be missing a list of changes between v2 and v3. Please add such a list when posting new versions.
Quoted 16 lines
common/time.c new file +16
-0+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);
Quoted 8 lines
common/time.c line 62
Quoted 5 lines
common/time.c new file +5
-0+ 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_deltabelow 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.
Quoted 6 lines
AFAICT uou _cannot_ access any of the
vcpu_time_infofields without checking for the version (in order to avoid reading inconsistent data during an update), yet below you readtsc_to_system_mulandtsc_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..
Quoted 41 lines
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.
Quoted 35 lines
common/time.c new file +19
-0+ +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: +*/ include/xtf/time.h new file +10
-0@@ -0,0 +1,31 @@ +/** +* @file include/xtf/time.h +* +* Time management +*/ +#ifndef XTF_TIME_H +# define XTF_TIME_H + +#include <xtf/types.h> +
Quoted 9 lines
include/xtf/time.h line 13
Quoted 3 lines
include/xtf/time.h new file +3
-0+#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).
Quoted 4 lines
https://lists.xenproject.org/archives/html/xen-devel/2018-04/msg00546.html
Roger.
Thanks,
Signature · 6 lines
Paul _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
Quoted 39 lines · Paul Semel
Quoted 29 lines · Roger Pau Monné
Quoted 9 lines
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_timegets the time in nanoseconds from the moment the VM has bootedSigned-off-by Paul Semel <[email protected]>---
This seems to be missing a list of changes between v2 and v3. Please add such a list when posting new versions.
Quoted 16 lines
common/time.c new file +16
-0+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);Quoted 8 lines
common/time.c line 62
Quoted 5 lines
common/time.c new file +5
-0+ 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_deltabelow is OK.
Quoted 3 lines
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.
You should send that version then :).
Quoted 11 lines
Quoted 6 lines
AFAICT uou _cannot_ access any of the
vcpu_time_infofields without checking for the version (in order to avoid reading inconsistent data during an update), yet below you readtsc_to_system_mulandtsc_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_versiontoo in the first version of those patches, but after chatting with Andrew, It appeared that it was not necessary..
AFAICT the following should work:
do
{
ver1 = 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();
/* NB: this barrier is probably not needed if rdtsc is serializing. */
smp_rmb();
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,
time.tsc_shift);
I'm not sure the second barrier is actually needed, since rdtsc_ordered should be serializing.
Quoted 51 lines
Quoted 41 lines
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.
Quoted 35 lines
common/time.c new file +19
-0+ +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: +*/ include/xtf/time.h new file +10
-0@@ -0,0 +1,31 @@ +/** +* @file include/xtf/time.h +* +* Time management +*/ +#ifndef XTF_TIME_H +# define XTF_TIME_H + +#include <xtf/types.h> +Quoted 9 lines
include/xtf/time.h line 13
Quoted 3 lines
include/xtf/time.h new file +3
-0+#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:
Quoted 2 lines
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).
rdtsc is not a serializing instruction, and as such there's no guarantee it's not executed before or after any of it's preceding or following instructions. In order to make it serializing you need to add a lfence or mfence, or if you are not sure about the architecture you likely need to add both, see:
https://marc.info/?l=xen-devel&m=151983511212795&w=2
Also, as said in the previous review, the rdtsc helper should be placed in a x86 specific file because it's a x86 specific instruction. IMO it should be placed in arch/x86/include/arch/lib.h with the rest of the x86 specific instructions.
Hope this clarifies the changes.
Roger.
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
Quoted 59 lines · Roger Pau Monné
Quoted 38 lines
Quoted 29 lines
Quoted 9 lines
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_timegets the time in nanoseconds from the moment the VM has bootedSigned-off-by Paul Semel <[email protected]>---
This seems to be missing a list of changes between v2 and v3. Please add such a list when posting new versions.
Quoted 16 lines
common/time.c new file +16
-0+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);Quoted 8 lines
common/time.c line 62
Quoted 5 lines
common/time.c new file +5
-0+ 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_deltabelow is OK.Quoted 3 lines
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.You should send that version then :).
Quoted 11 lines
Quoted 6 lines
AFAICT uou _cannot_ access any of the
vcpu_time_infofields without checking for the version (in order to avoid reading inconsistent data during an update), yet below you readtsc_to_system_mulandtsc_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_versiontoo in the first version of those patches, but after chatting with Andrew, It appeared that it was not necessary..AFAICT the following should work:
do
Quoted 15 lines
{
ver1 = 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();
/* NB: this barrier is probably not needed if rdtsc is serializing. */
smp_rmb();
ver2 = ACCESS_ONCE(shared_info.vcpu_info[0].time.version);
} while ( ver2 & 1 || ver1 != ver2 );Just a (probably dumb) question. Why aren't you doing ACCESS_ONCE on every shared_info field accesses ?
As far as I understand, we need to do this as most as we can to avoid having completely broken data (or security issues). Am I missing something ?
Quoted 66 lines
system_time+= scale_delta(tsc -tsc_timestamp,tsc_to_system_mul,time.tsc_shift);I'm not sure the second barrier is actually needed, since
rdtsc_orderedshould be serializing.Quoted 51 lines
Quoted 41 lines
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.
Quoted 35 lines
common/time.c new file +19
-0+ +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: +*/ include/xtf/time.h new file +10
-0@@ -0,0 +1,31 @@ +/** +* @file include/xtf/time.h +* +* Time management +*/ +#ifndef XTF_TIME_H +# define XTF_TIME_H + +#include <xtf/types.h> +Quoted 9 lines
include/xtf/time.h line 13
Quoted 3 lines
include/xtf/time.h new file +3
-0+#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:
Quoted 2 lines
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).
rdtsc is not a serializing instruction, and as such there's no guarantee it's not executed before or after any of it's preceding or following instructions. In order to make it serializing you need to add a lfence or mfence, or if you are not sure about the architecture you likely need to add both, see:
Quoted 7 lines
https://marc.info/?l=xen-devel&m=151983511212795&w=2
Also, as said in the previous review, the rdtsc helper should be placed in a x86 specific file because it's a x86 specific instruction. IMO it should be placed in
arch/x86/include/arch/lib.hwith the rest of the x86 specific instructions.
Thanks for clarifying this point ! I will put the function in the correct place, and add an implementation of rdtsc_ordered with serialization 🙂
Quoted 1 line
Hope this clarifies the changes.
This definitely does, thanks !
Signature · 6 lines
Paul _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
Quoted 72 lines · Paul Semel
Quoted 71 lines · Roger Pau Monné
Quoted 38 lines
Quoted 29 lines
Quoted 9 lines
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_timegets the time in nanoseconds from the moment the VM has bootedSigned-off-by Paul Semel <[email protected]>---
This seems to be missing a list of changes between v2 and v3. Please add such a list when posting new versions.
Quoted 16 lines
common/time.c new file +16
-0+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);Quoted 8 lines
common/time.c line 62
Quoted 5 lines
common/time.c new file +5
-0+ 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_deltabelow is OK.Quoted 3 lines
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.You should send that version then :).
Quoted 11 lines
Quoted 6 lines
AFAICT uou _cannot_ access any of the
vcpu_time_infofields without checking for the version (in order to avoid reading inconsistent data during an update), yet below you readtsc_to_system_mulandtsc_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_versiontoo in the first version of those patches, but after chatting with Andrew, It appeared that it was not necessary..AFAICT the following should work:
do
{ ver1 = 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(); /* NB: this barrier is probably not needed if rdtsc is serializing. */ smp_rmb();
Quoted 8 lines
Quoted 3 lines
ver2 = ACCESS_ONCE(shared_info.vcpu_info[0].time.version); } while ( ver2 & 1 || ver1 != ver2 );Just a (probably dumb) question. Why aren't you doing
ACCESS_ONCEon everyshared_infofield accesses ?
As far as I understand, we need to do this as most as we can to avoid having completely broken data (or security issues). Am I missing something ?
ACCESS_ONCE prevents the reordering of the reads, but here AFAICT we don't really care about the order in which way they are performed as long as they are all done before the read barrier (smp_rmb).
Note that I used ACCESS_ONCE for the last access to the version field. I've done that to prevent the compiler from optimizing the code as:
} while ( shared_info.vcpu_info[0].time.version & 1 ||
ver1 != shared_info.vcpu_info[0].time.version );
Which would be incorrect, since we want to use the same version data for both checks in the while loop condition.
Roger.
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
Quoted 84 lines · Roger Pau Monné
Quoted 71 lines
Quoted 71 lines
Quoted 38 lines
Quoted 29 lines
Quoted 9 lines
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_timegets the time in nanoseconds from the moment the VM has bootedSigned-off-by Paul Semel <[email protected]>---
This seems to be missing a list of changes between v2 and v3. Please add such a list when posting new versions.
Quoted 16 lines
common/time.c new file +16
-0+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);Quoted 8 lines
common/time.c line 62
Quoted 5 lines
common/time.c new file +5
-0+ 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_deltabelow is OK.Quoted 3 lines
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.You should send that version then :).
Quoted 11 lines
Quoted 6 lines
AFAICT uou _cannot_ access any of the
vcpu_time_infofields without checking for the version (in order to avoid reading inconsistent data during an update), yet below you readtsc_to_system_mulandtsc_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_versiontoo in the first version of those patches, but after chatting with Andrew, It appeared that it was not necessary..AFAICT the following should work:
do
{ ver1 = 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(); /* NB: this barrier is probably not needed if rdtsc is serializing. */ smp_rmb();Quoted 8 lines
Quoted 3 lines
ver2 = ACCESS_ONCE(shared_info.vcpu_info[0].time.version); } while ( ver2 & 1 || ver1 != ver2 );Just a (probably dumb) question. Why aren't you doing
ACCESS_ONCEon everyshared_infofield accesses ?
As far as I understand, we need to do this as most as we can to avoid having completely broken data (or security issues). Am I missing something ?
ACCESS_ONCEprevents the reordering of the reads, but here AFAICT we don't really care about the order in which way they are performed as long as they are all done before the read barrier (smp_rmb).
Quoted 9 lines
Note that I used
ACCESS_ONCEfor the last access to the version field. I've done that to prevent the compiler from optimizing the code as:} while ( shared_info.vcpu_info[0].time.version & 1 ||
ver1 != shared_info.vcpu_info[0].time.version );Which would be incorrect, since we want to use the same version data for both checks in the while loop condition.
Okay, I really thought that it was also used to ensure that the accesses are not splitted into multiple instructions (for optimizations because of the loop), and thus put us in trouble if the shared memory was modified in between.
Thank you very much !
Signature · 6 lines
Paul _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
Quoted 91 lines · Paul Semel
Quoted 90 lines · Roger Pau Monné
Quoted 71 lines
Quoted 71 lines
Quoted 38 lines
Quoted 29 lines
Quoted 9 lines
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_timegets the time in nanoseconds from the moment the VM has bootedSigned-off-by Paul Semel <[email protected]>---
This seems to be missing a list of changes between v2 and v3. Please add such a list when posting new versions.
Quoted 16 lines
(quoted diff) +16
-0+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);Quoted 8 lines
Review
Quoted 5 lines
(quoted diff) +5
-0+ 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_deltabelow is OK.Quoted 3 lines
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.You should send that version then :).
Quoted 11 lines
Quoted 6 lines
AFAICT uou _cannot_ access any of the
vcpu_time_infofields without checking for the version (in order to avoid reading inconsistent data during an update), yet below you readtsc_to_system_mulandtsc_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_versiontoo in the first version of those patches, but after chatting with Andrew, It appeared that it was not necessary..AFAICT the following should work:
do
{ ver1 = 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(); /* NB: this barrier is probably not needed if rdtsc is serializing. */ smp_rmb();Quoted 8 lines
Quoted 3 lines
ver2 = ACCESS_ONCE(shared_info.vcpu_info[0].time.version); } while ( ver2 & 1 || ver1 != ver2 );Just a (probably dumb) question. Why aren't you doing
ACCESS_ONCEon everyshared_infofield accesses ?
As far as I understand, we need to do this as most as we can to avoid having completely broken data (or security issues). Am I missing something ?
ACCESS_ONCEprevents the reordering of the reads, but here AFAICT we don't really care about the order in which way they are performed as long as they are all done before the read barrier (smp_rmb).Note that I used
ACCESS_ONCEfor the last access to the version field. I've done that to prevent the compiler from optimizing the code as:} while ( shared_info.vcpu_info[0].time.version & 1 ||
ver1 != shared_info.vcpu_info[0].time.version );
Quoted 8 lines
Quoted 3 lines
Which would be incorrect, since we want to use the same version data for both checks in the while loop condition.
Okay, I really thought that it was also used to ensure that the accesses are not splitted into multiple instructions (for optimizations because of the loop), and thus put us in trouble if the shared memory was modified in between.
If the memory is modified in between either (ver2 & 1) == 1 or ver1 != ver2 because that's the protocol between the hypervisor and the guest in order to update vpcu_time_info, so we will discard the read data and start the loop again.
Roger.
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
Quoted 99 lines · Roger Pau Monné
Quoted 90 lines
Quoted 90 lines
Quoted 71 lines
Quoted 71 lines
Quoted 38 lines
Quoted 29 lines
Quoted 9 lines
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_timegets the time in nanoseconds from the moment the VM has bootedSigned-off-by Paul Semel <[email protected]>---
This seems to be missing a list of changes between v2 and v3. Please add such a list when posting new versions.
Quoted 16 lines
(quoted diff) +16
-0+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);Quoted 8 lines
Review
Quoted 5 lines
(quoted diff) +5
-0+ 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_deltabelow is OK.Quoted 3 lines
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.You should send that version then :).
Quoted 11 lines
Quoted 6 lines
AFAICT uou _cannot_ access any of the
vcpu_time_infofields without checking for the version (in order to avoid reading inconsistent data during an update), yet below you readtsc_to_system_mulandtsc_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_versiontoo in the first version of those patches, but after chatting with Andrew, It appeared that it was not necessary..AFAICT the following should work:
do
{ ver1 = 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(); /* NB: this barrier is probably not needed if rdtsc is serializing. */ smp_rmb();Quoted 8 lines
Quoted 3 lines
ver2 = ACCESS_ONCE(shared_info.vcpu_info[0].time.version); } while ( ver2 & 1 || ver1 != ver2 );Just a (probably dumb) question. Why aren't you doing
ACCESS_ONCEon everyshared_infofield accesses ?
As far as I understand, we need to do this as most as we can to avoid having completely broken data (or security issues). Am I missing something ?
ACCESS_ONCEprevents the reordering of the reads, but here AFAICT we don't really care about the order in which way they are performed as long as they are all done before the read barrier (smp_rmb).Note that I used
ACCESS_ONCEfor the last access to the version field. I've done that to prevent the compiler from optimizing the code as:} while ( shared_info.vcpu_info[0].time.version & 1 ||
ver1 != shared_info.vcpu_info[0].time.version );Quoted 8 lines
Quoted 3 lines
Which would be incorrect, since we want to use the same version data for both checks in the while loop condition.
Okay, I really thought that it was also used to ensure that the accesses are not splitted into multiple instructions (for optimizations because of the loop), and thus put us in trouble if the shared memory was modified in between.
Quoted 4 lines
If the memory is modified in between either (ver2 & 1) == 1 or ver1 != ver2 because that's the protocol between the hypervisor and the guest in order to update
vpcu_time_info, so we will discard the read data and start the loop again.
Oh okay, I get it now, sorry for missing this !
Thanks,
Signature · 6 lines
Paul _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel