[PATCH] Revert Jan's patch (c/s 18879) since now it can be achieved by xenpm tool now

[PATCH] Revert Jan's patch (c/s 18879) since now it can be achieved by xenpm tool now

[Liu, Jinsong]

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

Revert Jan's patch (c/s 18879) since now it can be achieved by 
xenpm tool now.

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

[-- Attachment #2: px-xen-1-revert-jan.patch --]
[-- Type: application/octet-stream, Size: 5154 bytes --]

Revert Jan's patch (c/s 18879) since now it can be achieved by 
xenpm tool now.

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

diff -r 9e3d81ea61fb xen/common/domain.c
--- a/xen/common/domain.c	Thu Dec 18 14:14:30 2008 +0800
+++ b/xen/common/domain.c	Fri Dec 19 15:09:32 2008 +0800
@@ -25,7 +25,6 @@
 #include <xen/percpu.h>
 #include <xen/multicall.h>
 #include <xen/rcupdate.h>
-#include <acpi/cpufreq/cpufreq.h>
 #include <asm/debugger.h>
 #include <public/sched.h>
 #include <public/vcpu.h>
@@ -42,25 +41,16 @@ enum cpufreq_controller cpufreq_controll
 enum cpufreq_controller cpufreq_controller;
 static void __init setup_cpufreq_option(char *str)
 {
-    char *arg;
-
     if ( !strcmp(str, "dom0-kernel") )
     {
         xen_processor_pmbits &= ~XEN_PROCESSOR_PM_PX;
         cpufreq_controller = FREQCTL_dom0_kernel;
         opt_dom0_vcpus_pin = 1;
-        return;
     }
-
-    if ( (arg = strpbrk(str, ",:")) != NULL )
-        *arg++ = '\0';
-
-    if ( !strcmp(str, "xen") )
+    else if ( !strcmp(str, "xen") )
     {
         xen_processor_pmbits |= XEN_PROCESSOR_PM_PX;
         cpufreq_controller = FREQCTL_xen;
-        if ( arg && *arg )
-            cpufreq_cmdline_parse(arg);
     }
 }
 custom_param("cpufreq", setup_cpufreq_option);
diff -r 9e3d81ea61fb xen/drivers/cpufreq/cpufreq_ondemand.c
--- a/xen/drivers/cpufreq/cpufreq_ondemand.c	Thu Dec 18 14:14:30 2008 +0800
+++ b/xen/drivers/cpufreq/cpufreq_ondemand.c	Fri Dec 19 15:09:32 2008 +0800
@@ -37,7 +37,6 @@
 #define TRANSITION_LATENCY_LIMIT                (10 * 1000 )
 
 static uint64_t def_sampling_rate;
-static uint64_t usr_sampling_rate;
 
 /* Sampling types */
 enum {DBS_NORMAL_SAMPLE, DBS_SUB_SAMPLE};
@@ -244,20 +243,7 @@ int cpufreq_governor_dbs(struct cpufreq_
             if (def_sampling_rate < MIN_STAT_SAMPLING_RATE)
                 def_sampling_rate = MIN_STAT_SAMPLING_RATE;
 
-            if (!usr_sampling_rate)
-                dbs_tuners_ins.sampling_rate = def_sampling_rate;
-            else if (usr_sampling_rate < MIN_SAMPLING_RATE) {
-                printk(KERN_WARNING "cpufreq/ondemand: "
-                       "specified sampling rate too low, using %"PRIu64"\n",
-                       MIN_SAMPLING_RATE);
-                dbs_tuners_ins.sampling_rate = MIN_SAMPLING_RATE;
-            } else if (usr_sampling_rate > MAX_SAMPLING_RATE) {
-                printk(KERN_WARNING "cpufreq/ondemand: "
-                       "specified sampling rate too high, using %"PRIu64"\n",
-                       MAX_SAMPLING_RATE);
-                dbs_tuners_ins.sampling_rate = MAX_SAMPLING_RATE;
-            } else
-                dbs_tuners_ins.sampling_rate = usr_sampling_rate;
+            dbs_tuners_ins.sampling_rate = def_sampling_rate;
         }
         dbs_timer_init(this_dbs_info);
 
@@ -297,55 +283,3 @@ static void cpufreq_gov_dbs_exit(void)
     cpufreq_unregister_governor(&cpufreq_gov_dbs);
 }
 __exitcall(cpufreq_gov_dbs_exit);
-
-void __init cpufreq_cmdline_parse(char *str)
-{
-    do {
-        char *val, *end = strchr(str, ',');
-
-        if ( end )
-            *end++ = '\0';
-        val = strchr(str, '=');
-        if ( val )
-            *val = '\0';
-
-        if ( !strcmp(str, "rate") && val )
-        {
-            usr_sampling_rate = simple_strtoull(val, NULL, 0);
-        }
-        else if ( !strcmp(str, "threshold") && val )
-        {
-            unsigned long tmp = simple_strtoul(val, NULL, 0);
-
-            if ( tmp < MIN_FREQUENCY_UP_THRESHOLD )
-            {
-                printk(XENLOG_WARNING "cpufreq/ondemand: "
-                       "specified threshold too low, using %d\n",
-                       MIN_FREQUENCY_UP_THRESHOLD);
-                tmp = MIN_FREQUENCY_UP_THRESHOLD;
-            }
-            else if ( tmp > MAX_FREQUENCY_UP_THRESHOLD )
-            {
-                printk(XENLOG_WARNING "cpufreq/ondemand: "
-                       "specified threshold too high, using %d\n",
-                       MAX_FREQUENCY_UP_THRESHOLD);
-                tmp = MAX_FREQUENCY_UP_THRESHOLD;
-            }
-            dbs_tuners_ins.up_threshold = tmp;
-        }
-        else if ( !strcmp(str, "bias") && val )
-        {
-            unsigned long tmp = simple_strtoul(val, NULL, 0);
-
-            if ( tmp > 1000 )
-            {
-                printk(XENLOG_WARNING "cpufreq/ondemand: "
-                       "specified bias too high, using 1000\n");
-                tmp = 1000;
-            }
-            dbs_tuners_ins.powersave_bias = tmp;
-        }
-
-        str = end;
-    } while ( str );
-}
diff -r 9e3d81ea61fb xen/include/acpi/cpufreq/cpufreq.h
--- a/xen/include/acpi/cpufreq/cpufreq.h	Thu Dec 18 14:14:30 2008 +0800
+++ b/xen/include/acpi/cpufreq/cpufreq.h	Fri Dec 19 15:09:32 2008 +0800
@@ -52,8 +52,6 @@ extern struct cpufreq_policy *cpufreq_cp
 
 extern int __cpufreq_set_policy(struct cpufreq_policy *data,
                                 struct cpufreq_policy *policy);
-
-void cpufreq_cmdline_parse(char *);
 
 #define CPUFREQ_SHARED_TYPE_NONE (0) /* None */
 #define CPUFREQ_SHARED_TYPE_HW   (1) /* HW does needed coordination */

[-- 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] Revert Jan's patch (c/s 18879) since now it canbe achieved by xenpm tool now

[Jan Beulich]

>>> "Liu, Jinsong" <jinsong.liu@intel.com> 20.12.08 14:37 >>>
>Revert Jan's patch (c/s 18879) since now it can be achieved by 
>xenpm tool now.

Please don't, and rather (as Keir suggested) add an option to also select
the initial governor on the command line. While the xenpm tool is nice,
older distro-s won't run it by default, and having to fiddle with the system
startup scripts or running it manually (which wouldn't necessarily work if
you don't do a full install of the Xen tool - e.g. to keep the distro's tools
intact) isn't really desirable in certain cases.

Jan

RE: [PATCH] Revert Jan's patch (c/s 18879) since now it canbe achieved by xenpm tool now

[Liu, Jinsong]

Jan Beulich wrote:
>>>> "Liu, Jinsong" <jinsong.liu@intel.com> 20.12.08 14:37 >>>
>> Revert Jan's patch (c/s 18879) since now it can be achieved by
>> xenpm tool now.
> 
> Please don't, and rather (as Keir suggested) add an option to also
> select the initial governor on the command line. While the xenpm tool
> is nice, older distro-s won't run it by default, and having to fiddle
> with the system startup scripts or running it manually (which
> wouldn't necessarily work if you don't do a full install of the Xen
> tool - e.g. to keep the distro's tools intact) isn't really desirable
> in certain cases. 
> 
> Jan

Jan,

It's good for old distro-s user to use cmdline to set parameter, but I have some concern, since in fact cpufreq have 5 governors, with ~20 status parameters and ~10 control parameters. If we add option at grub cmdline to select initial governor, and add further options to set control parameters, it may make confuse for user, especially considered that some of the control parameters are governor-dependent (i.e. in c/s 18879, the control para 'sample rate' and 'upthreshold' can only be used for ondemand or conservative governor). User may confuse what the default governor is, and what control parameters can be used at grub cmdline for that governor.

Thanks,
Jinsong

RE: [PATCH] Revert Jan's patch (c/s 18879) since now itcanbe achieved by xenpm tool now

[Jan Beulich]

>>> "Liu, Jinsong" <jinsong.liu@intel.com> 22.12.08 10:40 >>>
>Jan Beulich wrote:
>>>>> "Liu, Jinsong" <jinsong.liu@intel.com> 20.12.08 14:37 >>>
>>> Revert Jan's patch (c/s 18879) since now it can be achieved by
>>> xenpm tool now.
>> 
>> Please don't, and rather (as Keir suggested) add an option to also
>> select the initial governor on the command line. While the xenpm tool
>> is nice, older distro-s won't run it by default, and having to fiddle
>> with the system startup scripts or running it manually (which
>> wouldn't necessarily work if you don't do a full install of the Xen
>> tool - e.g. to keep the distro's tools intact) isn't really desirable
>> in certain cases. 
>> 
>> Jan
>
>Jan,
>
>It's good for old distro-s user to use cmdline to set parameter, but I have
>some concern, since in fact cpufreq have 5 governors, with ~20 status
>parameters and ~10 control parameters. If we add option at grub
>cmdline to select initial governor, and add further options to set control
>parameters, it may make confuse for user, especially considered that
>some of the control parameters are governor-dependent (i.e. in c/s
>18879, the control para 'sample rate' and 'upthreshold' can only be used
>for ondemand or conservative governor). User may confuse what the

Which is why they are being parsed in the ondemand governor's source
file...

>default governor is, and what control parameters can be used at grub
>cmdline for that governor.

Anything not applicable would be ignored. I don't think there's much
confusion associated with this.

Jan

RE: [PATCH] Revert Jan's patch (c/s 18879) since now itcanbe achieved by xenpm tool now

[Liu, Jinsong]

Jan Beulich wrote:
>>>> "Liu, Jinsong" <jinsong.liu@intel.com> 22.12.08 10:40 >>>
>> Jan Beulich wrote:
>>>>>> "Liu, Jinsong" <jinsong.liu@intel.com> 20.12.08 14:37 >>>
>>>> Revert Jan's patch (c/s 18879) since now it can be achieved by
>>>> xenpm tool now.
>>> 
>>> Please don't, and rather (as Keir suggested) add an option to also
>>> select the initial governor on the command line. While the xenpm
>>> tool is nice, older distro-s won't run it by default, and having to
>>> fiddle with the system startup scripts or running it manually (which
>>> wouldn't necessarily work if you don't do a full install of the Xen
>>> tool - e.g. to keep the distro's tools intact) isn't really
>>> desirable in certain cases. 
>>> 
>>> Jan
>> 
>> Jan,
>> 
>> It's good for old distro-s user to use cmdline to set parameter, but
>> I have some concern, since in fact cpufreq have 5 governors, with
>> ~20 status parameters and ~10 control parameters. If we add option
>> at grub 
>> cmdline to select initial governor, and add further options to set
>> control parameters, it may make confuse for user, especially
>> considered that 
>> some of the control parameters are governor-dependent (i.e. in c/s
>> 18879, the control para 'sample rate' and 'upthreshold' can only be
>> used for ondemand or conservative governor). User may confuse what
>> the 
> 
> Which is why they are being parsed in the ondemand governor's source
> file...
> 
>> default governor is, and what control parameters can be used at grub
>> cmdline for that governor.
> 
> Anything not applicable would be ignored. I don't think there's much
> confusion associated with this.
> 
> Jan

Some cpufreq governor may fail at init stage, i.e. performance governor cannot start at some platform with hardware flaw.
In this case, if user select performance gov at cmdline, governor init will fail and then whole cpufreq init will fail, xen will have no cpufreq then.

Our idea is, first step cpufreq logic select a 'safe' governor as default, say, userspace governor. It will never fail, since it keep cpu freq just like it didn't work at init stage, so will not have any influence to perfromance, power, ..., etc.
Second step, user can change to any other governor as he like, if fail, cpufreq logic can gracefully back to the 'old-safe-governor'.
This way at least xen can ensure cpufreq logic successful, and safe change between different governors.

Thanks,
Jinsong

Re: [PATCH] Revert Jan's patch (c/s 18879) since now itcanbe achieved by xenpm tool now

[Keir Fraser]

On 23/12/2008 02:29, "Liu, Jinsong" <jinsong.liu@intel.com> wrote:

> Some cpufreq governor may fail at init stage, i.e. performance governor cannot
> start at some platform with hardware flaw.
> In this case, if user select performance gov at cmdline, governor init will
> fail and then whole cpufreq init will fail, xen will have no cpufreq then.
> 
> Our idea is, first step cpufreq logic select a 'safe' governor as default,
> say, userspace governor. It will never fail, since it keep cpu freq just like
> it didn't work at init stage, so will not have any influence to perfromance,
> power, ..., etc.
> Second step, user can change to any other governor as he like, if fail,
> cpufreq logic can gracefully back to the 'old-safe-governor'.
> This way at least xen can ensure cpufreq logic successful, and safe change
> between different governors.

Two thoughts: Firstly, the user should be wary of such behaviour if they
have explicitly selected a governor on the command line. Secondly, if it is
appropriate to have cpufreq always on, why not hardcode a safe governor as a
fallback?

 -- Keir

RE: [PATCH] Revert Jan's patch (c/s 18879) since now itcanbe achieved by xenpm tool now

[Liu, Jinsong]

Keir Fraser wrote:
> On 23/12/2008 02:29, "Liu, Jinsong" <jinsong.liu@intel.com> wrote:
> 
>> Some cpufreq governor may fail at init stage, i.e. performance
>> governor cannot start at some platform with hardware flaw.
>> In this case, if user select performance gov at cmdline, governor
>> init will fail and then whole cpufreq init will fail, xen will have
>> no cpufreq then. 
>> 
>> Our idea is, first step cpufreq logic select a 'safe' governor as
>> default, say, userspace governor. It will never fail, since it keep
>> cpu freq just like it didn't work at init stage, so will not have
>> any influence to perfromance, power, ..., etc. Second step, user can
>> change to any other governor as he like, if fail, cpufreq logic can
>> gracefully back to the 'old-safe-governor'. 
>> This way at least xen can ensure cpufreq logic successful, and safe
>> change between different governors.
> 
> Two thoughts: Firstly, the user should be wary of such behaviour if
> they have explicitly selected a governor on the command line.
> Secondly, if it is appropriate to have cpufreq always on, why not
> hardcode a safe governor as a fallback?
> 
>  -- Keir

It's fine to me to keep cmdline parse to select cpufreq governor.
I will update my patch, final result is:
1. keep Jan's patch (c/s 18879);
2. add governor setting at cmdline;
3. set xen as default cpufreq;
4. set userspace as default governor;
5. if user set governor at cmdline --> if gov startup success, then cpufreq run it; if gov startup fail, then cpufreq back to default safe governor;
Is it OK?

Thanks,
Jinsong

Re: [PATCH] Revert Jan's patch (c/s 18879) since now itcanbe achieved by xenpm tool now

[Keir Fraser]

On 23/12/2008 09:02, "Liu, Jinsong" <jinsong.liu@intel.com> wrote:

> It's fine to me to keep cmdline parse to select cpufreq governor.
> I will update my patch, final result is:
> 1. keep Jan's patch (c/s 18879);
> 2. add governor setting at cmdline;
> 3. set xen as default cpufreq;
> 4. set userspace as default governor;
> 5. if user set governor at cmdline --> if gov startup success, then cpufreq
> run it; if gov startup fail, then cpufreq back to default safe governor;
> Is it OK?

Sounds fine to me. Let's see what Jan thinks too.

 -- Keir

Re: [PATCH] Revert Jan's patch (c/s 18879) since nowitcanbe achieved by xenpm tool now

[Jan Beulich]

>>> Keir Fraser <keir.fraser@eu.citrix.com> 23.12.08 10:06 >>>
>On 23/12/2008 09:02, "Liu, Jinsong" <jinsong.liu@intel.com> wrote:
>
>> It's fine to me to keep cmdline parse to select cpufreq governor.
>> I will update my patch, final result is:
>> 1. keep Jan's patch (c/s 18879);
>> 2. add governor setting at cmdline;
>> 3. set xen as default cpufreq;
>> 4. set userspace as default governor;
>> 5. if user set governor at cmdline --> if gov startup success, then cpufreq
>> run it; if gov startup fail, then cpufreq back to default safe governor;
>> Is it OK?
>
>Sounds fine to me. Let's see what Jan thinks too.

I'm okay with this too, though I'm not entirely clear why the userspace
governor would be safer/better than the performance one (in particular,
I'm unclear why the latter - supposedly not playing with frequencies at
all - has dependencies on certain tables to be loaded successfully). But
I admit that I didn't get around to look at the recent code changes, yet.

Jan