[PATCH v2:3/3]Export cpu topology by sysfs

[PATCH v2:3/3]Export cpu topology by sysfs

[Yanmin Zhang]

From: Zhang, Yanmin <yanmin.zhang@intel.com>

The third patch implements the topology exportation by sysfs.

Items (attributes) are similar to /proc/cpuinfo.

1) /sys/devices/system/cpu/cpuX/topology/physical_package_id:
represent the physical package id of  cpu X;
2) /sys/devices/system/cpu/cpuX/topology/core_id:
represent the cpu core id to cpu X;
3) /sys/devices/system/cpu/cpuX/topology/thread_id:
represent the cpu thread id  to cpu X;
4) /sys/devices/system/cpu/cpuX/topology/thread_siblings:
represent the thread siblings to cpu X in the same core;
5) /sys/devices/system/cpu/cpuX/topology/core_siblings:
represent the thread siblings to cpu X in the same physical package;

If one architecture wants to support this feature, it just needs to
implement 5 defines, typically in file include/asm-XXX/topology.h.
The 5 defines are:
#define topology_physical_package_id(cpu)
#define topology_core_id(cpu)
#define topology_thread_id(cpu)
#define topology_thread_siblings(cpu)
#define topology_core_siblings(cpu)

The type of siblings is cpumask_t.

If an attribute isn't defined on an architecture, it won't be exported.

The patch against 2.6.15-rc5 provides defines for i386/x86_64/ia64.

Signed-off-by: Zhang, Yanmin <yanmin.zhang@intel.com>

#diffstat export_topology_2.6.15-rc5_v4.patch
 Documentation/cputopology.txt |   31 ++++++
 arch/ia64/kernel/topology.c   |    2
 drivers/base/Makefile         |    1
 drivers/base/topology.c       |  201 ++++++++++++++++++++++++++++++++++++++++++
 include/asm-i386/topology.h   |    8 +
 include/asm-ia64/topology.h   |    8 +
 include/asm-x86_64/topology.h |    8 +
 7 files changed, 258 insertions, 1 deletion

---

diff -Nraup linux-2.6.15-rc5/arch/ia64/kernel/topology.c linux-2.6.15-rc5_topology/arch/ia64/kernel/topology.c
--- linux-2.6.15-rc5/arch/ia64/kernel/topology.c	2005-11-07 02:34:06.000000000 +0800
+++ linux-2.6.15-rc5_topology/arch/ia64/kernel/topology.c	2005-12-14 21:20:54.000000000 +0800
@@ -98,4 +98,4 @@ out:
 	return err;
 }
 
-__initcall(topology_init);
+subsys_initcall(topology_init);
diff -Nraup linux-2.6.15-rc5/Documentation/cputopology.txt linux-2.6.15-rc5_topology/Documentation/cputopology.txt
--- linux-2.6.15-rc5/Documentation/cputopology.txt	1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.15-rc5_topology/Documentation/cputopology.txt	2005-12-20 17:55:26.000000000 +0800
@@ -0,0 +1,31 @@
+
+Export cpu topology info by sysfs. Items (attributes) are similar
+to /proc/cpuinfo.
+
+1) /sys/devices/system/cpu/cpuX/topology/physical_package_id:
+represent the physical package id of  cpu X;
+2) /sys/devices/system/cpu/cpuX/topology/core_id:
+represent the cpu core id to cpu X;
+3) /sys/devices/system/cpu/cpuX/topology/thread_id:
+represent the cpu thread id  to cpu X;
+4) /sys/devices/system/cpu/cpuX/topology/thread_siblings:
+represent the thread siblings to cpu X in the same core;
+5) /sys/devices/system/cpu/cpuX/topology/core_siblings:
+represent the thread siblings to cpu X in the same physical package;
+
+To implement it in an architecture-neutral way, a new source file,
+driver/base/topology.c, is to export the 5 attributes.
+
+If one architecture wants to support this feature, it just needs to
+implement 5 defines, typically in file include/asm-XXX/topology.h.
+The 5 defines are:
+#define topology_physical_package_id(cpu)
+#define topology_core_id(cpu)
+#define topology_thread_id(cpu)
+#define topology_thread_siblings(cpu)
+#define topology_core_siblings(cpu)
+
+The type of siblings is cpumask_t.
+
+If an attribute isn't defined on an architecture, it won't be exported.
+
diff -Nraup linux-2.6.15-rc5/drivers/base/Makefile linux-2.6.15-rc5_topology/drivers/base/Makefile
--- linux-2.6.15-rc5/drivers/base/Makefile	2005-12-13 23:07:35.000000000 +0800
+++ linux-2.6.15-rc5_topology/drivers/base/Makefile	2005-12-14 20:47:00.000000000 +0800
@@ -8,6 +8,7 @@ obj-y			+= power/
 obj-$(CONFIG_FW_LOADER)	+= firmware_class.o
 obj-$(CONFIG_NUMA)	+= node.o
 obj-$(CONFIG_MEMORY_HOTPLUG) += memory.o
+obj-$(CONFIG_SMP)	+= topology.o
 
 ifeq ($(CONFIG_DEBUG_DRIVER),y)
 EXTRA_CFLAGS += -DDEBUG
