pci/intr_remapping: allocate irq_iommu on node

PATCH In review Yinghai Lu 4 participants 11 messages Aug 4, 2009 – Aug 11, 2009

Subsystems (2)

PCI IRQ

lore

Share & tools

lore

Yinghai Lu

author

2009-08-04 16:05 UTC #0 lore raw

PATCH 1/1 pci/intr_remapping: allocate irq_iommu on node

make it use node from irq_desc.

Signed-off-by Yinghai Lu <[email protected]>

---
 drivers/pci/intr_remapping.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/pci/intr_remapping.c +2 -2

Index: linux-2.6/drivers/pci/intr_remapping.c
===================================================================
@@ -201,7 +201,7 @@ int alloc_irte(struct intel_iommu *iommu
 	for (i = index; i < index + count; i++)
 		table->base[i].present = 1;
 
-	irq_iommu = irq_2_iommu_alloc(irq);
+	irq_iommu = irq_2_iommu_alloc_node(irq, irq_to_desc(irq)->node);
 	if (!irq_iommu) {
 		spin_unlock_irqrestore(&irq_2_ir_lock, flags);
 		printk(KERN_ERR "can't allocate irq_2_iommu\n");

@@ -255,7 +255,7 @@ int set_irte_irq(int irq, struct intel_i
 
 	spin_lock_irqsave(&irq_2_ir_lock, flags);
 
-	irq_iommu = irq_2_iommu_alloc(irq);
+	irq_iommu = irq_2_iommu_alloc_node(irq, irq_to_desc(irq)->node);
 
 	if (!irq_iommu) {
 		spin_unlock_irqrestore(&irq_2_ir_lock, flags);

Suresh Siddha

2009-08-04 17:23 UTC #1 lore raw

Quoted 13 lines · Yinghai Lu

make it use node from irq_desc.
Signed-off-by Yinghai Lu <[email protected]>
---
 drivers/pci/intr_remapping.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Index: linux-2.6/drivers/pci/intr_remapping.c
===================================================================

drivers/pci/intr_remapping.c int alloc_irte(struct intel_iommu *iommu · line 204

Quoted 5 lines

drivers/pci/intr_remapping.c +1 -1

@@ -201,7 +201,7 @@ int alloc_irte(struct intel_iommu *iommu
 	for (i = index; i < index + count; i++)
 		table->base[i].present = 1;
 
-	irq_iommu = irq_2_iommu_alloc(irq);
+	irq_iommu = irq_2_iommu_alloc_node(irq, irq_to_desc(irq)->node);

As far as I can see, this node will always be boot_cpu_id's node. We allocate the IRTE at the time of the create_irq() and at that time, desc->node will always be of the boot_cpu_id's.

Can you please elaborate when this node will be different to make this an effective patch?

thanks,
suresh

Yinghai Lu

author

2009-08-05 09:15 UTC #2 lore raw

Quoted 24 lines · Suresh Siddha

Quoted 13 lines · Yinghai Lu
make it use node from irq_desc.
Signed-off-by Yinghai Lu <[email protected]>
---
 drivers/pci/intr_remapping.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Index: linux-2.6/drivers/pci/intr_remapping.c
===================================================================
drivers/pci/intr_remapping.c int alloc_irte(struct intel_iommu *iommu · line 204
Quoted 5 lines
drivers/pci/intr_remapping.c +1 -1
@@ -201,7 +201,7 @@ int alloc_irte(struct intel_iommu *iommu
 	for (i = index; i < index + count; i++)
 		table->base[i].present = 1;
 
-	irq_iommu = irq_2_iommu_alloc(irq);
+	irq_iommu = irq_2_iommu_alloc_node(irq, irq_to_desc(irq)->node);
As far as I can see, this node will always be boot_cpu_id's node. We allocate the IRTE at the time of the create_irq() and at that time, desc->node will always be of the boot_cpu_id's.

Quoted 2 lines

Can you please elaborate when this node will be different to make this an effective patch?

without this patch

[  299.175436] igbvf 0000:41:10.0: enabling device (0000 -> 0002)
[  299.180009] igbvf 0000:41:10.0: using 64bit DMA mask
[  299.183077] igbvf 0000:41:10.0: using 64bit consistent DMA mask
[  299.199870] igbvf 0000:41:10.0: enabling bus mastering
[  299.205534] igbvf 0000:41:10.0: setting latency timer to 64
[  299.219654] reserve_memtype added 0xcfc00000-0xcfc04000, track uncached-minus, req uncached-minus, ret uncached-minus
[  299.239615] reserve_memtype added 0xcfc20000-0xcfc21000, track uncached-minus, req uncached-minus, ret uncached-minus
[  299.260181]   alloc irq_desc for 217 on node 2
[  299.263501]   alloc kstat_irqs on node 2
[  299.276276] alloc irq_2_iommu on node 0
[  299.281847] igbvf 0000:41:10.0: irq 217 for MSI/MSI-X
[  299.285511]   alloc irq_desc for 218 on node 2
[  299.297813]   alloc kstat_irqs on node 2
[  299.302543] alloc irq_2_iommu on node 0
[  299.315358] igbvf 0000:41:10.0: irq 218 for MSI/MSI-X
[  299.321102]   alloc irq_desc for 219 on node 2
[  299.325932]   alloc kstat_irqs on node 2
[  299.338616] alloc irq_2_iommu on node 0
[  299.340789] igbvf 0000:41:10.0: irq 219 for MSI/MSI-X
[  299.356532] igbvf 0000:41:10.0: PF still in reset state, assigning new address
[  299.365128] igbvf 0000:41:10.0: PF still resetting
[  299.377625] igbvf 0000:41:10.0: Intel(R) 82576 Virtual Function
[  299.385619] igbvf 0000:41:10.0: Address: c6:e3:de:62:de:eb
[  299.395957] igbvf 0000:41:10.0: MAC: 1

thought we could put irq_2_iommu to same node too.

Suresh Siddha

2009-08-06 00:40 UTC #3 lore raw

Quoted 27 lines · Yinghai Lu

without this patch

[  299.175436] igbvf 0000:41:10.0: enabling device (0000 -> 0002)
[  299.180009] igbvf 0000:41:10.0: using 64bit DMA mask
[  299.183077] igbvf 0000:41:10.0: using 64bit consistent DMA mask
[  299.199870] igbvf 0000:41:10.0: enabling bus mastering
[  299.205534] igbvf 0000:41:10.0: setting latency timer to 64
[  299.219654] reserve_memtype added 0xcfc00000-0xcfc04000, track uncached-minus, req uncached-minus, ret uncached-minus
[  299.239615] reserve_memtype added 0xcfc20000-0xcfc21000, track uncached-minus, req uncached-minus, ret uncached-minus
[  299.260181]   alloc irq_desc for 217 on node 2
[  299.263501]   alloc kstat_irqs on node 2
[  299.276276] alloc irq_2_iommu on node 0
[  299.281847] igbvf 0000:41:10.0: irq 217 for MSI/MSI-X
[  299.285511]   alloc irq_desc for 218 on node 2
[  299.297813]   alloc kstat_irqs on node 2
[  299.302543] alloc irq_2_iommu on node 0
[  299.315358] igbvf 0000:41:10.0: irq 218 for MSI/MSI-X
[  299.321102]   alloc irq_desc for 219 on node 2
[  299.325932]   alloc kstat_irqs on node 2
[  299.338616] alloc irq_2_iommu on node 0
[  299.340789] igbvf 0000:41:10.0: irq 219 for MSI/MSI-X
[  299.356532] igbvf 0000:41:10.0: PF still in reset state, assigning new address
[  299.365128] igbvf 0000:41:10.0: PF still resetting
[  299.377625] igbvf 0000:41:10.0: Intel(R) 82576 Virtual Function
[  299.385619] igbvf 0000:41:10.0: Address: c6:e3:de:62:de:eb
[  299.395957] igbvf 0000:41:10.0: MAC: 1

Quoted 1 line

thought we could put irq_2_iommu to same node too.

Yinghai, even with this patch, we will still be allocating from node-0. No?

Yinghai Lu

author

2009-08-06 01:21 UTC #4 lore raw

Quoted 28 lines · Suresh Siddha

Quoted 27 lines · Yinghai Lu

without this patch

[  299.175436] igbvf 0000:41:10.0: enabling device (0000 -> 0002)
[  299.180009] igbvf 0000:41:10.0: using 64bit DMA mask
[  299.183077] igbvf 0000:41:10.0: using 64bit consistent DMA mask
[  299.199870] igbvf 0000:41:10.0: enabling bus mastering
[  299.205534] igbvf 0000:41:10.0: setting latency timer to 64
[  299.219654] reserve_memtype added 0xcfc00000-0xcfc04000, track uncached-minus, req uncached-minus, ret uncached-minus
[  299.239615] reserve_memtype added 0xcfc20000-0xcfc21000, track uncached-minus, req uncached-minus, ret uncached-minus
[  299.260181]   alloc irq_desc for 217 on node 2
[  299.263501]   alloc kstat_irqs on node 2
[  299.276276] alloc irq_2_iommu on node 0
[  299.281847] igbvf 0000:41:10.0: irq 217 for MSI/MSI-X
[  299.285511]   alloc irq_desc for 218 on node 2
[  299.297813]   alloc kstat_irqs on node 2
[  299.302543] alloc irq_2_iommu on node 0
[  299.315358] igbvf 0000:41:10.0: irq 218 for MSI/MSI-X
[  299.321102]   alloc irq_desc for 219 on node 2
[  299.325932]   alloc kstat_irqs on node 2
[  299.338616] alloc irq_2_iommu on node 0
[  299.340789] igbvf 0000:41:10.0: irq 219 for MSI/MSI-X
[  299.356532] igbvf 0000:41:10.0: PF still in reset state, assigning new address
[  299.365128] igbvf 0000:41:10.0: PF still resetting
[  299.377625] igbvf 0000:41:10.0: Intel(R) 82576 Virtual Function
[  299.385619] igbvf 0000:41:10.0: Address: c6:e3:de:62:de:eb
[  299.395957] igbvf 0000:41:10.0: MAC: 1

Quoted 4 lines

Quoted 1 line
thought we could put irq_2_iommu to same node too.
Yinghai, even with this patch, we will still be allocating from node-0. No?

with this patch will get
alloc irq_2_iommu on node 2

Suresh Siddha

Acked-by

2009-08-06 19:20 UTC #5 lore raw

Quoted 7 lines · Yinghai Lu

Quoted 3 lines · Suresh Siddha
Yinghai, even with this patch, we will still be allocating from node-0. No?
with this patch will get
alloc irq_2_iommu on node 2

Looking closer, I missed the fact that we are now using the create_irq_nr() which has the node information.

Acked-by Suresh Siddha <[email protected]>

Thanks.

tip-bot for Yinghai Lu

author

2009-08-08 15:22 UTC #6 lore raw

[tip:irq/core] pci/intr_remapping: Allocate irq_iommu on node

In reply to Yinghai Lu

Commit-ID:  94fb9a6b49d3a63831c4e752f6f0ab8da0a99eb5
Gitweb:     http://git.kernel.org/tip/94fb9a6b49d3a63831c4e752f6f0ab8da0a99eb5
Author:     Yinghai Lu <[email protected]>
AuthorDate: Tue, 4 Aug 2009 09:04:39 -0700
Committer:  Ingo Molnar <[email protected]>
CommitDate: Sat, 8 Aug 2009 17:07:37 +0200

pci/intr_remapping: Allocate irq_iommu on node

Make it use node from irq_desc - this results in more optimal memory layout.

Signed-off-by Yinghai Lu <[email protected]>

Cc Jesse Barnes <[email protected]>

Acked-by Suresh Siddha <[email protected]>

LKML-Reference: <[email protected]>

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

---
 drivers/pci/intr_remapping.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

drivers/pci/intr_remapping.c +2 -2

index 4f5b871..e27a28c 100644
@@ -201,7 +201,7 @@ int alloc_irte(struct intel_iommu *iommu, int irq, u16 count)
 	for (i = index; i < index + count; i++)
 		table->base[i].present = 1;
 
-	irq_iommu = irq_2_iommu_alloc(irq);
+	irq_iommu = irq_2_iommu_alloc_node(irq, irq_to_desc(irq)->node);
 	if (!irq_iommu) {
 		spin_unlock_irqrestore(&irq_2_ir_lock, flags);
 		printk(KERN_ERR "can't allocate irq_2_iommu\n");

@@ -255,7 +255,7 @@ int set_irte_irq(int irq, struct intel_iommu *iommu, u16 index, u16 subhandle)
 
 	spin_lock_irqsave(&irq_2_ir_lock, flags);
 
-	irq_iommu = irq_2_iommu_alloc(irq);
+	irq_iommu = irq_2_iommu_alloc_node(irq, irq_to_desc(irq)->node);
 
 	if (!irq_iommu) {
 		spin_unlock_irqrestore(&irq_2_ir_lock, flags);

Ingo Molnar

2009-08-09 10:44 UTC #7 lore raw

Re: [tip:irq/core] pci/intr_remapping: Allocate irq_iommu on node

Quoted 8 lines · tip-bot for Yinghai Lu

Commit-ID:  94fb9a6b49d3a63831c4e752f6f0ab8da0a99eb5
Gitweb:     http://git.kernel.org/tip/94fb9a6b49d3a63831c4e752f6f0ab8da0a99eb5
Author:     Yinghai Lu <[email protected]>
AuthorDate: Tue, 4 Aug 2009 09:04:39 -0700
Committer:  Ingo Molnar <[email protected]>
CommitDate: Sat, 8 Aug 2009 17:07:37 +0200

pci/intr_remapping: Allocate irq_iommu on node

-tip testing found that this patch broke the x86 build:

drivers/pci/intr_remapping.c: In function ‘alloc_irte’:
drivers/pci/intr_remapping.c:204: error: ‘struct irq_desc’ has no member named ‘node’ drivers/pci/intr_remapping.c: In function ‘set_irte_irq’:
drivers/pci/intr_remapping.c:258: error: ‘struct irq_desc’ has no member named ‘node’

Ingo

Jesse Barnes

2009-08-10 16:29 UTC #8 lore raw

Re: [tip:irq/core] pci/intr_remapping: Allocate irq_iommu on node

Quoted 12 lines · On Sun, 9 Aug 2009 12:44:00 +0200 Ingo Molnar <[email protected]> wrote:

Quoted 8 lines · tip-bot for Yinghai Lu
Commit-ID: 94fb9a6b49d3a63831c4e752f6f0ab8da0a99eb5 Gitweb:
http://git.kernel.org/tip/94fb9a6b49d3a63831c4e752f6f0ab8da0a99eb5
Author: Yinghai Lu <[email protected]> AuthorDate: Tue, 4 Aug
2009 09:04:39 -0700 Committer: Ingo Molnar <[email protected]>
CommitDate: Sat, 8 Aug 2009 17:07:37 +0200
pci/intr_remapping: Allocate irq_iommu on node

Quoted 7 lines

-tip testing found that this patch broke the x86 build:
drivers/pci/intr_remapping.c: In function ‘alloc_irte’:
drivers/pci/intr_remapping.c:204: error: ‘struct irq_desc’ has no member named ‘node’ drivers/pci/intr_remapping.c: In function ‘set_irte_irq’: drivers/pci/intr_remapping.c:258: error: ‘struct irq_desc’ has no member named ‘node’

Looks like we need a wrapper function for this with proper CONFIG_NUMA checks... I'll apply it to my linux-next tree when someone sends me a new version.

Thanks,

Signature · 1 line

Jesse Barnes, Intel Open Source Technology Center

Yinghai Lu

author

2009-08-11 12:00 UTC #9 lore raw

PATCH 1/1 pci/intr_remapping: allocate irq_iommu on node -v2

make it use node from irq_desc.

also need to make sure legacy irq node is right, when there is no ram on node 0

Signed-off-by Yinghai Lu <[email protected]>

---
 drivers/pci/intr_remapping.c |   10 +++++++++-
 kernel/irq/handle.c          |    5 ++++-
 2 files changed, 13 insertions(+), 2 deletions(-)

drivers/pci/intr_remapping.c +9 -1

Index: linux-2.6/drivers/pci/intr_remapping.c
===================================================================
@@ -79,7 +79,15 @@ static struct irq_2_iommu *irq_2_iommu_a
 
 static struct irq_2_iommu *irq_2_iommu_alloc(unsigned int irq)
 {
-	return irq_2_iommu_alloc_node(irq, cpu_to_node(boot_cpu_id));
+	int node;
+
+#ifdef CONFIG_SMP
+	node = irq_to_desc(irq)->node;
+#else
+	node = cpu_to_node(boot_cpu_id);
+#endif
+
+	return irq_2_iommu_alloc_node(irq, node);
 }
 
 #else /* !CONFIG_SPARSE_IRQ */

kernel/irq/handle.c +4 -1

Index: linux-2.6/kernel/irq/handle.c
===================================================================
@@ -161,7 +161,7 @@ int __init early_irq_init(void)
 
 	desc = irq_desc_legacy;
 	legacy_count = ARRAY_SIZE(irq_desc_legacy);
- 	node = first_online_node;
+	node = first_online_node;
 
 	/* allocate irq_desc_ptrs array based on nr_irqs */
 	irq_desc_ptrs = kcalloc(nr_irqs, sizeof(void *), GFP_NOWAIT);

@@ -172,6 +172,9 @@ int __init early_irq_init(void)
 
 	for (i = 0; i < legacy_count; i++) {
 		desc[i].irq = i;
+#ifdef CONFIG_SMP
+		desc[i].node = node;
+#endif
 		desc[i].kstat_irqs = kstat_irqs_legacy + i * nr_cpu_ids;
 		lockdep_set_class(&desc[i].lock, &irq_desc_lock_class);
 		alloc_desc_masks(&desc[i], node, true);

Ingo Molnar

2009-08-11 13:25 UTC #10 lore raw

Re: [PATCH] pci/intr_remapping: allocate irq_iommu on node -v2

drivers/pci/intr_remapping.c static struct irq_2_iommu *irq_2_iommu_a · line 91

Quoted 13 lines · Yinghai Lu

static struct irq_2_iommu *irq_2_iommu_alloc(unsigned int irq) {

drivers/pci/intr_remapping.c +9 -1

-	return irq_2_iommu_alloc_node(irq, cpu_to_node(boot_cpu_id));
+	int node;
+
+#ifdef CONFIG_SMP
+	node = irq_to_desc(irq)->node;
+#else
+	node = cpu_to_node(boot_cpu_id);
+#endif
+
+	return irq_2_iommu_alloc_node(irq, node);
 }

To avoid the ifdef, shouldnt there be an new irq_node(irq) primitive that maps to desc->node on SMP and does the right thing on UP?

Ingo