| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 2 Sep 2009 11:15:52 +0530
From: Arun R Bharadwaj <arun@...ux.vnet.ibm.com>
To: Balbir Singh <balbir@...ux.vnet.ibm.com>
Cc: Joel Schopp <jschopp@...tin.ibm.com>,
Benjamin Herrenschmidt <benh@...nel.crashing.org>,
Paul Mackerras <paulus@...ba.org>,
Peter Zijlstra <a.p.zijlstra@...llo.nl>,
Ingo Molnar <mingo@...e.hu>,
Vaidyanathan Srinivasan <svaidy@...ux.vnet.ibm.com>,
Dipankar Sarma <dipankar@...ibm.com>,
Gautham R Shenoy <ego@...ibm.com>,
linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
aun@...ux.vnet.ibm.com
Subject: Re: [v4 PATCH 1/5]: cpuidle: Cleanup drivers/cpuidle/cpuidle.c
* Balbir Singh <balbir@...ux.vnet.ibm.com> [2009-09-01 22:58:25]:
> * Arun R B <arun@...ux.vnet.ibm.com> [2009-09-01 17:08:40]:
>
> > * Arun R Bharadwaj <arun@...ux.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.
>
> > Also remove unwanted functions cpuidle_[un]install_idle_handler,
> > cpuidle_kick_cpus()
> >
> > Signed-off-by: Arun R Bharadwaj <arun@...ux.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?
>
Just observed the use case for cpuidle_pause_and_lock/unlock.
It is not clear as to why we need to switch back to the old idle
handler and then again back to cpuidle's idle handler. Wouldn't it
make more sense to just register the idle handler when the first
cpuidle device is being registered and unregister the idle handler
when the last cpuidle device is unregistered?
--arun
>
> > @@ -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.
>
> > +}
> > /**
> > * 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists