[PATCH][RFC] Add default CPU topology information

[PATCH][RFC] Add default CPU topology information

[Ben Hutchings]

Not all architectures and configurations define CPU topology information.
This can result in an empty topology directory in sysfs, and requires
in-kernel users to protect all uses with #ifdef - see
<http://marc.info/?l=linux-netdev&m=120639033904472&w=2>.

The documentation of CPU topology specifies what the defaults should be
if only partial information is available from the hardware.  So we can
provide these defaults as a fallback.

This patch:

- Adds default definitions of the 4 topology macros to
  include/asm-generic/topology.h.
- Changes include/asm-*/topology.h to include <asm-generic/topology.h>
  unconditionally.
- Changes drivers/base/topology.c to use the topology macros unconditionally
  and to cope with definitions that aren't lvalues.
- Updates documentation accordingly.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
diff --git a/Documentation/cputopology.txt b/Documentation/cputopology.txt
index b61cb95..c035481 100644
--- a/Documentation/cputopology.txt
+++ b/Documentation/cputopology.txt
@@ -14,9 +14,8 @@ represent the thread siblings to cpu X in the same physical package;
 To implement it in an architecture-neutral way, a new source file,
 drivers/base/topology.c, is to export the 4 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:
+If one architecture wants to support this feature, it must define
+some of these macros in include/asm-XXX/topology.h:
 #define topology_physical_package_id(cpu)
 #define topology_core_id(cpu)
 #define topology_thread_siblings(cpu)
@@ -25,17 +24,10 @@ The 4 defines are:
 The type of **_id is int.
 The type of siblings is cpumask_t.
 
-To be consistent on all architectures, the 4 attributes should have
-default 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.
-
+To be consistent on all architectures, include/asm-generic/topology.h
+provides default definitions for any of the above macros that are
+not already defined:
+1) physical_package_id: -1
+2) core_id: 0
+3) thread_siblings: just the given CPU
+4) core_siblings: just the given CPU
diff --git a/drivers/base/topology.c b/drivers/base/topology.c
index e1d3ad4..dfcb2e2 100644
--- a/drivers/base/topology.c
+++ b/drivers/base/topology.c
@@ -45,47 +45,28 @@ 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));	\
+	cpumask_t siblings = topology_##name(cpu);			\
+	len = cpumask_scnprintf(buf, NR_CPUS+1, siblings);		\
 	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
+	&attr_physical_package_id.attr,
+	&attr_core_id.attr,
+	&attr_thread_siblings.attr,
+	&attr_core_siblings.attr,
 	NULL
 };
 
diff --git a/include/asm-alpha/topology.h b/include/asm-alpha/topology.h
index 420ccde..e59ec8a 100644
--- a/include/asm-alpha/topology.h
+++ b/include/asm-alpha/topology.h
@@ -41,8 +41,8 @@ static inline cpumask_t node_to_cpumask(int node)
 
 #define pcibus_to_cpumask(bus)	(cpu_online_map)
 
-#else /* CONFIG_NUMA */
-# include <asm-generic/topology.h>
-#endif /* !CONFIG_NUMA */
+#endif /* CONFIG_NUMA */
+
+#include <asm-generic/topology.h>
 
 #endif /* _ASM_ALPHA_TOPOLOGY_H */
diff --git a/include/asm-frv/topology.h b/include/asm-frv/topology.h
index abe7298..9427243 100644
--- a/include/asm-frv/topology.h
+++ b/include/asm-frv/topology.h
@@ -5,10 +5,8 @@
 
 #error NUMA not supported yet
 
-#else /* !CONFIG_NUMA */
+#endif /* CONFIG_NUMA */
 
 #include <asm-generic/topology.h>
 
-#endif /* CONFIG_NUMA */
-
 #endif /* _ASM_TOPOLOGY_H */
diff --git a/include/asm-generic/topology.h b/include/asm-generic/topology.h
index 342a2a0..373502b 100644
--- a/include/asm-generic/topology.h
+++ b/include/asm-generic/topology.h
@@ -27,6 +27,8 @@
 #ifndef _ASM_GENERIC_TOPOLOGY_H
 #define _ASM_GENERIC_TOPOLOGY_H
 
+#include <linux/cpumask.h>
+
 /* Other architectures wishing to use this simple topology API should fill
    in the below functions as appropriate in their own <asm/topology.h> file. */
 #ifndef cpu_to_node
@@ -52,4 +54,17 @@
 				)
 #endif
 
+#ifndef topology_physical_package_id
+#define topology_physical_package_id(cpu)	((void)(cpu), -1)
+#endif
+#ifndef topology_core_id
+#define topology_core_id(cpu)			((void)(cpu), 0)
+#endif
+#ifndef topology_thread_siblings
+#define topology_thread_siblings(cpu)		cpumask_of_cpu(cpu)
+#endif
+#ifndef topology_core_siblings
+#define topology_core_siblings(cpu)		cpumask_of_cpu(cpu)
+#endif
+
 #endif /* _ASM_GENERIC_TOPOLOGY_H */
diff --git a/include/asm-mips/topology.h b/include/asm-mips/topology.h
index 259145e..ce837a4 100644
--- a/include/asm-mips/topology.h
+++ b/include/asm-mips/topology.h
@@ -8,10 +8,10 @@
 #ifndef __ASM_TOPOLOGY_H
 #define __ASM_TOPOLOGY_H
 
-#include <topology.h>
-
 #ifdef CONFIG_SMP
 #define smt_capable()   (smp_num_siblings > 1)
 #endif
 
+#include <asm-generic/topology.h>
+
 #endif /* __ASM_TOPOLOGY_H */
