From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH v2] x86: reduce redundancy in tsc_[gs]et_info() Date: Tue, 6 May 2014 14:49:08 +0100 Message-ID: <5368E854.2050303@citrix.com> References: <5368FFD0020000780000F64D@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============2919881530705600240==" Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WhfkJ-0003pl-IZ for xen-devel@lists.xenproject.org; Tue, 06 May 2014 13:49:23 +0000 In-Reply-To: <5368FFD0020000780000F64D@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: xen-devel , Boris Ostrovsky , Keir Fraser List-Id: xen-devel@lists.xenproject.org --===============2919881530705600240== Content-Type: multipart/alternative; boundary="------------020908090601060203010405" --------------020908090601060203010405 Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit On 06/05/14 14:29, Jan Beulich wrote: > - some of the case statements are effectively or mostly special cases > of others, so there's no good reason not to share the code > - in the "get" function, a variable can be made case-wide instead of > having multiple instance of it (and those even with a pointless > initializer) > - minor formatting adjustments > > Signed-off-by: Jan Beulich > Reviewed-by: Boris Ostrovsky With one query below, Reviewed-by: Andrew Cooper > --- > v2: A few more formatting adjustments. Re-base on top of 82713ec8 > ("x86: use native RDTSC(P) execution when guest and host > frequencies are the same"), replacing that commit's outer || > expression with a ?: one, thus better matching the comment (no > functional change as incarnation == 0 implies > d->arch.tsc_khz == cpu_khz). > > --- a/xen/arch/x86/time.c > +++ b/xen/arch/x86/time.c > @@ -1794,39 +1794,34 @@ void tsc_get_info(struct domain *d, uint > > switch ( *tsc_mode ) > { > + uint64_t tsc; > + > case TSC_MODE_NEVER_EMULATE: > - *elapsed_nsec = *gtsc_khz = 0; > + *elapsed_nsec = *gtsc_khz = 0; > break; > - case TSC_MODE_ALWAYS_EMULATE: > - *elapsed_nsec = get_s_time() - d->arch.vtsc_offset; > - *gtsc_khz = d->arch.tsc_khz; > - break; > case TSC_MODE_DEFAULT: > if ( d->arch.vtsc ) > { > + case TSC_MODE_ALWAYS_EMULATE: > *elapsed_nsec = get_s_time() - d->arch.vtsc_offset; > - *gtsc_khz = d->arch.tsc_khz; > - } > - else > - { > - uint64_t tsc = 0; > - rdtscll(tsc); > - *elapsed_nsec = scale_delta(tsc,&d->arch.vtsc_to_ns); > - *gtsc_khz = cpu_khz; > + *gtsc_khz = d->arch.tsc_khz; > + break; > } > + rdtscll(tsc); > + *elapsed_nsec = scale_delta(tsc, &d->arch.vtsc_to_ns); > + *gtsc_khz = cpu_khz; > break; > case TSC_MODE_PVRDTSCP: > if ( d->arch.vtsc ) > { > *elapsed_nsec = get_s_time() - d->arch.vtsc_offset; > - *gtsc_khz = cpu_khz; > + *gtsc_khz = cpu_khz; > } > else > { > - uint64_t tsc = 0; > rdtscll(tsc); > - *elapsed_nsec = (scale_delta(tsc,&d->arch.vtsc_to_ns) - > - d->arch.vtsc_offset); > + *elapsed_nsec = scale_delta(tsc, &d->arch.vtsc_to_ns) - > + d->arch.vtsc_offset; > *gtsc_khz = 0; /* ignored by tsc_set_info */ > } > break; > @@ -1883,38 +1878,32 @@ void tsc_set_info(struct domain *d, > > switch ( d->arch.tsc_mode = tsc_mode ) > { > - case TSC_MODE_NEVER_EMULATE: > - d->arch.vtsc = 0; > - break; > - case TSC_MODE_ALWAYS_EMULATE: > - d->arch.vtsc = 1; > - d->arch.vtsc_offset = get_s_time() - elapsed_nsec; > - d->arch.tsc_khz = gtsc_khz ? gtsc_khz : cpu_khz; > - set_time_scale(&d->arch.vtsc_to_ns, d->arch.tsc_khz * 1000 ); > - d->arch.ns_to_vtsc = scale_reciprocal(d->arch.vtsc_to_ns); > - break; > case TSC_MODE_DEFAULT: > - d->arch.vtsc = 1; > + case TSC_MODE_ALWAYS_EMULATE: > d->arch.vtsc_offset = get_s_time() - elapsed_nsec; > - d->arch.tsc_khz = gtsc_khz ? gtsc_khz : cpu_khz; > - set_time_scale(&d->arch.vtsc_to_ns, d->arch.tsc_khz * 1000 ); > + d->arch.tsc_khz = gtsc_khz ?: cpu_khz; > + set_time_scale(&d->arch.vtsc_to_ns, d->arch.tsc_khz * 1000); > /* > - * Use native TSC if the host has safe TSC and: > + * 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 ( host_tsc_is_safe() && > - ((has_hvm_container_domain(d) && > - (d->arch.tsc_khz == cpu_khz || cpu_has_tsc_ratio)) || > + if ( tsc_mode == TSC_MODE_DEFAULT && host_tsc_is_safe() && > + (has_hvm_container_domain(d) ? > + d->arch.tsc_khz == cpu_khz || cpu_has_tsc_ratio : > incarnation == 0) ) > + { > + case TSC_MODE_NEVER_EMULATE: > d->arch.vtsc = 0; > - else > - d->arch.ns_to_vtsc = scale_reciprocal(d->arch.vtsc_to_ns); > + break; > + } > + d->arch.vtsc = 1; > + d->arch.ns_to_vtsc = scale_reciprocal(d->arch.vtsc_to_ns); > break; > case TSC_MODE_PVRDTSCP: > - d->arch.vtsc = boot_cpu_has(X86_FEATURE_RDTSCP) && > - host_tsc_is_safe() ? 0 : 1; > + d->arch.vtsc = boot_cpu_has(X86_FEATURE_RDTSCP) && > + host_tsc_is_safe() ? 0 : 1; Can this be reduced to boot_cpu_has(X86_FEATURE_RDTSCP) && !host_tsc_is_safe() ? I believe this is the correct way around with the precedence between && and ?!, but it is far from clear. Alternatively, could some brackets be introduced for clarity? ~Andrew > d->arch.tsc_khz = cpu_khz; > set_time_scale(&d->arch.vtsc_to_ns, d->arch.tsc_khz * 1000 ); > d->arch.ns_to_vtsc = scale_reciprocal(d->arch.vtsc_to_ns); > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel --------------020908090601060203010405 Content-Type: text/html; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit
On 06/05/14 14:29, Jan Beulich wrote:
- some of the case statements are effectively or mostly special cases
  of others, so there's no good reason not to share the code