diff -Nraup linux-2.6.15-rc5/drivers/base/topology.c linux-2.6.15-rc5_topology/drivers/base/topology.c
--- linux-2.6.15-rc5/drivers/base/topology.c	1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.15-rc5_topology/drivers/base/topology.c	2005-12-20 00:33:49.000000000 +0800
@@ -0,0 +1,201 @@
+/*
+ * driver/base/topology.c - Populate driverfs with cpu topology information
+ *
+ * Written by: Zhang Yanmin, Intel Corporation
+ *
+ * Copyright (C) 2005, Intel Corp.
+ *
+ * All rights reserved.          
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
+ * NON INFRINGEMENT.  See the GNU General Public License for more
+ * details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ *
+ */
+#include <linux/sysdev.h>
+#include <linux/init.h>
+#include <linux/mm.h>
+#include <linux/cpu.h>
+#include <linux/module.h>
+#include <linux/topology.h>
+
+struct _topology_attr {
+	struct attribute attr;
+	ssize_t (*show)(int cpu, char *);
+	ssize_t (*store)(const char *, size_t count);
+};
+
+#define to_attr(a) container_of(a, struct _topology_attr, attr)
+
+/* pointer to kobject for cpuX/topology */
+static struct kobject cpu_topology_kobject[NR_CPUS];
+
+#define define_one_ro(_name) 		\
+static struct _topology_attr _name =	\
+	__ATTR(_name, 0444, show_##_name, NULL)
+
+#define define_id_show_func(name)				\
+static ssize_t show_##name(int cpu, char *buf)			\
+{								\
+	return sprintf(buf, "%d\n", topology_##name(cpu));	\
+}
+
+#define define_siblings_show_func(name)					\
+static ssize_t show_##name(int cpu, char *buf)				\
+{									\
+	ssize_t len = -1;						\
+	len = cpumask_scnprintf(buf, NR_CPUS+1, topology_##name(cpu));	\
+	return (len + sprintf(buf + len, "\n"));			\
+}
+
+#ifdef	topology_physical_package_id
+define_id_show_func(physical_package_id);
+define_one_ro(physical_package_id);
+#define ref_physical_package_id_attr	&physical_package_id.attr,
+#else
+#define ref_physical_package_id_attr
+#endif
+
+#ifdef topology_core_id
+define_id_show_func(core_id);
+define_one_ro(core_id);
+#define ref_core_id_attr		&core_id.attr,
+#else
+#define ref_core_id_attr
+#endif
+
+#ifdef topology_thread_id
+define_id_show_func(thread_id);
+define_one_ro(thread_id);
+#define ref_thread_id_attr		&thread_id.attr,
+#else
+#define ref_thread_id_attr
+#endif
+
+#ifdef topology_thread_siblings
+define_siblings_show_func(thread_siblings);
+define_one_ro(thread_siblings);
+#define ref_thread_siblings_attr	&thread_siblings.attr,
+#else
+#define ref_thread_siblings_attr
+#endif
+
+#ifdef topology_core_siblings
+define_siblings_show_func(core_siblings);
+define_one_ro(core_siblings);
+#define ref_core_siblings_attr		&core_siblings.attr,
+#else
+#define ref_core_siblings_attr
+#endif
+
+static struct attribute * topology_default_attrs[] = {
+	ref_physical_package_id_attr
+	ref_core_id_attr
+	ref_thread_id_attr
+	ref_thread_siblings_attr
+	ref_core_siblings_attr
+	NULL
+};
+
+static ssize_t topology_show(struct kobject * kobj, struct attribute * attr, char * buf)
+{
+	unsigned int cpu;
+        struct _topology_attr *fattr = to_attr(attr);
+        ssize_t ret;
+
+	cpu = container_of(kobj->parent, struct sys_device, kobj)->id;
+        ret = fattr->show ? fattr->show(cpu, buf): 0;
+        return ret;
+}
+
+static ssize_t topology_store(struct kobject * kobj, struct attribute * attr,
+		     const char * buf, size_t count)
+{
+	return 0;
+}
+
+static struct sysfs_ops topology_sysfs_ops = {
+	.show   = topology_show,
+	.store  = topology_store,
+};
+
+static struct kobj_type topology_ktype = {
+	.sysfs_ops	= &topology_sysfs_ops,
+	.default_attrs	= topology_default_attrs,
+};
+
+/* Add/Remove cpu_topology interface for CPU device */
+static int __cpuinit topology_add_dev(struct sys_device * sys_dev)
+{
+	unsigned int cpu = sys_dev->id;
+
+	memset(&cpu_topology_kobject[cpu], 0, sizeof(struct kobject));
+
+	cpu_topology_kobject[cpu].parent = &sys_dev->kobj;
+	kobject_set_name(&cpu_topology_kobject[cpu], "%s", "topology");
+	cpu_topology_kobject[cpu].ktype = &topology_ktype;
+
+	return  kobject_register(&cpu_topology_kobject[cpu]);
+}
+
+static int __cpuexit topology_remove_dev(struct sys_device * sys_dev)
+{
+	unsigned int cpu = sys_dev->id;
+
+	kobject_unregister(&cpu_topology_kobject[cpu]);
+
+	return 0;
+}
+
+static int __cpuinit topology_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:
+			topology_add_dev(sys_dev);
+			break;
+#ifdef	CONFIG_HOTPLUG_CPU
+		case CPU_DEAD:
+			topology_remove_dev(sys_dev);
+			break;
+#endif
+	}
+	return NOTIFY_OK;
+}
+
+static struct notifier_block topology_cpu_notifier =
+{
+	.notifier_call = topology_cpu_callback,
+};
+
+static int __cpuinit topology_sysfs_init(void)
+{
+	int i;
+
+	for_each_online_cpu(i) {
+		topology_cpu_callback(&topology_cpu_notifier, CPU_ONLINE,
+				(void *)(long)i);
+	}
+
+	register_cpu_notifier(&topology_cpu_notifier);
+
+	return 0;
+}
+
+device_initcall(topology_sysfs_init);
+
diff -Nraup linux-2.6.15-rc5/include/asm-i386/topology.h linux-2.6.15-rc5_topology/include/asm-i386/topology.h
--- linux-2.6.15-rc5/include/asm-i386/topology.h	2005-11-07 02:34:08.000000000 +0800
+++ linux-2.6.15-rc5_topology/include/asm-i386/topology.h	2005-12-15 19:15:47.000000000 +0800
@@ -27,6 +27,14 @@
 #ifndef _ASM_I386_TOPOLOGY_H
 #define _ASM_I386_TOPOLOGY_H
 
+#ifdef CONFIG_SMP
+#define topology_physical_package_id(cpu)	phys_proc_id[cpu]
+#define topology_core_id(cpu)			cpu_core_id[cpu]
+#define topology_thread_id(cpu)			cpu_thread_id[cpu]
+#define topology_core_siblings(cpu)		cpu_core_map[cpu]
+#define topology_thread_siblings(cpu)		cpu_sibling_map[cpu]
+#endif
+
 #ifdef CONFIG_NUMA
 
 #include <asm/mpspec.h>
diff -Nraup linux-2.6.15-rc5/include/asm-ia64/topology.h linux-2.6.15-rc5_topology/include/asm-ia64/topology.h
--- linux-2.6.15-rc5/include/asm-ia64/topology.h	2005-11-07 02:34:08.000000000 +0800
+++ linux-2.6.15-rc5_topology/include/asm-ia64/topology.h	2005-12-14 23:21:58.000000000 +0800
@@ -100,6 +100,14 @@ void build_cpu_to_node_map(void);
 
 #endif /* CONFIG_NUMA */
 
+#ifdef CONFIG_SMP
+#define topology_physical_package_id(cpu)	cpu_data(cpu)->socket_id
+#define topology_core_id(cpu)		cpu_data(cpu)->core_id
+#define topology_thread_id(cpu)		cpu_data(cpu)->thread_id
+#define topology_core_siblings(cpu)		cpu_core_map[cpu]
+#define topology_thread_siblings(cpu)	cpu_sibling_map[cpu]
+#endif
+
 #include <asm-generic/topology.h>
 
 #endif /* _ASM_IA64_TOPOLOGY_H */
diff -Nraup linux-2.6.15-rc5/include/asm-x86_64/topology.h linux-2.6.15-rc5_topology/include/asm-x86_64/topology.h
--- linux-2.6.15-rc5/include/asm-x86_64/topology.h	2005-12-13 23:07:39.000000000 +0800
+++ linux-2.6.15-rc5_topology/include/asm-x86_64/topology.h	2005-12-15 19:16:05.000000000 +0800
@@ -58,6 +58,14 @@ extern int __node_distance(int, int);
 
 #endif
 
+#ifdef CONFIG_SMP
+#define topology_physical_package_id(cpu)	phys_proc_id[cpu]
+#define topology_core_id(cpu)			cpu_core_id[cpu]
+#define topology_thread_id(cpu)			cpu_thread_id[cpu]
+#define topology_core_siblings(cpu)		cpu_core_map[cpu]
+#define topology_thread_siblings(cpu)		cpu_sibling_map[cpu]
+#endif
+
 #include <asm-generic/topology.h>
 
 #endif

Re: [PATCH v2:3/3]Export cpu topology by sysfs

[Greg KH]

On Tue, Dec 20, 2005 at 06:09:29PM -0800, Yanmin Zhang wrote:
> --- linux-2.6.15-rc5/drivers/base/topology.c	1970-01-01 08:00:00.000000000 +0800
> +++ linux-2.6.15-rc5_topology/drivers/base/topology.c	2005-12-20 00:33:49.000000000 +0800
> @@ -0,0 +1,201 @@
> +/*
> + * driver/base/topology.c - Populate driverfs with cpu topology information

driverfs?  Where did you cut/paste this code from?

And why does it have to be in driver/base?

> +struct _topology_attr {
> +	struct attribute attr;
> +	ssize_t (*show)(int cpu, char *);
> +	ssize_t (*store)(const char *, size_t count);
> +};

Don't put a '_' at the beginning of a structure please.

> +static ssize_t topology_show(struct kobject * kobj, struct attribute * attr, char * buf)
> +{
> +	unsigned int cpu;
> +        struct _topology_attr *fattr = to_attr(attr);
> +        ssize_t ret;
> +
> +	cpu = container_of(kobj->parent, struct sys_device, kobj)->id;
> +        ret = fattr->show ? fattr->show(cpu, buf): 0;
> +        return ret;
> +}

Mix of spaces and tabs :(

> +static ssize_t topology_store(struct kobject * kobj, struct attribute * attr,
> +		     const char * buf, size_t count)
> +{
> +	return 0;
> +}

Why is this function even needed?

> +static struct sysfs_ops topology_sysfs_ops = {
> +	.show   = topology_show,
> +	.store  = topology_store,
> +};
> +
> +static struct kobj_type topology_ktype = {
> +	.sysfs_ops	= &topology_sysfs_ops,
> +	.default_attrs	= topology_default_attrs,
> +};
> +
> +/* Add/Remove cpu_topology interface for CPU device */
> +static int __cpuinit topology_add_dev(struct sys_device * sys_dev)
> +{
> +	unsigned int cpu = sys_dev->id;
> +
> +	memset(&cpu_topology_kobject[cpu], 0, sizeof(struct kobject));
> +
> +	cpu_topology_kobject[cpu].parent = &sys_dev->kobj;
> +	kobject_set_name(&cpu_topology_kobject[cpu], "%s", "topology");
> +	cpu_topology_kobject[cpu].ktype = &topology_ktype;
> +
> +	return  kobject_register(&cpu_topology_kobject[cpu]);
> +}

Can't you just use an attribute group and attach it to the cpu kobject?
That would save an array of kobjects I think.

> +static int __cpuinit topology_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:
> +			topology_add_dev(sys_dev);
> +			break;
> +#ifdef	CONFIG_HOTPLUG_CPU
> +		case CPU_DEAD:
> +			topology_remove_dev(sys_dev);
> +			break;
> +#endif

Why ifdef?  Isn't it safe to just always have this in?

thanks,

greg k-h

RE: [PATCH v2:3/3]Export cpu topology by sysfs

[Zhang, Yanmin]

>>-----Original Message-----
>>From: Greg KH [mailto:greg@kroah.com]
>>Sent: 2005年12月23日 8:17
>>To: Yanmin Zhang
>>Cc: linux-kernel@vger.kernel.org; discuss@x86-64.org; linux-ia64@vger.kernel.org; Zhang, Yanmin; Siddha, Suresh B; Shah, Rajesh;
>>Pallipadi, Venkatesh
>>Subject: Re: [PATCH v2:3/3]Export cpu topology by sysfs
>>
>>On Tue, Dec 20, 2005 at 06:09:29PM -0800, Yanmin Zhang wrote:
>>> --- linux-2.6.15-rc5/drivers/base/topology.c	1970-01-01 08:00:00.000000000 +0800
>>> +++ linux-2.6.15-rc5_topology/drivers/base/topology.c	2005-12-20 00:33:49.000000000 +0800
>>> @@ -0,0 +1,201 @@
>>> +/*
>>> + * driver/base/topology.c - Populate driverfs with cpu topology information
>>
>>driverfs?  Where did you cut/paste this code from?
>>
>>And why does it have to be in driver/base?
I copied it from driver/base/node.c which implements node exportation by sysfs. I will change it. 
I couldn't find a better place than driver/base.


>>
>>> +struct _topology_attr {
>>> +	struct attribute attr;
>>> +	ssize_t (*show)(int cpu, char *);
>>> +	ssize_t (*store)(const char *, size_t count);
>>> +};
>>
>>Don't put a '_' at the beginning of a structure please.
I will change it.

>>
>>> +static ssize_t topology_show(struct kobject * kobj, struct attribute * attr, char * buf)
>>> +{
>>> +	unsigned int cpu;
>>> +        struct _topology_attr *fattr = to_attr(attr);
>>> +        ssize_t ret;
>>> +
>>> +	cpu = container_of(kobj->parent, struct sys_device, kobj)->id;
>>> +        ret = fattr->show ? fattr->show(cpu, buf): 0;
>>> +        return ret;
>>> +}
>>
>>Mix of spaces and tabs :(
It's a stupid problem. I will fix it.


>>
>>> +static ssize_t topology_store(struct kobject * kobj, struct attribute * attr,
>>> +		     const char * buf, size_t count)
>>> +{
>>> +	return 0;
>>> +}
>>
>>Why is this function even needed?
It's not needed and will be deleted.


>>
>>> +static struct sysfs_ops topology_sysfs_ops = {
>>> +	.show   = topology_show,
>>> +	.store  = topology_store,
>>> +};
>>> +
>>> +static struct kobj_type topology_ktype = {
>>> +	.sysfs_ops	= &topology_sysfs_ops,
>>> +	.default_attrs	= topology_default_attrs,
>>> +};
>>> +
>>> +/* Add/Remove cpu_topology interface for CPU device */
>>> +static int __cpuinit topology_add_dev(struct sys_device * sys_dev)
>>> +{
>>> +	unsigned int cpu = sys_dev->id;
>>> +
>>> +	memset(&cpu_topology_kobject[cpu], 0, sizeof(struct kobject));
>>> +
>>> +	cpu_topology_kobject[cpu].parent = &sys_dev->kobj;
>>> +	kobject_set_name(&cpu_topology_kobject[cpu], "%s", "topology");
>>> +	cpu_topology_kobject[cpu].ktype = &topology_ktype;
>>> +
>>> +	return  kobject_register(&cpu_topology_kobject[cpu]);
>>> +}
>>
>>Can't you just use an attribute group and attach it to the cpu kobject?
>>That would save an array of kobjects I think.
As you know, current i386/x86_64 arch also export cache info under /sys/device/system/cpu/cpuX/cache. Is it clearer to export topology under a new directory than under cpu directly?


>>
>>> +static int __cpuinit topology_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:
>>> +			topology_add_dev(sys_dev);
>>> +			break;
>>> +#ifdef	CONFIG_HOTPLUG_CPU
>>> +		case CPU_DEAD:
>>> +			topology_remove_dev(sys_dev);
>>> +			break;
>>> +#endif
>>
>>Why ifdef?  Isn't it safe to just always have this in?
If no ifdef here, gcc reported a compiling warning when I compiled it on IA64 with CONFIG_HOTPLUG_CPU=n.


>>
>>thanks,
>>
>>greg k-h
Thank you for the good comments. I will work out a new patch.

Re: [PATCH v2:3/3]Export cpu topology by sysfs

[Greg KH]

On Fri, Dec 23, 2005 at 12:03:27PM +0800, Zhang, Yanmin wrote:
> >>> +static struct sysfs_ops topology_sysfs_ops = {
> >>> +	.show   = topology_show,
> >>> +	.store  = topology_store,
> >>> +};
> >>> +
> >>> +static struct kobj_type topology_ktype = {
> >>> +	.sysfs_ops	= &topology_sysfs_ops,
> >>> +	.default_attrs	= topology_default_attrs,
> >>> +};
> >>> +
> >>> +/* Add/Remove cpu_topology interface for CPU device */
> >>> +static int __cpuinit topology_add_dev(struct sys_device * sys_dev)
> >>> +{
> >>> +	unsigned int cpu = sys_dev->id;
> >>> +
> >>> +	memset(&cpu_topology_kobject[cpu], 0, sizeof(struct kobject));
> >>> +
> >>> +	cpu_topology_kobject[cpu].parent = &sys_dev->kobj;
> >>> +	kobject_set_name(&cpu_topology_kobject[cpu], "%s", "topology");
> >>> +	cpu_topology_kobject[cpu].ktype = &topology_ktype;
> >>> +
> >>> +	return  kobject_register(&cpu_topology_kobject[cpu]);
> >>> +}
> >>
> >>Can't you just use an attribute group and attach it to the cpu kobject?
> >>That would save an array of kobjects I think.
> As you know, current i386/x86_64 arch also export cache info under
> /sys/device/system/cpu/cpuX/cache. Is it clearer to export topology
> under a new directory than under cpu directly?

No, the place in the sysfs tree you are putting this is just fine.  I'm
just saying that you do not need to create a new kobject for these
attributes.  Just use an attribute group, and you will get the same
naming, without the need for an extra kobject (and the whole array of
kobjects) at all.

Does that make more sense?

> >>> +static int __cpuinit topology_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:
> >>> +			topology_add_dev(sys_dev);
> >>> +			break;
> >>> +#ifdef	CONFIG_HOTPLUG_CPU
> >>> +		case CPU_DEAD:
> >>> +			topology_remove_dev(sys_dev);
> >>> +			break;
> >>> +#endif
> >>
> >>Why ifdef?  Isn't it safe to just always have this in?
> If no ifdef here, gcc reported a compiling warning when I compiled it
> on IA64 with CONFIG_HOTPLUG_CPU=n.

Then you should probably go change it so that CPU_DEAD is defined on
non-smp builds, otherwise the code gets quite messy like the above :)

