On 09/22/2013 03:21 AM, Viresh Kumar wrote:
> Currently we have multiple definitions of few routines based on following
> config
> option: CONFIG_CPU_IDLE_MULTIPLE_DRIVERS.
>
> These are present to save space by not creating per-cpu variable for platforms
> which need only one cpuidle driver to be registered for all CPUs.
>
> But this setup has a problem. For ARM multi-platform kernel use case this
> option
> will get enabled and so we will have per-cpu variables even for platforms that
> don't need it.
>
> The bigger problem is two separate code paths for such platforms for single &
> multi platform kernels. Which doesn't sound good.
>
> A better way of solving this problem would be to create cpuidle driver's list
> that can be used to manage all information we need. Then we don't really have
> to
> write any special code for handling platforms with
> CONFIG_CPU_IDLE_MULTIPLE_DRIVERS option set.
>
> This patch does it.
If you introduce a list, you will have to introduce a lock to protect
it. This lock will be in the fast path cpuidle_idle_call with the
get_driver function and conforming to the comment: "NOTE: no locks or
semaphores should be used here".
A lock has been introduced in this function already and the system hangs
with 1024 cpus.
> Signed-off-by: Viresh Kumar <viresh.ku...@linaro.org>
> ---
> drivers/cpuidle/driver.c | 106
> ++++++++++++-----------------------------------
> include/linux/cpuidle.h | 1 +
> 2 files changed, 27 insertions(+), 80 deletions(-)
>
> diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
> index a4a93b4..320b4ec 100644
> --- a/drivers/cpuidle/driver.c
> +++ b/drivers/cpuidle/driver.c
> @@ -18,10 +18,19 @@
> #include "cpuidle.h"
>
> DEFINE_SPINLOCK(cpuidle_driver_lock);
> +static LIST_HEAD(cpuidle_detected_drivers);
>
> -#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS
> +static inline struct cpuidle_driver *
> +__cpuidle_get_driver(const struct cpumask *cpumask)
> +{
> + struct cpuidle_driver *drv;
> +
> + list_for_each_entry(drv, &cpuidle_detected_drivers, driver_list)
> + if (cpumask_intersects(drv->cpumask, cpumask))
> + return drv;
>
> -static DEFINE_PER_CPU(struct cpuidle_driver *, cpuidle_drivers);
> + return NULL;
> +}
>
> /**
> * __cpuidle_get_cpu_driver - return the cpuidle driver tied to a CPU.
> @@ -32,103 +41,39 @@ static DEFINE_PER_CPU(struct cpuidle_driver *,
> cpuidle_drivers);
> */
> static inline struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu)
> {
> - return per_cpu(cpuidle_drivers, cpu);
> + return __cpuidle_get_driver(cpumask_of(cpu));
> }
>
> /**
> - * __cpuidle_unset_driver - unset per CPU driver variables.
> + * __cpuidle_add_driver - adds a cpuidle driver to list.
> * @drv: a valid pointer to a struct cpuidle_driver
> *
> - * For each CPU in the driver's CPU mask, unset the registered driver per CPU
> - * variable. If @drv is different from the registered driver, the
> corresponding
> - * variable is not cleared.
> - */
> -static inline void __cpuidle_unset_driver(struct cpuidle_driver *drv)
> -{
> - int cpu;
> -
> - for_each_cpu(cpu, drv->cpumask) {
> -
> - if (drv != __cpuidle_get_cpu_driver(cpu))
> - continue;
> -
> - per_cpu(cpuidle_drivers, cpu) = NULL;
> - }
> -}
> -
> -/**
> - * __cpuidle_set_driver - set per CPU driver variables for the given driver.
> - * @drv: a valid pointer to a struct cpuidle_driver
> - *
> - * For each CPU in the driver's cpumask, unset the registered driver per CPU
> - * to @drv.
> + * Adds cpuidle driver to cpuidle_detected_drivers list if no driver is
> already
> + * registered for any CPUs present in drv->cpumask.
> *
> * Returns 0 on success, -EBUSY if the CPUs have driver(s) already.
> */
> -static inline int __cpuidle_set_driver(struct cpuidle_driver *drv)
> -{
> - int cpu;
> -
> - for_each_cpu(cpu, drv->cpumask) {
> -
> - if (__cpuidle_get_cpu_driver(cpu)) {
> - __cpuidle_unset_driver(drv);
> - return -EBUSY;
> - }
> -
> - per_cpu(cpuidle_drivers, cpu) = drv;
> - }
> -
> - return 0;
> -}
> -
> -#else
> -
> -static struct cpuidle_driver *cpuidle_curr_driver;
> -
> -/**
> - * __cpuidle_get_cpu_driver - return the global cpuidle driver pointer.
> - * @cpu: ignored without the multiple driver support
> - *
> - * Return a pointer to a struct cpuidle_driver object or NULL if no driver
> was
> - * previously registered.
> - */
> -static inline struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu)
> -{
> - return cpuidle_curr_driver;
> -}
> -
> -/**
> - * __cpuidle_set_driver - assign the global cpuidle driver variable.
> - * @drv: pointer to a struct cpuidle_driver object
> - *
> - * Returns 0 on success, -EBUSY if the driver is already registered.
> - */
> -static inline int __cpuidle_set_driver(struct cpuidle_driver *drv)
> +static inline int __cpuidle_add_driver(struct cpuidle_driver *drv)
> {
> - if (cpuidle_curr_driver)
> + if (__cpuidle_get_driver(drv->cpumask))
> return -EBUSY;
>
> - cpuidle_curr_driver = drv;
> + list_add(&drv->driver_list, &cpuidle_detected_drivers);
>
> return 0;
> }
>
> /**
> - * __cpuidle_unset_driver - unset the global cpuidle driver variable.
> - * @drv: a pointer to a struct cpuidle_driver
> + * __cpuidle_remove_driver - remove cpuidle driver from list.
> + * @drv: a valid pointer to a struct cpuidle_driver
> *
> - * Reset the global cpuidle variable to NULL. If @drv does not match the
> - * registered driver, do nothing.
> + * Removes cpuidle driver from cpuidle_detected_drivers list.
> */
> -static inline void __cpuidle_unset_driver(struct cpuidle_driver *drv)
> +static inline void __cpuidle_remove_driver(struct cpuidle_driver *drv)
> {
> - if (drv == cpuidle_curr_driver)
> - cpuidle_curr_driver = NULL;
> + list_del(&drv->driver_list);
> }
>
> -#endif
> -
> /**
> * cpuidle_setup_broadcast_timer - enable/disable the broadcast timer
> * @arg: a void pointer used to match the SMP cross call API
> @@ -158,6 +103,7 @@ static void __cpuidle_driver_init(struct cpuidle_driver
> *drv)
> int i;
>
> drv->refcnt = 0;
> + INIT_LIST_HEAD(&drv->driver_list);
>
> /*
> * Use all possible CPUs as the default, because if the kernel boots
> @@ -244,7 +190,7 @@ static int __cpuidle_register_driver(struct
> cpuidle_driver *drv)
>
> __cpuidle_driver_init(drv);
>
> - ret = __cpuidle_set_driver(drv);
> + ret = __cpuidle_add_driver(drv);
> if (ret)
> return ret;
>
> @@ -277,7 +223,7 @@ static void __cpuidle_unregister_driver(struct
> cpuidle_driver *drv)
> (void *)CLOCK_EVT_NOTIFY_BROADCAST_OFF, 1);
> }
>
> - __cpuidle_unset_driver(drv);
> + __cpuidle_remove_driver(drv);
> }
>
> /**
> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> index 0f0da17..81b74d2 100644
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -129,6 +129,7 @@ struct cpuidle_driver {
>
> /* the driver handles the cpus in cpumask */
> struct cpumask *cpumask;
> + struct list_head driver_list;
> };
>
> #ifdef CONFIG_CPU_IDLE
>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
