| Message ID | alpine.DEB.2.10.1604221043200.6744@sstabellini-ThinkPad-X260 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 22.04.16 at 12:08, <sstabellini@kernel.org> wrote: > On Fri, 22 Apr 2016, Jan Beulich wrote: >> >>> On 21.04.16 at 15:29, <sstabellini@kernel.org> wrote: >> > --- a/xen/arch/x86/time.c >> > +++ b/xen/arch/x86/time.c >> > @@ -784,7 +784,7 @@ static void __update_vcpu_system_time(struct vcpu *v, > int force) >> > struct cpu_time *t; >> > struct vcpu_time_info *u, _u = {}; >> > struct domain *d = v->domain; >> > - s_time_t tsc_stamp; >> > + s_time_t stime_stamp, tsc_stamp = 0; >> >> I don't see why the initializer needs adding here. > > Ops, sorry, I developed the patch against 4.6, the useless > initialization derives from it. > > >> > @@ -807,7 +808,11 @@ static void __update_vcpu_system_time(struct vcpu *v, > int force) >> > tsc_stamp = -gtime_to_gtsc(d, -stime); >> > } >> > else >> > + { >> > tsc_stamp = gtime_to_gtsc(d, stime); >> > + if (!tsc_stamp) >> >> Coding style. >> >> > + stime_stamp = d->arch.vtsc_offset; >> > + } >> >> While I can see this being the right thing for getting the two stamps >> in sync, is that really helping the guest? Time ought to be not moving >> forward until getting past vtsc_offset afaict, and that can't be good. > > It helps a lot in my test case: without this Linux hangs due to lost > timer interrupts (because they are set in the past). > > >> I.e. it would seem to me that it's gtime_to_gtsc() that needs >> adjustment to properly deal with time < d->arch.vtsc_offset. > > I agree that it would be nice to fix gtime_to_gtsc, but how do you > suggest to do it? See below. >> Plus I can't see why, in the worst case, the gTSC value can't be >> wrapped through zero into negative (or really huge positive) range: >> Such TSC values are certainly not invalid, and guests shouldn't really >> make assumptions on TSC values being in the small positive range >> when they boot. > > Am I understanding correctly that you are suggesting to let the > subtraction in gtime_to_gtsc return a negative -- actually a wrapped > around positive? Something like: > > diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c > index 7a01c90..896fd9f 100644 > --- a/xen/arch/x86/time.c > +++ b/xen/arch/x86/time.c > @@ -1757,8 +1757,8 @@ custom_param("tsc", tsc_parse); > u64 gtime_to_gtsc(struct domain *d, u64 time) > { > if ( !is_hvm_domain(d) ) > - time = max_t(s64, time - d->arch.vtsc_offset, 0); > - return scale_delta(time, &d->arch.ns_to_vtsc); > + time = time - d->arch.vtsc_offset; > + return scale_delta(time2, &d->arch.ns_to_vtsc); > } > > Unfortunately that wouldn't solve the problem because of the scaling. Of course. I thought more along the lines of u64 gtime_to_gtsc(struct domain *d, u64 time) { if ( !is_hvm_domain(d) ) { if ( time < d->arch.vtsc_offset ) return -scale_delta(d->arch.vtsc_offset - time, &d->arch.ns_to_vtsc); time -= d->arch.vtsc_offset; } return scale_delta(time, &d->arch.ns_to_vtsc); } Jan
On Fri, 22 Apr 2016, Jan Beulich wrote: > >>> On 22.04.16 at 12:08, <sstabellini@kernel.org> wrote: > > On Fri, 22 Apr 2016, Jan Beulich wrote: > >> >>> On 21.04.16 at 15:29, <sstabellini@kernel.org> wrote: > >> > --- a/xen/arch/x86/time.c > >> > +++ b/xen/arch/x86/time.c > >> > @@ -784,7 +784,7 @@ static void __update_vcpu_system_time(struct vcpu *v, > > int force) > >> > struct cpu_time *t; > >> > struct vcpu_time_info *u, _u = {}; > >> > struct domain *d = v->domain; > >> > - s_time_t tsc_stamp; > >> > + s_time_t stime_stamp, tsc_stamp = 0; > >> > >> I don't see why the initializer needs adding here. > > > > Ops, sorry, I developed the patch against 4.6, the useless > > initialization derives from it. > > > > > >> > @@ -807,7 +808,11 @@ static void __update_vcpu_system_time(struct vcpu *v, > > int force) > >> > tsc_stamp = -gtime_to_gtsc(d, -stime); > >> > } > >> > else > >> > + { > >> > tsc_stamp = gtime_to_gtsc(d, stime); > >> > + if (!tsc_stamp) > >> > >> Coding style. > >> > >> > + stime_stamp = d->arch.vtsc_offset; > >> > + } > >> > >> While I can see this being the right thing for getting the two stamps > >> in sync, is that really helping the guest? Time ought to be not moving > >> forward until getting past vtsc_offset afaict, and that can't be good. > > > > It helps a lot in my test case: without this Linux hangs due to lost > > timer interrupts (because they are set in the past). > > > > > >> I.e. it would seem to me that it's gtime_to_gtsc() that needs > >> adjustment to properly deal with time < d->arch.vtsc_offset. > > > > I agree that it would be nice to fix gtime_to_gtsc, but how do you > > suggest to do it? > > See below. > > >> Plus I can't see why, in the worst case, the gTSC value can't be > >> wrapped through zero into negative (or really huge positive) range: > >> Such TSC values are certainly not invalid, and guests shouldn't really > >> make assumptions on TSC values being in the small positive range > >> when they boot. > > > > Am I understanding correctly that you are suggesting to let the > > subtraction in gtime_to_gtsc return a negative -- actually a wrapped > > around positive? Something like: > > > > diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c > > index 7a01c90..896fd9f 100644 > > --- a/xen/arch/x86/time.c > > +++ b/xen/arch/x86/time.c > > @@ -1757,8 +1757,8 @@ custom_param("tsc", tsc_parse); > > u64 gtime_to_gtsc(struct domain *d, u64 time) > > { > > if ( !is_hvm_domain(d) ) > > - time = max_t(s64, time - d->arch.vtsc_offset, 0); > > - return scale_delta(time, &d->arch.ns_to_vtsc); > > + time = time - d->arch.vtsc_offset; > > + return scale_delta(time2, &d->arch.ns_to_vtsc); > > } > > > > Unfortunately that wouldn't solve the problem because of the scaling. > > Of course. I thought more along the lines of > > u64 gtime_to_gtsc(struct domain *d, u64 time) > { > if ( !is_hvm_domain(d) ) > { > if ( time < d->arch.vtsc_offset ) > return -scale_delta(d->arch.vtsc_offset - time, &d->arch.ns_to_vtsc); > time -= d->arch.vtsc_offset; > } > return scale_delta(time, &d->arch.ns_to_vtsc); > } This works, thanks! I'll resend a patch along these lines with your authorship.
diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c index 7a01c90..896fd9f 100644 --- a/xen/arch/x86/time.c +++ b/xen/arch/x86/time.c @@ -1757,8 +1757,8 @@ custom_param("tsc", tsc_parse); u64 gtime_to_gtsc(struct domain *d, u64 time) { if ( !is_hvm_domain(d) ) - time = max_t(s64, time - d->arch.vtsc_offset, 0); - return scale_delta(time, &d->arch.ns_to_vtsc); + time = time - d->arch.vtsc_offset; + return scale_delta(time2, &d->arch.ns_to_vtsc); } Unfortunately that wouldn't solve the problem because of the scaling.