[patch 0/4] dynamically create "cache" and "cpufreq" entries with CPU hotplug

[patch 0/4] dynamically create "cache" and "cpufreq" entries with CPU hotplug

[Ashok Raj]

Hi,

[Resend... somehow first post never made out of here..]

This fixes the 2 entries in /sys/devices/system/cpu/cpuX to be created when 
CPU is onlined, and removed when they are offlined.

Andrew: please consider for next -mm.


Cheers,
ashok

[patch 1/4] introduce get_cpu_sysdev() to retrieve a sysfs entry for a cpu.

[Ashok Raj]

[-- Attachment #1: get-cpu-sysdev --]
[-- Type: text/plain, Size: 2492 bytes --]

some modules creating sysfs entries under /sys/devices/system/cpu/cpuX/
need to know the parent sysfs entry to make devices under them. This will
just return the sysfs entry for a given cpu.

sysfs entries showing under each cpu sysfs can be easily created if such
entries can be created by registering a sysfs driver for cpuclass. The issue
is when the entry is created the CPU may not be online, hence we would need to 
defer the creation until the online notification comes.

Current users: cache entries for Intel CPU's and cpufreq subsystem.

Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
----------------------------------------------------
 drivers/base/cpu.c  |   17 +++++++++++++++++
 include/linux/cpu.h |    1 +
 2 files changed, 18 insertions(+)

Index: linux-2.6.14-rc4-mm1/drivers/base/cpu.c
===================================================================
--- linux-2.6.14-rc4-mm1.orig/drivers/base/cpu.c
+++ linux-2.6.14-rc4-mm1/drivers/base/cpu.c
@@ -9,6 +9,16 @@
 #include <linux/topology.h>
 #include <linux/device.h>
 
+static struct sys_device	*cpu_sys_devices[NR_CPUS];
+
+struct sys_device *get_cpu_sysdev(int cpu)
+{
+	if (cpu < NR_CPUS)
+		return cpu_sys_devices[cpu];
+	else
+		return NULL;
+}
+EXPORT_SYMBOL(get_cpu_sysdev);
 
 struct sysdev_class cpu_sysdev_class = {
 	set_kset_name("cpu"),
@@ -64,12 +74,15 @@ static void __devinit register_cpu_contr
 void unregister_cpu(struct cpu *cpu, struct node *root)
 {
 
+	int logical_cpu = cpu->sysdev.id;
+
 	if (root)
 		sysfs_remove_link(&root->sysdev.kobj,
 				  kobject_name(&cpu->sysdev.kobj));
 	sysdev_remove_file(&cpu->sysdev, &attr_online);
 
 	sysdev_unregister(&cpu->sysdev);
+	cpu_sys_devices[logical_cpu] = NULL;
 
 	return;
 }
@@ -102,6 +115,10 @@ int __devinit register_cpu(struct cpu *c
 					  kobject_name(&cpu->sysdev.kobj));
 	if (!error && !cpu->no_control)
 		register_cpu_control(cpu);
+
+	if (!error)
+		cpu_sys_devices[num] = &cpu->sysdev;
+
 	return error;
 }
 
Index: linux-2.6.14-rc4-mm1/include/linux/cpu.h
===================================================================
--- linux-2.6.14-rc4-mm1.orig/include/linux/cpu.h
+++ linux-2.6.14-rc4-mm1/include/linux/cpu.h
@@ -32,6 +32,7 @@ struct cpu {
 };
 
 extern int register_cpu(struct cpu *, int, struct node *);
+extern struct sys_device *get_cpu_sysdev(int cpu);
 #ifdef CONFIG_HOTPLUG_CPU
 extern void unregister_cpu(struct cpu *, struct node *);
 #endif

--

[patch 2/4] create and destroy cache sysfs entries based on cpu notifiers.

[Ashok Raj]

[-- Attachment #1: dynamic-sysfs-cache --]
[-- Type: text/plain, Size: 5151 bytes --]

cpu cache entries should be populated only when cpu is online and removed
when they are logically offlined. 

Without which entries are not removed when cpu is offlined, or dont 
appear when we boot with maxcpus=1 and then kick the rest of the cpus
via echo 1 to the sysfs online file.

- Changed __devinit to __cpuinit for consistency.
- Changed sysfs_driver_register to register_cpu_notifier.


Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
----------------------------------------------------------------
 arch/i386/kernel/cpu/intel_cacheinfo.c |   60 ++++++++++++++++++++++++---------
 1 files changed, 44 insertions(+), 16 deletions(-)

Index: linux-2.6.14-rc4-mm1/arch/i386/kernel/cpu/intel_cacheinfo.c
===================================================================
--- linux-2.6.14-rc4-mm1.orig/arch/i386/kernel/cpu/intel_cacheinfo.c
+++ linux-2.6.14-rc4-mm1/arch/i386/kernel/cpu/intel_cacheinfo.c
@@ -3,6 +3,7 @@
  *
  *      Changes:
  *      Venkatesh Pallipadi	: Adding cache identification through cpuid(4)
+ *		Ashok Raj <ashok.raj@intel.com>: Work with CPU hotplug infrastructure.
  */
 
 #include <linux/init.h>
@@ -29,7 +30,7 @@ struct _cache_table
 };
 
 /* all the cache descriptor types we care about (no TLB or trace cache entries) */
-static struct _cache_table cache_table[] __devinitdata =
+static struct _cache_table cache_table[] __cpuinitdata =
 {
 	{ 0x06, LVL_1_INST, 8 },	/* 4-way set assoc, 32 byte line size */
 	{ 0x08, LVL_1_INST, 16 },	/* 4-way set assoc, 32 byte line size */
@@ -120,7 +121,7 @@ struct _cpuid4_info {
 
 static unsigned short			num_cache_leaves;
 
-static int __devinit cpuid4_cache_lookup(int index, struct _cpuid4_info *this_leaf)
+static int __cpuinit cpuid4_cache_lookup(int index, struct _cpuid4_info *this_leaf)
 {
 	unsigned int		eax, ebx, ecx, edx;
 	union _cpuid4_leaf_eax	cache_eax;
@@ -155,7 +156,7 @@ static int __init find_num_cache_leaves(
 	return i;
 }
 
-unsigned int __devinit init_intel_cacheinfo(struct cpuinfo_x86 *c)
+unsigned int __cpuinit init_intel_cacheinfo(struct cpuinfo_x86 *c)
 {
 	unsigned int trace = 0, l1i = 0, l1d = 0, l2 = 0, l3 = 0; /* Cache sizes */
 	unsigned int new_l1d = 0, new_l1i = 0; /* Cache sizes from cpuid(4) */
@@ -290,7 +291,7 @@ static struct _cpuid4_info *cpuid4_info[
 #define CPUID4_INFO_IDX(x,y)    (&((cpuid4_info[x])[y]))
 
 #ifdef CONFIG_SMP
-static void __devinit cache_shared_cpu_map_setup(unsigned int cpu, int index)
+static void __cpuinit cache_shared_cpu_map_setup(unsigned int cpu, int index)
 {
 	struct _cpuid4_info	*this_leaf;
 	unsigned long num_threads_sharing;
@@ -323,7 +324,7 @@ static void free_cache_attributes(unsign
 	cpuid4_info[cpu] = NULL;
 }
 
-static int __devinit detect_cache_attributes(unsigned int cpu)
+static int __cpuinit detect_cache_attributes(unsigned int cpu)
 {
 	struct _cpuid4_info	*this_leaf;
 	unsigned long 		j;
@@ -500,7 +501,7 @@ static void cpuid4_cache_sysfs_exit(unsi
 	free_cache_attributes(cpu);
 }
 
-static int __devinit cpuid4_cache_sysfs_init(unsigned int cpu)
+static int __cpuinit cpuid4_cache_sysfs_init(unsigned int cpu)
 {
 
 	if (num_cache_leaves == 0)
@@ -531,7 +532,7 @@ err_out:
 }
 
 /* Add/Remove cache interface for CPU device */
-static int __devinit cache_add_dev(struct sys_device * sys_dev)
+static int __cpuinit cache_add_dev(struct sys_device * sys_dev)
 {
 	unsigned int cpu = sys_dev->id;
 	unsigned long i, j;
@@ -568,7 +569,7 @@ static int __devinit cache_add_dev(struc
 	return retval;
 }
 
-static int __devexit cache_remove_dev(struct sys_device * sys_dev)
+static void __cpuexit cache_remove_dev(struct sys_device * sys_dev)
 {
 	unsigned int cpu = sys_dev->id;
 	unsigned long i;
@@ -577,24 +578,51 @@ static int __devexit cache_remove_dev(st
 		kobject_unregister(&(INDEX_KOBJECT_PTR(cpu,i)->kobj));
 	kobject_unregister(cache_kobject[cpu]);
 	cpuid4_cache_sysfs_exit(cpu);
-	return 0;
+	return;
+}
+
+static int __cpuinit cacheinfo_cpu_callback(struct notifier_block *nfb,
+						unsigned long action, void *hcpu)
+{
+	unsigned int cpu = (unsigned long)hcpu;
+	struct sys_device *sys_dev;
+
+	sys_dev = get_cpu_sysdev(cpu);
+	switch (action) {
+		case CPU_ONLINE:
+			(void) cache_add_dev(sys_dev);
+			break;
+		case CPU_DEAD:
+			cache_remove_dev(sys_dev);
+			break;
+	}
+	return NOTIFY_OK;
 }
 
-static struct sysdev_driver cache_sysdev_driver = {
-	.add = cache_add_dev,
-	.remove = __devexit_p(cache_remove_dev),
+static struct notifier_block cacheinfo_cpu_notifier =
+{
+    .notifier_call = cacheinfo_cpu_callback,
 };
 
-/* Register/Unregister the cpu_cache driver */
-static int __devinit cache_register_driver(void)
+static int __cpuinit cache_sysfs_init(void)
 {
+	int i;
+
 	if (num_cache_leaves == 0)
 		return 0;
 
-	return sysdev_driver_register(&cpu_sysdev_class,&cache_sysdev_driver);
+	register_cpu_notifier(&cacheinfo_cpu_notifier);
+
+	for_each_online_cpu(i) {
+		cacheinfo_cpu_callback(&cacheinfo_cpu_notifier, CPU_ONLINE,
+			(void *)(long)i);
+	}
+
+
+	return 0;
 }
 
-device_initcall(cache_register_driver);
+device_initcall(cache_sysfs_init);
 
 #endif
 

--

[patch 3/4] Remove cpu_sys_devices in cpufreq subsystem.

[Ashok Raj]

[-- Attachment #1: remove-cpufreq-cpu-sysdev --]
[-- Type: text/plain, Size: 2933 bytes --]

cpu_sys_devices is redundant with the new API get_cpu_sysdev().
So nuking this usage since its not needed.

Depends on: get_cpu_sysdev() patch.

Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
------------------------------------------------------------
 drivers/cpufreq/cpufreq.c |   16 +++-------------
 1 files changed, 3 insertions(+), 13 deletions(-)

Index: linux-2.6.14-rc4-mm1/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-2.6.14-rc4-mm1.orig/drivers/cpufreq/cpufreq.c
+++ linux-2.6.14-rc4-mm1/drivers/cpufreq/cpufreq.c
@@ -36,13 +36,6 @@ static struct cpufreq_policy	*cpufreq_cp
 static DEFINE_SPINLOCK(cpufreq_driver_lock);
 
 
-/* we keep a copy of all ->add'ed CPU's struct sys_device here;
- * as it is only accessed in ->add and ->remove, no lock or reference
- * count is necessary.
- */
-static struct sys_device	*cpu_sys_devices[NR_CPUS];
-
-
 /* internal prototypes */
 static int __cpufreq_governor(struct cpufreq_policy *policy, unsigned int event);
 static void handle_update(void *data);
@@ -582,7 +575,6 @@ static int cpufreq_add_dev (struct sys_d
 	 * CPU because it is in the same boat. */
 	policy = cpufreq_cpu_get(cpu);
 	if (unlikely(policy)) {
-		cpu_sys_devices[cpu] = sys_dev;
 		dprintk("CPU already managed, adding link\n");
 		sysfs_create_link(&sys_dev->kobj, &policy->kobj, "cpufreq");
 		cpufreq_debug_enable_ratelimit();
@@ -656,7 +648,6 @@ static int cpufreq_add_dev (struct sys_d
 	}
 
 	module_put(cpufreq_driver->owner);
-	cpu_sys_devices[cpu] = sys_dev;
 	dprintk("initialization complete\n");
 	cpufreq_debug_enable_ratelimit();
 	
@@ -697,6 +688,7 @@ static int cpufreq_remove_dev (struct sy
 	unsigned int cpu = sys_dev->id;
 	unsigned long flags;
 	struct cpufreq_policy *data;
+	struct sys_device *cpu_sys_dev;
 #ifdef CONFIG_SMP
 	unsigned int j;
 #endif
@@ -709,7 +701,6 @@ static int cpufreq_remove_dev (struct sy
 
 	if (!data) {
 		spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
-		cpu_sys_devices[cpu] = NULL;
 		cpufreq_debug_enable_ratelimit();
 		return -EINVAL;
 	}
@@ -724,14 +715,12 @@ static int cpufreq_remove_dev (struct sy
 		dprintk("removing link\n");
 		spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
 		sysfs_remove_link(&sys_dev->kobj, "cpufreq");
-		cpu_sys_devices[cpu] = NULL;
 		cpufreq_cpu_put(data);
 		cpufreq_debug_enable_ratelimit();
 		return 0;
 	}
 #endif
 
-	cpu_sys_devices[cpu] = NULL;
 
 	if (!kobject_get(&data->kobj)) {
 		spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
@@ -760,7 +749,8 @@ static int cpufreq_remove_dev (struct sy
 			if (j == cpu)
 				continue;
 			dprintk("removing link for cpu %u\n", j);
-			sysfs_remove_link(&cpu_sys_devices[j]->kobj, "cpufreq");
+			cpu_sys_dev = get_cpu_sysdev(j);
+			sysfs_remove_link(&cpu_sys_dev->kobj, "cpufreq");
 			cpufreq_cpu_put(data);
 		}
 	}

--

[patch 4/4] create and destroy cpufreq sysfs entries based on cpu notifiers.

[Ashok Raj]

[-- Attachment #1: dynamic-sysfs-cpufreq --]
[-- Type: text/plain, Size: 8028 bytes --]

cpufreq entries in sysfs should only be populated when CPU is online state.
When we either boot with maxcpus=x and then boot the other cpus by 
echoing to sysfs online file, these entries should be created and destroyed
when CPU_DEAD is notified. Same treatement as cache entries under sysfs.

We place the processor in the lowest frequency, so hw managed P-State 
transitions can still work on the other threads to save power.

Primary goal was to just make these directories appear/disapper dynamically.

There is one in this patch i had to do, which i really dont like myself
but probably best if someone handling the cpufreq infrastructure could give
this code right treatment if this is not acceptable. I guess its probably
good for the first cut.

- Converting lock_cpu_hotplug()/unlock_cpu_hotplug() to disable/enable preempt.
  The locking was smack in the middle of the notification path, when the
  hotplug is already holding the lock. I tried another solution to avoid this
  so avoid taking locks if we know we are from notification path. The solution
  was getting very ugly and i decided this was probably good for this iteration
  until someone who understands cpufreq could do a better job than me.

Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>

----------------------------------------------------------
 drivers/cpufreq/cpufreq.c       |   68 +++++++++++++++++++++++++++++++++++++---
 drivers/cpufreq/cpufreq_stats.c |   42 ++++++++++++++++++++++--
 2 files changed, 102 insertions(+), 8 deletions(-)

Index: linux-2.6.14-rc4-mm1/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-2.6.14-rc4-mm1.orig/drivers/cpufreq/cpufreq.c
+++ linux-2.6.14-rc4-mm1/drivers/cpufreq/cpufreq.c
@@ -3,6 +3,9 @@
  *
  *  Copyright (C) 2001 Russell King
  *            (C) 2002 - 2003 Dominik Brodowski <linux@brodo.de>
+ *
+ *  Oct 2005 - Ashok Raj <ashok.raj@intel.com>
+ *         		Added handling for CPU hotplug
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -567,6 +570,9 @@ static int cpufreq_add_dev (struct sys_d
 	unsigned long flags;
 	unsigned int j;
 
+	if (cpu_is_offline(cpu))
+		return 0;
+
 	cpufreq_debug_disable_ratelimit();
 	dprintk("adding CPU %u\n", cpu);
 
@@ -672,7 +678,7 @@ err_out:
 
 nomem_out:
 	module_put(cpufreq_driver->owner);
- module_out:
+module_out:
 	cpufreq_debug_enable_ratelimit();
 	return ret;
 }
@@ -761,7 +767,6 @@ static int cpufreq_remove_dev (struct sy
 	down(&data->lock);
 	if (cpufreq_driver->target)
 		__cpufreq_governor(data, CPUFREQ_GOV_STOP);
-	cpufreq_driver->target = NULL;
 	up(&data->lock);
 
 	kobject_unregister(&data->kobj);
@@ -1108,12 +1113,26 @@ int __cpufreq_driver_target(struct cpufr
 			    unsigned int relation)
 {
 	int retval = -EINVAL;
-	lock_cpu_hotplug();
+
+	/*
+	 * Converted the lock_cpu_hotplug to preempt_disable()
+	 * and preempt enable. This is a bit kludgy and relies on
+	 * how cpu hotplug works. All we need is a gaurantee that cpu hotplug
+	 * wont make progress on any cpu. Once we do preempt_disable(), this
+	 * would ensure hotplug threads dont get on this cpu, thereby delaying
+	 * the cpu remove process.
+	 *
+	 * we removed the lock_cpu_hotplug since we need to call this function via
+	 * cpu hotplug callbacks, which result in locking the cpu hotplug
+	 * thread itself. Agree this is not very clean, cpufreq community
+	 * could improve this if required. - Ashok Raj <ashok.raj@intel.com>
+	 */
+	preempt_disable();
 	dprintk("target for CPU %u: %u kHz, relation %u\n", policy->cpu,
 		target_freq, relation);
 	if (cpu_online(policy->cpu) && cpufreq_driver->target)
 		retval = cpufreq_driver->target(policy, target_freq, relation);
-	unlock_cpu_hotplug();
+	preempt_enable();
 	return retval;
 }
 EXPORT_SYMBOL_GPL(__cpufreq_driver_target);
@@ -1405,6 +1424,45 @@ int cpufreq_update_policy(unsigned int c
 }
 EXPORT_SYMBOL(cpufreq_update_policy);
 
+static int __cpuinit cpufreq_cpu_callback(struct notifier_block *nfb,
+						unsigned long action, void *hcpu)
+{
+	unsigned int cpu = (unsigned long)hcpu;
+	struct cpufreq_policy *policy;
+	struct sys_device *sys_dev;
+
+	sys_dev = get_cpu_sysdev(cpu);
+
+	if (sys_dev) {
+		switch (action) {
+			case CPU_ONLINE:
+				(void) cpufreq_add_dev(sys_dev);
+				break;
+			case CPU_DOWN_PREPARE:
+				/*
+				 * We attempt to put this cpu in lowest frequency possible
+				 * before going down. This will permit hardware managed
+				 * P-State to switch other related threads to min or
+				 * higher speeds if possible.
+				 */
+				policy = cpufreq_cpu_data[cpu];
+				if (policy) {
+					cpufreq_driver_target(policy, policy->min,
+							CPUFREQ_RELATION_H);
+				}
+				break;
+			case CPU_DEAD:
+				(void) cpufreq_remove_dev(sys_dev);
+				break;
+		}
+	}
+	return NOTIFY_OK;
+}
+
+static struct notifier_block cpufreq_cpu_notifier =
+{
+    .notifier_call = cpufreq_cpu_callback,
+};
 
 /*********************************************************************
  *               REGISTER / UNREGISTER CPUFREQ DRIVER                *
@@ -1465,6 +1523,7 @@ int cpufreq_register_driver(struct cpufr
 	}
 
 	if (!ret) {
+		register_cpu_notifier(&cpufreq_cpu_notifier);
 		dprintk("driver %s up and running\n", driver_data->name);
 		cpufreq_debug_enable_ratelimit();
 	}
@@ -1496,6 +1555,7 @@ int cpufreq_unregister_driver(struct cpu
 	dprintk("unregistering driver %s\n", driver->name);
 
 	sysdev_driver_unregister(&cpu_sysdev_class, &cpufreq_sysdev_driver);
+	unregister_cpu_notifier(&cpufreq_cpu_notifier);
 
 	spin_lock_irqsave(&cpufreq_driver_lock, flags);
 	cpufreq_driver = NULL;
Index: linux-2.6.14-rc4-mm1/drivers/cpufreq/cpufreq_stats.c
===================================================================
--- linux-2.6.14-rc4-mm1.orig/drivers/cpufreq/cpufreq_stats.c
+++ linux-2.6.14-rc4-mm1/drivers/cpufreq/cpufreq_stats.c
@@ -19,6 +19,7 @@
 #include <linux/percpu.h>
 #include <linux/kobject.h>
 #include <linux/spinlock.h>
+#include <linux/notifier.h>
 #include <asm/cputime.h>
 
 static spinlock_t cpufreq_stats_lock;
@@ -296,6 +297,27 @@ cpufreq_stat_notifier_trans (struct noti
 	return 0;
 }
 
+static int __cpuinit cpufreq_stat_cpu_callback(struct notifier_block *nfb,
+						unsigned long action, void *hcpu)
+{
+	unsigned int cpu = (unsigned long)hcpu;
+
+	switch (action) {
+		case CPU_ONLINE:
+			cpufreq_update_policy(cpu);
+			break;
+		case CPU_DEAD:
+			cpufreq_stats_free_table(cpu);
+			break;
+	}
+	return NOTIFY_OK;
+}
+
+static struct notifier_block cpufreq_stat_cpu_notifier =
+{
+    .notifier_call = cpufreq_stat_cpu_callback,
+};
+
 static struct notifier_block notifier_policy_block = {
 	.notifier_call = cpufreq_stat_notifier_policy
 };
@@ -309,6 +331,7 @@ __init cpufreq_stats_init(void)
 {
 	int ret;
 	unsigned int cpu;
+
 	spin_lock_init(&cpufreq_stats_lock);
 	if ((ret = cpufreq_register_notifier(&notifier_policy_block,
 				CPUFREQ_POLICY_NOTIFIER)))
@@ -321,20 +344,31 @@ __init cpufreq_stats_init(void)
 		return ret;
 	}
 
-	for_each_cpu(cpu)
-		cpufreq_update_policy(cpu);
+	register_cpu_notifier(&cpufreq_stat_cpu_notifier);
+	lock_cpu_hotplug();
+	for_each_online_cpu(cpu) {
+		cpufreq_stat_cpu_callback(&cpufreq_stat_cpu_notifier, CPU_ONLINE,
+			(void *)(long)cpu);
+	}
+	unlock_cpu_hotplug();
 	return 0;
 }
 static void
 __exit cpufreq_stats_exit(void)
 {
 	unsigned int cpu;
+
 	cpufreq_unregister_notifier(&notifier_policy_block,
 			CPUFREQ_POLICY_NOTIFIER);
 	cpufreq_unregister_notifier(&notifier_trans_block,
 			CPUFREQ_TRANSITION_NOTIFIER);
-	for_each_cpu(cpu)
-		cpufreq_stats_free_table(cpu);
+	unregister_cpu_notifier(&cpufreq_stat_cpu_notifier);
+	lock_cpu_hotplug();
+	for_each_online_cpu(cpu) {
+		cpufreq_stat_cpu_callback(&cpufreq_stat_cpu_notifier, CPU_DEAD,
+			(void *)(long)cpu);
+	}
+	unlock_cpu_hotplug();
 }
 
 MODULE_AUTHOR ("Zou Nan hai <nanhai.zou@intel.com>");

--

Re: [patch 4/4] create and destroy cpufreq sysfs entries based on cpu notifiers.

[Nathan Lynch]

Hi Ashok,

Ashok Raj wrote:
> cpufreq entries in sysfs should only be populated when CPU is online state.
> When we either boot with maxcpus=x and then boot the other cpus by 
> echoing to sysfs online file, these entries should be created and destroyed
> when CPU_DEAD is notified. Same treatement as cache entries under sysfs.
> 
> We place the processor in the lowest frequency, so hw managed P-State 
> transitions can still work on the other threads to save power.
> 
> Primary goal was to just make these directories appear/disapper dynamically.

I see this patch series has already been merged, but in light of the
issues that it has caused[1], and the hack that Andrew is carrying to
deal with them[2], could we revisit the original justification for
these changes?

Why is it important that cpufreq-related files in sysfs be added and
removed as cpus go online and offline?  I see that the information
that these entries provide can be derived only when the cpu is online,
is that the primary justification?

Would it be undesirable for the cpufreq drivers to create their
entries under all cpu sysdevs at init time, regardless of whether the
cpus are online?  The "show" methods for entries attached to offline
cpus could be made to return "Unavailable" or some equivalent.

I'm not terribly familiar with x86 or cpufreq, so forgive me if I'm
missing something obvious.


[1] http://lkml.org/lkml/2005/10/31/144
[2] ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.14/2.6.14-mm1/broken-out/cpu-hotplug-fix-locking-in-cpufreq-drivers.patch

Re: [patch 4/4] create and destroy cpufreq sysfs entries based on cpu notifiers.

[Ashok Raj]

On Mon, Nov 07, 2005 at 05:41:44AM -0600, Nathan Lynch wrote:
> > 
> > Primary goal was to just make these directories appear/disapper dynamically.
> 
> I see this patch series has already been merged, but in light of the
> issues that it has caused[1], and the hack that Andrew is carrying to
> deal with them[2], could we revisit the original justification for
> these changes?

Agreed, the patch wasnt clean, just a sde effect that i didnt 
want to make very many changes in a subsystem to have to validate 
if the changes were extensive. So i took some shortcuts that made it 
ugly and now those reworks. (saying that is like beating the dead horse)

the problem isnt creating and destroyoing sysfs files, we need to put the 
cpu going away to the lowst possible freq state to save power etc which
was the code path that caused trouble.

> 
> Why is it important that cpufreq-related files in sysfs be added and
> removed as cpus go online and offline?  I see that the information
> that these entries provide can be derived only when the cpu is online,
> is that the primary justification?

There are just not for informational purposes, there are some files
that get created for control as well, say to set a cpu frequency.

Sure  you can do cpu_online() to each step, but iam sure that kind of looks
ugly workaround. There is no reason for them to exist when the cpu is offline.

The data also gets populated upfront at creation time. say if i start
with maxcpus=2, in a 4 cpu system, only 2 online cpus get their
cpufreq entries get created. since these are sysfs subdriver, when 
the sysfs entrries are created, the subdriver _add functions get called
for all present cpus, but the _add only succeeds for online cpus.

now we could re-organize all of that to just create the entries
and collect the data each time rather than just create them
when its possible, and remove them when not necessary anymore. 


the orign of the patch fiasco was since the lock was smack in the 
lowest function, that had about 14 different places it got called from.
So if i need to move the lock to higher caller, we need to properly
analyze which ones are truly higher level functions in cpufreq.

I wasnt motivated to that, not having understood the entire cpufreq mechanics
tried to solve the symtom instead.

> 
> Would it be undesirable for the cpufreq drivers to create their
> entries under all cpu sysdevs at init time, regardless of whether the
> cpus are online?  The "show" methods for entries attached to offline
> cpus could be made to return "Unavailable" or some equivalent.
> 
> I'm not terribly familiar with x86 or cpufreq, so forgive me if I'm
> missing something obvious.

Hope this helps.
> 
-- 
Cheers,
Ashok Raj
- Open Source Technology Center