- in the "get" function, a variable can be made case-wide instead of
  having multiple instance of it (and those even with a pointless
  initializer)
- minor formatting adjustments

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

With one query below,

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

---
v2: A few more formatting adjustments. Re-base on top of 82713ec8
    ("x86: use native RDTSC(P) execution when guest and host
    frequencies are the same"), replacing that commit's outer ||
    expression with a ?: one, thus better matching the comment (no
    functional change as incarnation == 0 implies
    d->arch.tsc_khz == cpu_khz).

--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1794,39 +1794,34 @@ void tsc_get_info(struct domain *d, uint
 
     switch ( *tsc_mode )
     {
+        uint64_t tsc;
+
     case TSC_MODE_NEVER_EMULATE:
-        *elapsed_nsec =  *gtsc_khz = 0;
+        *elapsed_nsec = *gtsc_khz = 0;
         break;
-    case TSC_MODE_ALWAYS_EMULATE:
-        *elapsed_nsec = get_s_time() - d->arch.vtsc_offset;
-        *gtsc_khz =  d->arch.tsc_khz;
-         break;
     case TSC_MODE_DEFAULT:
         if ( d->arch.vtsc )
         {
+    case TSC_MODE_ALWAYS_EMULATE:
             *elapsed_nsec = get_s_time() - d->arch.vtsc_offset;
-            *gtsc_khz =  d->arch.tsc_khz;
-        }
-        else
-        {
-            uint64_t tsc = 0;
-            rdtscll(tsc);
-            *elapsed_nsec = scale_delta(tsc,&d->arch.vtsc_to_ns);
-            *gtsc_khz =  cpu_khz;
+            *gtsc_khz = d->arch.tsc_khz;
+            break;
         }
+        rdtscll(tsc);
+        *elapsed_nsec = scale_delta(tsc, &d->arch.vtsc_to_ns);
+        *gtsc_khz = cpu_khz;
         break;
     case TSC_MODE_PVRDTSCP:
         if ( d->arch.vtsc )
         {
             *elapsed_nsec = get_s_time() - d->arch.vtsc_offset;
-            *gtsc_khz =  cpu_khz;
+            *gtsc_khz = cpu_khz;
         }
         else
         {
-            uint64_t tsc = 0;
             rdtscll(tsc);
-            *elapsed_nsec = (scale_delta(tsc,&d->arch.vtsc_to_ns) -
-                             d->arch.vtsc_offset);
+            *elapsed_nsec = scale_delta(tsc, &d->arch.vtsc_to_ns) -
+                            d->arch.vtsc_offset;
             *gtsc_khz = 0; /* ignored by tsc_set_info */
         }
         break;
@@ -1883,38 +1878,32 @@ void tsc_set_info(struct domain *d,
 
     switch ( d->arch.tsc_mode = tsc_mode )
     {
-    case TSC_MODE_NEVER_EMULATE:
-        d->arch.vtsc = 0;
-        break;
-    case TSC_MODE_ALWAYS_EMULATE:
-        d->arch.vtsc = 1;
-        d->arch.vtsc_offset = get_s_time() - elapsed_nsec;
-        d->arch.tsc_khz = gtsc_khz ? gtsc_khz : cpu_khz;
-        set_time_scale(&d->arch.vtsc_to_ns, d->arch.tsc_khz * 1000 );
-        d->arch.ns_to_vtsc = scale_reciprocal(d->arch.vtsc_to_ns);
-        break;
     case TSC_MODE_DEFAULT:
-        d->arch.vtsc = 1;
+    case TSC_MODE_ALWAYS_EMULATE:
         d->arch.vtsc_offset = get_s_time() - elapsed_nsec;
-        d->arch.tsc_khz = gtsc_khz ? gtsc_khz : cpu_khz;
-        set_time_scale(&d->arch.vtsc_to_ns, d->arch.tsc_khz * 1000 );
+        d->arch.tsc_khz = gtsc_khz ?: cpu_khz;
+        set_time_scale(&d->arch.vtsc_to_ns, d->arch.tsc_khz * 1000);
         /*
-         * Use native TSC if the host has safe TSC and:
+         * 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 ( host_tsc_is_safe() &&
-             ((has_hvm_container_domain(d) &&
-               (d->arch.tsc_khz == cpu_khz || cpu_has_tsc_ratio)) ||
+        if ( tsc_mode == TSC_MODE_DEFAULT && host_tsc_is_safe() &&
+             (has_hvm_container_domain(d) ?
+              d->arch.tsc_khz == cpu_khz || cpu_has_tsc_ratio :
               incarnation == 0) )
+        {
+    case TSC_MODE_NEVER_EMULATE:
             d->arch.vtsc = 0;
-        else 
-            d->arch.ns_to_vtsc = scale_reciprocal(d->arch.vtsc_to_ns);
+            break;
+        }
+        d->arch.vtsc = 1;
+        d->arch.ns_to_vtsc = scale_reciprocal(d->arch.vtsc_to_ns);
         break;
     case TSC_MODE_PVRDTSCP:
-        d->arch.vtsc =  boot_cpu_has(X86_FEATURE_RDTSCP) &&
-                        host_tsc_is_safe() ?  0 : 1;
+        d->arch.vtsc = boot_cpu_has(X86_FEATURE_RDTSCP) &&
+                       host_tsc_is_safe() ?  0 : 1;

Can this be reduced to boot_cpu_has(X86_FEATURE_RDTSCP) && !host_tsc_is_safe() ?

I believe this is the correct way around with the precedence between && and ?!, but it is far from clear.  Alternatively, could some brackets be introduced for clarity?

~Andrew

         d->arch.tsc_khz = cpu_khz;
         set_time_scale(&d->arch.vtsc_to_ns, d->arch.tsc_khz * 1000 );
         d->arch.ns_to_vtsc = scale_reciprocal(d->arch.vtsc_to_ns);





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

--------------020908090601060203010405-- --===============2919881530705600240== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============2919881530705600240==--