thanks,

greg k-h

RE: [PATCH v2:3/3]Export cpu topology by sysfs

[Zhang, Yanmin]

>>-----Original Message-----
>>From: Greg KH [mailto:greg@kroah.com]
>>Sent: 2005年12月24日 3:16
>>To: Zhang, Yanmin
>>Cc: Yanmin Zhang; linux-kernel@vger.kernel.org; discuss@x86-64.org; linux-ia64@vger.kernel.org; Siddha, Suresh B; Shah, Rajesh;
>>Pallipadi, Venkatesh
>>Subject: Re: [PATCH v2:3/3]Export cpu topology by sysfs
>>
>>On Fri, Dec 23, 2005 at 12:03:27PM +0800, Zhang, Yanmin wrote:
>>> >>Can't you just use an attribute group and attach it to the cpu kobject?
>>> >>That would save an array of kobjects I think.
>>> As you know, current i386/x86_64 arch also export cache info under
>>> /sys/device/system/cpu/cpuX/cache. Is it clearer to export topology
>>> under a new directory than under cpu directly?
>>
>>No, the place in the sysfs tree you are putting this is just fine.  I'm
>>just saying that you do not need to create a new kobject for these
>>attributes.  Just use an attribute group, and you will get the same
>>naming, without the need for an extra kobject (and the whole array of
>>kobjects) at all.
>>
>>Does that make more sense?
Yes, indeed. Now, I used your idea and the patch became simpler. Thanks.


>>
>>> >>> +static int __cpuinit topology_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:
>>> >>> +			topology_add_dev(sys_dev);
>>> >>> +			break;
>>> >>> +#ifdef	CONFIG_HOTPLUG_CPU
>>> >>> +		case CPU_DEAD:
>>> >>> +			topology_remove_dev(sys_dev);
>>> >>> +			break;
>>> >>> +#endif
>>> >>
>>> >>Why ifdef?  Isn't it safe to just always have this in?
>>> If no ifdef here, gcc reported a compiling warning when I compiled it
>>> on IA64 with CONFIG_HOTPLUG_CPU=n.
>>
>>Then you should probably go change it so that CPU_DEAD is defined on
>>non-smp builds, otherwise the code gets quite messy like the above :)

Sorry. My previous explanation is confusing. It's a link warning on ia64. On ia64, the kernel vmlinux doesn't include section .exit.text, so ld will report a link warning when a function is in section .exit.text and another function (not in .exit.text) calls the first one. When CONFIG_HOTPLUG_CPU=n, function topology_remove_dev is in section .exit.text, but its caller topology_remove_dev is not in .exit.text.

i386 and x86_64 kernel vmlinux does include .exit.text and discard it only when running, so there is no such warning on i386/x86_64.

There is no other better approach to get rid of the warning unless we change arch/ia64/kernel/vmlinux.lds.S to keep all .exit.text in kernel image.

Re: [PATCH v2:3/3]Export cpu topology by sysfs

[Yanmin Zhang]

On Mon, Dec 26, 2005 at 03:38:16PM +0800, Zhang, Yanmin wrote:
> >>-----Original Message-----
> >>From: Greg KH [mailto:greg@kroah.com]
> >>Sent: 2005Äê12ÔÂ24ÈÕ 3:16
> >>To: Zhang, Yanmin
> >>Cc: Yanmin Zhang; linux-kernel@vger.kernel.org; discuss@x86-64.org; linux-ia64@vger.kernel.org; Siddha, Suresh B; Shah, Rajesh;
> >>Pallipadi, Venkatesh
> >>Subject: Re: [PATCH v2:3/3]Export cpu topology by sysfs
> >>
> >>On Fri, Dec 23, 2005 at 12:03:27PM +0800, Zhang, Yanmin wrote:
> >>> >>Can't you just use an attribute group and attach it to the cpu kobject?
> >>> >>That would save an array of kobjects I think.
> >>> As you know, current i386/x86_64 arch also export cache info under
> >>> /sys/device/system/cpu/cpuX/cache. Is it clearer to export topology
> >>> under a new directory than under cpu directly?
> >>
> >>No, the place in the sysfs tree you are putting this is just fine.  I'm
> >>just saying that you do not need to create a new kobject for these
> >>attributes.  Just use an attribute group, and you will get the same
> >>naming, without the need for an extra kobject (and the whole array of
> >>kobjects) at all.
> >>
> >>Does that make more sense?
> Yes, indeed. Now, I used your idea and the patch became simpler. Thanks.
> 
> 
> >>
> >>> >>> +static int __cpuinit topology_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:
> >>> >>> +			topology_add_dev(sys_dev);
> >>> >>> +			break;
> >>> >>> +#ifdef	CONFIG_HOTPLUG_CPU
> >>> >>> +		case CPU_DEAD:
> >>> >>> +			topology_remove_dev(sys_dev);
> >>> >>> +			break;
> >>> >>> +#endif
> >>> >>
> >>> >>Why ifdef?  Isn't it safe to just always have this in?
> >>> If no ifdef here, gcc reported a compiling warning when I compiled it
> >>> on IA64 with CONFIG_HOTPLUG_CPU=n.
> >>
> >>Then you should probably go change it so that CPU_DEAD is defined on
> >>non-smp builds, otherwise the code gets quite messy like the above :)
> 
> Sorry. My previous explanation is confusing. It's a link warning on ia64. On ia64, the kernel vmlinux doesn't include section .exit.text, so ld will report a link warning when a function is in section .exit.text and another function (not in .exit.text) calls the first one. When CONFIG_HOTPLUG_CPU=n, function topology_remove_dev is in section .exit.text, but its caller topology_remove_dev is not in .exit.text.
> 
> i386 and x86_64 kernel vmlinux does include .exit.text and discard it only when running, so there is no such warning on i386/x86_64.
> 
> There is no other better approach to get rid of the warning unless we change arch/ia64/kernel/vmlinux.lds.S to keep all .exit.text in kernel image.

Here is the new patch. Thank Greg.

---

diff -Nraup linux-2.6.15_rc3/arch/ia64/kernel/topology.c linux-2.6.15_rc3_topology/arch/ia64/kernel/topology.c
--- linux-2.6.15_rc3/arch/ia64/kernel/topology.c	2001-06-06 08:57:20.000000000 +0800
+++ linux-2.6.15_rc3_topology/arch/ia64/kernel/topology.c	2001-06-06 08:55:35.000000000 +0800
@@ -98,4 +98,4 @@ out:
 	return err;
 }
 
