[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v3 1/7] introduce time managment in xtf
On 04/10/2018 10:08 AM, 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@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.
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 :).
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'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..
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 );
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 ?
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.
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:
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.
Thanks for clarifying this point ! I will put the function in the
correct place, and add an implementation of rdtsc_ordered with
serialization 🙂
Hope this clarifies the changes.
This definitely does, thanks !
--
Paul
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
