Change pcibus_to_cpumask() to pcibus_to_node()

RFC PATCH In review Matthew Dobson 8 participants 23 messages Jul 27, 2004 – Jul 30, 2004

Subsystems (2)

PCI Generic include/asm header files

lore

Share & tools

lore

Matthew Dobson

author

2004-07-27 00:10 UTC #0 lore raw

RFC PATCH 1/1 Change pcibus_to_cpumask() to pcibus_to_node()

So in discussions with Jesse at OLS, we decided that pcibus_to_node() is a more generally useful function than pcibus_to_cpumask(). If anyone disagrees with that, now would be a good time to let us know.

This is just a preliminary patch. It needs review for x86_64, as I don't know how to properly populate the mp_bus_to_node (which used to be mp_bus_to_cpumask) array.

The main changes are as follows:

1) Replace instances of pcibus_to_cpumask(bus) with node_to_cpumask(pcibus_to_node(bus)). There are currently only 2 uses of pcibus_to_cpumask(): flush_gart() in arch/x86_64/kernel/pci-gart.c and pci_bus_show_cpuaffinity() in drivers/pci/probe.c.
2) Define the asm-generic version of pcibus_to_node() to always return node 0, as this is the sensible non-NUMA behavior.
3) Drop the mips/mach-ip27 and ppc64 versions of pcibus_to_cpumask() entirely, since they were simply defined to be identical to the asm-generic version.
4) Define the i386 version of pcibus_to_node().

Future work:

1) Correctly map PCI buses to nodes for x86_64.
2) IA64 implementation?
3) Other arch implementations?

[mcd@arrakis source]$ diffstat ~/linux/patches/pcibus_to_node.patch

 arch/x86_64/kernel/mpparse.c          |    3 ++-
 arch/x86_64/kernel/pci-gart.c         |    2 +-
 drivers/pci/probe.c                   |    2 +-
 include/asm-generic/topology.h        |    4 ++--
 include/asm-i386/topology.h           |    4 ++--
 include/asm-mips/mach-ip27/topology.h |    1 -
 include/asm-ppc64/topology.h          |    2 --
 include/asm-x86_64/mpspec.h           |    2 +-
 include/asm-x86_64/topology.h         |    6 ++----
 9 files changed, 11 insertions(+), 15 deletions(-)

-Matt

arch/x86_64/kernel/mpparse.c +2 -1

diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.7-mm7/arch/x86_64/kernel/mpparse.c linux-2.6.7-mm7+pcibus_to_node/arch/x86_64/kernel/mpparse.c
@@ -44,7 +44,7 @@ int acpi_found_madt;
 int apic_version [MAX_APICS];
 unsigned char mp_bus_id_to_type [MAX_MP_BUSSES] = { [0 ... MAX_MP_BUSSES-1] = -1 };
 int mp_bus_id_to_pci_bus [MAX_MP_BUSSES] = { [0 ... MAX_MP_BUSSES-1] = -1 };
-cpumask_t mp_bus_to_cpumask [MAX_MP_BUSSES] = { [0 ... MAX_MP_BUSSES-1] = CPU_MASK_ALL };
+int mp_bus_to_node [MAX_MP_BUSSES] = { [0 ... MAX_MP_BUSSES-1] = -1 };
 
 int mp_current_pci_id = 0;
 /* I/O APIC entries */

@@ -169,6 +169,7 @@ static void __init MP_bus_info (struct m
 		mp_bus_id_to_type[m->mpc_busid] = MP_BUS_PCI;
 		mp_bus_id_to_pci_bus[m->mpc_busid] = mp_current_pci_id;
 		mp_current_pci_id++;
+		/* FIXME: Setup PCI bus to Node mapping here? */
 	} else if (strncmp(str, "MCA", 3) == 0) {
 		mp_bus_id_to_type[m->mpc_busid] = MP_BUS_MCA;
 	} else {

arch/x86_64/kernel/pci-gart.c +1 -1

diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.7-mm7/arch/x86_64/kernel/pci-gart.c linux-2.6.7-mm7+pcibus_to_node/arch/x86_64/kernel/pci-gart.c
@@ -148,7 +148,7 @@ static void flush_gart(struct pci_dev *d
 { 
 	unsigned long flags;
 	int bus = dev ? dev->bus->number : -1;
-	cpumask_t bus_cpumask = pcibus_to_cpumask(bus);
+	cpumask_t bus_cpumask = node_to_cpumask(pcibus_to_node(bus));
 	int flushed = 0;
 	int i;

drivers/pci/probe.c +1 -1

diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.7-mm7/drivers/pci/probe.c linux-2.6.7-mm7+pcibus_to_node/drivers/pci/probe.c
@@ -54,7 +54,7 @@ postcore_initcall(pcibus_class_init);
  */
 static ssize_t pci_bus_show_cpuaffinity(struct class_device *class_dev, char *buf)
 {
-	cpumask_t cpumask = pcibus_to_cpumask((to_pci_bus(class_dev))->number);
+	cpumask_t cpumask = node_to_cpumask(pcibus_to_node((to_pci_bus(class_dev))->number));
 	int ret;
 
 	ret = cpumask_scnprintf(buf, PAGE_SIZE, cpumask);

include/asm-generic/topology.h +2 -2

diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.7-mm7/include/asm-generic/topology.h linux-2.6.7-mm7+pcibus_to_node/include/asm-generic/topology.h
@@ -41,8 +41,8 @@
 #ifndef node_to_first_cpu
 #define node_to_first_cpu(node)	(0)
 #endif
-#ifndef pcibus_to_cpumask
-#define pcibus_to_cpumask(bus)	(cpu_online_map)
+#ifndef pcibus_to_node
+#define pcibus_to_node(bus)	(0)
 #endif
 
 /* Cross-node load balancing interval. */

include/asm-i386/topology.h +2 -2

diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.7-mm7/include/asm-i386/topology.h linux-2.6.7-mm7+pcibus_to_node/include/asm-i386/topology.h
@@ -61,9 +61,9 @@ static inline int node_to_first_cpu(int 
 }
 
 /* Returns the number of the node containing PCI bus 'bus' */
-static inline cpumask_t pcibus_to_cpumask(int bus)
+static inline int pcibus_to_node(int bus)
 {
-	return node_to_cpumask(mp_bus_id_to_node[bus]);
+	return mp_bus_id_to_node[bus];
 }
 
 /* Node-to-Node distance */

include/asm-mips/mach-ip27/topology.h +0 -1

diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.7-mm7/include/asm-mips/mach-ip27/topology.h linux-2.6.7-mm7+pcibus_to_node/include/asm-mips/mach-ip27/topology.h
@@ -7,7 +7,6 @@
 #define parent_node(node)	(node)
 #define node_to_cpumask(node)	(HUB_DATA(node)->h_cpus)
 #define node_to_first_cpu(node)	(first_cpu(node_to_cpumask(node)))
-#define pcibus_to_cpumask(bus)	(cpu_online_map)
 
 extern int node_distance(nasid_t nasid_a, nasid_t nasid_b);
 #define node_distance(from, to)	node_distance(from, to)

include/asm-ppc64/topology.h +0 -2

diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.7-mm7/include/asm-ppc64/topology.h linux-2.6.7-mm7+pcibus_to_node/include/asm-ppc64/topology.h
@@ -33,8 +33,6 @@ static inline int node_to_first_cpu(int 
 	return first_cpu(tmp);
 }
 
-#define pcibus_to_cpumask(bus)	(cpu_online_map)
-
 #define nr_cpus_node(node)	(nr_cpus_in_node[node])
 
 /* Cross-node load balancing interval. */

include/asm-x86_64/mpspec.h +1 -1

diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.7-mm7/include/asm-x86_64/mpspec.h linux-2.6.7-mm7+pcibus_to_node/include/asm-x86_64/mpspec.h
@@ -166,7 +166,7 @@ enum mp_bustype {
 };
 extern unsigned char mp_bus_id_to_type [MAX_MP_BUSSES];
 extern int mp_bus_id_to_pci_bus [MAX_MP_BUSSES];
-extern cpumask_t mp_bus_to_cpumask [MAX_MP_BUSSES];
+extern cpumask_t mp_bus_to_node [MAX_MP_BUSSES];
 
 extern unsigned int boot_cpu_physical_apicid;
 extern int smp_found_config;

include/asm-x86_64/topology.h +2 -4

diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.7-mm7/include/asm-x86_64/topology.h linux-2.6.7-mm7+pcibus_to_node/include/asm-x86_64/topology.h
@@ -20,11 +20,9 @@ extern cpumask_t     node_to_cpumask[];
 #define node_to_first_cpu(node) 	(__ffs(node_to_cpumask[node]))
 #define node_to_cpumask(node)		(node_to_cpumask[node])
 
-static inline cpumask_t pcibus_to_cpumask(int bus)
+static inline int pcibus_to_node(int bus)
 {
-	cpumask_t tmp;
-	cpus_and(tmp, mp_bus_to_cpumask[bus], cpu_online_map);
-	return tmp;
+	return mp_bus_to_node[bus];
 }
 
 #define NODE_BALANCE_RATE 30	/* CHECKME */

Jesse Barnes

2004-07-27 03:39 UTC #1 lore raw

Quoted 3 lines · Matthew Dobson

So in discussions with Jesse at OLS, we decided that pcibus_to_node() is a more generally useful function than pcibus_to_cpumask(). If anyone disagrees with that, now would be a good time to let us know.

Thanks for putting the fact that I was an idiot so kindly... :)

Quoted 10 lines

1) Replace instances of pcibus_to_cpumask(bus) with node_to_cpumask(pcibus_to_node(bus)). There are currently only 2 uses of pcibus_to_cpumask(): flush_gart() in arch/x86_64/kernel/pci-gart.c and pci_bus_show_cpuaffinity() in drivers/pci/probe.c.
2) Define the asm-generic version of pcibus_to_node() to always return node 0, as this is the sensible non-NUMA behavior.
3) Drop the mips/mach-ip27 and ppc64 versions of pcibus_to_cpumask() entirely, since they were simply defined to be identical to the asm-generic version.
4) Define the i386 version of pcibus_to_node().

Looks good to me.

Quoted 4 lines

Future work:
1) Correctly map PCI buses to nodes for x86_64.
2) IA64 implementation?

I'll put this together, though the implementation will probably change as we add PROM support in the SLIT and SRAT tables for our host to PCI bridges.

Platforms that support it should probably also use pcibus_to_node in their pci_alloc_consistent and dma_alloc_coherent APIs if possible.

Thanks,
Jesse

Christoph Hellwig

2004-07-27 09:52 UTC #2 lore raw

In reply to Matthew Dobson

Quoted 1 line · Matthew Dobson

So in discussions with Jesse at OLS, we decided that pcibus_to_node() is

Please do pcibus_to_nodemask() instead - there could be dual-ported pci bridges.

Jesse Barnes

2004-07-27 15:30 UTC #3 lore raw

Quoted 5 lines · Christoph Hellwig

Quoted 1 line · Matthew Dobson
So in discussions with Jesse at OLS, we decided that pcibus_to_node() is
Please do pcibus_to_nodemask() instead - there could be dual-ported pci bridges.

Do you know of any? On sn2 there are dual ported xio->pci bridges, but in that case, half the busses are associated with one node and the other half with another node, so pcibus_to_node would work in that case. And for numalink->pci bridges, we'll return the node id of the bridge in that case (which may not have any memory, but in that case alloc_pages_node will fall back to the next node).

I wonder though if we shouldn't add

...
#ifdef CONFIG_NUMA
int node; /* or nodemask_t if necessary */
#endif
...

to struct pci_bus instead? That would make the existing code paths a little faster and avoid the need for a global array, which tends to lead to TLB misses.

Anyway, my needs are very simple. I'd like to do alloc_pages_node(pci_to_node(pci_dev)); in the sn2 version of pci_alloc_consistent and use the new routine to simplify the initial irq setup code, making it look more like build_zonelists and the sched domains patch I posted yesterday. So as long as those needs are provided for, I'm ok with the interface.

Thanks,
Jesse

Matthew Dobson

author

2004-07-27 18:34 UTC #4 lore raw

Quoted 22 lines · Jesse Barnes

Quoted 5 lines · Christoph Hellwig
Quoted 1 line · Matthew Dobson
So in discussions with Jesse at OLS, we decided that pcibus_to_node() is
Please do pcibus_to_nodemask() instead - there could be dual-ported pci bridges.
Do you know of any? On sn2 there are dual ported xio->pci bridges, but in that case, half the busses are associated with one node and the other half with another node, so pcibus_to_node would work in that case. And for numalink->pci bridges, we'll return the node id of the bridge in that case (which may not have any memory, but in that case alloc_pages_node will fall back to the next node).
I wonder though if we shouldn't add
...
#ifdef CONFIG_NUMA
int node; /* or nodemask_t if necessary */
#endif
...

Quoted 3 lines

to struct pci_bus instead? That would make the existing code paths a little faster and avoid the need for a global array, which tends to lead to TLB misses.

I like that idea! Stick a nodemask_t in struct pci_bus, initialize it to NODE_MASK_ALL. If a particular arch wants to put something more accurate in there, then great, if not, we're just in the same boat we're in now.

Anyone else have opinions one way or the other on Jesse's idea?

Quoted 9 lines

Anyway, my needs are very simple. I'd like to do alloc_pages_node(pci_to_node(pci_dev)); in the sn2 version of pci_alloc_consistent and use the new routine to simplify the initial irq setup code, making it look more like build_zonelists and the sched domains patch I posted yesterday. So as long as those needs are provided for, I'm ok with the interface.
Thanks,
Jesse

I'm trying to keep the dependency of topology on what the pci_dev and pci_bus structs look like to a minimum. That's why I'd like to keep the topology function based on PCI bus numbers (or possibly struct pci_bus), not struct pci_dev. The pci_bus is what really has the node affinity anyway, and the device only has that affinity through the fact that it is physically plugged into a particular bus.

-Matt

Jesse Barnes

2004-07-27 18:53 UTC #5 lore raw

Quoted 6 lines · Matthew Dobson

Quoted 6 lines
...
#ifdef CONFIG_NUMA
int node; /* or nodemask_t if necessary */
#endif
...

Quoted 8 lines

Quoted 3 lines
to struct pci_bus instead? That would make the existing code paths a little faster and avoid the need for a global array, which tends to lead to TLB misses.
I like that idea! Stick a nodemask_t in struct pci_bus, initialize it to NODE_MASK_ALL. If a particular arch wants to put something more accurate in there, then great, if not, we're just in the same boat we're in now.

Cool, sounds like that'll work well.

Quoted 6 lines

I'm trying to keep the dependency of topology on what the pci_dev and pci_bus structs look like to a minimum. That's why I'd like to keep the topology function based on PCI bus numbers (or possibly struct pci_bus), not struct pci_dev. The pci_bus is what really has the node affinity anyway, and the device only has that affinity through the fact that it is physically plugged into a particular bus.

Sure, that make sense. And it's easy enough to get a pci_bus from a pci_dev that we probably won't run into trouble.

Thanks,
Jesse

Matthew Dobson

author

2004-07-29 00:07 UTC #6 lore raw

Quoted 11 lines · Jesse Barnes

Quoted 10 lines · Matthew Dobson
Quoted 9 lines
...
#ifdef CONFIG_NUMA
int node; /* or nodemask_t if necessary */
#endif
...
to struct pci_bus instead? That would make the existing code paths a little faster and avoid the need for a global array, which tends to lead to TLB misses.

Quoted 6 lines

Quoted 4 lines
I like that idea! Stick a nodemask_t in struct pci_bus, initialize it to NODE_MASK_ALL. If a particular arch wants to put something more accurate in there, then great, if not, we're just in the same boat we're in now.
Cool, sounds like that'll work well.

Ok, so I'm no longer convinced that this will work as well as I once thought. It's pretty trivial to add a nodemask_t to the struct pci_bus, and even initialize it to a reasonable value (ie: NODE_MASK_ALL) since there's the convenient pci_alloc_bus() function in drivers/pci/probe.c. The problem is where to put hooks for individual arches to put the *real* nodemask in this field... My only thought right now is to create a per-arch callback function, arch_get_pcibus_nodemask() or something, and use the value it returns to populate pci_bus->nodemask. We would have to call this function anywhere a struct pci_bus is allocated, and probably pass along the PCI bus number so the arch could determine which nodes it belongs to. Would that work for everyone that cares? We could overload that to return NODE_MASK_ALL for non-NUMA systems, and have it do the right thing for arches that care...

Current, nowhere near complete patch attached...

-Matt

arch/x86_64/kernel/mpparse.c +0 -1

diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.8-rc2-mm1/arch/x86_64/kernel/mpparse.c linux-2.6.8-rc2-mm1+pcibus_to_nodemask/arch/x86_64/kernel/mpparse.c
@@ -44,7 +44,6 @@ int acpi_found_madt;
 int apic_version [MAX_APICS];
 unsigned char mp_bus_id_to_type [MAX_MP_BUSSES] = { [0 ... MAX_MP_BUSSES-1] = -1 };
 int mp_bus_id_to_pci_bus [MAX_MP_BUSSES] = { [0 ... MAX_MP_BUSSES-1] = -1 };
-cpumask_t pci_bus_to_cpumask [256] = { [0 ... 255] = CPU_MASK_ALL };
 
 int mp_current_pci_id = 0;
 /* I/O APIC entries */

drivers/pci/probe.c +3 -2

diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.8-rc2-mm1/drivers/pci/probe.c linux-2.6.8-rc2-mm1+pcibus_to_nodemask/drivers/pci/probe.c
@@ -6,7 +6,7 @@
 #include <linux/pci.h>
 #include <linux/slab.h>
 #include <linux/module.h>
-#include <linux/cpumask.h>
+#include <linux/topology.h>
 
 #undef DEBUG

@@ -54,7 +54,7 @@ postcore_initcall(pcibus_class_init);
  */
 static ssize_t pci_bus_show_cpuaffinity(struct class_device *class_dev, char *buf)
 {
-	cpumask_t cpumask = pcibus_to_cpumask((to_pci_bus(class_dev))->number);
+	cpumask_t cpumask = nodemask_to_cpumask(pcibus_to_nodemask(to_pci_bus(class_dev)));
 	int ret;
 
 	ret = cpumask_scnprintf(buf, PAGE_SIZE, cpumask);

@@ -270,6 +270,7 @@ static struct pci_bus * __devinit pci_al
 		INIT_LIST_HEAD(&b->node);
 		INIT_LIST_HEAD(&b->children);
 		INIT_LIST_HEAD(&b->devices);
+		b->nodemask = NODE_MASK_ALL;
 	}
 	return b;
 }

include/asm-alpha/topology.h +0 -2

diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.8-rc2-mm1/include/asm-alpha/topology.h linux-2.6.8-rc2-mm1+pcibus_to_nodemask/include/asm-alpha/topology.h
@@ -42,8 +42,6 @@ static inline cpumask_t node_to_cpumask(
 /* Cross-node load balancing interval. */
 # define NODE_BALANCE_RATE 10
 
-#define pcibus_to_cpumask(bus)	(cpu_online_map)
-
 #else /* CONFIG_NUMA */
 # include <asm-generic/topology.h>
 #endif /* !CONFIG_NUMA */

include/asm-generic/topology.h +0 -3

diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.8-rc2-mm1/include/asm-generic/topology.h linux-2.6.8-rc2-mm1+pcibus_to_nodemask/include/asm-generic/topology.h
@@ -41,9 +41,6 @@
 #ifndef node_to_first_cpu
 #define node_to_first_cpu(node)	(0)
 #endif
-#ifndef pcibus_to_cpumask
-#define pcibus_to_cpumask(bus)	(cpu_online_map)
-#endif
 
 /* Cross-node load balancing interval. */
 #ifndef NODE_BALANCE_RATE

include/asm-i386/topology.h +0 -6

diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.8-rc2-mm1/include/asm-i386/topology.h linux-2.6.8-rc2-mm1+pcibus_to_nodemask/include/asm-i386/topology.h
@@ -60,12 +60,6 @@ static inline int node_to_first_cpu(int 
 	return first_cpu(mask);
 }
 
-/* Returns the number of the node containing PCI bus 'bus' */
-static inline cpumask_t pcibus_to_cpumask(int bus)
-{
-	return node_to_cpumask(mp_bus_id_to_node[bus]);
-}
-
 /* Node-to-Node distance */
 #define node_distance(from, to) (from != to)

include/asm-mips/mach-ip27/topology.h +0 -1

diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.8-rc2-mm1/include/asm-mips/mach-ip27/topology.h linux-2.6.8-rc2-mm1+pcibus_to_nodemask/include/asm-mips/mach-ip27/topology.h
@@ -7,7 +7,6 @@
 #define parent_node(node)	(node)
 #define node_to_cpumask(node)	(HUB_DATA(node)->h_cpus)
 #define node_to_first_cpu(node)	(first_cpu(node_to_cpumask(node)))
-#define pcibus_to_cpumask(bus)	(cpu_online_map)
 
 extern int node_distance(nasid_t nasid_a, nasid_t nasid_b);
 #define node_distance(from, to)	node_distance(from, to)

include/asm-ppc64/topology.h +0 -2

diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.8-rc2-mm1/include/asm-ppc64/topology.h linux-2.6.8-rc2-mm1+pcibus_to_nodemask/include/asm-ppc64/topology.h
@@ -33,8 +33,6 @@ static inline int node_to_first_cpu(int 
 	return first_cpu(tmp);
 }
 
-#define pcibus_to_cpumask(bus)	(cpu_online_map)
-
 #define nr_cpus_node(node)	(nr_cpus_in_node[node])
 
 /* Cross-node load balancing interval. */

include/asm-x86_64/mpspec.h +0 -1

diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.8-rc2-mm1/include/asm-x86_64/mpspec.h linux-2.6.8-rc2-mm1+pcibus_to_nodemask/include/asm-x86_64/mpspec.h
@@ -166,7 +166,6 @@ enum mp_bustype {
 };
 extern unsigned char mp_bus_id_to_type [MAX_MP_BUSSES];
 extern int mp_bus_id_to_pci_bus [MAX_MP_BUSSES];
-extern cpumask_t pci_bus_to_cpumask [256];
 
 extern unsigned int boot_cpu_physical_apicid;
 extern int smp_found_config;

include/asm-x86_64/topology.h +0 -7

diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.8-rc2-mm1/include/asm-x86_64/topology.h linux-2.6.8-rc2-mm1+pcibus_to_nodemask/include/asm-x86_64/topology.h
@@ -20,13 +20,6 @@ extern cpumask_t     node_to_cpumask[];
 #define node_to_first_cpu(node) 	(__ffs(node_to_cpumask[node]))
 #define node_to_cpumask(node)		(node_to_cpumask[node])
 
-static inline cpumask_t pcibus_to_cpumask(int bus)
-{
-	cpumask_t res;
-	cpus_and(res,  pci_bus_to_cpumask[bus], cpu_online_map);
-	return res;
-}
-
 #define NODE_BALANCE_RATE 30	/* CHECKME */ 
 
 #endif

include/linux/pci.h +2 -0

diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.8-rc2-mm1/include/linux/pci.h linux-2.6.8-rc2-mm1+pcibus_to_nodemask/include/linux/pci.h
@@ -590,6 +590,8 @@ struct pci_bus {
 	unsigned short  pad2;
 	struct device		*bridge;
 	struct class_device	class_dev;
+	nodemask_t	nodemask;	/* For NUMA systems, we care about which 
+					   node(s) this PCI bus is on/close to. */
 };
 
 #define pci_bus_b(n)	list_entry(n, struct pci_bus, node)

include/linux/topology.h +18 -0

diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.8-rc2-mm1/include/linux/topology.h linux-2.6.8-rc2-mm1+pcibus_to_nodemask/include/linux/topology.h
@@ -28,6 +28,7 @@
 #define _LINUX_TOPOLOGY_H
 
 #include <linux/cpumask.h>
+#include <linux/nodemask.h>
 #include <linux/bitops.h>
 #include <linux/mmzone.h>
 #include <linux/smp.h>

@@ -54,6 +55,18 @@ static inline int __next_node_with_cpus(
 #define for_each_node_with_cpus(node) \
 	for (node = 0; node < numnodes; node = __next_node_with_cpus(node))
 
+static inline cpumask_t nodemask_to_cpumask(nodemask_t nodemask)
+{
+	cpumask_t ret, tmp;
+	int node;
+	cpus_clear(ret);
+	for_each_node_mask(node, nodemask) {
+		tmp = node_to_cpumask(node);
+		cpus_or(ret, ret, tmp);
+	}
+	return ret;
+}
+
 #ifndef node_distance
 #define node_distance(from,to)	(from != to)
 #endif

@@ -61,4 +74,9 @@ static inline int __next_node_with_cpus(
 #define PENALTY_FOR_NODE_WITH_CPUS	(1)
 #endif
 
+static inline nodemask_t pcibus_to_nodemask(struct pci_bus *bus)
+{
+	return bus->nodemask;
+}
+
 #endif /* _LINUX_TOPOLOGY_H */

Jesse Barnes

2004-07-29 16:01 UTC #7 lore raw

Quoted 7 lines · Matthew Dobson

Ok, so I'm no longer convinced that this will work as well as I once thought. It's pretty trivial to add a nodemask_t to the struct pci_bus, and even initialize it to a reasonable value (ie: NODE_MASK_ALL) since there's the convenient pci_alloc_bus() function in drivers/pci/probe.c. The problem is where to put hooks for individual arches to put the *real* nodemask in this field... My only thought right now is to create a per-arch callback function, arch_get_pcibus_nodemask() or something,

Yeah, that sounds reasonable. You could protect a generic definition with #ifndef ARCH_HAS_PCIBUS_TO_NODEMASK or something...

Quoted 6 lines

and use the value it returns to populate pci_bus->nodemask. We would have to call this function anywhere a struct pci_bus is allocated, and probably pass along the PCI bus number so the arch could determine which nodes it belongs to. Would that work for everyone that cares? We could overload that to return NODE_MASK_ALL for non-NUMA systems, and have it do the right thing for arches that care...

Yeah, I think that would work. The alternative is to simply add the field, initialize it in pci_alloc_bus like you're doing, and leave it to the arches to fill it in however they see fit.

Jesse

Matthew Dobson

author

2004-07-29 22:40 UTC #8 lore raw

Quoted 19 lines · Jesse Barnes

Quoted 7 lines · Matthew Dobson
Ok, so I'm no longer convinced that this will work as well as I once thought. It's pretty trivial to add a nodemask_t to the struct pci_bus, and even initialize it to a reasonable value (ie: NODE_MASK_ALL) since there's the convenient pci_alloc_bus() function in drivers/pci/probe.c. The problem is where to put hooks for individual arches to put the *real* nodemask in this field... My only thought right now is to create a per-arch callback function, arch_get_pcibus_nodemask() or something,
Yeah, that sounds reasonable. You could protect a generic definition with #ifndef ARCH_HAS_PCIBUS_TO_NODEMASK or something...
Quoted 6 lines
and use the value it returns to populate pci_bus->nodemask. We would have to call this function anywhere a struct pci_bus is allocated, and probably pass along the PCI bus number so the arch could determine which nodes it belongs to. Would that work for everyone that cares? We could overload that to return NODE_MASK_ALL for non-NUMA systems, and have it do the right thing for arches that care...

Quoted 5 lines

Yeah, I think that would work. The alternative is to simply add the field, initialize it in pci_alloc_bus like you're doing, and leave it to the arches to fill it in however they see fit.
Jesse

Ok... Still an RFC, but moving closer to something that we can use. Anyone have any comments on this untested iteration? ;)

What I'm doing is basically ripping out all the old pcibus_to_cpumask() calls. The only arch that defined it to be anything other than CPU_MASK_ALL was i386, and theirs should still work. x86_64 had the beginnings of a PCI bus to CPU mask mapping, but it was never filled in, just populated with CPU_MASK_ALL, so it does the same with NODE_MASK_ALL now. Those two arches, in their include/asm-$ARCH/topology.h define both ARCH_HAS_GET_PCIBUS_NODEMASK and get_pcibus_nodemask(bus). include/linux/topology.h defines a simple get_pcibus_nodemask(bus) if there isn't an arch-specific one provided. We then, in drivers/pci/probe.c, populate the nodemask field of struct pci_bus with this nodemask. Lookup involves simply returning the nodemask stored in the struct pci_bus.

[mcd@arrakis source]$ diffstat ~/linux/patches/pcibus_to_nodemask.patch

 arch/x86_64/kernel/mpparse.c          |    2 +-
 drivers/pci/probe.c                   |    6 ++++--
 include/asm-alpha/topology.h          |    2 --
 include/asm-generic/topology.h        |    3 ---
 include/asm-i386/topology.h           |    7 ++-----
 include/asm-mips/mach-ip27/topology.h |    1 -
 include/asm-ppc64/topology.h          |    2 --
 include/asm-x86_64/mpspec.h           |    2 +-
 include/asm-x86_64/topology.h         |   10 +++-------
 include/linux/pci.h                   |    2 ++
 include/linux/topology.h              |   22 ++++++++++++++++++++++
 11 files changed, 35 insertions(+), 24 deletions(-)

-Matt

arch/x86_64/kernel/mpparse.c +1 -1

diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.8-rc2-mm1/arch/x86_64/kernel/mpparse.c linux-2.6.8-rc2-mm1+pcibus_to_nodemask/arch/x86_64/kernel/mpparse.c
@@ -44,7 +44,7 @@ int acpi_found_madt;
 int apic_version [MAX_APICS];
 unsigned char mp_bus_id_to_type [MAX_MP_BUSSES] = { [0 ... MAX_MP_BUSSES-1] = -1 };
 int mp_bus_id_to_pci_bus [MAX_MP_BUSSES] = { [0 ... MAX_MP_BUSSES-1] = -1 };
-cpumask_t pci_bus_to_cpumask [256] = { [0 ... 255] = CPU_MASK_ALL };
+nodemask_t pci_bus_to_nodemask [256] = { [0 ... 255] = NODE_MASK_ALL };
 
 int mp_current_pci_id = 0;
 /* I/O APIC entries */

drivers/pci/probe.c +4 -2

diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.8-rc2-mm1/drivers/pci/probe.c linux-2.6.8-rc2-mm1+pcibus_to_nodemask/drivers/pci/probe.c
@@ -6,7 +6,7 @@
 #include <linux/pci.h>
 #include <linux/slab.h>
 #include <linux/module.h>
-#include <linux/cpumask.h>
+#include <linux/topology.h>
 
 #undef DEBUG

@@ -54,7 +54,7 @@ postcore_initcall(pcibus_class_init);
  */
 static ssize_t pci_bus_show_cpuaffinity(struct class_device *class_dev, char *buf)
 {
-	cpumask_t cpumask = pcibus_to_cpumask((to_pci_bus(class_dev))->number);
+	cpumask_t cpumask = nodemask_to_cpumask(pcibus_to_nodemask(to_pci_bus(class_dev)));
 	int ret;
 
 	ret = cpumask_scnprintf(buf, PAGE_SIZE, cpumask);

@@ -305,6 +305,7 @@ pci_alloc_child_bus(struct pci_bus *pare
 	child->number = child->secondary = busnr;
 	child->primary = parent->secondary;
 	child->subordinate = 0xff;
+	child->nodemask = get_pcibus_nodemask(busnr);
 
 	/* Set up default resource pointers and names.. */
 	for (i = 0; i < 4; i++) {

@@ -786,6 +787,7 @@ struct pci_bus * __devinit pci_scan_bus_
 	b->number = b->secondary = bus;
 	b->resource[0] = &ioport_resource;
 	b->resource[1] = &iomem_resource;
+	b->nodemask = get_pcibus_nodemask(bus);
 
 	b->subordinate = pci_scan_child_bus(b);

include/asm-alpha/topology.h +0 -2

diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.8-rc2-mm1/include/asm-alpha/topology.h linux-2.6.8-rc2-mm1+pcibus_to_nodemask/include/asm-alpha/topology.h
@@ -42,8 +42,6 @@ static inline cpumask_t node_to_cpumask(
 /* Cross-node load balancing interval. */
 # define NODE_BALANCE_RATE 10
 
-#define pcibus_to_cpumask(bus)	(cpu_online_map)
-
 #else /* CONFIG_NUMA */
 # include <asm-generic/topology.h>
 #endif /* !CONFIG_NUMA */

include/asm-generic/topology.h +0 -3

diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.8-rc2-mm1/include/asm-generic/topology.h linux-2.6.8-rc2-mm1+pcibus_to_nodemask/include/asm-generic/topology.h
@@ -41,9 +41,6 @@
 #ifndef node_to_first_cpu
 #define node_to_first_cpu(node)	(0)
 #endif
-#ifndef pcibus_to_cpumask
-#define pcibus_to_cpumask(bus)	(cpu_online_map)
-#endif
 
 /* Cross-node load balancing interval. */
 #ifndef NODE_BALANCE_RATE

include/asm-i386/topology.h +2 -5

diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.8-rc2-mm1/include/asm-i386/topology.h linux-2.6.8-rc2-mm1+pcibus_to_nodemask/include/asm-i386/topology.h
@@ -60,11 +60,8 @@ static inline int node_to_first_cpu(int 
 	return first_cpu(mask);
 }
 
-/* Returns the number of the node containing PCI bus 'bus' */
-static inline cpumask_t pcibus_to_cpumask(int bus)
-{
-	return node_to_cpumask(mp_bus_id_to_node[bus]);
-}
+#define ARCH_HAS_GET_PCIBUS_NODEMASK
+#define get_pcibus_nodemask(bus)	(nodemask_of_node(mp_bus_id_to_node[bus]))
 
 /* Node-to-Node distance */
 #define node_distance(from, to) (from != to)

include/asm-mips/mach-ip27/topology.h +0 -1

diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.8-rc2-mm1/include/asm-mips/mach-ip27/topology.h linux-2.6.8-rc2-mm1+pcibus_to_nodemask/include/asm-mips/mach-ip27/topology.h
@@ -7,7 +7,6 @@
 #define parent_node(node)	(node)
 #define node_to_cpumask(node)	(HUB_DATA(node)->h_cpus)
 #define node_to_first_cpu(node)	(first_cpu(node_to_cpumask(node)))
-#define pcibus_to_cpumask(bus)	(cpu_online_map)
 
 extern int node_distance(nasid_t nasid_a, nasid_t nasid_b);
 #define node_distance(from, to)	node_distance(from, to)

include/asm-ppc64/topology.h +0 -2

diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.8-rc2-mm1/include/asm-ppc64/topology.h linux-2.6.8-rc2-mm1+pcibus_to_nodemask/include/asm-ppc64/topology.h
@@ -33,8 +33,6 @@ static inline int node_to_first_cpu(int 
 	return first_cpu(tmp);
 }
 
-#define pcibus_to_cpumask(bus)	(cpu_online_map)
-
 #define nr_cpus_node(node)	(nr_cpus_in_node[node])
 
 /* Cross-node load balancing interval. */

include/asm-x86_64/mpspec.h +1 -1

diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.8-rc2-mm1/include/asm-x86_64/mpspec.h linux-2.6.8-rc2-mm1+pcibus_to_nodemask/include/asm-x86_64/mpspec.h
@@ -166,7 +166,7 @@ enum mp_bustype {
 };
 extern unsigned char mp_bus_id_to_type [MAX_MP_BUSSES];
 extern int mp_bus_id_to_pci_bus [MAX_MP_BUSSES];
-extern cpumask_t pci_bus_to_cpumask [256];
+extern nodemask_t pci_bus_to_nodemask [256];
 
 extern unsigned int boot_cpu_physical_apicid;
 extern int smp_found_config;

include/asm-x86_64/topology.h +3 -7

diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.8-rc2-mm1/include/asm-x86_64/topology.h linux-2.6.8-rc2-mm1+pcibus_to_nodemask/include/asm-x86_64/topology.h
@@ -20,15 +20,11 @@ extern cpumask_t     node_to_cpumask[];
 #define node_to_first_cpu(node) 	(__ffs(node_to_cpumask[node]))
 #define node_to_cpumask(node)		(node_to_cpumask[node])
 
-static inline cpumask_t pcibus_to_cpumask(int bus)
-{
-	cpumask_t res;
-	cpus_and(res,  pci_bus_to_cpumask[bus], cpu_online_map);
-	return res;
-}
-
 #define NODE_BALANCE_RATE 30	/* CHECKME */ 
 
+#define ARCH_HAS_GET_PCIBUS_NODEMASK
+#define get_pcibus_nodemask(bus)	(pci_bus_to_nodemask[bus])
+
 #endif
 
 #include <asm-generic/topology.h>

include/linux/pci.h +2 -0

diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.8-rc2-mm1/include/linux/pci.h linux-2.6.8-rc2-mm1+pcibus_to_nodemask/include/linux/pci.h
@@ -590,6 +590,8 @@ struct pci_bus {
 	unsigned short  pad2;
 	struct device		*bridge;
 	struct class_device	class_dev;
+	nodemask_t	nodemask;	/* For NUMA systems, we care about which 
+					   node(s) this PCI bus is on/close to. */
 };
 
 #define pci_bus_b(n)	list_entry(n, struct pci_bus, node)

include/linux/topology.h +22 -0

diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.8-rc2-mm1/include/linux/topology.h linux-2.6.8-rc2-mm1+pcibus_to_nodemask/include/linux/topology.h
@@ -28,6 +28,7 @@
 #define _LINUX_TOPOLOGY_H
 
 #include <linux/cpumask.h>
+#include <linux/nodemask.h>
 #include <linux/bitops.h>
 #include <linux/mmzone.h>
 #include <linux/smp.h>

@@ -54,6 +55,18 @@ static inline int __next_node_with_cpus(
 #define for_each_node_with_cpus(node) \
 	for (node = 0; node < numnodes; node = __next_node_with_cpus(node))
 
+static inline cpumask_t nodemask_to_cpumask(nodemask_t nodemask)
+{
+	cpumask_t ret, tmp;
+	int node;
+	cpus_clear(ret);
+	for_each_node_mask(node, nodemask) {
+		tmp = node_to_cpumask(node);
+		cpus_or(ret, ret, tmp);
+	}
+	return ret;
+}
+
 #ifndef node_distance
 #define node_distance(from,to)	(from != to)
 #endif

@@ -61,4 +74,13 @@ static inline int __next_node_with_cpus(
 #define PENALTY_FOR_NODE_WITH_CPUS	(1)
 #endif
 
+static inline nodemask_t pcibus_to_nodemask(struct pci_bus *bus)
+{
+	return bus->nodemask;
+}
+
+#ifndef ARCH_HAS_GET_PCIBUS_NODEMASK
+#define get_pcibus_nodemask(busnr)		(NODE_MASK_ALL)
+#endif
+
 #endif /* _LINUX_TOPOLOGY_H */

Jesse Barnes

2004-07-30 15:43 UTC #9 lore raw

Quoted 4 lines · Matthew Dobson

What I'm doing is basically ripping out all the old pcibus_to_cpumask() calls. The only arch that defined it to be anything other than CPU_MASK_ALL was i386, and theirs should still work. x86_64 had the beginnings of a PCI bus to CPU mask mapping, but it was never filled in,

Quoted 8 lines

just populated with CPU_MASK_ALL, so it does the same with NODE_MASK_ALL now. Those two arches, in their include/asm-$ARCH/topology.h define both ARCH_HAS_GET_PCIBUS_NODEMASK and get_pcibus_nodemask(bus). include/linux/topology.h defines a simple get_pcibus_nodemask(bus) if there isn't an arch-specific one provided. We then, in drivers/pci/probe.c, populate the nodemask field of struct pci_bus with this nodemask. Lookup involves simply returning the nodemask stored in the struct pci_bus.

I think this will work. My tree didn't have nodemask_t though, so it didn't compile :) Here's a first stab at an ia64 portion of the patch.

Jesse

===== arch/ia64/sn/io/machvec/pci_dma.c 1.30 vs edited =====

arch/ia64/sn/io/machvec/pci_dma.c +13 -0

@@ -662,6 +662,19 @@
 	return 0;
 }
 
+/**
+ * sn_get_pcibus_nodemask - return set of nearby nodes for a given PCI bus
+ * @bus: bus number
+ *
+ * Return a nodemask_t with nearby node(s).
+ */
+nodemask_t sn_get_pcibus_nodemask(int bus)
+{
+	nodemask_t nodes;
+	nodes_clear(nodes);
+	return node_set(nasid_to_cnode(busnum_to_nid[bus]), nodes);
+}
+
 EXPORT_SYMBOL(sn_dma_mapping_error);
 EXPORT_SYMBOL(sn_pci_unmap_single);
 EXPORT_SYMBOL(sn_pci_map_single);

===== include/asm-ia64/io.h 1.19 vs edited =====

include/asm-ia64/io.h +5 -0

@@ -391,6 +391,11 @@
 # define outl_p		outl
 #endif
 
+static inline nodemask_t __ia64_get_pcibus_nodemask(int bus)
+{
+	return NODE_MASK_ALL;
+}
+
 /*
  * An "address" in IO memory space is not clearly either an integer or a pointer. We will
  * accept both, thus the casts.

===== include/asm-ia64/machvec.h 1.25 vs edited =====

include/asm-ia64/machvec.h +7 -0

@@ -70,6 +70,7 @@
 typedef unsigned short ia64_mv_readw_relaxed_t (void *);
 typedef unsigned int ia64_mv_readl_relaxed_t (void *);
 typedef unsigned long ia64_mv_readq_relaxed_t (void *);
+typedef nodemask_t ia64_mv_get_pcibus_nodemask_t (int bus);
 
 static inline void
 machvec_noop (void)

@@ -138,6 +139,7 @@
 #  define platform_readw_relaxed        ia64_mv.readw_relaxed
 #  define platform_readl_relaxed        ia64_mv.readl_relaxed
 #  define platform_readq_relaxed        ia64_mv.readq_relaxed
+#  define platform_get_pcibus_nodemask  ia64_mv.get_pcibus_nodemask
 # endif
 
 /* __attribute__((__aligned__(16))) is required to make size of the

@@ -184,6 +186,7 @@
 	ia64_mv_readw_relaxed_t *readw_relaxed;
 	ia64_mv_readl_relaxed_t *readl_relaxed;
 	ia64_mv_readq_relaxed_t *readq_relaxed;
+	ia64_mv_get_pcibus_nodemask_t *get_pcibus_nodemask;
 } __attribute__((__aligned__(16))); /* align attrib? see above comment */
 
 #define MACHVEC_INIT(name)			\

@@ -226,6 +229,7 @@
 	platform_readw_relaxed,			\
 	platform_readl_relaxed,			\
 	platform_readq_relaxed,			\
+	platform_get_pcibus_nodemask,		\
 }
 
 extern struct ia64_machine_vector ia64_mv;

@@ -367,6 +371,9 @@
 #endif
 #ifndef platform_readq_relaxed
 # define platform_readq_relaxed	__ia64_readq_relaxed
+#endif
+#ifndef platform_get_pcibus_nodemask
+# define platform_get_pcibus_nodemask __ia64_get_pcibus_nodemask
 #endif
 
 #endif /* _ASM_IA64_MACHVEC_H */

===== include/asm-ia64/machvec_sn2.h 1.14 vs edited =====

include/asm-ia64/machvec_sn2.h +2 -0

@@ -69,6 +69,7 @@
 extern ia64_mv_dma_sync_sg_for_device	sn_dma_sync_sg_for_device;
 extern ia64_mv_dma_mapping_error	sn_dma_mapping_error;
 extern ia64_mv_dma_supported		sn_dma_supported;
+extern ia64_mv_get_pcibus_nodemask_t	sn_get_pcibus_nodemask;
 
 /*
  * This stuff has dual use!

@@ -116,6 +117,7 @@
 #define platform_dma_sync_sg_for_device	sn_dma_sync_sg_for_device
 #define platform_dma_mapping_error		sn_dma_mapping_error
 #define platform_dma_supported		sn_dma_supported
+#define platform_get_pcibus_nodemask	sn_get_pcibus_nodemask
 
 #include <asm/sn/sn2/io.h>

===== include/asm-ia64/topology.h 1.10 vs edited =====

include/asm-ia64/topology.h +3 -0

@@ -45,6 +45,9 @@
 
 void build_cpu_to_node_map(void);
 
+#define ARCH_HAS_GET_PCIBUS_NODEMASK
+extern nodemask_t get_pcibus_nodemask(int bus);
+
 #endif /* CONFIG_NUMA */
 
 #include <asm-generic/topology.h>

Matthew Dobson

author

2004-07-30 22:22 UTC #10 lore raw

Quoted 4 lines · Jesse Barnes

I think this will work. My tree didn't have nodemask_t though, so it didn't compile :) Here's a first stab at an ia64 portion of the patch.
Jesse

Andrew picked it up in 2.6.8-rc2-mm1, so if you base your patch against that it should compile... That's what I based my patch off. Our lab has been down for a few days so I hope to do some testing on Monday for my patches. If all goes well, I'll add your code into my patch and submit it early next week, ok?

Thanks!

-Matt

Jesse Barnes

2004-07-30 22:25 UTC #11 lore raw

Quoted 11 lines · Matthew Dobson

Quoted 4 lines · Jesse Barnes
I think this will work. My tree didn't have nodemask_t though, so it didn't compile :) Here's a first stab at an ia64 portion of the patch.
Jesse
Andrew picked it up in 2.6.8-rc2-mm1, so if you base your patch against that it should compile... That's what I based my patch off. Our lab has been down for a few days so I hope to do some testing on Monday for my patches. If all goes well, I'll add your code into my patch and submit it early next week, ok?

Sounds good, but it will probably need some fixes before it works correctly (my stuff I mean), so when you have something that looks good give me a few minutes with it before you send it on to Andrew if you would.

Thanks,
Jesse

Matthew Dobson

author

2004-07-30 22:33 UTC #12 lore raw

Quoted 13 lines · Jesse Barnes

Quoted 11 lines · Matthew Dobson
Quoted 4 lines · Jesse Barnes
I think this will work. My tree didn't have nodemask_t though, so it didn't compile :) Here's a first stab at an ia64 portion of the patch.
Jesse
Andrew picked it up in 2.6.8-rc2-mm1, so if you base your patch against that it should compile... That's what I based my patch off. Our lab has been down for a few days so I hope to do some testing on Monday for my patches. If all goes well, I'll add your code into my patch and submit it early next week, ok?

Quoted 6 lines

Sounds good, but it will probably need some fixes before it works correctly (my stuff I mean), so when you have something that looks good give me a few minutes with it before you send it on to Andrew if you would.
Thanks,
Jesse

No problem, Jesse. Like I said, with lab machines being decidedly unfriendly, I won't even be able to run any real tests on my code until Monday at the earliest. I'll certainly give you at least 5 minutes warning before I post any untested, potentially dangerous code! ;)

-Matt

Rajesh Shah

2004-07-29 17:07 UTC #13 lore raw

In reply to Matthew Dobson

Quoted 11 lines · Matthew Dobson

thought. It's pretty trivial to add a nodemask_t to the struct pci_bus, and even initialize it to a reasonable value (ie: NODE_MASK_ALL) since there's the convenient pci_alloc_bus() function in drivers/pci/probe.c. The problem is where to put hooks for individual arches to put the *real* nodemask in this field... My only thought right now is to create a per-arch callback function, arch_get_pcibus_nodemask() or something, and use the value it returns to populate pci_bus->nodemask. We would have to call this function anywhere a struct pci_bus is allocated, and probably pass along the PCI bus number so the arch could determine which nodes it belongs to. Would that work for everyone that cares? We could

With PCI root/p2p bridge hotplug, the code dealing with the hotplug (e.g. ACPI hotplug code) will have this information, not arch specific code. How about having the PCI subsystem export an interface to set the nodemask, and have the arch or hotplug code call it to change the defaults? That way, pci_alloc_bus() simply sets the default and does not perform any callback. Does that work for everyone?

Rajesh

Matthew Dobson

author

2004-07-29 22:28 UTC #14 lore raw

Quoted 21 lines · Rajesh Shah

Quoted 11 lines · Matthew Dobson
thought. It's pretty trivial to add a nodemask_t to the struct pci_bus, and even initialize it to a reasonable value (ie: NODE_MASK_ALL) since there's the convenient pci_alloc_bus() function in drivers/pci/probe.c. The problem is where to put hooks for individual arches to put the *real* nodemask in this field... My only thought right now is to create a per-arch callback function, arch_get_pcibus_nodemask() or something, and use the value it returns to populate pci_bus->nodemask. We would have to call this function anywhere a struct pci_bus is allocated, and probably pass along the PCI bus number so the arch could determine which nodes it belongs to. Would that work for everyone that cares? We could
With PCI root/p2p bridge hotplug, the code dealing with the hotplug (e.g. ACPI hotplug code) will have this information, not arch specific code. How about having the PCI subsystem export an interface to set the nodemask, and have the arch or hotplug code call it to change the defaults? That way, pci_alloc_bus() simply sets the default and does not perform any callback. Does that work for everyone?

Quoted 1 line

Rajesh

Does the patch I just posted in this thread work for you? You could have ACPI define the get_pcibus_nodemask(bus) call, and all should work fine...

-Matt

Rajesh Shah

2004-07-30 00:08 UTC #15 lore raw

Quoted 22 lines · Matthew Dobson

Quoted 12 lines · Rajesh Shah
Quoted 10 lines · Matthew Dobson
and even initialize it to a reasonable value (ie: NODE_MASK_ALL) since there's the convenient pci_alloc_bus() function in drivers/pci/probe.c. The problem is where to put hooks for individual arches to put the *real* nodemask in this field... My only thought right now is to create a per-arch callback function, arch_get_pcibus_nodemask() or something, and use the value it returns to populate pci_bus->nodemask. We would have to call this function anywhere a struct pci_bus is allocated, and probably pass along the PCI bus number so the arch could determine which nodes it belongs to. Would that work for everyone that cares? We could
Quoted 8 lines
With PCI root/p2p bridge hotplug, the code dealing with the hotplug (e.g. ACPI hotplug code) will have this information, not arch specific code. How about having the PCI subsystem export an interface to set the nodemask, and have the arch or hotplug code call it to change the defaults? That way, pci_alloc_bus() simply sets the default and does not perform any callback. Does that work for everyone?

Quoted 4 lines

Does the patch I just posted in this thread work for you? You could have ACPI define the get_pcibus_nodemask(bus) call, and all should work fine...

Yes, the patch you posted is fine. I was talking about the part that was not in the patch but mentioned above (arch callbacks). I'm working on ACPI based root/p2p bridge hotplug but am far from being done. I can post the patches to get/set nodemask later, when my work is farther along.

Rajesh

Martin J. Bligh

2004-07-28 15:03 UTC #16 lore raw

In reply to Matthew Dobson

Quoted 12 lines

Quoted 11 lines
I wonder though if we shouldn't add
...
# ifdef CONFIG_NUMA
  int node; /* or nodemask_t if necessary */
# endif
  ...
to struct pci_bus instead? That would make the existing code paths a little faster and avoid the need for a global array, which tends to lead to TLB misses.

Quoted 6 lines

I like that idea! Stick a nodemask_t in struct pci_bus, initialize it to NODE_MASK_ALL. If a particular arch wants to put something more accurate in there, then great, if not, we're just in the same boat we're in now.
Anyone else have opinions one way or the other on Jesse's idea?

Sounds great - if it's possible to add it to something more generic than PCI, that'd be even better, but pci would still be very useful.

Matthew Dobson

author

2004-07-28 19:12 UTC #17 lore raw

Quoted 17 lines · Martin J. Bligh

Quoted 17 lines
Quoted 11 lines
I wonder though if we shouldn't add
...
# ifdef CONFIG_NUMA
  int node; /* or nodemask_t if necessary */
# endif
  ...
to struct pci_bus instead? That would make the existing code paths a little faster and avoid the need for a global array, which tends to lead to TLB misses.
I like that idea! Stick a nodemask_t in struct pci_bus, initialize it to NODE_MASK_ALL. If a particular arch wants to put something more accurate in there, then great, if not, we're just in the same boat we're in now.

Quoted 6 lines

Quoted 1 line
Anyone else have opinions one way or the other on Jesse's idea?
Sounds great - if it's possible to add it to something more generic than PCI, that'd be even better, but pci would still be very useful.
M.

Is there anything like that? I'm not aware of any structure that keeps track of general "buses", which would be what we want. Something that keeps track of PCI buses, Infiniband buses, arch-specific fabric buses, etc. Barring the existence of such a structure, I'll just shove it in the PCI bus structure for now.

-Matt

Andi Kleen

2004-07-27 14:18 UTC #18 lore raw

In reply to Matthew Dobson

Quoted 3 lines · On Mon, 26 Jul 2004 17:10:08 -0700 Matthew Dobson <[email protected]> wrote:

So in discussions with Jesse at OLS, we decided that pcibus_to_node() is a more generally useful function than pcibus_to_cpumask(). If anyone disagrees with that, now would be a good time to let us know.

Not sure that is a good idea. Sometimes this information is not available. With pcibus_to_cpumask() the fallback is obvious, but it isn't with pcibus_to_node(). Returning a random node is wrong.

Quoted 3 lines

This is just a preliminary patch. It needs review for x86_64, as I don't know how to properly populate the mp_bus_to_node (which used to be mp_bus_to_cpumask) array.

It's impossible currently - I need an ACPI 3.0 BIOS to get this information. Even then there will be machines who don't supply it.

I tried some time ago to get it from the hardware, but the hardware registers were arcane enough that I didn't find it easy enough. Relying on firmware for this thing is probably a better idea anyways.

-Andi

Jesse Barnes

2004-07-27 15:22 UTC #19 lore raw

Quoted 10 lines · Andi Kleen

On Mon, 26 Jul 2004 17:10:08 -0700
Quoted 3 lines · Matthew Dobson
So in discussions with Jesse at OLS, we decided that pcibus_to_node() is a more generally useful function than pcibus_to_cpumask(). If anyone disagrees with that, now would be a good time to let us know.
Not sure that is a good idea. Sometimes this information is not available. With pcibus_to_cpumask() the fallback is obvious, but it isn't with pcibus_to_node(). Returning a random node is wrong.

Hmm... so there's no way for you to get a node or nodemask at all?

Quoted 6 lines

It's impossible currently - I need an ACPI 3.0 BIOS to get this information. Even then there will be machines who don't supply it.
I tried some time ago to get it from the hardware, but the hardware registers were arcane enough that I didn't find it easy enough. Relying on firmware for this thing is probably a better idea anyways.

Yeah, it's easier that way, but for the first cut on ia64, I'm gonna have to do it by hand too. It's not that bad on sn2 at least.

Jesse

Andi Kleen

2004-07-27 16:02 UTC #20 lore raw

Quoted 8 lines · On Tue, 27 Jul 2004 08:15:39 -0700 Jesse Barnes <[email protected]> wrote:

Quoted 7 lines · Andi Kleen
On Mon, 26 Jul 2004 17:10:08 -0700
Quoted 3 lines · Matthew Dobson
So in discussions with Jesse at OLS, we decided that pcibus_to_node() is a more generally useful function than pcibus_to_cpumask(). If anyone disagrees with that, now would be a good time to let us know.

Quoted 5 lines

Quoted 3 lines
Not sure that is a good idea. Sometimes this information is not available. With pcibus_to_cpumask() the fallback is obvious, but it isn't with pcibus_to_node(). Returning a random node is wrong.
Hmm... so there's no way for you to get a node or nodemask at all?

When the BIOS has _PXM methods there will be probably.
Just I cannot guarantee it has that, so there should be some clean fallback path.

If cpumask is too complicated for you a pcibus_to_nodemask would be fine for me too, just please no single node number.

-Andi

Matthew Dobson

author

2004-07-27 18:20 UTC #21 lore raw

Quoted 17 lines · Andi Kleen

Quoted 8 lines · On Tue, 27 Jul 2004 08:15:39 -0700 Jesse Barnes <[email protected]> wrote:
Quoted 7 lines · Andi Kleen
On Mon, 26 Jul 2004 17:10:08 -0700
Quoted 3 lines · Matthew Dobson
So in discussions with Jesse at OLS, we decided that pcibus_to_node() is a more generally useful function than pcibus_to_cpumask(). If anyone disagrees with that, now would be a good time to let us know.
Quoted 5 lines
Quoted 3 lines
Not sure that is a good idea. Sometimes this information is not available. With pcibus_to_cpumask() the fallback is obvious, but it isn't with pcibus_to_node(). Returning a random node is wrong.
Hmm... so there's no way for you to get a node or nodemask at all?

Quoted 8 lines

When the BIOS has _PXM methods there will be probably.
Just I cannot guarantee it has that, so there should be some clean fallback path.
If cpumask is too complicated for you a pcibus_to_nodemask would be fine for me too, just please no single node number.
-Andi

I guess I'm OK with a nodemask instead of a node. That will make this patch dependent on my nodemask_t patch, which I'll also be sending out again later today, though... A nodemask instead of a node also allows us to return a mask of nearby memory-only nodes as well as CPU-only nodes, if the arch supports that, for allocating buffers/doing DMA from...

-Matt

Paul Jackson

2004-07-29 08:35 UTC #22 lore raw

Quoted 1 line · Matthew

That will make this patch dependent on my nodemask_t patch,

Did you notice that Andrew included your most recently published nodemask_t in 2.6.8-rc2-mm1?

Congratulations !

Signature · 3 lines

                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <[email protected]> 1.650.933.1373