-__initcall(topology_init);
+subsys_initcall(topology_init);
diff -Nraup linux-2.6.15_rc3/Documentation/cputopology.txt linux-2.6.15_rc3_topology/Documentation/cputopology.txt
--- linux-2.6.15_rc3/Documentation/cputopology.txt	1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.15_rc3_topology/Documentation/cputopology.txt	2001-06-06 08:55:35.000000000 +0800
@@ -0,0 +1,31 @@
+
+Export cpu topology info by sysfs. Items (attributes) are similar
+to /proc/cpuinfo.
+
+1) /sys/devices/system/cpu/cpuX/topology/physical_package_id:
+represent the physical package id of  cpu X;
+2) /sys/devices/system/cpu/cpuX/topology/core_id:
+represent the cpu core id to cpu X;
+3) /sys/devices/system/cpu/cpuX/topology/thread_id:
+represent the cpu thread id  to cpu X;
+4) /sys/devices/system/cpu/cpuX/topology/thread_siblings:
+represent the thread siblings to cpu X in the same core;
+5) /sys/devices/system/cpu/cpuX/topology/core_siblings:
+represent the thread siblings to cpu X in the same physical package;
+
+To implement it in an architecture-neutral way, a new source file,
+driver/base/topology.c, is to export the 5 attributes.
+
+If one architecture wants to support this feature, it just needs to
+implement 5 defines, typically in file include/asm-XXX/topology.h.
+The 5 defines are:
+#define topology_physical_package_id(cpu)
+#define topology_core_id(cpu)
+#define topology_thread_id(cpu)
+#define topology_thread_siblings(cpu)
+#define topology_core_siblings(cpu)
+
+The type of siblings is cpumask_t.
+
+If an attribute isn't defined on an architecture, it won't be exported.
+
diff -Nraup linux-2.6.15_rc3/drivers/base/Makefile linux-2.6.15_rc3_topology/drivers/base/Makefile
--- linux-2.6.15_rc3/drivers/base/Makefile	2001-06-06 08:57:20.000000000 +0800
+++ linux-2.6.15_rc3_topology/drivers/base/Makefile	2001-06-06 08:55:35.000000000 +0800
@@ -8,6 +8,7 @@ obj-y			+= power/
 obj-$(CONFIG_FW_LOADER)	+= firmware_class.o
 obj-$(CONFIG_NUMA)	+= node.o
 obj-$(CONFIG_MEMORY_HOTPLUG) += memory.o
+obj-$(CONFIG_SMP)	+= topology.o
 
 ifeq ($(CONFIG_DEBUG_DRIVER),y)
 EXTRA_CFLAGS += -DDEBUG
diff -Nraup linux-2.6.15_rc3/drivers/base/topology.c linux-2.6.15_rc3_topology/drivers/base/topology.c
--- linux-2.6.15_rc3/drivers/base/topology.c	1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.15_rc3_topology/drivers/base/topology.c	2001-06-06 09:01:30.000000000 +0800
@@ -0,0 +1,159 @@
+/*
+ * driver/base/topology.c - Populate sysfs with cpu topology information
+ *
+ * Written by: Zhang Yanmin, Intel Corporation
+ *
+ * Copyright (C) 2005, Intel Corp.
+ *
+ * All rights reserved.          
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
+ * NON INFRINGEMENT.  See the GNU General Public License for more
+ * details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ *
+ */
+#include <linux/sysdev.h>
+#include <linux/init.h>
+#include <linux/mm.h>
+#include <linux/cpu.h>
+#include <linux/module.h>
+#include <linux/topology.h>
+
+#define define_one_ro(_name) 		\
+static SYSDEV_ATTR(_name, 0444, show_##_name, NULL)
+
+#define define_id_show_func(name)				\
+static ssize_t show_##name(struct sys_device *dev, char *buf)	\
+{								\
+	unsigned int cpu = dev->id;				\
+	return sprintf(buf, "%d\n", topology_##name(cpu));	\
+}
+
+#define define_siblings_show_func(name)					\
+static ssize_t show_##name(struct sys_device *dev, char *buf)		\
+{									\
+	ssize_t len = -1;						\
+	unsigned int cpu = dev->id;					\
+	len = cpumask_scnprintf(buf, NR_CPUS+1, topology_##name(cpu));	\
+	return (len + sprintf(buf + len, "\n"));			\
+}
+
+#ifdef	topology_physical_package_id
+define_id_show_func(physical_package_id);
+define_one_ro(physical_package_id);
+#define ref_physical_package_id_attr	&attr_physical_package_id.attr,
+#else
+#define ref_physical_package_id_attr
+#endif
+
+#ifdef topology_core_id
+define_id_show_func(core_id);
+define_one_ro(core_id);
+#define ref_core_id_attr		&attr_core_id.attr,
+#else
+#define ref_core_id_attr
+#endif
+
+#ifdef topology_thread_id
+define_id_show_func(thread_id);
+define_one_ro(thread_id);
+#define ref_thread_id_attr		&attr_thread_id.attr,
+#else
+#define ref_thread_id_attr
+#endif
+
+#ifdef topology_thread_siblings
+define_siblings_show_func(thread_siblings);
+define_one_ro(thread_siblings);
+#define ref_thread_siblings_attr	&attr_thread_siblings.attr,
+#else
+#define ref_thread_siblings_attr
+#endif
+
+#ifdef topology_core_siblings
+define_siblings_show_func(core_siblings);
+define_one_ro(core_siblings);
+#define ref_core_siblings_attr		&attr_core_siblings.attr,
+#else
+#define ref_core_siblings_attr
+#endif
+
+static struct attribute * default_attrs[] = {
+	ref_physical_package_id_attr
+	ref_core_id_attr
+	ref_thread_id_attr
+	ref_thread_siblings_attr
+	ref_core_siblings_attr
+	NULL
+};
+
+static struct attribute_group topology_attr_group = {
+	.attrs = default_attrs,
+	.name = "topology"
+};
+
+/* Add/Remove cpu_topology interface for CPU device */
+static int __cpuinit topology_add_dev(struct sys_device * sys_dev)
+{
+	sysfs_create_group(&sys_dev->kobj, &topology_attr_group);
+	return 0;
+}
+
+static int __cpuexit topology_remove_dev(struct sys_device * sys_dev)
+{
+	sysfs_remove_group(&sys_dev->kobj, &topology_attr_group);
+	return 0;
+}
+
+static int __cpuinit topology_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:
+			topology_add_dev(sys_dev);
+			break;
+#ifdef	CONFIG_HOTPLUG_CPU
+		case CPU_DEAD:
+			topology_remove_dev(sys_dev);
+			break;
+#endif
+	}
+	return NOTIFY_OK;
+}
+
+static struct notifier_block topology_cpu_notifier =
+{
+	.notifier_call = topology_cpu_callback,
+};
+
+static int __cpuinit topology_sysfs_init(void)
+{
+	int i;
+
+	for_each_online_cpu(i) {
+		topology_cpu_callback(&topology_cpu_notifier, CPU_ONLINE,
+				(void *)(long)i);
+	}
+
+	register_cpu_notifier(&topology_cpu_notifier);
+
+	return 0;
+}
+
+device_initcall(topology_sysfs_init);
+
diff -Nraup linux-2.6.15_rc3/include/asm-i386/topology.h linux-2.6.15_rc3_topology/include/asm-i386/topology.h
--- linux-2.6.15_rc3/include/asm-i386/topology.h	2001-06-06 08:57:20.000000000 +0800
+++ linux-2.6.15_rc3_topology/include/asm-i386/topology.h	2001-06-06 08:55:35.000000000 +0800
@@ -27,6 +27,14 @@
 #ifndef _ASM_I386_TOPOLOGY_H
 #define _ASM_I386_TOPOLOGY_H
 
+#ifdef CONFIG_SMP
+#define topology_physical_package_id(cpu)	phys_proc_id[cpu]
+#define topology_core_id(cpu)			cpu_core_id[cpu]
+#define topology_thread_id(cpu)			cpu_thread_id[cpu]
+#define topology_core_siblings(cpu)		cpu_core_map[cpu]
+#define topology_thread_siblings(cpu)		cpu_sibling_map[cpu]
+#endif
+
 #ifdef CONFIG_NUMA
 
 #include <asm/mpspec.h>
diff -Nraup linux-2.6.15_rc3/include/asm-ia64/topology.h linux-2.6.15_rc3_topology/include/asm-ia64/topology.h
--- linux-2.6.15_rc3/include/asm-ia64/topology.h	2001-06-06 08:57:20.000000000 +0800
+++ linux-2.6.15_rc3_topology/include/asm-ia64/topology.h	2001-06-06 08:55:35.000000000 +0800
@@ -100,6 +100,14 @@ void build_cpu_to_node_map(void);
 
 #endif /* CONFIG_NUMA */
 
+#ifdef CONFIG_SMP
+#define topology_physical_package_id(cpu)	cpu_data(cpu)->socket_id
+#define topology_core_id(cpu)		cpu_data(cpu)->core_id
+#define topology_thread_id(cpu)		cpu_data(cpu)->thread_id
+#define topology_core_siblings(cpu)		cpu_core_map[cpu]
+#define topology_thread_siblings(cpu)	cpu_sibling_map[cpu]
+#endif
+
 #include <asm-generic/topology.h>
 
 #endif /* _ASM_IA64_TOPOLOGY_H */
diff -Nraup linux-2.6.15_rc3/include/asm-x86_64/topology.h linux-2.6.15_rc3_topology/include/asm-x86_64/topology.h
--- linux-2.6.15_rc3/include/asm-x86_64/topology.h	2001-06-06 08:57:20.000000000 +0800
+++ linux-2.6.15_rc3_topology/include/asm-x86_64/topology.h	2001-06-06 08:55:35.000000000 +0800
@@ -58,6 +58,14 @@ extern int __node_distance(int, int);
 
 #endif
 
+#ifdef CONFIG_SMP
+#define topology_physical_package_id(cpu)	phys_proc_id[cpu]
+#define topology_core_id(cpu)			cpu_core_id[cpu]
+#define topology_thread_id(cpu)			cpu_thread_id[cpu]
+#define topology_core_siblings(cpu)		cpu_core_map[cpu]
+#define topology_thread_siblings(cpu)		cpu_sibling_map[cpu]
+#endif
+
 #include <asm-generic/topology.h>
 
 #endif

Re: [PATCH v2:3/3]Export cpu topology by sysfs

[Greg KH]

On Mon, Dec 26, 2005 at 03:38:16PM +0800, Zhang, Yanmin wrote:
> >>> >>> +static int __cpuinit topology_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:
> >>> >>> +			topology_add_dev(sys_dev);
> >>> >>> +			break;
> >>> >>> +#ifdef	CONFIG_HOTPLUG_CPU
> >>> >>> +		case CPU_DEAD:
> >>> >>> +			topology_remove_dev(sys_dev);
> >>> >>> +			break;
> >>> >>> +#endif
> >>> >>
> >>> >>Why ifdef?  Isn't it safe to just always have this in?
> >>> If no ifdef here, gcc reported a compiling warning when I compiled it
> >>> on IA64 with CONFIG_HOTPLUG_CPU=n.
> >>
> >>Then you should probably go change it so that CPU_DEAD is defined on
> >>non-smp builds, otherwise the code gets quite messy like the above :)
> 
> Sorry. My previous explanation is confusing. It's a link warning on
> ia64. On ia64, the kernel vmlinux doesn't include section .exit.text,
> so ld will report a link warning when a function is in section
> .exit.text and another function (not in .exit.text) calls the first
> one. When CONFIG_HOTPLUG_CPU=n, function topology_remove_dev is in
> section .exit.text, but its caller topology_remove_dev is not in
> .exit.text.
> 
> i386 and x86_64 kernel vmlinux does include .exit.text and discard it
> only when running, so there is no such warning on i386/x86_64.
> 
> There is no other better approach to get rid of the warning unless we
> change arch/ia64/kernel/vmlinux.lds.S to keep all .exit.text in kernel
> image.

