Re: [v4 PATCH 1/5]: cpuidle: Cleanup drivers/cpuidle/cpuidle.c
From: Arun R Bharadwaj
Date: Wed Sep 02 2009 - 01:21:54 EST
* Balbir Singh <balbir@xxxxxxxxxxxxxxxxxx> [2009-09-01 22:58:25]:
> * Arun R B <arun@xxxxxxxxxxxxxxxxxx> [2009-09-01 17:08:40]:
>
> > * Arun R Bharadwaj <arun@xxxxxxxxxxxxxxxxxx> [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@xxxxxxxxxxxxxxxxxx>
> > ---
> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/