cputopology: Always define CPU topology information [4th try]

PATCH In review Ben Hutchings 4 participants 11 messages Jun 4, 2008 – Jul 16, 2008

Subsystems (2)

Documentation Driver core

lore

Share & tools

lore

Ben Hutchings

author

2008-06-04 15:45 UTC #0 lore raw

PATCH 1/1 cputopology: Always define CPU topology information [4th try]

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 <linux/topology.h>
- 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 <[email protected]>

---
 Documentation/cputopology.txt |   26 +++++++++-----------------
 drivers/base/topology.c       |   38 ++++++++++----------------------------
 include/linux/topology.h      |   13 +++++++++++++
 3 files changed, 32 insertions(+), 45 deletions(-)

Documentation/cputopology.txt +9 ~~-17~~

index b61cb95..bd699da 100644
@@ -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:
+For an architecture 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/linux/topology.h
+provides default definitions for any of the above macros that are
+not defined by include/asm-XXX/topology.h:
+1) physical_package_id: -1
+2) core_id: 0
+3) thread_siblings: just the given CPU
+4) core_siblings: just the given CPU

drivers/base/topology.c +10 ~~-28~~

index 1efe162..8999828 100644
@@ -62,14 +62,16 @@ static ssize_t show_cpumap(int type, cpumask_t *mask, char *buf)
 static ssize_t show_##name(struct sys_device *dev, char *buf)	\
 {									\
 	unsigned int cpu = dev->id;					\
-	return show_cpumap(0, &(topology_##name(cpu)), buf);		\
+	cpumask_t siblings = topology_##name(cpu);			\
+	return show_cpumap(0, &siblings, buf);				\
 }
 
 #define define_siblings_show_list(name)					\
 static ssize_t show_##name##_list(struct sys_device *dev, char *buf) \
 {									\
 	unsigned int cpu = dev->id;					\
-	return show_cpumap(1, &(topology_##name(cpu)), buf);		\
+	cpumask_t siblings = topology_##name(cpu);			\
+	return show_cpumap(1, &siblings, buf);				\
 }
 
 #else

@@ -93,47 +95,27 @@ static ssize_t show_##name##_list(struct sys_device *dev, char *buf) \
 #define define_siblings_show_func(name)		\
 	define_siblings_show_map(name); define_siblings_show_list(name)
 
-#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_one_ro(thread_siblings_list);
-#define ref_thread_siblings_attr	\
-		&attr_thread_siblings.attr, &attr_thread_siblings_list.attr,
-#else
-#define ref_thread_siblings_attr
-#endif
 
-#ifdef topology_core_siblings
 define_siblings_show_func(core_siblings);
 define_one_ro(core_siblings);
 define_one_ro(core_siblings_list);
-#define ref_core_siblings_attr		\
-		&attr_core_siblings.attr, &attr_core_siblings_list.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_thread_siblings_list.attr,
+	&attr_core_siblings.attr,
+	&attr_core_siblings_list.attr,
 	NULL
 };

include/linux/topology.h +13 -0

index 24f3d22..2158fc0 100644
@@ -179,4 +179,17 @@ void arch_update_cpu_topology(void);
 #endif
 #endif /* CONFIG_NUMA */
 
+#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 /* _LINUX_TOPOLOGY_H */

Signature · 2 lines

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

Andrew Morton

2008-06-05 04:47 UTC #1 lore raw

Quoted 9 lines · On Wed, 4 Jun 2008 16:44:56 +0100 Ben Hutchings <[email protected]>…

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.

Quoted 6 lines

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

See, this is what I meant. After your patch we have:

#ifdef arch_provides_topology_pointers
#define define_siblings_show_map(name) \
static ssize_t show_##name(struct sys_device *dev, char *buf) \

{									\
	unsigned int cpu = dev->id;					\
	cpumask_t siblings = topology_##name(cpu);			\
	return show_cpumap(0, &siblings, buf);				\
}

#define define_siblings_show_list(name) \
static ssize_t show_##name##_list(struct sys_device *dev, char *buf) \

{									\
	unsigned int cpu = dev->id;					\
	cpumask_t siblings = topology_##name(cpu);			\
	return show_cpumap(1, &siblings, buf);				\
}

#else
#define define_siblings_show_map(name) \
static ssize_t show_##name(struct sys_device *dev, char *buf) \

{									\
	unsigned int cpu = dev->id;					\
	cpumask_t mask = topology_##name(cpu);				\
	return show_cpumap(0, &mask, buf);				\
}

#define define_siblings_show_list(name) \
static ssize_t show_##name##_list(struct sys_device *dev, char *buf) \

{									\
	unsigned int cpu = dev->id;					\
	cpumask_t mask = topology_##name(cpu);				\
	return show_cpumap(1, &mask, buf);				\
}

#endif

they're the same!

Ben Hutchings

author

2008-06-05 12:09 UTC #2 lore raw

Quoted 11 lines · Andrew Morton

Quoted 9 lines · On Wed, 4 Jun 2008 16:44:56 +0100 Ben Hutchings <[email protected]>…
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.

Quoted 8 lines

Quoted 6 lines
This patch:
- Adds default definitions of the 4 topology macros to <linux/topology.h>
- Changes drivers/base/topology.c to use the topology macros unconditionally and to cope with definitions that aren't lvalues
- Updates documentation accordingly
See, this is what I meant. After your patch we have:

[...]

Sorry, I don't know how that escaped me. My changes to the show functions should be unnecessary - though I think that the two different implementations for lvalues and rvalues are a premature optimisation.

Ben.

Signature · 2 lines

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

Andrew Morton

2008-06-05 16:29 UTC #3 lore raw

Quoted 19 lines · On Thu, 5 Jun 2008 13:08:30 +0100 Ben Hutchings <[email protected]>…

Quoted 18 lines · Andrew Morton
Quoted 15 lines · On Wed, 4 Jun 2008 16:44:56 +0100 Ben Hutchings <[email protected]>…
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 <linux/topology.h>
- Changes drivers/base/topology.c to use the topology macros unconditionally and to cope with definitions that aren't lvalues
- Updates documentation accordingly

Quoted 6 lines

Quoted 1 line
See, this is what I meant. After your patch we have:
[...]
Sorry, I don't know how that escaped me. My changes to the show functions should be unnecessary - though I think that the two different implementations for lvalues and rvalues are a premature optimisation.

um, what does this mean. Will you be sending an updated patch or should I drop those two or...?

Ben Hutchings

author

2008-06-05 16:37 UTC #4 lore raw

PATCH 1/1 cputopology: Always define CPU topology information [5th try]

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 <linux/topology.h>
- Changes drivers/base/topology.c to use the topology macros unconditionally
- Updates documentation accordingly

Signed-off-by Ben Hutchings <[email protected]>

---
 Documentation/cputopology.txt |   26 +++++++++-----------------
 drivers/base/topology.c       |   32 ++++++--------------------------
 include/linux/topology.h      |   13 +++++++++++++
 3 files changed, 28 insertions(+), 43 deletions(-)

Documentation/cputopology.txt +9 ~~-17~~

index b61cb95..bd699da 100644
@@ -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:
+For an architecture 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/linux/topology.h
+provides default definitions for any of the above macros that are
+not defined by include/asm-XXX/topology.h:
+1) physical_package_id: -1
+2) core_id: 0
+3) thread_siblings: just the given CPU
+4) core_siblings: just the given CPU

