[PATCH] x86_64 irq: Use irq_domain in ioapic_retrigger_irq

[PATCH] x86_64 irq: Use irq_domain in ioapic_retrigger_irq

[Eric W. Biederman]

Thanks to YH Lu for spotting this.  It appears I missed
this function when I refactored allocate_irq_vector and
introduced irq_domain, with the result that all retriggered
irqs would go to cpu 0 even if we were not prepared to
receive them there.

While reviewing YH's patch I also noticed that this function was
missing locking, and since I am now reading two values from
two diffrent arrays that looks like a race we might be able
to hit in the real world.

Cc: Yinghai Lu <yinghai.lu@amd.com>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 arch/x86_64/kernel/io_apic.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86_64/kernel/io_apic.c b/arch/x86_64/kernel/io_apic.c
index 49e94f7..2207d4a 100644
--- a/arch/x86_64/kernel/io_apic.c
+++ b/arch/x86_64/kernel/io_apic.c
@@ -1255,12 +1255,15 @@ static int ioapic_retrigger_irq(unsigned
 {
 	cpumask_t mask;
 	unsigned vector;
+	unsigned long flags;
 
+	spin_lock_irqsave(&vector_lock, flags);
 	vector = irq_vector[irq];
 	cpus_clear(mask);
-	cpu_set(vector >> 8, mask);
+	cpu_set(first_cpu(irq_domain[irq]), mask);
 
-	send_IPI_mask(mask, vector & 0xff);
+	send_IPI_mask(mask, vector);
+	spin_unlock_irqrestore(&vector_lock, flags);
 
 	return 1;
 }
-- 
1.4.2.rc3.g7e18e-dirty

[PATCH] x86_64: typo in __assign_irq_vector when update pos for vector and offset

[Eric W. Biederman]

From: Yinghai Lu <yinghai.lu@amd.com>

typo with cpu instead of new_cpu

Signed-off-by: Yinghai Lu <yinghai.lu@amd.com>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 arch/x86_64/kernel/io_apic.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86_64/kernel/io_apic.c b/arch/x86_64/kernel/io_apic.c
index 2207d4a..8a9a357 100644
--- a/arch/x86_64/kernel/io_apic.c
+++ b/arch/x86_64/kernel/io_apic.c
@@ -651,12 +651,12 @@ next:
 		if (vector == IA32_SYSCALL_VECTOR)
 			goto next;
 		for_each_cpu_mask(new_cpu, domain)
-			if (per_cpu(vector_irq, cpu)[vector] != -1)
+			if (per_cpu(vector_irq, new_cpu)[vector] != -1)
 				goto next;
 		/* Found one! */
 		for_each_cpu_mask(new_cpu, domain) {
-			pos[cpu].vector = vector;
-			pos[cpu].offset = offset;
+			pos[new_cpu].vector = vector;
+			pos[new_cpu].offset = offset;
 		}
 		if (old_vector >= 0) {
 			int old_cpu;
-- 
1.4.2.rc3.g7e18e-dirty

RE: [PATCH] x86_64 irq: Use irq_domain in ioapic_retrigger_irq

[Lu, Yinghai]

Can you change "unsigned vector" to "int vector" like other function?

YH

-----Original Message-----
From: ebiederm@xmission.com [mailto:ebiederm@xmission.com] 
Sent: Tuesday, October 17, 2006 11:03 AM
To: Linus Torvalds
Cc: Andrew Morton; Andi Kleen; linux-kernel@vger.kernel.org; Natalie
Protasevich; Lu, Yinghai
Subject: [PATCH] x86_64 irq: Use irq_domain in ioapic_retrigger_irq


Thanks to YH Lu for spotting this.  It appears I missed
this function when I refactored allocate_irq_vector and
introduced irq_domain, with the result that all retriggered
irqs would go to cpu 0 even if we were not prepared to
receive them there.

While reviewing YH's patch I also noticed that this function was
missing locking, and since I am now reading two values from
two diffrent arrays that looks like a race we might be able
to hit in the real world.

Cc: Yinghai Lu <yinghai.lu@amd.com>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 arch/x86_64/kernel/io_apic.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86_64/kernel/io_apic.c b/arch/x86_64/kernel/io_apic.c
index 49e94f7..2207d4a 100644
--- a/arch/x86_64/kernel/io_apic.c
+++ b/arch/x86_64/kernel/io_apic.c
@@ -1255,12 +1255,15 @@ static int ioapic_retrigger_irq(unsigned
 {
 	cpumask_t mask;
 	unsigned vector;
+	unsigned long flags;
 
+	spin_lock_irqsave(&vector_lock, flags);
 	vector = irq_vector[irq];
 	cpus_clear(mask);
-	cpu_set(vector >> 8, mask);
+	cpu_set(first_cpu(irq_domain[irq]), mask);
 
-	send_IPI_mask(mask, vector & 0xff);
+	send_IPI_mask(mask, vector);
+	spin_unlock_irqrestore(&vector_lock, flags);
 
 	return 1;
 }
-- 
1.4.2.rc3.g7e18e-dirty

[PATCH] x86_64: Put more than one cpu in TARGET_CPUS

[Eric W. Biederman]

TARGET_CPUS is the default irq routing poicy.  It specifies which cpus
the kernel should aim an irq at.  In physflat delivery mode we can
route an irq to a single cpu.  But that doesn't mean our default
policy should only be a single cpu is allowed. 

By allowing the irq routing code to select from multiple cpus this
enables systems with more irqs then we can service on a single
processor to actually work. 

I just audited and tested the code and irqbalance doesn't care, and
the io_apic.c doesn't care if we have extra cpus in the mask.
Everything will use or assume we are using the lowest numbered cpu in
the mask if we can't use them all.

So this should result in no behavior changes except on systems that need it.

Thanks for YH Lu for spotting this problem in his testing.

Cc: Yinghai Lu <yinghai.lu@amd.com>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 arch/x86_64/kernel/genapic_flat.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86_64/kernel/genapic_flat.c b/arch/x86_64/kernel/genapic_flat.c
index 0dfc223..7c01db8 100644
--- a/arch/x86_64/kernel/genapic_flat.c
+++ b/arch/x86_64/kernel/genapic_flat.c
@@ -153,7 +153,7 @@ struct genapic apic_flat =  {
 
 static cpumask_t physflat_target_cpus(void)
 {
-	return cpumask_of_cpu(0);
+	return cpu_online_map;
 }
 
 static cpumask_t physflat_vector_allocation_domain(int cpu)
-- 
1.4.2.rc3.g7e18e-dirty

Re: [PATCH] x86_64 irq: Use irq_domain in ioapic_retrigger_irq

[Eric W. Biederman]

"Lu, Yinghai" <yinghai.lu@amd.com> writes:

> Can you change "unsigned vector" to "int vector" like other function?

It doesn't matter it is an 8 bit unsigned value.
So I made the minimal change necessary to get the bugs fixed.

Coping with the inconsistencies of the usage can be a separate patch.
That way we won't get functional and style changes confused.

Eric