[PATCH 2] X86: cpufreq get_cur_val adjustment

[PATCH 2] X86: cpufreq get_cur_val adjustment

[Liu, Jinsong]

[-- Attachment #1: Type: text/plain, Size: 2071 bytes --]

X86: cpufreq get_cur_val adjustment

c/s 19149 update cpufreq get_cur_val logic to avoid cross processor call, it's a good point.
However, to avoid null drv_data pointer, we adjust some logic in this patch, to keep advantage of c/s 19149 and at same time to avoid null drv_data pointer.

Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com>

diff -r fa8d08857294 xen/arch/x86/acpi/cpufreq/cpufreq.c
--- a/xen/arch/x86/acpi/cpufreq/cpufreq.c	Sun Feb 08 13:07:46 2009 +0800
+++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c	Sun Feb 08 19:18:16 2009 +0800
@@ -183,7 +183,7 @@ static void drv_read(struct drv_cmd *cmd
     ASSERT(cpus_weight(cmd->mask) == 1);
 
     /* to reduce IPI for the sake of performance */
-    if (cpu_isset(smp_processor_id(), cmd->mask))
+    if (likely(cpu_isset(smp_processor_id(), cmd->mask)))
         do_drv_read((void *)cmd);
     else
         on_selected_cpus( cmd->mask, do_drv_read, (void *)cmd, 0, 1);
@@ -196,6 +196,7 @@ static void drv_write(struct drv_cmd *cm
 
 static u32 get_cur_val(cpumask_t mask)
 {
+    struct cpufreq_policy *policy;
     struct processor_performance *perf;
     struct drv_cmd cmd;
     unsigned int cpu = smp_processor_id();
@@ -205,18 +206,19 @@ static u32 get_cur_val(cpumask_t mask)
 
     if (!cpu_isset(cpu, mask))
         cpu = first_cpu(mask);
+    policy = cpufreq_cpu_policy[cpu];
 
-    if (cpu >= NR_CPUS || !drv_data[cpu])
+    if (cpu >= NR_CPUS || !policy || !drv_data[policy->cpu])
         return 0;    
 
-    switch (drv_data[cpu]->cpu_feature) {
+    switch (drv_data[policy->cpu]->cpu_feature) {
     case SYSTEM_INTEL_MSR_CAPABLE:
         cmd.type = SYSTEM_INTEL_MSR_CAPABLE;
         cmd.addr.msr.reg = MSR_IA32_PERF_STATUS;
         break;
     case SYSTEM_IO_CAPABLE:
         cmd.type = SYSTEM_IO_CAPABLE;
-        perf = drv_data[cpu]->acpi_data;
+        perf = drv_data[policy->cpu]->acpi_data;
         cmd.addr.io.port = perf->control_register.address;
         cmd.addr.io.bit_width = perf->control_register.bit_width;
         break;

[-- Attachment #2: px-xen-2-getval.patch --]
[-- Type: application/octet-stream, Size: 2023 bytes --]

X86: cpufreq get_cur_val adjustment

c/s 19149 update cpufreq get_cur_val logic to avoid cross processor call, it's a good point.
However, to avoid null drv_data pointer, we adjust some logic in this patch, to keep advantage of c/s 19149 and at same time to avoid null drv_data pointer.

Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com>

diff -r fa8d08857294 xen/arch/x86/acpi/cpufreq/cpufreq.c
--- a/xen/arch/x86/acpi/cpufreq/cpufreq.c	Sun Feb 08 13:07:46 2009 +0800
+++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c	Sun Feb 08 19:18:16 2009 +0800
@@ -183,7 +183,7 @@ static void drv_read(struct drv_cmd *cmd
     ASSERT(cpus_weight(cmd->mask) == 1);
 
     /* to reduce IPI for the sake of performance */
-    if (cpu_isset(smp_processor_id(), cmd->mask))
+    if (likely(cpu_isset(smp_processor_id(), cmd->mask)))
         do_drv_read((void *)cmd);
     else
         on_selected_cpus( cmd->mask, do_drv_read, (void *)cmd, 0, 1);
@@ -196,6 +196,7 @@ static void drv_write(struct drv_cmd *cm
 
 static u32 get_cur_val(cpumask_t mask)
 {
+    struct cpufreq_policy *policy;
     struct processor_performance *perf;
     struct drv_cmd cmd;
     unsigned int cpu = smp_processor_id();
@@ -205,18 +206,19 @@ static u32 get_cur_val(cpumask_t mask)
 
     if (!cpu_isset(cpu, mask))
         cpu = first_cpu(mask);
+    policy = cpufreq_cpu_policy[cpu];
 
-    if (cpu >= NR_CPUS || !drv_data[cpu])
+    if (cpu >= NR_CPUS || !policy || !drv_data[policy->cpu])
         return 0;    
 
-    switch (drv_data[cpu]->cpu_feature) {
+    switch (drv_data[policy->cpu]->cpu_feature) {
     case SYSTEM_INTEL_MSR_CAPABLE:
         cmd.type = SYSTEM_INTEL_MSR_CAPABLE;
         cmd.addr.msr.reg = MSR_IA32_PERF_STATUS;
         break;
     case SYSTEM_IO_CAPABLE:
         cmd.type = SYSTEM_IO_CAPABLE;
-        perf = drv_data[cpu]->acpi_data;
+        perf = drv_data[policy->cpu]->acpi_data;
         cmd.addr.io.port = perf->control_register.address;
         cmd.addr.io.bit_width = perf->control_register.bit_width;
         break;

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH 2] X86: cpufreq get_cur_val adjustment

[Jan Beulich]

>>> "Liu, Jinsong" <jinsong.liu@intel.com> 11.02.09 09:46 >>>
>X86: cpufreq get_cur_val adjustment
>
>c/s 19149 update cpufreq get_cur_val logic to avoid cross processor call, it's a good point.
>However, to avoid null drv_data pointer, we adjust some logic in this patch, to keep advantage of c/s 19149 and at same time
>to avoid null drv_data pointer.

Are you saying that there are cases where cpufreq_cpu_policy[cpu]->cpu != cpu?

And shouldn't drv_data[] be initialized for all known CPUs (possibly set to the same value for several of them)?

The patch you submitted would only be needed if the answer is 'yes' to the first question, and 'no' to the second (and even then I would think fixing drv_data[] initialization would be better than the patch presented here).

Jan

RE: [PATCH 2] X86: cpufreq get_cur_val adjustment

[Liu, Jinsong]

Jan Beulich wrote:
>>>> "Liu, Jinsong" <jinsong.liu@intel.com> 11.02.09 09:46 >>>
>> X86: cpufreq get_cur_val adjustment
>> 
>> c/s 19149 update cpufreq get_cur_val logic to avoid cross processor
>> call, it's a good point. 
>> However, to avoid null drv_data pointer, we adjust some logic in
>> this patch, to keep advantage of c/s 19149 and at same time to avoid
>> null drv_data pointer. 
> 
> Are you saying that there are cases where
> cpufreq_cpu_policy[cpu]->cpu != cpu? 
> 
> And shouldn't drv_data[] be initialized for all known CPUs (possibly
> set to the same value for several of them)? 
> 
> The patch you submitted would only be needed if the answer is 'yes'
> to the first question, and 'no' to the second (and even then I would
> think fixing drv_data[] initialization would be better than the patch
> presented here).   
> 
> Jan

You have eagle eyes :)

For the 1st question, the answer is yes.
    - in cpufreq_cpu_policy[cpu], cpu is the processor number as a index;
    - in cpufreq_cpu_policy[cpu]->cpu, the later cpu is the 'main cpu' of the coordination domain;

For the 2nd question, the answer is no.
    - this is inherited from native linux, although it seems a little strange, native linux really works so;
    - in a _PSD domain, there is a 'main cpu', and only drv_data[main_cpu] is not null;
    - I agree with you, logically drv_data[] can be a per-domain data structure rather than as current per-cpu data structure, however, native linux don't have per-domain level structure, and considering different coordination type, per-domain structure will be very complex. I think current drv_data is OK, it works and compatible with latest native linux (i.e. 2.6.26.5).

Thanks,
Jinsong