drivers/base/topology.c +6 ~~-26~~

index 1efe162..3f6d9b0 100644
@@ -93,47 +93,27 @@ static ssize_t show_##name##_list(struct sys_device *dev, char *buf) \
 #define define_siblings_show_func(name)		\
 	define_siblings_show_map(name); define_siblings_show_list(name)
 
-#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_one_ro(thread_siblings_list);
-#define ref_thread_siblings_attr	\
-		&attr_thread_siblings.attr, &attr_thread_siblings_list.attr,
-#else
-#define ref_thread_siblings_attr
-#endif
 
-#ifdef topology_core_siblings
 define_siblings_show_func(core_siblings);
 define_one_ro(core_siblings);
 define_one_ro(core_siblings_list);
-#define ref_core_siblings_attr		\
-		&attr_core_siblings.attr, &attr_core_siblings_list.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_thread_siblings_list.attr,
+	&attr_core_siblings.attr,
+	&attr_core_siblings_list.attr,
 	NULL
 };

include/linux/topology.h +13 -0

index 24f3d22..2158fc0 100644
@@ -179,4 +179,17 @@ void arch_update_cpu_topology(void);
 #endif
 #endif /* CONFIG_NUMA */
 
+#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 /* _LINUX_TOPOLOGY_H */

Signature · 2 lines

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

Ingo Molnar

Applied

2008-06-13 05:17 UTC #5 lore raw

Re: [PATCH] cputopology: Always define CPU topology information [5th try]

Quoted 9 lines · 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.

Quoted 5 lines

This patch:
- Adds default definitions of the 4 topology macros to <linux/topology.h>
- Changes drivers/base/topology.c to use the topology macros unconditionally
- Updates documentation accordingly

applied to tip/core/topology - thanks Ben.

Ingo

Ben Hutchings

author

2008-06-13 10:35 UTC #6 lore raw

Re: [PATCH] cputopology: Always define CPU topology information [5th try]

Quoted 12 lines · Ingo Molnar

Quoted 9 lines · 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.

Quoted 7 lines

Quoted 5 lines
This patch:
- Adds default definitions of the 4 topology macros to <linux/topology.h>
- Changes drivers/base/topology.c to use the topology macros unconditionally
- Updates documentation accordingly
applied to tip/core/topology - thanks Ben.

Unfortunately you lost the first paragraph of the commit message, then created a second commit (difference between 4th and 5th versions) with your own commit message and me as the author. Maybe it's nitpicking but I would prefer to have strictly accurate metadata in the commit log.

Ben.

Signature · 2 lines

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

Ingo Molnar

2008-06-13 11:02 UTC #7 lore raw

Re: [PATCH] cputopology: Always define CPU topology information [5th try]

Quoted 6 lines · Ben Hutchings

Quoted 6 lines
Quoted 5 lines
This patch:
- Adds default definitions of the 4 topology macros to <linux/topology.h>
- Changes drivers/base/topology.c to use the topology macros unconditionally
- Updates documentation accordingly

Quoted 7 lines

Quoted 1 line
applied to tip/core/topology - thanks Ben.
Unfortunately you lost the first paragraph of the commit message, then created a second commit (difference between 4th and 5th versions) with your own commit message and me as the author. Maybe it's nitpicking but I would prefer to have strictly accurate metadata in the commit log.

that's OK, see the delta patch below, it's rather trivial. It's better to do these small delta patches where each change stands on its own than to review a more complex combo patch. (Suggest me any other commit metadata for this delta if you prefer, we can still change it.)

Ingo

-------------->
commit 131b943ae643b1ad6febd67cdbb31d955706ecf4

Author: Ben Hutchings <[email protected]>
Date:   Thu Jun 5 17:37:15 2008 +0100

cputopology: always define CPU topology information, clean up

simplify drivers/base/topology.c a bit.

Signed-off-by Ben Hutchings <[email protected]>

Signed-off-by Ingo Molnar <[email protected]>

drivers/base/topology.c +2 -4

index 24d29a9..f0cb270 100644
@@ -59,16 +59,14 @@ static ssize_t show_cpumap(int type, cpumask_t *mask, char *buf)
 static inline ssize_t show_##name(struct sys_device *dev, char *buf)	\
 {									\
 	unsigned int cpu = dev->id;					\
-	cpumask_t siblings = topology_##name(cpu);			\
-	return show_cpumap(0, &siblings, buf);				\
+	return show_cpumap(0, &(topology_##name(cpu)), buf);		\
 }
 
 #define define_siblings_show_list(name)					\
 static inline ssize_t show_##name##_list(struct sys_device *dev, char *buf) \
 {									\
 	unsigned int cpu = dev->id;					\
-	cpumask_t siblings = topology_##name(cpu);			\
-	return show_cpumap(1, &siblings, buf);				\
+	return show_cpumap(1, &(topology_##name(cpu)), buf);		\
 }
 
 #define define_siblings_show_func(name)		\

Ben Hutchings

author

2008-06-13 15:37 UTC #8 lore raw

Re: [PATCH] cputopology: Always define CPU topology information [5th try]

Quoted 17 lines · Ingo Molnar

Quoted 6 lines · Ben Hutchings
Quoted 6 lines
Quoted 5 lines
This patch:
- Adds default definitions of the 4 topology macros to <linux/topology.h>
- Changes drivers/base/topology.c to use the topology macros unconditionally
- Updates documentation accordingly
Quoted 7 lines
Quoted 1 line
applied to tip/core/topology - thanks Ben.
Unfortunately you lost the first paragraph of the commit message, then created a second commit (difference between 4th and 5th versions) with your own commit message and me as the author. Maybe it's nitpicking but I would prefer to have strictly accurate metadata in the commit log.

Quoted 4 lines

that's OK, see the delta patch below, it's rather trivial. It's better to do these small delta patches where each change stands on its own than to review a more complex combo patch. (Suggest me any other commit metadata for this delta if you prefer, we can still change it.)

[...]

The message should be:

Revert first two hunks of "cputopology: Always define CPU topology information" which were redundant with "x86: cleanup early per cpu variables/accesses v4".

Ben.

Signature · 2 lines

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

Nathan Lynch

2008-07-16 21:37 UTC #9 lore raw

In reply to Ben Hutchings

Hi Ben-

Quoted 8 lines · 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.

I've been looking at adding topology information to powerpc and I came across this.

I understand the need for fallback definitions of the topology APIs within the kernel, but I'm not sure I agree with exposing these things in sysfs unconditionally -- the default values for physical_package_id and core_id don't really make sense on powerpc (and other non-x86 architectures, I suspect).

Would you object to a patch which exposes in sysfs only the topology information which the architecture provides?

Ben Hutchings

author

2008-07-16 23:11 UTC #10 lore raw

Quoted 12 lines · Nathan Lynch

Hi Ben-
Quoted 8 lines · 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.

Quoted 8 lines

I've been looking at adding topology information to powerpc and I came across this.
I understand the need for fallback definitions of the topology APIs within the kernel, but I'm not sure I agree with exposing these things in sysfs unconditionally -- the default values for physical_package_id and core_id don't really make sense on powerpc (and other non-x86 architectures, I suspect).

In what way are they wrong?

Quoted 2 lines

Would you object to a patch which exposes in sysfs only the topology information which the architecture provides?

I was primarily concerned with having the fallbacks available in-kernel. However, I don't think you will be doing user-space any favours by requiring checks for missing attributes for ever.

Ben.

Signature · 2 lines

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