diff --git a/include/asm-powerpc/topology.h b/include/asm-powerpc/topology.h
index ca23b68..e587788 100644
--- a/include/asm-powerpc/topology.h
+++ b/include/asm-powerpc/topology.h
@@ -96,9 +96,6 @@ static inline void sysfs_remove_device_from_node(struct sys_device *dev,
 {
 }
 
-
-#include <asm-generic/topology.h>
-
 #endif /* CONFIG_NUMA */
 
 #ifdef CONFIG_SMP
@@ -112,5 +109,7 @@ static inline void sysfs_remove_device_from_node(struct sys_device *dev,
 #endif
 #endif
 
+#include <asm-generic/topology.h>
+
 #endif /* __KERNEL__ */
 #endif	/* _ASM_POWERPC_TOPOLOGY_H */
diff --git a/include/asm-x86/topology.h b/include/asm-x86/topology.h
index 8af05a9..7f31b8d 100644
--- a/include/asm-x86/topology.h
+++ b/include/asm-x86/topology.h
@@ -165,11 +165,7 @@ extern int __node_distance(int, int);
 #define node_distance(a, b) __node_distance(a, b)
 #endif
 
-#else /* CONFIG_NUMA */
-
-#include <asm-generic/topology.h>
-
-#endif
+#endif /* CONFIG_NUMA */
 
 extern cpumask_t cpu_coregroup_map(int cpu);
 
@@ -185,4 +181,6 @@ extern cpumask_t cpu_coregroup_map(int cpu);
 #define smt_capable()			(smp_num_siblings > 1)
 #endif
 
+#include <asm-generic/topology.h>
+
 #endif

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.

Re: [PATCH][RFC] Add default CPU topology information

[Ingo Molnar]

* Ben Hutchings <bhutchings@solarflare.com> wrote:

> Not all architectures and configurations define CPU topology information.
> This can result in an empty topology directory in sysfs, and requires
> in-kernel users to protect all uses with #ifdef - see
> <http://marc.info/?l=linux-netdev&m=120639033904472&w=2>.
> 
> The documentation of CPU topology specifies what the defaults should be
> if only partial information is available from the hardware.  So we can
> provide these defaults as a fallback.
> 
> This patch:
> 
> - Adds default definitions of the 4 topology macros to
>   include/asm-generic/topology.h.
> - Changes include/asm-*/topology.h to include <asm-generic/topology.h>
>   unconditionally.
> - Changes drivers/base/topology.c to use the topology macros unconditionally
>   and to cope with definitions that aren't lvalues.
> - Updates documentation accordingly.
> 
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>

very nice - i suspect this would be a patch for -mm.

Acked-by: Ingo Molnar <mingo@elte.hu>

	Ingo

Re: [PATCH][RFC] Add default CPU topology information

[Andrew Morton]

On Mon, 31 Mar 2008 12:44:13 +0100
Ben Hutchings <bhutchings@solarflare.com> wrote:

> Not all architectures and configurations define CPU topology information.
> This can result in an empty topology directory in sysfs, and requires
> in-kernel users to protect all uses with #ifdef - see
> <http://marc.info/?l=linux-netdev&m=120639033904472&w=2>.
> 
> The documentation of CPU topology specifies what the defaults should be
> if only partial information is available from the hardware.  So we can
> provide these defaults as a fallback.
> 
> This patch:
> 
> - Adds default definitions of the 4 topology macros to
>   include/asm-generic/topology.h.
> - Changes include/asm-*/topology.h to include <asm-generic/topology.h>
>   unconditionally.
> - Changes drivers/base/topology.c to use the topology macros unconditionally
>   and to cope with definitions that aren't lvalues.
> - Updates documentation accordingly.
> 

I ran away in terror when I saw this patch.  It's dark and dirty stuff
you're unearthing there and it looks like a minefield of compilation
problems.  Not aided by the fact that we don't have a topology maintainer
afaik.  

We shall see.

Re: [PATCH][RFC] Add default CPU topology information

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> > This patch:
> > 
> > - Adds default definitions of the 4 topology macros to
> >   include/asm-generic/topology.h.
> > - Changes include/asm-*/topology.h to include <asm-generic/topology.h>
> >   unconditionally.
> > - Changes drivers/base/topology.c to use the topology macros unconditionally
> >   and to cope with definitions that aren't lvalues.
> > - Updates documentation accordingly.
> > 
> 
> I ran away in terror when I saw this patch.  It's dark and dirty stuff 
> you're unearthing there and it looks like a minefield of compilation 
> problems.  Not aided by the fact that we don't have a topology 
> maintainer afaik.
> 
> We shall see.

i could pick it up into sched.git - it's a frequent party to topology 
patches. We've now got automated cross-build checks as well by virtue of 
Thomas so common build failures ought to be rare.

	Ingo

Re: [PATCH][RFC] Add default CPU topology information

[Andrew Morton]

On Wed, 16 Apr 2008 14:51:33 +0200
Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > > This patch:
> > > 
> > > - Adds default definitions of the 4 topology macros to
> > >   include/asm-generic/topology.h.
> > > - Changes include/asm-*/topology.h to include <asm-generic/topology.h>
> > >   unconditionally.
> > > - Changes drivers/base/topology.c to use the topology macros unconditionally
> > >   and to cope with definitions that aren't lvalues.
> > > - Updates documentation accordingly.
> > > 
> > 
> > I ran away in terror when I saw this patch.  It's dark and dirty stuff 
> > you're unearthing there and it looks like a minefield of compilation 
> > problems.  Not aided by the fact that we don't have a topology 
> > maintainer afaik.
> > 
> > We shall see.
> 
> i could pick it up into sched.git

There seem to be multiple sched.gits

> - it's a frequent party to topology 
> patches. We've now got automated cross-build checks as well by virtue of 
> Thomas so common build failures ought to be rare.

OK, whatever.  There's a tangle between this change and the s390 tree which
already has a tangle with one of the sched-gits which is sort-of getting
sorted out.

git-s390 wasn't an appropriate place to host changes to
drivers/base/topology.c.

Re: [PATCH][RFC] Add default CPU topology information

[Andrew Morton]

On Mon, 31 Mar 2008 12:44:13 +0100 Ben Hutchings <bhutchings@solarflare.com> wrote:

> Not all architectures and configurations define CPU topology information.
> This can result in an empty topology directory in sysfs, and requires
> in-kernel users to protect all uses with #ifdef - see
> <http://marc.info/?l=linux-netdev&m=120639033904472&w=2>.
> 
> The documentation of CPU topology specifies what the defaults should be
> if only partial information is available from the hardware.  So we can
> provide these defaults as a fallback.

powerpc allmodconfig:

arch/powerpc/kernel/pci_64.c:655: error: expected identifier or '(' before 'void'
arch/powerpc/kernel/pci_64.c:655: error: expected ')' before '(' token

Because this:

#ifdef CONFIG_NUMA
int pcibus_to_node(struct pci_bus *bus)
{
	struct pci_controller *phb = pci_bus_to_host(bus);
	return phb->node;
}
#endif

is expanding to

int ((void)(bus), -1)(struct pci_bus *bus)

which the compiler has trouble with.

This:

+++ a/include/asm-powerpc/topology.h
@@ -109,6 +109,8 @@ static inline void sysfs_remove_device_f
 #endif
 #endif
 
+#define pcibus_to_node pcibus_to_node
+
 #include <asm-generic/topology.h>
 
 #endif /* __KERNEL__ */
_

gets it working, by tricking the preprocessor into expanding pcibus_to_node
to itself, but it isn't exactly pleasing.

Re: [PATCH][RFC] Add default CPU topology information

[Andrew Morton]

On Mon, 31 Mar 2008 12:44:13 +0100 Ben Hutchings <bhutchings@solarflare.com> wrote:

> Not all architectures and configurations define CPU topology information.
> This can result in an empty topology directory in sysfs, and requires
> in-kernel users to protect all uses with #ifdef - see
> <http://marc.info/?l=linux-netdev&m=120639033904472&w=2>.
> 
> The documentation of CPU topology specifies what the defaults should be
> if only partial information is available from the hardware.  So we can
> provide these defaults as a fallback.
> 
> This patch:
> 
> - Adds default definitions of the 4 topology macros to
>   include/asm-generic/topology.h.
> - Changes include/asm-*/topology.h to include <asm-generic/topology.h>
>   unconditionally.
> - Changes drivers/base/topology.c to use the topology macros unconditionally
>   and to cope with definitions that aren't lvalues.
> - Updates documentation accordingly.

I get a huuuuuuuge crash early in boot on the t61p dual x86_64 laptop. 
Before netconsole has started and it has no serial port, so all I have is a
partially-scrolled-off jpg, sorry.

I'll drop the patch.

http://userweb.kernel.org/~akpm/p4175278.jpg
http://userweb.kernel.org/~akpm/config-t61p.txt
http://userweb.kernel.org/~akpm/dmesg-t61p.txt


If I didn't have all that wrong crap on the stack we'd have a full
trace here.  I have CONFIG_FRAME_POINTER=y, but that doesn't seem to
have helped.

Re: [PATCH][RFC] Add default CPU topology information

[Andrew Morton]

On Fri, 18 Apr 2008 00:10:24 -0700 Andrew Morton <akpm@linux-foundation.org> wrote:

> On Mon, 31 Mar 2008 12:44:13 +0100 Ben Hutchings <bhutchings@solarflare.com> wrote:
> 
> > Not all architectures and configurations define CPU topology information.
> > This can result in an empty topology directory in sysfs, and requires
> > in-kernel users to protect all uses with #ifdef - see
> > <http://marc.info/?l=linux-netdev&m=120639033904472&w=2>.
> > 
> > The documentation of CPU topology specifies what the defaults should be
> > if only partial information is available from the hardware.  So we can
> > provide these defaults as a fallback.
> > 
> > This patch:
> > 
> > - Adds default definitions of the 4 topology macros to
> >   include/asm-generic/topology.h.
> > - Changes include/asm-*/topology.h to include <asm-generic/topology.h>
> >   unconditionally.
> > - Changes drivers/base/topology.c to use the topology macros unconditionally
> >   and to cope with definitions that aren't lvalues.
> > - Updates documentation accordingly.
> 
> I get a huuuuuuuge crash early in boot on the t61p dual x86_64 laptop. 
> Before netconsole has started and it has no serial port, so all I have is a
> partially-scrolled-off jpg, sorry.
> 
> I'll drop the patch.
> 
> http://userweb.kernel.org/~akpm/p4175278.jpg
> http://userweb.kernel.org/~akpm/config-t61p.txt
> http://userweb.kernel.org/~akpm/dmesg-t61p.txt
> 
> 
> If I didn't have all that wrong crap on the stack we'd have a full
> trace here.  I have CONFIG_FRAME_POINTER=y, but that doesn't seem to
> have helped.
> 

ooh, I just discovered kstack=.  That should be hoisted up to core kernel
then pushed out to other architectures, not hidden in x86_64 code.

But first I guess it should be make to work - it fails miserably, because
only one of our seventy fix stack-dumping functions actually looks at it.


This:

--- a/arch/x86/kernel/traps_64.c~a
+++ a/arch/x86/kernel/traps_64.c
@@ -426,7 +426,7 @@ _show_stack(struct task_struct *tsk, str
 		printk(" %016lx", *stack++);
 		touch_nmi_watchdog();
 	}
-	show_trace(tsk, regs, sp, bp);
+//	show_trace(tsk, regs, sp, bp);
 }
 
 void show_stack(struct task_struct *tsk, unsigned long * sp)


works heaps better.

Here's the rest of your oops: http://userweb.kernel.org/~akpm/p4175279.jpg

I'd be suspecting startup ordering problems - maybe the page allocator
isn't ready yet.

otoh, it _should_ be ready by the time we run init_sched_domains().

Re: [PATCH][RFC] Add default CPU topology information

[Ben Hutchings]

Andrew Morton wrote:
> On Mon, 31 Mar 2008 12:44:13 +0100 Ben Hutchings <bhutchings@solarflare.com> wrote:
> 
> > Not all architectures and configurations define CPU topology information.
> > This can result in an empty topology directory in sysfs, and requires
> > in-kernel users to protect all uses with #ifdef - see
> > <http://marc.info/?l=linux-netdev&m=120639033904472&w=2>.
> > 
> > The documentation of CPU topology specifies what the defaults should be
> > if only partial information is available from the hardware.  So we can
> > provide these defaults as a fallback.
> 
> powerpc allmodconfig:
> 
> arch/powerpc/kernel/pci_64.c:655: error: expected identifier or '(' before 'void'
> arch/powerpc/kernel/pci_64.c:655: error: expected ')' before '(' token
<snip 
> gets it working, by tricking the preprocessor into expanding pcibus_to_node
> to itself, but it isn't exactly pleasing.

No, it isn't!  But cputopology.txt does say these should be macros, not
functions.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.

Re: [PATCH][RFC] Add default CPU topology information

[Ben Hutchings]

Andrew Morton wrote:
> On Mon, 31 Mar 2008 12:44:13 +0100 Ben Hutchings <bhutchings@solarflare.com> wrote:
> 
> > Not all architectures and configurations define CPU topology information.
> > This can result in an empty topology directory in sysfs, and requires
> > in-kernel users to protect all uses with #ifdef - see
> > <http://marc.info/?l=linux-netdev&m=120639033904472&w=2>.
> > 
> > The documentation of CPU topology specifies what the defaults should be
> > if only partial information is available from the hardware.  So we can
> > provide these defaults as a fallback.
> > 
> > This patch:
> > 
> > - Adds default definitions of the 4 topology macros to
> >   include/asm-generic/topology.h.
> > - Changes include/asm-*/topology.h to include <asm-generic/topology.h>
> >   unconditionally.
> > - Changes drivers/base/topology.c to use the topology macros unconditionally
> >   and to cope with definitions that aren't lvalues.
> > - Updates documentation accordingly.
> 
> I get a huuuuuuuge crash early in boot on the t61p dual x86_64 laptop. 
> Before netconsole has started and it has no serial port, so all I have is a
> partially-scrolled-off jpg, sorry.
>
> I'll drop the patch.
<snip>

Damn, x86 is also using functions instead of macros.  Sorry about that; I'll
have another try.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.