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

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

[yhlu]

typo with cpu instead of new_cpu when update the pos struct.

Signed-off-by: Yinghai Lu <yinghai.lu@amd.com>

diff --git a/arch/x86_64/kernel/io_apic.c b/arch/x86_64/kernel/io_apic.c
index 44b55f8..3857440 100644
--- a/arch/x86_64/kernel/io_apic.c
+++ b/arch/x86_64/kernel/io_apic.c
@@ -655,8 +655,8 @@ next:
                                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;

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

[yhlu]

[-- Attachment #1: Type: text/plain, Size: 1012 bytes --]

Please use this one

typo with cpu instead of new_cpu

Signed-off-by: Yinghai Lu <yinghai.lu@amd.com>

diff --git a/arch/x86_64/kernel/io_apic.c b/arch/x86_64/kernel/io_apic.c
index 44b55f8..756d097 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;

[-- Attachment #2: io_apic_x.diff --]
[-- Type: text/x-patch, Size: 644 bytes --]

diff --git a/arch/x86_64/kernel/io_apic.c b/arch/x86_64/kernel/io_apic.c
index 44b55f8..756d097 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;

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

[Yinghai Lu]

[-- Attachment #1: Type: text/plain, Size: 2519 bytes --]

Please check the patch.

Also I have a question about TARGET_CPUS in io_apic.c.

for a 16 sockets system with 32 non coherent ht chain. and if every
chain have 8 irq for devices, the genapic will use physflat. and it
should use you new added "different cpu can have same vector for
different irq".  --- in the i8259.c

but the setup_IOAPIC_irqs and arch_setup_ht_irq and arch_setup_msi_irq
is still using TARGET_CPUS ( it is cpumask_of_cpu(0) for physflat), so
the assign_irq_vector will not get vector for them, becase cpu 0 does
not have that much vector to be alllocated. and later
setup_affinity_xxx_irq can not be used because before irq is not there
and show on /proc/interrupts.

So I want to
1. for arch_setup_ht_irq and arch_setup_msi_irq, we can use the dev it
takes to get bus and use bus->sysdata to get bus->node mapping that is
created in fillin_cpumask_to_bus, to get real target_cpus instead of
cpu0.
2. for ioapics, may need to add another array,
ioapic_node[MAX_IOAPICS], and use ioapic address to get the numa node
for it. So later can use it to get real targets cpus when need to use
TARGET_CPUS.

Please comments.

Thanks

Yinghai Lu





---------- Forwarded message ----------
From: yhlu <yhlu.kernel@gmail.com>
Date: Oct 15, 2006 12:21 PM
Subject: re: [PATCH] x86_64: typo in __assign_irq_vector when update
pos for vector and offset
To: "Eric W. Biederman" <ebiederm@xmission.com>, Andi Kleen <ak@muc.de>
Cc: linux kernel mailing list <linux-kernel@vger.kernel.org>,
yhlu.kernel@gmail.com


Please use this one

typo with cpu instead of new_cpu

Signed-off-by: Yinghai Lu <yinghai.lu@amd.com>

diff --git a/arch/x86_64/kernel/io_apic.c b/arch/x86_64/kernel/io_apic.c
index 44b55f8..756d097 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;

[-- Attachment #2: io_apic_x.diff --]
[-- Type: text/x-patch, Size: 644 bytes --]

diff --git a/arch/x86_64/kernel/io_apic.c b/arch/x86_64/kernel/io_apic.c
index 44b55f8..756d097 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;

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

[Eric W. Biederman]

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

> Please check the patch.

It looks good.  I think I was having a bad day when I introduced those thinkos.
I'm a bit distracted at the moment so I don't trust myself to forward
the patch on.  Hopefully later today.

> Also I have a question about TARGET_CPUS in io_apic.c.
>
> for a 16 sockets system with 32 non coherent ht chain. and if every
> chain have 8 irq for devices, the genapic will use physflat. and it
> should use you new added "different cpu can have same vector for
> different irq".  --- in the i8259.c
>
> but the setup_IOAPIC_irqs and arch_setup_ht_irq and arch_setup_msi_irq
> is still using TARGET_CPUS ( it is cpumask_of_cpu(0) for physflat), so
> the assign_irq_vector will not get vector for them, becase cpu 0 does
> not have that much vector to be alllocated. and later
> setup_affinity_xxx_irq can not be used because before irq is not there
> and show on /proc/interrupts.

Yep, that is a bug.  I hadn't realized real systems that exhausted the
number of vectors were quite so close.  That totally explains your
interest.  That is one I/O heavy development system you have YH.

Unless there is a reason not to, TARGET_CPUS should be cpu_online map.
I don't know why it is restricted to a single cpu.  I'm curious how
that would affect the user space irq balancer.

> So I want to
> 1. for arch_setup_ht_irq and arch_setup_msi_irq, we can use the dev it
> takes to get bus and use bus->sysdata to get bus->node mapping that is
> created in fillin_cpumask_to_bus, to get real target_cpus instead of
> cpu0.

That sounds like a very reasonable approach.

> 2. for ioapics, may need to add another array,
> ioapic_node[MAX_IOAPICS], and use ioapic address to get the numa node
> for it. So later can use it to get real targets cpus when need to use
> TARGET_CPUS.

Yes.  Numa affinity for the ioapics seems to make sense.  I don't
think we can count on looking at the address though?  Are your
ioapics pci devices?  If so that would be the easiest way to
deal with this problem.

> Please comments.

So far that is the job of the user space irqbalancer, but I don't see
why the kernel can't setup some reasonable defaults, if we have all of
the information needed.

For 2.6.19 we should be able to get my typos fixed, and probably
the default mask increased so that we are given a choice of something
other than cpu 0.

Beyond that it is going to take some additional working and thinking
and so it probably makes sense to have the code sit in the -mm
or Andi's tree for a while, and let it mature for 2.6.20.

Eric

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

[Andi Kleen]

On Mon, Oct 16, 2006 at 12:05:35PM -0600, Eric W. Biederman wrote:
> For 2.6.19 we should be able to get my typos fixed, and probably
> the default mask increased so that we are given a choice of something
> other than cpu 0.
> 
> Beyond that it is going to take some additional working and thinking
> and so it probably makes sense to have the code sit in the -mm
> or Andi's tree for a while, and let it mature for 2.6.20.

I admit I lost track of the patches for this new code which went
in while I was away.

Is that the only patch needed or are there other known problems too?

-Andi

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

[Eric W. Biederman]

Andi Kleen <ak@muc.de> writes:

> On Mon, Oct 16, 2006 at 12:05:35PM -0600, Eric W. Biederman wrote:
>> For 2.6.19 we should be able to get my typos fixed, and probably
>> the default mask increased so that we are given a choice of something
>> other than cpu 0.
>> 
>> Beyond that it is going to take some additional working and thinking
>> and so it probably makes sense to have the code sit in the -mm
>> or Andi's tree for a while, and let it mature for 2.6.20.
>
> I admit I lost track of the patches for this new code which went
> in while I was away.
>
> Is that the only patch needed or are there other known problems too?

The story.

The code went from -mm to -linus and everything was going smoothly
until we found a hyperthreaded cpu that in lowest priority delivery
mode would delivery mode would deliver irqs to hyperthreads even
if they were not in the mask.

Because of that and because Linus really likes lowest priority delivery
mode I submitted a patch that generalized the vector allocator to be
able to work on multiple cpus.

The patch was good except I missed one spot in retrigger irqs,
that YH caught, and I while the logic was fine in the vector allocator
I had several cases of using the wrong variable in the vector allocator.

Grumble! I made the vector allocator too general....

The one thinko that really affects people I fixed.  There are two more
cases that YH caught this weekend, that are still to be pushed.  Plus
there is the case that YH just caught that TARGET_CPUS being only a single
cpu is a real problem if you have more than 240 distinct irq sources.

So all that is pending are those two thinko fixes from YH.  The first
that fixes the retrigger irqs is a real bug fix.  The second that started
this thread is clearly the right thing to do, but I don't think there
are actually any real side effects from it.

Given how many thinkos I made last time I attacked this I'm not wanting
to do anything until I know I am thinking clearly :)

Eric

[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