RE: [Xen-devel] [PATCH] 1/3 Refactor Xen support for Intel Turbo boost

["Yu, Ke" <ke.yu@xxxxxxxxx>]

This patch makes sense to me. Just several comments.

> -----Original Message-----
> From: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx [mailto:xen-devel-
> bounces@xxxxxxxxxxxxxxxxxxx] On Behalf Of Mark Langsdorf
> Sent: Tuesday, March 30, 2010 2:28 AM
> To: xen-devel@xxxxxxxxxxxxxxxxxxx; guanqun.lu@xxxxxxxxx
> Subject: [Xen-devel] [PATCH] 1/3 Refactor Xen support for Intel Turbo boost
> 
> # HG changeset patch
> # User mark.langsdorf@xxxxxxx
> # Date 1269540083 18000
> # Node ID 6d42054cb1947841ab34f04c1172971068487922
> # Parent  18f4db5f72d756dda30e28d5872d91d35fdf9f0c
> Refactor the existing code that supports the Intel Turbo feature to
> move all the driver specific bits in the cpufreq driver.  Create
> a tri-state interface for the Turbo feature that can distinguish
> amongst enabled Turbo, disabled Turbo, and processors that don't
> support Turbo at all.
> 
> Changing the code this way is necessary because the similar AMD
> feature Boost has a similar external interface but a very
> different internal implementation.
> 
> Signed-off-by: Mark Langsdorf <mark.langsdorf@xxxxxxx>
> 
> diff -r 18f4db5f72d7 -r 6d42054cb194 tools/misc/xenpm.c
> --- a/tools/misc/xenpm.c      Thu Mar 25 10:01:05 2010 +0000
> +++ b/tools/misc/xenpm.c      Thu Mar 25 13:01:23 2010 -0500
> @@ -529,8 +529,13 @@
>                 p_cpufreq->u.ondemand.sampling_rate);
>          printf("    up_threshold     : %u\n",
>                 p_cpufreq->u.ondemand.up_threshold);
> -        printf("    turbo mode       : %s\n",
> -               p_cpufreq->u.ondemand.turbo_enabled ? "enabled" : "disabled");
> +        if (p_cpufreq->u.ondemand.turbo_enabled != 0) {
> +               printf("    turbo mode       : ");
> +               if (p_cpufreq->u.ondemand.turbo_enabled == 1)
> +                   printf("enabled\n");
> +               else
> +                   printf("disabled\n");
> +        }
>      }

Since the "turbo_enabled" is changed from governor specific flag to global 
cpufreq flag,  this code should also be changed accordingly. E.g. 
- move "turbo_enabled" from "struct  xen_ondemand" to "struct 
xen_get_cpufreq_para"
- display turbo_enabled as cpufreq parameter, not dbs governor parameter.

