LinuxLists.cc - [PATCH v2:3/3]Export cpu topology by sysfs

[permalink] [raw]

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?

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

2005-12-23 04:03:32

[permalink] [raw]

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

>>-----Original Message-----
>>From: Greg KH [mailto:[email protected]]
>>Sent: 2005??12??23?? 8:17
>>To: Yanmin Zhang
>>Cc: [email protected]; [email protected]; [email protected]; 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.

2005-12-23 19:17:18

[permalink] [raw]

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

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

2005-12-26 07:38:27

[permalink] [raw]

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

>>-----Original Message-----
>>From: Greg KH [mailto:[email protected]]
>>Sent: 2005??12??24?? 3:16
>>To: Zhang, Yanmin
>>Cc: Yanmin Zhang; [email protected]; [email protected]; [email protected]; 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.

2005-12-26 08:28:41

by Yanmin Zhang

[permalink] [raw]

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

On Mon, Dec 26, 2005 at 03:38:16PM +0800, Zhang, Yanmin wrote:
> >>-----Original Message-----
> >>From: Greg KH [mailto:[email protected]]
> >>Sent: 2005??12??24?? 3:16
> >>To: Zhang, Yanmin
> >>Cc: Yanmin Zhang; [email protected]; [email protected]; [email protected]; 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

2005-12-27 05:37:53

[permalink] [raw]

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

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

2005-12-27 05:37:55

[permalink] [raw]

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

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:[email protected]]
> > >>Sent: 2005??12??24?? 3:16
> > >>To: Zhang, Yanmin
> > >>Cc: Yanmin Zhang; [email protected]; [email protected]; [email protected]; 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

2005-12-27 10:22:58

[permalink] [raw]

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

>>-----Original Message-----
>>From: Greg KH [mailto:[email protected]]
>>Sent: 2005??12??27?? 13:35
>>To: Zhang, Yanmin
>>Cc: Yanmin Zhang; [email protected]; [email protected]; [email protected]; 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.

2005-12-27 10:41:48

by Yanmin Zhang

[permalink] [raw]

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

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

From: Zhang, Yanmin <[email protected]>

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 <[email protected]>

#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

2005-12-27 21:19:48

[permalink] [raw]

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.

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

2005-12-28 13:46:43

[permalink] [raw]

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

>>-----Original Message-----
>>From: Nathan Lynch [mailto:[email protected]]
>>Sent: 2005??12??28?? 5:20
>>To: Yanmin Zhang
>>Cc: [email protected]; [email protected]; [email protected]; [email protected]; 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

2005-12-28 19:14:15

[permalink] [raw]

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

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

2005-12-31 08:43:37

[permalink] [raw]

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

>>-----Original Message-----
>>From: [email protected] [mailto:[email protected]] On Behalf Of Nathan Lynch
>>Sent: 2005??12??29?? 3:14
>>To: Zhang, Yanmin
>>Cc: Yanmin Zhang; [email protected]; [email protected]; [email protected]; [email protected]; 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.

2006-01-04 09:07:21

by Yanmin Zhang

[permalink] [raw]

Subject: Re: [PATCH v3]Export cpu topology by sysfs

Here is version 3. Based on Nathan's comments, the main changes are:
1) Drop thread id, so the first 2 patches of version 2 are dropped;
2) Set consistent default values for the 4 attributes.

---Detailed descriptions---
From: Zhang, Yanmin <[email protected]>

The 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_siblings:
represent the thread siblings to cpu X in the same core;
4) /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 4 defines, typically in file include/asm-XXX/topology.h.
The 4 defines are:
#define topology_physical_package_id(cpu)
#define topology_core_id(cpu)
#define topology_thread_siblings(cpu)
#define topology_core_siblings(cpu)

The type of **_id is int.
The type of siblings is cpumask_t.

To be consistent on all architectures, the 4 attributes should have
deafult values if their values are unavailable. Below is the rule.
1) physical_package_id: If cpu has no physical package id, -1 is the
default value.
2) core_id: 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/Multi-thread.

So be careful when declaring the 4 defines in include/asm-XXX/topology.h.

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

Thank Nathan, Greg, Andi, Paul and Venki.

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

Signed-off-by: Zhang, Yanmin <[email protected]>