Or just move that function to not be __exit, as you are calling it from
an __init function.  That would be the best solution.

thanks,

greg k-h

Re: [PATCH v2:3/3]Export cpu topology by sysfs

[Greg KH]

On Mon, Dec 26, 2005 at 12:28:39AM -0800, Yanmin Zhang wrote:
> On Mon, Dec 26, 2005 at 03:38:16PM +0800, Zhang, Yanmin wrote:
> > >>-----Original Message-----
> > >>From: Greg KH [mailto:greg@kroah.com]
> > >>Sent: 2005??12??24?? 3:16
> > >>To: Zhang, Yanmin
> > >>Cc: Yanmin Zhang; linux-kernel@vger.kernel.org; discuss@x86-64.org; linux-ia64@vger.kernel.org; Siddha, Suresh B; Shah, Rajesh;
> > >>Pallipadi, Venkatesh
> > >>Subject: Re: [PATCH v2:3/3]Export cpu topology by sysfs
> > >>
> > >>On Fri, Dec 23, 2005 at 12:03:27PM +0800, Zhang, Yanmin wrote:
> > >>> >>Can't you just use an attribute group and attach it to the cpu kobject?
> > >>> >>That would save an array of kobjects I think.
> > >>> As you know, current i386/x86_64 arch also export cache info under
> > >>> /sys/device/system/cpu/cpuX/cache. Is it clearer to export topology
> > >>> under a new directory than under cpu directly?
> > >>
> > >>No, the place in the sysfs tree you are putting this is just fine.  I'm
> > >>just saying that you do not need to create a new kobject for these
> > >>attributes.  Just use an attribute group, and you will get the same
> > >>naming, without the need for an extra kobject (and the whole array of
> > >>kobjects) at all.
> > >>
> > >>Does that make more sense?
> > Yes, indeed. Now, I used your idea and the patch became simpler. Thanks.



> > 
> > 
> > >>
> > >>> >>> +static int __cpuinit topology_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:
> > >>> >>> +			topology_add_dev(sys_dev);
> > >>> >>> +			break;
> > >>> >>> +#ifdef	CONFIG_HOTPLUG_CPU
> > >>> >>> +		case CPU_DEAD:
> > >>> >>> +			topology_remove_dev(sys_dev);
> > >>> >>> +			break;
> > >>> >>> +#endif
> > >>> >>
> > >>> >>Why ifdef?  Isn't it safe to just always have this in?
> > >>> If no ifdef here, gcc reported a compiling warning when I compiled it
> > >>> on IA64 with CONFIG_HOTPLUG_CPU=n.
> > >>
> > >>Then you should probably go change it so that CPU_DEAD is defined on
> > >>non-smp builds, otherwise the code gets quite messy like the above :)
> > 
> > Sorry. My previous explanation is confusing. It's a link warning on ia64. On ia64, the kernel vmlinux doesn't include section .exit.text, so ld will report a link warning when a function is in section .exit.text and another function (not in .exit.text) calls the first one. When CONFIG_HOTPLUG_CPU=n, function topology_remove_dev is in section .exit.text, but its caller topology_remove_dev is not in .exit.text.
> > 
> > i386 and x86_64 kernel vmlinux does include .exit.text and discard it only when running, so there is no such warning on i386/x86_64.
> > 
> > There is no other better approach to get rid of the warning unless we change arch/ia64/kernel/vmlinux.lds.S to keep all .exit.text in kernel image.
> 
> Here is the new patch. Thank Greg.

Much nicer, thanks for the changes.  But you forgot the description and
the Signed-off-by: line so that it can be picked up :(

Care to try again?

thanks,

greg k-h

RE: [PATCH v2:3/3]Export cpu topology by sysfs

[Zhang, Yanmin]

>>-----Original Message-----
>>From: Greg KH [mailto:greg@kroah.com]
>>Sent: 2005年12月27日 13:35
>>To: Zhang, Yanmin
>>Cc: Yanmin Zhang; linux-kernel@vger.kernel.org; discuss@x86-64.org; linux-ia64@vger.kernel.org; Siddha, Suresh B; Shah, Rajesh;
>>Pallipadi, Venkatesh
>>Subject: Re: [PATCH v2:3/3]Export cpu topology by sysfs
>>
>>> Sorry. My previous explanation is confusing. It's a link warning on
>>> ia64. On ia64, the kernel vmlinux doesn't include section .exit.text,
>>> so ld will report a link warning when a function is in section
>>> .exit.text and another function (not in .exit.text) calls the first
>>> one. When CONFIG_HOTPLUG_CPU=n, function topology_remove_dev is in
>>> section .exit.text, but its caller topology_remove_dev is not in
>>> .exit.text.
>>>
>>> i386 and x86_64 kernel vmlinux does include .exit.text and discard it
>>> only when running, so there is no such warning on i386/x86_64.
>>>
>>> There is no other better approach to get rid of the warning unless we
>>> change arch/ia64/kernel/vmlinux.lds.S to keep all .exit.text in kernel
>>> image.
>>
>>Or just move that function to not be __exit, as you are calling it from
>>an __init function.  That would be the best solution.

I will change topology_remove_dev's attribute from __cpuexit to __cpuinit, and the problem will be resolved thoroughly.

Thanks.

Re: [PATCH v2:3/3]Export cpu topology by sysfs

[Yanmin Zhang]

Here is the new version of the third patch. Thank Nathan, Greg, Andi,
Paul and Venki.

From: Zhang, Yanmin <yanmin.zhang@intel.com>

The third patch implements the topology exportation by sysfs.

Items (attributes) are similar to /proc/cpuinfo.

1) /sys/devices/system/cpu/cpuX/topology/physical_package_id:
represent the physical package id of  cpu X;
2) /sys/devices/system/cpu/cpuX/topology/core_id:
represent the cpu core id to cpu X;
3) /sys/devices/system/cpu/cpuX/topology/thread_id:
represent the cpu thread id  to cpu X;
4) /sys/devices/system/cpu/cpuX/topology/thread_siblings:
represent the thread siblings to cpu X in the same core;
5) /sys/devices/system/cpu/cpuX/topology/core_siblings:
represent the thread siblings to cpu X in the same physical package;

If one architecture wants to support this feature, it just needs to
implement 5 defines, typically in file include/asm-XXX/topology.h.
The 5 defines are:
#define topology_physical_package_id(cpu)
#define topology_core_id(cpu)
#define topology_thread_id(cpu)
#define topology_thread_siblings(cpu)
#define topology_core_siblings(cpu)

The type of siblings is cpumask_t.

If an attribute isn't defined on an architecture, it won't be exported.

The patch against 2.6.15-rc5 provides defines for i386/x86_64/ia64.

Signed-off-by: Zhang, Yanmin <yanmin.zhang@intel.com>

#diffstat export_topology_2.6.15-rc5_v7.patch
 Documentation/cputopology.txt |   31 ++++++++
 arch/ia64/kernel/topology.c   |    2
 drivers/base/Makefile         |    1
 drivers/base/topology.c       |  157 ++++++++++++++++++++++++++++++++++++++++++
 include/asm-i386/topology.h   |    8 ++
 include/asm-ia64/topology.h   |    8 ++
 include/asm-x86_64/topology.h |    8 ++
 7 files changed, 214 insertions(+), 1 deletion(-)

---

diff -Nraup linux-2.6.15_rc3/arch/ia64/kernel/topology.c linux-2.6.15_rc3_topology/arch/ia64/kernel/topology.c
--- linux-2.6.15_rc3/arch/ia64/kernel/topology.c	2001-06-06 09:21:40.000000000 +0800
+++ linux-2.6.15_rc3_topology/arch/ia64/kernel/topology.c	2001-06-06 09:21:45.000000000 +0800
@@ -98,4 +98,4 @@ out:
 	return err;
 }
 
