Xen devel - Feb 2009 - [PATCH 2] X86: cpufreq get_cur_val adjustment

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;

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

>>> "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


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

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

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