---

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 2006-01-03 22:54:40.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 2006-01-04 00:54:38.000000000 +0800
@@ -0,0 +1,41 @@
+
+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_siblings:
+represent the thread siblings to cpu X in the same core;
+4) /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 4 defines, typically in file include/asm-XXX/topology.h.
+The 4 defines are:
+#define topology_physical_package_id(cpu)
+#define topology_core_id(cpu)
+#define topology_thread_siblings(cpu)
+#define topology_core_siblings(cpu)
+
+The type of **_id is int.
+The type of siblings is cpumask_t.
+
+To be consistent on all architectures, the 4 attributes should have
+deafult values if their values are unavailable. Below is the rule.
+1) physical_package_id: If cpu has no physical package id, -1 is the
+default value.
+2) core_id: 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/Multi-thread.
+
+So be careful when declaring the 4 defines in include/asm-XXX/topology.h.
+
+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 2006-01-03 22:54:40.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 2006-01-04 00:54:48.000000000 +0800
@@ -0,0 +1,148 @@
+/*
+ * driver/base/topology.c - Populate sysfs with cpu topology information
+ *
+ * Written by: Zhang Yanmin, Intel Corporation
+ *
+ * Copyright (C) 2006, 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_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_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-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 2006-01-04 00:16:13.000000000 +0800
@@ -27,6 +27,15 @@
#ifndef _ASM_I386_TOPOLOGY_H
#define _ASM_I386_TOPOLOGY_H

+#ifdef CONFIG_SMP
+#define topology_physical_package_id(cpu) \
+ ((int)(phys_proc_id[cpu] == BAD_APICID ? -1 : phys_proc_id[cpu]))
+#define topology_core_id(cpu) \
+ ((int)(cpu_core_id[cpu] == BAD_APICID ? 0 : cpu_core_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 2006-01-04 00:07:47.000000000 +0800
@@ -100,6 +100,13 @@ 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_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 2006-01-04 00:14:22.000000000 +0800
@@ -58,6 +58,15 @@ extern int __node_distance(int, int);

#endif

+#ifdef CONFIG_SMP
+#define topology_physical_package_id(cpu) \
+ ((int)(phys_proc_id[cpu] == BAD_APICID ? -1 : phys_proc_id[cpu]))
+#define topology_core_id(cpu) \
+ ((int)(cpu_core_id[cpu] == BAD_APICID ? 0 : cpu_core_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

2006-01-09 04:52:12

[permalink] [raw]

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? Is there really no
way to discover a cpu's core and thread id without onlining it on
these platforms?

2006-01-09 05:10:26

[permalink] [raw]

Subject: Re: [PATCH v3]Export cpu topology by sysfs

(sorry for the delay in response)

Yanmin Zhang wrote:
> Here is version 3. Based on Nathan's comments, the main changes are:
> 1) Drop thread id, so the first 2 patches of version 2 are dropped;

Thanks.

> 2) Set consistent default values for the 4 attributes.
>

<snip>

> If one architecture wants to support this feature, it just needs to
> implement 4 defines, typically in file include/asm-XXX/topology.h.
> The 4 defines are:
> #define topology_physical_package_id(cpu)
> #define topology_core_id(cpu)
> #define topology_thread_siblings(cpu)
> #define topology_core_siblings(cpu)
>
> The type of **_id is int.
> The type of siblings is cpumask_t.
>
> To be consistent on all architectures, the 4 attributes should have
> deafult values if their values are unavailable. Below is the rule.
> 1) physical_package_id: If cpu has no physical package id, -1 is the
> default value.
> 2) core_id: If cpu doesn't support multi-core, its core id is 0.

Why not -1 as with the physical package id? 0 could be a valid core
id.

> 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/Multi-thread.

Really, I think the least confusing interface would not export those
attributes which are not relevant for the system. E.g. if the system
isn't multi-core, you don't see core_id and core_siblings attributes.

Failing that, let's at least have consistent, unambiguous values for
the ids which are not applicable.

<snip>
> +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 still oppose this bit. I want the attributes there for powerpc even
for offline cpus -- the topology information (if obtainable, which it
is on powerpc) is useful regardless of the cpu's online state. The
attributes should appear when a cpu is made present, and go away when
a cpu is removed.

This week I'll try to do an implementation for powerpc.

2006-01-09 06:36:04