-__initcall(topology_init);
+subsys_initcall(topology_init);
diff -Nraup linux-2.6.15_rc3/Documentation/cputopology.txt linux-2.6.15_rc3_topology/Documentation/cputopology.txt
--- linux-2.6.15_rc3/Documentation/cputopology.txt	1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.15_rc3_topology/Documentation/cputopology.txt	2001-06-06 09:21:45.000000000 +0800
@@ -0,0 +1,31 @@
+
+Export cpu topology info by sysfs. Items (attributes) are similar
+to /proc/cpuinfo.
+
+1) /sys/devices/system/cpu/cpuX/topology/physical_package_id:
+represent the physical package id of  cpu X;
+2) /sys/devices/system/cpu/cpuX/topology/core_id:
+represent the cpu core id to cpu X;
+3) /sys/devices/system/cpu/cpuX/topology/thread_id:
+represent the cpu thread id  to cpu X;
+4) /sys/devices/system/cpu/cpuX/topology/thread_siblings:
+represent the thread siblings to cpu X in the same core;
+5) /sys/devices/system/cpu/cpuX/topology/core_siblings:
+represent the thread siblings to cpu X in the same physical package;
+
+To implement it in an architecture-neutral way, a new source file,
+driver/base/topology.c, is to export the 5 attributes.
+
+If one architecture wants to support this feature, it just needs to
+implement 5 defines, typically in file include/asm-XXX/topology.h.
+The 5 defines are:
+#define topology_physical_package_id(cpu)
+#define topology_core_id(cpu)
+#define topology_thread_id(cpu)
+#define topology_thread_siblings(cpu)
+#define topology_core_siblings(cpu)
+
+The type of siblings is cpumask_t.
+
+If an attribute isn't defined on an architecture, it won't be exported.
+
diff -Nraup linux-2.6.15_rc3/drivers/base/Makefile linux-2.6.15_rc3_topology/drivers/base/Makefile
--- linux-2.6.15_rc3/drivers/base/Makefile	2001-06-06 09:21:40.000000000 +0800
+++ linux-2.6.15_rc3_topology/drivers/base/Makefile	2001-06-06 09:21:45.000000000 +0800
@@ -8,6 +8,7 @@ obj-y			+= power/
 obj-$(CONFIG_FW_LOADER)	+= firmware_class.o
 obj-$(CONFIG_NUMA)	+= node.o
 obj-$(CONFIG_MEMORY_HOTPLUG) += memory.o
+obj-$(CONFIG_SMP)	+= topology.o
 
 ifeq ($(CONFIG_DEBUG_DRIVER),y)
 EXTRA_CFLAGS += -DDEBUG
diff -Nraup linux-2.6.15_rc3/drivers/base/topology.c linux-2.6.15_rc3_topology/drivers/base/topology.c
--- linux-2.6.15_rc3/drivers/base/topology.c	1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.15_rc3_topology/drivers/base/topology.c	2001-06-07 11:33:05.000000000 +0800
@@ -0,0 +1,157 @@
+/*
+ * driver/base/topology.c - Populate sysfs with cpu topology information
+ *
+ * Written by: Zhang Yanmin, Intel Corporation
+ *
+ * Copyright (C) 2005, Intel Corp.
+ *
+ * All rights reserved.          
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
+ * NON INFRINGEMENT.  See the GNU General Public License for more
+ * details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ *
+ */
+#include <linux/sysdev.h>
+#include <linux/init.h>
+#include <linux/mm.h>
+#include <linux/cpu.h>
+#include <linux/module.h>
+#include <linux/topology.h>
+
+#define define_one_ro(_name) 		\
+static SYSDEV_ATTR(_name, 0444, show_##_name, NULL)
+
+#define define_id_show_func(name)				\
+static ssize_t show_##name(struct sys_device *dev, char *buf)	\
+{								\
+	unsigned int cpu = dev->id;				\
+	return sprintf(buf, "%d\n", topology_##name(cpu));	\
+}
+
+#define define_siblings_show_func(name)					\
+static ssize_t show_##name(struct sys_device *dev, char *buf)		\
+{									\
+	ssize_t len = -1;						\
+	unsigned int cpu = dev->id;					\
+	len = cpumask_scnprintf(buf, NR_CPUS+1, topology_##name(cpu));	\
+	return (len + sprintf(buf + len, "\n"));			\
+}
+
+#ifdef	topology_physical_package_id
+define_id_show_func(physical_package_id);
+define_one_ro(physical_package_id);
+#define ref_physical_package_id_attr	&attr_physical_package_id.attr,
+#else
+#define ref_physical_package_id_attr
+#endif
+
+#ifdef topology_core_id
+define_id_show_func(core_id);
+define_one_ro(core_id);
+#define ref_core_id_attr		&attr_core_id.attr,
+#else
+#define ref_core_id_attr
+#endif
+
+#ifdef topology_thread_id
+define_id_show_func(thread_id);
+define_one_ro(thread_id);
+#define ref_thread_id_attr		&attr_thread_id.attr,
+#else
+#define ref_thread_id_attr
+#endif
+
+#ifdef topology_thread_siblings
+define_siblings_show_func(thread_siblings);
+define_one_ro(thread_siblings);
+#define ref_thread_siblings_attr	&attr_thread_siblings.attr,
+#else
+#define ref_thread_siblings_attr
+#endif
+
+#ifdef topology_core_siblings
+define_siblings_show_func(core_siblings);
+define_one_ro(core_siblings);
+#define ref_core_siblings_attr		&attr_core_siblings.attr,
+#else
+#define ref_core_siblings_attr
+#endif
+
+static struct attribute * default_attrs[] = {
+	ref_physical_package_id_attr
+	ref_core_id_attr
+	ref_thread_id_attr
+	ref_thread_siblings_attr
+	ref_core_siblings_attr
+	NULL
+};
+
+static struct attribute_group topology_attr_group = {
+	.attrs = default_attrs,
+	.name = "topology"
+};
+
+/* Add/Remove cpu_topology interface for CPU device */
+static int __cpuinit topology_add_dev(struct sys_device * sys_dev)
+{
+	sysfs_create_group(&sys_dev->kobj, &topology_attr_group);
+	return 0;
+}
+
+static int __cpuinit topology_remove_dev(struct sys_device * sys_dev)
+{
+	sysfs_remove_group(&sys_dev->kobj, &topology_attr_group);
+	return 0;
+}
+
+static int __cpuinit topology_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:
+			topology_add_dev(sys_dev);
+			break;
+		case CPU_DEAD:
+			topology_remove_dev(sys_dev);
+			break;
+	}
+	return NOTIFY_OK;
+}
+
+static struct notifier_block topology_cpu_notifier =
+{
+	.notifier_call = topology_cpu_callback,
+};
+
+static int __cpuinit topology_sysfs_init(void)
+{
+	int i;
+
+	for_each_online_cpu(i) {
+		topology_cpu_callback(&topology_cpu_notifier, CPU_ONLINE,
+				(void *)(long)i);
+	}
+
+	register_cpu_notifier(&topology_cpu_notifier);
+
+	return 0;
+}
+
+device_initcall(topology_sysfs_init);
+
diff -Nraup linux-2.6.15_rc3/include/asm-i386/topology.h linux-2.6.15_rc3_topology/include/asm-i386/topology.h
--- linux-2.6.15_rc3/include/asm-i386/topology.h	2001-06-06 09:21:40.000000000 +0800
+++ linux-2.6.15_rc3_topology/include/asm-i386/topology.h	2001-06-06 09:21:45.000000000 +0800
@@ -27,6 +27,14 @@
 #ifndef _ASM_I386_TOPOLOGY_H
 #define _ASM_I386_TOPOLOGY_H
 
+#ifdef CONFIG_SMP
+#define topology_physical_package_id(cpu)	phys_proc_id[cpu]
+#define topology_core_id(cpu)			cpu_core_id[cpu]
+#define topology_thread_id(cpu)			cpu_thread_id[cpu]
+#define topology_core_siblings(cpu)		cpu_core_map[cpu]
+#define topology_thread_siblings(cpu)		cpu_sibling_map[cpu]
+#endif
+
 #ifdef CONFIG_NUMA
 
 #include <asm/mpspec.h>
diff -Nraup linux-2.6.15_rc3/include/asm-ia64/topology.h linux-2.6.15_rc3_topology/include/asm-ia64/topology.h
--- linux-2.6.15_rc3/include/asm-ia64/topology.h	2001-06-06 09:21:40.000000000 +0800
+++ linux-2.6.15_rc3_topology/include/asm-ia64/topology.h	2001-06-06 09:21:45.000000000 +0800
@@ -100,6 +100,14 @@ void build_cpu_to_node_map(void);
 
 #endif /* CONFIG_NUMA */
 
+#ifdef CONFIG_SMP
+#define topology_physical_package_id(cpu)	cpu_data(cpu)->socket_id
+#define topology_core_id(cpu)		cpu_data(cpu)->core_id
+#define topology_thread_id(cpu)		cpu_data(cpu)->thread_id
+#define topology_core_siblings(cpu)		cpu_core_map[cpu]
+#define topology_thread_siblings(cpu)	cpu_sibling_map[cpu]
+#endif
+
 #include <asm-generic/topology.h>
 
 #endif /* _ASM_IA64_TOPOLOGY_H */
diff -Nraup linux-2.6.15_rc3/include/asm-x86_64/topology.h linux-2.6.15_rc3_topology/include/asm-x86_64/topology.h
--- linux-2.6.15_rc3/include/asm-x86_64/topology.h	2001-06-06 09:21:40.000000000 +0800
+++ linux-2.6.15_rc3_topology/include/asm-x86_64/topology.h	2001-06-06 09:21:45.000000000 +0800
@@ -58,6 +58,14 @@ extern int __node_distance(int, int);
 
 #endif
 
+#ifdef CONFIG_SMP
+#define topology_physical_package_id(cpu)	phys_proc_id[cpu]
+#define topology_core_id(cpu)			cpu_core_id[cpu]
+#define topology_thread_id(cpu)			cpu_thread_id[cpu]
+#define topology_core_siblings(cpu)		cpu_core_map[cpu]
+#define topology_thread_siblings(cpu)		cpu_sibling_map[cpu]
+#endif
+
 #include <asm-generic/topology.h>
 
 #endif

Re: [PATCH v2:3/3]Export cpu topology by sysfs

[Nathan Lynch]

Yanmin Zhang wrote:
> 
> Items (attributes) are similar to /proc/cpuinfo.
> 
> 1) /sys/devices/system/cpu/cpuX/topology/physical_package_id:
> represent the physical package id of  cpu X;
> 2) /sys/devices/system/cpu/cpuX/topology/core_id:
> represent the cpu core id to cpu X;
> 3) /sys/devices/system/cpu/cpuX/topology/thread_id:
> represent the cpu thread id  to cpu X;

So what is the format of the *_id attributes?  Looks like this is
determined by the architecture, which is okay, but it doesn't seem
explicit.

What about sane default values for the *_id attributes?  For example,
say I have a uniprocessor PC without HT or multicore -- should all of
these attributes have zero values, or some kind of "special" value to
mean "not applicable"?

Hmm, why should thread_id be exported at all?  Is it useful to
userspace in a way that the logical cpu id is not?

