lkml.org 
[lkml]   [2009]   [Sep]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [v4 PATCH 1/5]: cpuidle: Cleanup drivers/cpuidle/cpuidle.c
    * Balbir Singh <balbir@linux.vnet.ibm.com> [2009-09-01 22:58:25]:

    > * Arun R B <arun@linux.vnet.ibm.com> [2009-09-01 17:08:40]:
    >
    > > * Arun R Bharadwaj <arun@linux.vnet.ibm.com> [2009-09-01 17:07:04]:
    > >
    > > Cleanup drivers/cpuidle/cpuidle.c
    > >
    > > Cpuidle maintains a pm_idle_old void pointer because, currently in x86
    > > there is no clean way of registering and unregistering a idle function.
    > >
    > > So remove pm_idle_old and leave the responsibility of maintaining the
    > > list of registered idle loops to the architecture specific code. If the
    > > architecture registers cpuidle_idle_call as its idle loop, only then
    > > this loop is called.
    > >
    >
    > It sounds as if there is a side-effect of this
    > patch on x86 (am I reading it incorrectly), which can be fixed, but
    > it will need a patch or so to get back the old behaviour on x86.
    >

    Hi Balbir,

    Yes, your understanding is correct. Currently, x86 exports pm_idle and
    this pm_idle is set to cpuidle_idle_call inside cpuidle.c

    So instead of that x86 should just export a function called
    set_arch_idle() which will be called from within
    register_idle_function() and set pm_idle to the idle handler which is
    currently being registered.

    I have implemented this for pseries, and in the process of doing it
    for x86 too.

    > > Also remove unwanted functions cpuidle_[un]install_idle_handler,
    > > cpuidle_kick_cpus()
    > >
    > > Signed-off-by: Arun R Bharadwaj <arun@linux.vnet.ibm.com>
    > > ---
    > > drivers/cpuidle/cpuidle.c | 51 +++++++++++++++------------------------------
    > > drivers/cpuidle/governor.c | 3 --
    > > 2 files changed, 17 insertions(+), 37 deletions(-)
    > >
    > > Index: linux.trees.git/drivers/cpuidle/cpuidle.c
    > > ===================================================================
    > > --- linux.trees.git.orig/drivers/cpuidle/cpuidle.c
    > > +++ linux.trees.git/drivers/cpuidle/cpuidle.c
    > > @@ -24,9 +24,14 @@ DEFINE_PER_CPU(struct cpuidle_device *,
    > >
    > > DEFINE_MUTEX(cpuidle_lock);
    > > LIST_HEAD(cpuidle_detected_devices);
    > > -static void (*pm_idle_old)(void);
    > >
    > > static int enabled_devices;
    > > +static int idle_function_registered;
    > > +
    > > +struct idle_function_desc cpuidle_idle_desc = {
    > > + .name = "cpuidle_loop",
    > > + .idle_func = cpuidle_idle_call,
    > > +};
    > >
    > > #if defined(CONFIG_ARCH_HAS_CPU_IDLE_WAIT)
    > > static void cpuidle_kick_cpus(void)
    > > @@ -54,13 +59,10 @@ static void cpuidle_idle_call(void)
    > >
    > > /* check if the device is ready */
    > > if (!dev || !dev->enabled) {
    > > - if (pm_idle_old)
    > > - pm_idle_old();
    > > - else
    > > #if defined(CONFIG_ARCH_HAS_DEFAULT_IDLE)
    > > - default_idle();
    > > + default_idle();
    > > #else
    > > - local_irq_enable();
    > > + local_irq_enable();
    > > #endif
    > > return;
    > > }
    > > @@ -94,35 +96,11 @@ static void cpuidle_idle_call(void)
    > > }
    > >
    > > /**
    > > - * cpuidle_install_idle_handler - installs the cpuidle idle loop handler
    > > - */
    > > -void cpuidle_install_idle_handler(void)
    > > -{
    > > - if (enabled_devices && (pm_idle != cpuidle_idle_call)) {
    > > - /* Make sure all changes finished before we switch to new idle */
    > > - smp_wmb();
    > > - pm_idle = cpuidle_idle_call;
    > > - }
    > > -}
    > > -
    > > -/**
    > > - * cpuidle_uninstall_idle_handler - uninstalls the cpuidle idle loop handler
    > > - */
    > > -void cpuidle_uninstall_idle_handler(void)
    > > -{
    > > - if (enabled_devices && pm_idle_old && (pm_idle != pm_idle_old)) {
    > > - pm_idle = pm_idle_old;
    > > - cpuidle_kick_cpus();
    > > - }
    > > -}
    > > -
    > > -/**
    > > * cpuidle_pause_and_lock - temporarily disables CPUIDLE
    > > */
    > > void cpuidle_pause_and_lock(void)
    > > {
    > > mutex_lock(&cpuidle_lock);
    > > - cpuidle_uninstall_idle_handler();
    > > }
    > >
    > > EXPORT_SYMBOL_GPL(cpuidle_pause_and_lock);
    > > @@ -132,7 +110,6 @@ EXPORT_SYMBOL_GPL(cpuidle_pause_and_lock
    > > */
    > > void cpuidle_resume_and_unlock(void)
    > > {
    > > - cpuidle_install_idle_handler();
    > > mutex_unlock(&cpuidle_lock);
    > > }
    > >
    >
    > What does this mean for users of cpuidle_pause_and_lock/unlock?
    > Should we be calling register/unregister_idle_function here?
    >

    Yes, you are right. I have missed out on this part.
    register/unregister_idle_function should replace
    install/uninstall_idle_handler at those places. Thanks.

    >
    > > @@ -287,6 +264,12 @@ static int __cpuidle_register_device(str
    > > return 0;
    > > }
    > >
    > > +static void register_cpuidle_idle_function(void)
    > > +{
    > > + register_idle_function(&cpuidle_idle_desc);
    > > +
    > > + idle_function_registered = 1;
    >
    > Use booleans if possible, unless you intend to extend the meaning of
    > registered someday.
    >

    I don't intend to extend the meaning of idle_function_registered.
    Will use boolean here.

    > > +}
    > > /**
    > > * cpuidle_register_device - registers a CPU's idle PM feature
    > > * @dev: the cpu
    > > @@ -303,7 +286,9 @@ int cpuidle_register_device(struct cpuid
    > > }
    > >
    > > cpuidle_enable_device(dev);
    > > - cpuidle_install_idle_handler();
    > > +
    > > + if (!idle_function_registered)
    > > + register_cpuidle_idle_function();
    > >
    > > mutex_unlock(&cpuidle_lock);
    > >
    > > @@ -382,8 +367,6 @@ static int __init cpuidle_init(void)
    > > {
    > > int ret;
    > >
    > > - pm_idle_old = pm_idle;
    > > -
    > > ret = cpuidle_add_class_sysfs(&cpu_sysdev_class);
    > > if (ret)
    > > return ret;
    > > Index: linux.trees.git/drivers/cpuidle/governor.c
    > > ===================================================================
    > > --- linux.trees.git.orig/drivers/cpuidle/governor.c
    > > +++ linux.trees.git/drivers/cpuidle/governor.c
    > > @@ -48,8 +48,6 @@ int cpuidle_switch_governor(struct cpuid
    > > if (gov == cpuidle_curr_governor)
    > > return 0;
    > >
    > > - cpuidle_uninstall_idle_handler();
    > > -
    > > if (cpuidle_curr_governor) {
    > > list_for_each_entry(dev, &cpuidle_detected_devices, device_list)
    > > cpuidle_disable_device(dev);
    > > @@ -63,7 +61,6 @@ int cpuidle_switch_governor(struct cpuid
    > > return -EINVAL;
    > > list_for_each_entry(dev, &cpuidle_detected_devices, device_list)
    > > cpuidle_enable_device(dev);
    > > - cpuidle_install_idle_handler();
    > > printk(KERN_INFO "cpuidle: using governor %s\n", gov->name);
    > > }
    > >
    >
    > --
    > Balbir

    Thanks for the review!
    --arun


    \
     
     \ /
      Last update: 2009-09-02 07:25    [W:0.070 / U:0.044 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site