> 
>      printf("scaling_avail_freq   :");
> diff -r 18f4db5f72d7 -r 6d42054cb194 xen/arch/x86/acpi/cpufreq/cpufreq.c
> --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c     Thu Mar 25 10:01:05 2010
> +0000
> +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c     Thu Mar 25 13:01:23
> 2010 -0500
> @@ -410,6 +410,10 @@
>          return -ENODEV;
>      }
> 
> +    if (policy->turbo == -1)
> +        if (target_freq > policy->cpuinfo.second_max_freq)
> +            target_freq = policy->cpuinfo.second_max_freq;
> +
>      perf = data->acpi_data;
>      result = cpufreq_frequency_table_target(policy,
>                                              data->freq_table,
> @@ -610,12 +614,19 @@
>          break;
>      }
> 
> -    /* Check for APERF/MPERF support in hardware */
> +    /* Check for APERF/MPERF support in hardware
> +     * also check for boost support */
>      if (c->x86_vendor == X86_VENDOR_INTEL && c->cpuid_level >= 6) {
>          unsigned int ecx;
> +        unsigned int eax;
>          ecx = cpuid_ecx(6);
>          if (ecx & CPUID_6_ECX_APERFMPERF_CAPABILITY)
>              acpi_cpufreq_driver.getavg = get_measured_perf;
> +        eax = cpuid_eax(6);
> +        if ( eax & 0x2 ) {
> +            policy->turbo = 1;
> +            printk(XENLOG_INFO "Turbo Mode detected and enabled!\n");
> +        }
>      }
> 
>      /*
> diff -r 18f4db5f72d7 -r 6d42054cb194
> xen/drivers/cpufreq/cpufreq_ondemand.c
> --- a/xen/drivers/cpufreq/cpufreq_ondemand.c  Thu Mar 25 10:01:05
> 2010 +0000
> +++ b/xen/drivers/cpufreq/cpufreq_ondemand.c  Thu Mar 25 13:01:23
> 2010 -0500
> @@ -58,9 +58,6 @@
> 
>  static struct timer dbs_timer[NR_CPUS];
> 
> -/* Turbo Mode */
> -static int turbo_detected = 0;
> -
>  int write_ondemand_sampling_rate(unsigned int sampling_rate)
>  {
>      if ( (sampling_rate > MAX_SAMPLING_RATE / MICROSECS(1)) ||
> @@ -111,10 +108,6 @@
> 
>      policy = this_dbs_info->cur_policy;
>      max = policy->max;
> -    if (turbo_detected && !this_dbs_info->turbo_enabled) {
> -        if (max > policy->cpuinfo.second_max_freq)
> -            max = policy->cpuinfo.second_max_freq;
> -    }
> 
>      if (unlikely(policy->resume)) {
>          __cpufreq_driver_target(policy, max,CPUFREQ_RELATION_H);
> @@ -275,7 +268,6 @@
>              } else
>                  dbs_tuners_ins.sampling_rate = usr_sampling_rate;
>          }
> -        this_dbs_info->turbo_enabled = 1;
>          dbs_timer_init(this_dbs_info);
> 
>          break;
> @@ -352,13 +344,6 @@
> 
>  static int __init cpufreq_gov_dbs_init(void)
>  {
> -#ifdef CONFIG_X86
> -    unsigned int eax = cpuid_eax(6);
> -    if ( eax & 0x2 ) {
> -        turbo_detected = 1;
> -        printk(XENLOG_INFO "Turbo Mode detected!\n");
> -    }
> -#endif
>      return cpufreq_register_governor(&cpufreq_gov_dbs);
>  }
>  __initcall(cpufreq_gov_dbs_init);
> @@ -406,16 +391,27 @@
> 
>  void cpufreq_dbs_enable_turbo(int cpuid)
>  {
> -    per_cpu(cpu_dbs_info, cpuid).turbo_enabled = 1;
> +    struct cpufreq_policy *policy;
> +
> +    policy = per_cpu(cpu_dbs_info, cpuid).cur_policy;
> +    if (policy->turbo != 0)
> +        policy->turbo = 1;
>  }
> 
>  void cpufreq_dbs_disable_turbo(int cpuid)
>  {
> -    per_cpu(cpu_dbs_info, cpuid).turbo_enabled = 0;
> +    struct cpufreq_policy *policy;
> +
> +    policy = per_cpu(cpu_dbs_info, cpuid).cur_policy;
> +    if (policy->turbo != 0)
> +        policy->turbo = -1;
>  }
> 
>  unsigned int cpufreq_dbs_get_turbo_status(int cpuid)
>  {
> -    return turbo_detected && per_cpu(cpu_dbs_info, cpuid).turbo_enabled;
> +    struct cpufreq_policy *policy;
> +
> +    policy = per_cpu(cpu_dbs_info, cpuid).cur_policy;
> +    return policy->turbo;
>  }

Since "turbo_enabled " is cpufreq policy flag now, better to move these routine 
to xen/drivers/cpufreq/utility.c, and change name to something like 
cpufreq_enable_turbo...

> 
> diff -r 18f4db5f72d7 -r 6d42054cb194 xen/include/acpi/cpufreq/cpufreq.h
> --- a/xen/include/acpi/cpufreq/cpufreq.h      Thu Mar 25 10:01:05 2010
> +0000
> +++ b/xen/include/acpi/cpufreq/cpufreq.h      Thu Mar 25 13:01:23 2010 -0500
> @@ -55,6 +55,8 @@
> 
>      unsigned int        resume; /* flag for cpufreq 1st run
>                                   * S3 wakeup, hotplug cpu, etc */
> +    int                 turbo;  /* tristate flag: 0 for unsupported
> +                                 * -1 for disable, 1 for enabled */
>  };
>  extern struct cpufreq_policy *cpufreq_cpu_policy[NR_CPUS];

Better to define readable constant like:
#define CPUFREQ_TURBO_DISABLED  -1
#define CPUFREQ_TURBO_UNSUPPORTED       0
#define CPUFREQ_TURBO_ENABLED   1

instead of using magic number -1,0,1 in the above codes.

Best Regards
Ke

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel