| Message ID | 1489607321-6295-1-git-send-email-boris.ostrovsky@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 15.03.17 at 20:48, <boris.ostrovsky@oracle.com> wrote: > --- a/xen/arch/x86/time.c > +++ b/xen/arch/x86/time.c > @@ -2051,17 +2051,11 @@ void tsc_set_info(struct domain *d, > d->arch.vtsc_offset = get_s_time() - elapsed_nsec; > d->arch.tsc_khz = gtsc_khz ?: cpu_khz; > set_time_scale(&d->arch.vtsc_to_ns, d->arch.tsc_khz * 1000); > - /* > - * In default mode use native TSC if the host has safe TSC and: > - * HVM/PVH: host and guest frequencies are the same (either > - * "naturally" or via TSC scaling) > - * PV: guest has not migrated yet (and thus arch.tsc_khz == cpu_khz) > - */ > + > if ( tsc_mode == TSC_MODE_DEFAULT && host_tsc_is_safe() && > - (has_hvm_container_domain(d) ? > - (d->arch.tsc_khz == cpu_khz || > - hvm_get_tsc_scaling_ratio(d->arch.tsc_khz)) : > - incarnation == 0) ) > + (d->arch.tsc_khz == cpu_khz || incarnation == 0 || Is the incarnation comparison really needed here, i.e. doesn't it being zero imply the two frequencies to match in default mode? Jan
On 03/16/2017 06:40 AM, Jan Beulich wrote: >>>> On 15.03.17 at 20:48, <boris.ostrovsky@oracle.com> wrote: >> --- a/xen/arch/x86/time.c >> +++ b/xen/arch/x86/time.c >> @@ -2051,17 +2051,11 @@ void tsc_set_info(struct domain *d, >> d->arch.vtsc_offset = get_s_time() - elapsed_nsec; >> d->arch.tsc_khz = gtsc_khz ?: cpu_khz; >> set_time_scale(&d->arch.vtsc_to_ns, d->arch.tsc_khz * 1000); >> - /* >> - * In default mode use native TSC if the host has safe TSC and: >> - * HVM/PVH: host and guest frequencies are the same (either >> - * "naturally" or via TSC scaling) >> - * PV: guest has not migrated yet (and thus arch.tsc_khz == cpu_khz) >> - */ >> + >> if ( tsc_mode == TSC_MODE_DEFAULT && host_tsc_is_safe() && >> - (has_hvm_container_domain(d) ? >> - (d->arch.tsc_khz == cpu_khz || >> - hvm_get_tsc_scaling_ratio(d->arch.tsc_khz)) : >> - incarnation == 0) ) >> + (d->arch.tsc_khz == cpu_khz || incarnation == 0 || > Is the incarnation comparison really needed here, i.e. doesn't it > being zero imply the two frequencies to match in default mode? It is not necessary but I wanted to keep it for clarity so that it is explicit that when a domain is born we don't use vtsc. -boris
>>> On 16.03.17 at 13:44, <boris.ostrovsky@oracle.com> wrote: > On 03/16/2017 06:40 AM, Jan Beulich wrote: >>>>> On 15.03.17 at 20:48, <boris.ostrovsky@oracle.com> wrote: >>> --- a/xen/arch/x86/time.c >>> +++ b/xen/arch/x86/time.c >>> @@ -2051,17 +2051,11 @@ void tsc_set_info(struct domain *d, >>> d->arch.vtsc_offset = get_s_time() - elapsed_nsec; >>> d->arch.tsc_khz = gtsc_khz ?: cpu_khz; >>> set_time_scale(&d->arch.vtsc_to_ns, d->arch.tsc_khz * 1000); >>> - /* >>> - * In default mode use native TSC if the host has safe TSC and: >>> - * HVM/PVH: host and guest frequencies are the same (either >>> - * "naturally" or via TSC scaling) >>> - * PV: guest has not migrated yet (and thus arch.tsc_khz == cpu_khz) >>> - */ >>> + >>> if ( tsc_mode == TSC_MODE_DEFAULT && host_tsc_is_safe() && >>> - (has_hvm_container_domain(d) ? >>> - (d->arch.tsc_khz == cpu_khz || >>> - hvm_get_tsc_scaling_ratio(d->arch.tsc_khz)) : >>> - incarnation == 0) ) >>> + (d->arch.tsc_khz == cpu_khz || incarnation == 0 || >> Is the incarnation comparison really needed here, i.e. doesn't it >> being zero imply the two frequencies to match in default mode? > > It is not necessary but I wanted to keep it for clarity so that it is > explicit that when a domain is born we don't use vtsc. Well, considering the history here, I think its presence is rather going to raise questions than to answer any, so if anything I'd suggest to have an ASSERT() to that effect, at once serving as sort of documentation. But I may be the only one thinking this way ... Jan
On 03/16/2017 09:31 AM, Jan Beulich wrote: >>>> On 16.03.17 at 13:44, <boris.ostrovsky@oracle.com> wrote: >> On 03/16/2017 06:40 AM, Jan Beulich wrote: >>>>>> On 15.03.17 at 20:48, <boris.ostrovsky@oracle.com> wrote: >>>> --- a/xen/arch/x86/time.c >>>> +++ b/xen/arch/x86/time.c >>>> @@ -2051,17 +2051,11 @@ void tsc_set_info(struct domain *d, >>>> d->arch.vtsc_offset = get_s_time() - elapsed_nsec; >>>> d->arch.tsc_khz = gtsc_khz ?: cpu_khz; >>>> set_time_scale(&d->arch.vtsc_to_ns, d->arch.tsc_khz * 1000); >>>> - /* >>>> - * In default mode use native TSC if the host has safe TSC and: >>>> - * HVM/PVH: host and guest frequencies are the same (either >>>> - * "naturally" or via TSC scaling) >>>> - * PV: guest has not migrated yet (and thus arch.tsc_khz == cpu_khz) >>>> - */ >>>> + >>>> if ( tsc_mode == TSC_MODE_DEFAULT && host_tsc_is_safe() && >>>> - (has_hvm_container_domain(d) ? >>>> - (d->arch.tsc_khz == cpu_khz || >>>> - hvm_get_tsc_scaling_ratio(d->arch.tsc_khz)) : >>>> - incarnation == 0) ) >>>> + (d->arch.tsc_khz == cpu_khz || incarnation == 0 || >>> Is the incarnation comparison really needed here, i.e. doesn't it >>> being zero imply the two frequencies to match in default mode? >> It is not necessary but I wanted to keep it for clarity so that it is >> explicit that when a domain is born we don't use vtsc. > Well, considering the history here, I think its presence is rather > going to raise questions than to answer any, so if anything I'd > suggest to have an ASSERT() to that effect, at once serving as > sort of documentation. But I may be the only one thinking this > way ... Haven't thought about an ASSERT. I can do that. Something like ASSERT(gtsc_khz == 0 ? incarnation == 0 : true); -boris
>>> On 16.03.17 at 15:22, <boris.ostrovsky@oracle.com> wrote: > On 03/16/2017 09:31 AM, Jan Beulich wrote: >>>>> On 16.03.17 at 13:44, <boris.ostrovsky@oracle.com> wrote: >>> On 03/16/2017 06:40 AM, Jan Beulich wrote: >>>>>>> On 15.03.17 at 20:48, <boris.ostrovsky@oracle.com> wrote: >>>>> --- a/xen/arch/x86/time.c >>>>> +++ b/xen/arch/x86/time.c >>>>> @@ -2051,17 +2051,11 @@ void tsc_set_info(struct domain *d, >>>>> d->arch.vtsc_offset = get_s_time() - elapsed_nsec; >>>>> d->arch.tsc_khz = gtsc_khz ?: cpu_khz; >>>>> set_time_scale(&d->arch.vtsc_to_ns, d->arch.tsc_khz * 1000); >>>>> - /* >>>>> - * In default mode use native TSC if the host has safe TSC and: >>>>> - * HVM/PVH: host and guest frequencies are the same (either >>>>> - * "naturally" or via TSC scaling) >>>>> - * PV: guest has not migrated yet (and thus arch.tsc_khz == > cpu_khz) >>>>> - */ >>>>> + >>>>> if ( tsc_mode == TSC_MODE_DEFAULT && host_tsc_is_safe() && >>>>> - (has_hvm_container_domain(d) ? >>>>> - (d->arch.tsc_khz == cpu_khz || >>>>> - hvm_get_tsc_scaling_ratio(d->arch.tsc_khz)) : >>>>> - incarnation == 0) ) >>>>> + (d->arch.tsc_khz == cpu_khz || incarnation == 0 || >>>> Is the incarnation comparison really needed here, i.e. doesn't it >>>> being zero imply the two frequencies to match in default mode? >>> It is not necessary but I wanted to keep it for clarity so that it is >>> explicit that when a domain is born we don't use vtsc. >> Well, considering the history here, I think its presence is rather >> going to raise questions than to answer any, so if anything I'd >> suggest to have an ASSERT() to that effect, at once serving as >> sort of documentation. But I may be the only one thinking this >> way ... > > > Haven't thought about an ASSERT. I can do that. Something like > > ASSERT(gtsc_khz == 0 ? incarnation == 0 : true); ASSERT(incarnation || d->arch.tsc_khz == cpu_khz); Jan
diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c index faa638b..1a13f2f 100644 --- a/xen/arch/x86/time.c +++ b/xen/arch/x86/time.c @@ -2051,17 +2051,11 @@ void tsc_set_info(struct domain *d, d->arch.vtsc_offset = get_s_time() - elapsed_nsec; d->arch.tsc_khz = gtsc_khz ?: cpu_khz; set_time_scale(&d->arch.vtsc_to_ns, d->arch.tsc_khz * 1000); - /* - * In default mode use native TSC if the host has safe TSC and: - * HVM/PVH: host and guest frequencies are the same (either - * "naturally" or via TSC scaling) - * PV: guest has not migrated yet (and thus arch.tsc_khz == cpu_khz) - */ + if ( tsc_mode == TSC_MODE_DEFAULT && host_tsc_is_safe() && - (has_hvm_container_domain(d) ? - (d->arch.tsc_khz == cpu_khz || - hvm_get_tsc_scaling_ratio(d->arch.tsc_khz)) : - incarnation == 0) ) + (d->arch.tsc_khz == cpu_khz || incarnation == 0 || + (has_hvm_container_domain(d) && + hvm_get_tsc_scaling_ratio(d->arch.tsc_khz))) ) { case TSC_MODE_NEVER_EMULATE: d->arch.vtsc = 0;
Commit 82713ec8d2 ("x86: use native RDTSC(P) execution when guest and host frequencies are the same") left out optimization for PV guests when host and guest run at the same frequency. For such a case we should be able not to use virtual TSC regardless of whether we are runing before or after a migration (i.e. regardless of incarnation value). Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> Suggested-by: Jan Beulich <JBeulich@suse.com> --- xen/arch/x86/time.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)