linux-kernel - Re: [PATCH 2/4]: CPUIDLE: Introduce architecture independent cpuidle_pm

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <1251377607.18584.96.camel@twins>
Date:	Thu, 27 Aug 2009 14:53:27 +0200
From:	Peter Zijlstra <a.p.zijlstra@...llo.nl>
To:	arun@...ux.vnet.ibm.com
Cc:	Joel Schopp <jschopp@...tin.ibm.com>,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	Paul Mackerras <paulus@...ba.org>, Ingo Molnar <mingo@...e.hu>,
	Vaidyanathan Srinivasan <svaidy@...ux.vnet.ibm.com>,
	Dipankar Sarma <dipankar@...ibm.com>,
	Balbir Singh <balbir@...ux.vnet.ibm.com>,
	Gautham R Shenoy <ego@...ibm.com>,
	"Pallipadi, Venkatesh" <venkatesh.pallipadi@...el.com>,
	linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCH 2/4]: CPUIDLE: Introduce architecture independent
 cpuidle_pm_idle in drivers/cpuidle/cpuidle.c

On Thu, 2009-08-27 at 17:23 +0530, Arun R Bharadwaj wrote:
> * Arun R Bharadwaj <arun@...ux.vnet.ibm.com> [2009-08-27 17:19:08]:
> 
> Cpuidle infrastructure assumes pm_idle as the default idle routine.
> But, ppc_md.power_save is the default idle callback in case of pSeries.
> 
> So, create a more generic, architecture independent cpuidle_pm_idle
> function pointer in driver/cpuidle/cpuidle.c and allow the idle routines
> of architectures to be set to cpuidle_pm_idle.
> 
> Signed-off-by: Arun R Bharadwaj <arun@...ux.vnet.ibm.com>
> ---
>  drivers/cpuidle/cpuidle.c |   12 +++++++-----
>  include/linux/cpuidle.h   |    7 +++++++
>  2 files changed, 14 insertions(+), 5 deletions(-)
> 
> Index: linux.trees.git/drivers/cpuidle/cpuidle.c
> ===================================================================
> --- linux.trees.git.orig/drivers/cpuidle/cpuidle.c
> +++ linux.trees.git/drivers/cpuidle/cpuidle.c
> @@ -25,6 +25,7 @@ DEFINE_PER_CPU(struct cpuidle_device *, 
>  DEFINE_MUTEX(cpuidle_lock);
>  LIST_HEAD(cpuidle_detected_devices);
>  static void (*pm_idle_old)(void);
> +void (*cpuidle_pm_idle)(void);
>  
>  static int enabled_devices;
>  
> @@ -98,10 +99,10 @@ static void cpuidle_idle_call(void)
>   */
>  void cpuidle_install_idle_handler(void)
>  {
> -	if (enabled_devices && (pm_idle != cpuidle_idle_call)) {
> +	if (enabled_devices && (cpuidle_pm_idle != cpuidle_idle_call)) {
>  		/* Make sure all changes finished before we switch to new idle */
>  		smp_wmb();
> -		pm_idle = cpuidle_idle_call;
> +		cpuidle_pm_idle = cpuidle_idle_call;
>  	}
>  }
>  
> @@ -110,8 +111,9 @@ void cpuidle_install_idle_handler(void)
>   */
>  void cpuidle_uninstall_idle_handler(void)
>  {
> -	if (enabled_devices && pm_idle_old && (pm_idle != pm_idle_old)) {
> -		pm_idle = pm_idle_old;
> +	if (enabled_devices && pm_idle_old &&
> +			(cpuidle_pm_idle != pm_idle_old)) {
> +		cpuidle_pm_idle = pm_idle_old;
>  		cpuidle_kick_cpus();
>  	}
>  }
> @@ -382,7 +384,7 @@ static int __init cpuidle_init(void)
>  {
>  	int ret;
>  
> -	pm_idle_old = pm_idle;
> +	pm_idle_old = cpuidle_pm_idle;
>  
>  	ret = cpuidle_add_class_sysfs(&cpu_sysdev_class);
>  	if (ret)
> Index: linux.trees.git/include/linux/cpuidle.h
> ===================================================================
> --- linux.trees.git.orig/include/linux/cpuidle.h
> +++ linux.trees.git/include/linux/cpuidle.h
> @@ -188,4 +188,11 @@ static inline void cpuidle_unregister_go
>  #define CPUIDLE_DRIVER_STATE_START	0
>  #endif
>  
> +/*
> + * Idle callback used by cpuidle to call the cpuidle_idle_call().
> + * Platform drivers can use this to register to cpuidle's idle loop.
> + */
> +
> +extern void (*cpuidle_pm_idle)(void);
> +
>  #endif /* _LINUX_CPUIDLE_H */


I'm not quite seeing how this makes anything any better. Not we have 3
function pointers, where 1 should suffice.

/me wonders what's wrong with something like:

struct idle_func_desc {
	int		 power;
	int		 latency;
	void		 (*idle)(void);
	struct list_head list;
};

static void spin_idle(void)
{
	for (;;)
		cpu_relax();
}

static idle_func_desc default_idle_func = {
	power = 0, 	   /* doesn't safe any power */
	latency = INT_MAX, /* has max latency */
	idle = spin_idle,
	list = INIT_LIST_HEAD(default_idle_func.list),
};

void (*idle_func)(void);
static struct list_head idle_func_list;

static void pick_idle_func(void)
{
	struct idle_func_desc *desc, *idle = &default_idle_desc;

	list_for_each_entry(desc, &idle_func_list, list) {
		if (desc->power < idle->power)
			continue;
		if (desc->latency > target_latency);
			continue;
		idle = desc;
	}

	pm_idle = idle->idle;
}

void register_idle_func(struct idle_func_desc *desc)
{
	WARN_ON_ONCE(!list_empty(&desc->list));

	list_add_tail(&idle_func_list, &desc->list);
	pick_idle_func();
}

void unregister_idle_func(struct idle_func_desc *desc)
{
	WARN_ON_ONCE(list_empty(&desc->list));

	list_del_init(&desc->list);
	if (idle_func == desc->idle) 
		pick_idle_func();
}

--
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/