Re: [PATCH v2] x86: reduce redundancy in tsc_[gs]et_info()

[Andrew Cooper <andrew.cooper3@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5614 bytes --]

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


[-- Attachment #1.2: Type: text/html, Size: 6835 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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