Xen devel - Dec 2008 - [PATCH] Change cpufreq_driver->get so that it can get other cpu''s real physical freq

Change cpufreq_driver->get so that it can get other cpu''s real
physical freq.

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

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

>From: Liu, Jinsong
>Sent: Thursday, December 11, 2008 10:42 AM
>
>Change cpufreq_driver->get so that it can get other cpu''s real 
>physical freq.
>
>Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com>
@@ -174,7 +177,11 @@ static void drv_read(struct drv_cmd *cmd
 {
     cmd->val = 0;
 
-    do_drv_read(cmd);
+    /* to reduce IPI for the sake of performance */
+    if (first_cpu(cmd->mask) == smp_processor_id())
+        do_drv_read((void *)cmd);
+    else
+        on_selected_cpus( cmd->mask, do_drv_read, (void *)cmd, 0, 1);
 }

Above is a bad change where you kicks multiple cpus to update 
same variable without any coordination. Also why can short path only
be used with strict condition that current cpu must be first bit in
mask? As long as current cpu is on mask, you can always read
directly.

@@ -205,7 +220,7 @@ static u32 get_cur_val(cpumask_t mask)
         return 0;
     }
 
-    cmd.mask = mask;
+    cmd.mask = cpumask_of_cpu(cpu);
 
     drv_read(&cmd);
     return cmd.val;

Since do_drv_read can figure out which cpu to read from the mask,
why do you need above limitation then?

Thanks,
Kevin

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

Tian, Kevin wrote:
>> From: Liu, Jinsong
>> Sent: Thursday, December 11, 2008 10:42 AM
>> 
>> Change cpufreq_driver->get so that it can get other cpu''s
real
>> physical freq. 
>> 
>> Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com>
> 
> @@ -174,7 +177,11 @@ static void drv_read(struct drv_cmd *cmd
>  {
>      cmd->val = 0;
> 
> -    do_drv_read(cmd);
> +    /* to reduce IPI for the sake of performance */
> +    if (first_cpu(cmd->mask) == smp_processor_id())
> +        do_drv_read((void *)cmd);
> +    else
> +        on_selected_cpus( cmd->mask, do_drv_read, (void *)cmd, 0, 1);
>  }
> 
> Above is a bad change where you kicks multiple cpus to update
> same variable without any coordination. Also why can short path only
> be used with strict condition that current cpu must be first bit in
> mask? As long as current cpu is on mask, you can always read
> directly.
No need any coordination here, please notice under whatever situation, there is
only 1 bit set at cpumask here (we can only read the value of 1 cpu 1 time).
The first bit of cpumask here is not to set strict condition for short path. In
fact, becuase of only 1 bit set at cpumask, the first_cpu() is only used to get
its cpuid. If cpuid is the running cpu, go short path, otherwise go longer path.

In fact, get_cur_val() was called by 2 paths (driver path and check_freq path),
we design here is used to provide a unified interface to satisfied the
requirement of both paths.
> 
> @@ -205,7 +220,7 @@ static u32 get_cur_val(cpumask_t mask)
>          return 0;
>      }
> 
> -    cmd.mask = mask;
> +    cmd.mask = cpumask_of_cpu(cpu);
> 
>      drv_read(&cmd);
>      return cmd.val;
> 
> Since do_drv_read can figure out which cpu to read from the mask,
> why do you need above limitation then?
do_drv_read get the msr value of the cpu which it runs upon.
the cpumask limitation here is used to let drv_read() only read 1 cpu msr value,
otherwise it will be confused, on_selected_cpu will send IPI to multi cpus and
read msr value will mistake.

Thanks,
Jinsong
> 
> Thanks,
> Kevin

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

On 11/12/2008 04:15, "Liu, Jinsong" <jinsong.liu@intel.com>
wrote:
>> Above is a bad change where you kicks multiple cpus to update
>> same variable without any coordination. Also why can short path only
>> be used with strict condition that current cpu must be first bit in
>> mask? As long as current cpu is on mask, you can always read
>> directly.
> 
> No need any coordination here, please notice under whatever situation,
there
> is only 1 bit set at cpumask here (we can only read the value of 1 cpu 1
> time).
Then please ASSERT(cpus_weight(cmd->mask) == 1) to make this constraint
clear.
> The first bit of cpumask here is not to set strict condition for short
path.
> In fact, becuase of only 1 bit set at cpumask, the first_cpu() is only used
to
> get its cpuid. If cpuid is the running cpu, go short path, otherwise go
longer
> path.
The clearer (and faster) idiom is cpu_isset(smp_processor_id(), cmd->mask).

Please can you make these changes, test, and re-send.

 Thanks,
 Keir
> In fact, get_cur_val() was called by 2 paths (driver path and check_freq
> path), we design here is used to provide a unified interface to satisfied
the
> requirement of both paths.


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

Keir Fraser wrote:
> On 11/12/2008 04:15, "Liu, Jinsong" <jinsong.liu@intel.com>
wrote:
> 
>>> Above is a bad change where you kicks multiple cpus to update
>>> same variable without any coordination. Also why can short path
only
>>> be used with strict condition that current cpu must be first bit in
>>> mask? As long as current cpu is on mask, you can always read
>>> directly.
>> 
>> No need any coordination here, please notice under whatever
>> situation, there is only 1 bit set at cpumask here (we can only read
>> the value of 1 cpu 1 time).
> 
> Then please ASSERT(cpus_weight(cmd->mask) == 1) to make this
> constraint clear.
> 
>> The first bit of cpumask here is not to set strict condition for
>> short path. In fact, becuase of only 1 bit set at cpumask, the
>> first_cpu() is only used to get its cpuid. If cpuid is the running
>> cpu, go short path, otherwise go longer path.
> 
> The clearer (and faster) idiom is cpu_isset(smp_processor_id(),
> cmd->mask). 
> 
> Please can you make these changes, test, and re-send.
> 
>  Thanks,
>  Keir
> 
>> In fact, get_cur_val() was called by 2 paths (driver path and
>> check_freq path), we design here is used to provide a unified
>> interface to satisfied the requirement of both paths.
Keir,

Thanks for comments, the patch has been updated and tested, will send out later.

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