> 4) /sys/devices/system/cpu/cpuX/topology/thread_siblings:
> represent the thread siblings to cpu X in the same core;
> 5) /sys/devices/system/cpu/cpuX/topology/core_siblings:
> represent the thread siblings to cpu X in the same physical package;
> 
> If one architecture wants to support this feature, it just needs to
> implement 5 defines, typically in file include/asm-XXX/topology.h.
> The 5 defines are:
> #define topology_physical_package_id(cpu)
> #define topology_core_id(cpu)
> #define topology_thread_id(cpu)
> #define topology_thread_siblings(cpu)
> #define topology_core_siblings(cpu)
> 
> The type of siblings is cpumask_t.
> 
> If an attribute isn't defined on an architecture, it won't be
> exported.

Okay, but which combinations of attributes are valid?  E.g. I would
think that it's fine for an architecture to define topology_thread_id
and topology_thread_siblings without any of the others, correct?

Also I'd rather the architectures have the ability to define these as
functions instead of macros.

<snip>

> +/* Add/Remove cpu_topology interface for CPU device */
> +static int __cpuinit topology_add_dev(struct sys_device * sys_dev)
> +{
> +	sysfs_create_group(&sys_dev->kobj, &topology_attr_group);
> +	return 0;
> +}

Can't sysfs_create_group fail?

> +
> +static int __cpuinit topology_remove_dev(struct sys_device * sys_dev)
> +{
> +	sysfs_remove_group(&sys_dev->kobj, &topology_attr_group);
> +	return 0;
> +}
> +
> +static int __cpuinit topology_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:
> +			topology_add_dev(sys_dev);
> +			break;
> +		case CPU_DEAD:
> +			topology_remove_dev(sys_dev);
> +			break;
> +	}
> +	return NOTIFY_OK;
> +}

I don't think it makes much sense to add and remove the attribute
group for cpu online/offline events.  The topology information for an
offline cpu is potentially valuable -- it could help the admin decide
which processor to online at runtime, for example.

I believe the correct time to update the topology information is when
the topology actually changes (e.g. physical addition or removal of a
processor) -- this is independent of online/offline operations.

In general, I'm still a little uneasy with exporting the cpu topology
in this way.  I can see how the information would be useful, and right
now, Linux does not do a great job of exposing to userspace these
relationships between cpus.  So I see the need.  But the things about
this approach which I don't like are:

- Attributes which are not applicable to the running system will be
  exported anyway.  Discovery at runtime would be less confusing, I
  think.

- This locks us into exporting a three-level topology (thread, core,
  package), with hard-coded names, when it seems probable that there
  will be systems with more levels than that in the future.

Have you considered basing the exported topology on sched domains?


Nathan

RE: [PATCH v2:3/3]Export cpu topology by sysfs

[Zhang, Yanmin]

>>-----Original Message-----
>>From: Nathan Lynch [mailto:ntl@pobox.com]
>>Sent: 2005年12月28日 5:20
>>To: Yanmin Zhang
>>Cc: greg@kroah.com; linux-kernel@vger.kernel.org; discuss@x86-64.org; linux-ia64@vger.kernel.org; Siddha, Suresh B; Shah, Rajesh;
>>Pallipadi, Venkatesh; Zhang, Yanmin
>>Subject: Re: [PATCH v2:3/3]Export cpu topology by sysfs
>>
>>Yanmin Zhang wrote:
>>>
>>> Items (attributes) are similar to /proc/cpuinfo.
>>>
>>> 1) /sys/devices/system/cpu/cpuX/topology/physical_package_id:
>>> represent the physical package id of  cpu X;
>>> 2) /sys/devices/system/cpu/cpuX/topology/core_id:
>>> represent the cpu core id to cpu X;
>>> 3) /sys/devices/system/cpu/cpuX/topology/thread_id:
>>> represent the cpu thread id  to cpu X;
>>
>>So what is the format of the *_id attributes?  Looks like this is
>>determined by the architecture, which is okay, but it doesn't seem
>>explicit.
The type of *_id is int or u8 (like on x86_64). It's better to convert the value to int.

>>
>>What about sane default values for the *_id attributes?
It depends on the specific architectures. On i386/x86_64, the default vaules of *_id are 0xffu. On ia64, the default value of physical_package_id is -1.

  For example,
>>say I have a uniprocessor PC without HT or multicore -- should all of
>>these attributes have zero values, or some kind of "special" value to
>>mean "not applicable"?
This feature is disabled when CONFIG_SMP=n. I can't make decision that all arch should use the same default values, so let architectures to decide. Is it ok?

>>
>>Hmm, why should thread_id be exported at all?  Is it useful to
>>userspace in a way that the logical cpu id is not?
Just to make it clearer. Of course, physical_package_id /core_id/ logical cpu id could tell enough info like thread id.

>>
>>> 4) /sys/devices/system/cpu/cpuX/topology/thread_siblings:
>>> represent the thread siblings to cpu X in the same core;
>>> 5) /sys/devices/system/cpu/cpuX/topology/core_siblings:
>>> represent the thread siblings to cpu X in the same physical package;
>>>
>>> If one architecture wants to support this feature, it just needs to
>>> implement 5 defines, typically in file include/asm-XXX/topology.h.
>>> The 5 defines are:
>>> #define topology_physical_package_id(cpu)
>>> #define topology_core_id(cpu)
>>> #define topology_thread_id(cpu)
>>> #define topology_thread_siblings(cpu)
>>> #define topology_core_siblings(cpu)
>>>
>>> The type of siblings is cpumask_t.
>>>
>>> If an attribute isn't defined on an architecture, it won't be
>>> exported.
>>
>>Okay, but which combinations of attributes are valid?  E.g. I would
>>think that it's fine for an architecture to define topology_thread_id
>>and topology_thread_siblings without any of the others, correct?
I think topology_physical_package_id/topology_core_id/topology_core_siblings are also needed. For example, admin might want to bind some processes to specific physical cpu, or core.

>>
>>Also I'd rather the architectures have the ability to define these as
>>functions instead of macros.
It's easy. The architectures just need the defines to point to functions in their specific header files, typically in file include/asm-XXX/topology.h.

>>
>><snip>
>>
>>> +/* Add/Remove cpu_topology interface for CPU device */
>>> +static int __cpuinit topology_add_dev(struct sys_device * sys_dev)
>>> +{
>>> +	sysfs_create_group(&sys_dev->kobj, &topology_attr_group);
>>> +	return 0;
>>> +}
>>
>>Can't sysfs_create_group fail?
It might fail, but it doesn't matter. Later when topology_remove_dev is called, sysfs_remove_group will do nothing because the subdir is not created.


>>
>>> +
>>> +static int __cpuinit topology_remove_dev(struct sys_device * sys_dev)
>>> +{
>>> +	sysfs_remove_group(&sys_dev->kobj, &topology_attr_group);
>>> +	return 0;
>>> +}
>>> +
>>> +static int __cpuinit topology_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:
>>> +			topology_add_dev(sys_dev);
>>> +			break;
>>> +		case CPU_DEAD:
>>> +			topology_remove_dev(sys_dev);
>>> +			break;
>>> +	}
>>> +	return NOTIFY_OK;
>>> +}
>>
>>I don't think it makes much sense to add and remove the attribute
>>group for cpu online/offline events.  The topology information for an
>>offline cpu is potentially valuable -- it could help the admin decide
>>which processor to online at runtime, for example.
>>
>>I believe the correct time to update the topology information is when
>>the topology actually changes (e.g. physical addition or removal of a
>>processor) -- this is independent of online/offline operations.
Currently, on i386/x86_64/ia64, a cpu gets its own topology by itself and fills into a global array. If the cpu is offline since the machine is booted, we can't get its topology info.
And when a cpu is offline, current kernel deletes it from the thread_siblings and core_siblings of other cpu.

>>
>>In general, I'm still a little uneasy with exporting the cpu topology
>>in this way.  I can see how the information would be useful, and right
>>now, Linux does not do a great job of exposing to userspace these
>>relationships between cpus.  So I see the need.
I also think there are requirements. Many high-reliable applications, such like in telecom, need bind processes on specific cpus. The topology info is important for admin to do so.

  But the things about
>>this approach which I don't like are:
>>
>>- Attributes which are not applicable to the running system will be
>>  exported anyway.  Discovery at runtime would be less confusing, I
>>  think.
>>
>>- This locks us into exporting a three-level topology (thread, core,
>>  package), with hard-coded names, when it seems probable that there
>>  will be systems with more levels than that in the future.
It's easy to add more levels based on my implementations. 
Hard-coded names might be a problem. Is there any special requirement to change the names arch-specifically? If some architectures really need their specific names, I will change the names from hard-coded to arch-defined.

>>
>>Have you considered basing the exported topology on sched domains?
No. Sched domains consist of far more info. Zou Nanhai wrote a patch to export sched domain info by procfs. I think it's better if we could export sched domain by sysfs.

Thanks,
Yanmin

Re: [PATCH v2:3/3]Export cpu topology by sysfs

[Nathan Lynch]

> >>What about sane default values for the *_id attributes?

> It depends on the specific architectures. On i386/x86_64, the
> default vaules of *_id are 0xffu. On ia64, the default value of
> physical_package_id is -1.

> >>For example,
> >>say I have a uniprocessor PC without HT or multicore -- should all of
> >>these attributes have zero values, or some kind of "special" value to
> >>mean "not applicable"?
>
> This feature is disabled when CONFIG_SMP=n.

Irrelevant.  Running SMP kernels on UP boxes is not uncommon.

The point I was trying to make is that these new attributes will show
up on systems where they don't provide useful information -- the UP
case aside, there are plenty of SMP systems which aren't multicore or
multithreaded.  We need to take care that the attributes don't provide
misleading information on such systems.

> I can't make decision that all arch should use the same default
> values, so let architectures to decide. Is it ok? 

Not really -- inevitably we'll wind up with an interface that has
slightly different semantics on each architecture.

> >>Hmm, why should thread_id be exported at all?  Is it useful to
> >>userspace in a way that the logical cpu id is not?
>
> Just to make it clearer. Of course, physical_package_id /core_id/
> logical cpu id could tell enough info like thread id. 

Then let's drop thread_id until there's a need for it.


