[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/09/2018 04:35 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@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.

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..

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).

https://lists.xenproject.org/archives/html/xen-devel/2018-04/msg00546.html

Roger.


Thanks,

--
Paul

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.