> >>> +static int __cpuinit topology_remove_dev(struct sys_device * sys_dev)
> >>> +{
> >>> +	sysfs_remove_group(&sys_dev->kobj, &topology_attr_group);
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static int __cpuinit topology_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:
> >>> +			topology_add_dev(sys_dev);
> >>> +			break;
> >>> +		case CPU_DEAD:
> >>> +			topology_remove_dev(sys_dev);
> >>> +			break;
> >>> +	}
> >>> +	return NOTIFY_OK;
> >>> +}
> >>
> >>I don't think it makes much sense to add and remove the attribute
> >>group for cpu online/offline events.  The topology information for an
> >>offline cpu is potentially valuable -- it could help the admin decide
> >>which processor to online at runtime, for example.
> >>
> >>I believe the correct time to update the topology information is when
> >>the topology actually changes (e.g. physical addition or removal of a
> >>processor) -- this is independent of online/offline operations.
>
> Currently, on i386/x86_64/ia64, a cpu gets its own topology by
> itself and fills into a global array. If the cpu is offline since
> the machine is booted, we can't get its topology info.

Hmm, is this a limitation of those architectures?  On ppc64 (where
this feature would be applicable) Open Firmware provides such topology
information regardless of the cpus' states; does the firmware or ACPI
on these platforms not do the same?

> And when a cpu is offline, current kernel deletes it from the
> thread_siblings and core_siblings of other cpu. 

That's fine -- I'm just arguing against the addition/removal of the
attributes when cpus go online and offline.


> >>- This locks us into exporting a three-level topology (thread, core,
> >>  package), with hard-coded names, when it seems probable that there
> >>  will be systems with more levels than that in the future.
>

> It's easy to add more levels based on my implementations.
> Hard-coded names might be a problem. Is there any special
> requirement to change the names arch-specifically? If some
> architectures really need their specific names, I will change the
> names from hard-coded to arch-defined.

No, don't worry about it.  I withdraw that objection.

RE: [PATCH v2:3/3]Export cpu topology by sysfs

[Zhang, Yanmin]

>>-----Original Message-----
>>From: linux-ia64-owner@vger.kernel.org [mailto:linux-ia64-owner@vger.kernel.org] On Behalf Of Nathan Lynch
>>Sent: 2005年12月29日 3:14
>>To: Zhang, Yanmin
>>Cc: Yanmin Zhang; greg@kroah.com; linux-kernel@vger.kernel.org; discuss@x86-64.org; linux-ia64@vger.kernel.org; Siddha, Suresh B; Shah,
>>Rajesh; Pallipadi, Venkatesh
>>Subject: Re: [PATCH v2:3/3]Export cpu topology by sysfs
>>
>>
>>> >>What about sane default values for the *_id attributes?
>>Not really -- inevitably we'll wind up with an interface that has
>>slightly different semantics on each architecture.
How about below arrangement for default values?
1) physical_package_id: If cpu has no physical package id, the logical cpu id will be picked up.
2) core_id: 0. If cpu doesn't support multi-core, its core id is 0. 
3) thread_siblings: Just include itself, if the cpu doesn't support HT/multi-thread.
4) core_siblings: Just include itself, if the cpu doesn't support multi-core and HT.


>>
>>> >>Hmm, why should thread_id be exported at all?  Is it useful to
>>> >>userspace in a way that the logical cpu id is not?
>>>
>>> Just to make it clearer. Of course, physical_package_id /core_id/
>>> logical cpu id could tell enough info like thread id.
>>
>>Then let's drop thread_id until there's a need for it.
Ok. 


>>> >>> +	switch (action) {
>>> >>> +		case CPU_ONLINE:
>>> >>> +			topology_add_dev(sys_dev);
>>> >>> +			break;
>>> >>> +		case CPU_DEAD:
>>> >>> +			topology_remove_dev(sys_dev);
>>> >>> +			break;
>>> >>> +	}
>>> >>> +	return NOTIFY_OK;
>>> >>> +}
>>> >>
>>> >>I don't think it makes much sense to add and remove the attribute
>>> >>group for cpu online/offline events.  The topology information for an
>>> >>offline cpu is potentially valuable -- it could help the admin decide
>>> >>which processor to online at runtime, for example.
>>> >>
>>> >>I believe the correct time to update the topology information is when
>>> >>the topology actually changes (e.g. physical addition or removal of a
>>> >>processor) -- this is independent of online/offline operations.
>>>
>>> Currently, on i386/x86_64/ia64, a cpu gets its own topology by
>>> itself and fills into a global array. If the cpu is offline since
>>> the machine is booted, we can't get its topology info.
>>
>>Hmm, is this a limitation of those architectures?  On ppc64 (where
>>this feature would be applicable) Open Firmware provides such topology
>>information regardless of the cpus' states; does the firmware or ACPI
>>on these platforms not do the same?
On I386/x86_64/IA64, MADT, an ACPI table, provides apic id for all cpus. But we can't divide it to get core id and thread id becasue we couldn't know how many bits of apic id are for core id and how many bits are for thread id. These info are got only by executing cupid (on i386/x86_64) or PAL call (on ia64) by the online cpu itself.

>>
>>> And when a cpu is offline, current kernel deletes it from the
>>> thread_siblings and core_siblings of other cpu.
>>
>>That's fine -- I'm just arguing against the addition/removal of the
>>attributes when cpus go online and offline.
Based on above discussion, if the attributes of the cpu is not removed when the cpu is offline, the attribute values might be incorrect when the cpu is not up since machine boots.

Re: [PATCH v2:3/3]Export cpu topology by sysfs

[Nathan Lynch]

Zhang, Yanmin wrote:
> >>> >>I don't think it makes much sense to add and remove the attribute
> >>> >>group for cpu online/offline events.  The topology information for an
> >>> >>offline cpu is potentially valuable -- it could help the admin decide
> >>> >>which processor to online at runtime, for example.
> >>> >>
> >>> >>I believe the correct time to update the topology information is when
> >>> >>the topology actually changes (e.g. physical addition or removal of a
> >>> >>processor) -- this is independent of online/offline operations.
> >>>
> >>> Currently, on i386/x86_64/ia64, a cpu gets its own topology by
> >>> itself and fills into a global array. If the cpu is offline since
> >>> the machine is booted, we can't get its topology info.
> >>
> >>Hmm, is this a limitation of those architectures?  On ppc64 (where
> >>this feature would be applicable) Open Firmware provides such topology
> >>information regardless of the cpus' states; does the firmware or ACPI
> >>on these platforms not do the same?
>
> On I386/x86_64/IA64, MADT, an ACPI table, provides apic id for all
> cpus. But we can't divide it to get core id and thread id becasue we
> couldn't know how many bits of apic id are for core id and how many
> bits are for thread id. These info are got only by executing cupid
> (on i386/x86_64) or PAL call (on ia64) by the online cpu itself.

In practice does the division of bits between core and thread in the
apic id differ between cpus in the same system?  Is there really no
way to discover a cpu's core and thread id without onlining it on
these platforms?

RE: [PATCH v2:3/3]Export cpu topology by sysfs

[Zhang, Yanmin]

>>-----Original Message-----
>>From: linux-ia64-owner@vger.kernel.org [mailto:linux-ia64-owner@vger.kernel.org] On Behalf Of Nathan Lynch
>>Sent: 2006年1月9日 12:52
>>To: Zhang, Yanmin
>>Cc: Yanmin Zhang; greg@kroah.com; linux-kernel@vger.kernel.org; discuss@x86-64.org; linux-ia64@vger.kernel.org; Siddha, Suresh B; Shah,
>>Rajesh; Pallipadi, Venkatesh
>>Subject: Re: [PATCH v2:3/3]Export cpu topology by sysfs
>>
>>Zhang, Yanmin wrote:
>>> >>> >>I don't think it makes much sense to add and remove the attribute
>>> >>> >>group for cpu online/offline events.  The topology information for an
>>> >>> >>offline cpu is potentially valuable -- it could help the admin decide
>>> >>> >>which processor to online at runtime, for example.
>>> >>> >>
>>> >>> >>I believe the correct time to update the topology information is when
>>> >>> >>the topology actually changes (e.g. physical addition or removal of a
>>> >>> >>processor) -- this is independent of online/offline operations.
>>> >>>
>>> >>> Currently, on i386/x86_64/ia64, a cpu gets its own topology by
>>> >>> itself and fills into a global array. If the cpu is offline since
>>> >>> the machine is booted, we can't get its topology info.
>>> >>
>>> >>Hmm, is this a limitation of those architectures?  On ppc64 (where
>>> >>this feature would be applicable) Open Firmware provides such topology
>>> >>information regardless of the cpus' states; does the firmware or ACPI
>>> >>on these platforms not do the same?
>>>
>>> On I386/x86_64/IA64, MADT, an ACPI table, provides apic id for all
>>> cpus. But we can't divide it to get core id and thread id becasue we
>>> couldn't know how many bits of apic id are for core id and how many
>>> bits are for thread id. These info are got only by executing cupid
>>> (on i386/x86_64) or PAL call (on ia64) by the online cpu itself.
>>
>>In practice does the division of bits between core and thread in the
>>apic id differ between cpus in the same system?
On i386 and x86_64, I think the answer is no, so we could get core and thread id for offline cpu. But there are 2 concerns.
1) Current cpu hotplug supports to convert cpu state between online and cpuline. In the future, the implementation would add support conversion between non-present and present. If a cpu is not present, there might be no its corresponding MADT entry. Later, when the cpu becomes present, ACPI needs provide its apic id dynamically. As for this case, we still can't get the non-present cpu id.
2) On ia64, ia64 manual says that cpu physical id/core id/thread id are got by pal call. It doesn't mention how to divide apic id to get physical id/core id/thread id.

  Is there really no
>>way to discover a cpu's core and thread id without onlining it on
>>these platforms?

Re: [PATCH v2:3/3]Export cpu topology by sysfs

[Russ Anderson]

Zhang, Yanmin wrote:
> 
> >>In practice does the division of bits between core and thread in the
> >>apic id differ between cpus in the same system?
>
> 2) On ia64, ia64 manual says that cpu physical id/core id/thread id are got by pal call. It doesn't mention how to divide apic id to get physical id/core id/thread id.

The socket_id, core_id, and thread_id are part of cpuinfo_ia64.
They are set up in arch/ia64/kernel/smpboot.c: identify_siblings()

You cannot make assumptions about "the division of bits", but must rely on
the values returned by pal.


-- 
Russ Anderson, OS RAS/Partitioning Project Lead  
SGI - Silicon Graphics Inc          rja@sgi.com