RE: [PATCH] x86_64 irq: reset more to default when clear irq_vector for destroy_irq

RE: [PATCH] x86_64 irq: reset more to default when clear irq_vector for destroy_irq

[Lu, Yinghai]

Thanks.

I only found ht_irq and msi call destroy_irq. How about io_apic? 

YH

-----Original Message-----
From: Muli Ben-Yehuda [mailto:muli@il.ibm.com] 
Sent: Wednesday, October 25, 2006 4:30 AM
To: Lu, Yinghai
Cc: Andi Kleen; Eric W. Biederman; Andrew Morton; Linux Kernel Mailing
List
Subject: Re: [PATCH] x86_64 irq: reset more to default when clear
irq_vector for destroy_irq

On Tue, Oct 24, 2006 at 08:46:31PM -0700, Yinghai Lu wrote:
> resend with gmail.
> 
> Clear the irq releated entries in irq_vector, irq_domain and
vector_irq
> instead of clearing irq_vector only. So when new irq is created, it
> could reuse that vector. (actually is the second loop scanning from
> FIRST_DEVICE_VECTOR+8). This could avoid the vectors are used up
> with enough module inserting and removing
> 
> Cc: Eric W. Biedierman <ebiederm@xmission.com>
> Signed-off-By: Yinghai Lu <yinghai.lu@amd.com>

I hope I'm testing the right patch... this one boots and works fine.

Cheers,
Muli

Re: [PATCH] x86_64 irq: reset more to default when clear irq_vector for destroy_irq

[Eric W. Biederman]

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

> Thanks.
>
> I only found ht_irq and msi call destroy_irq. How about io_apic? 

Only on io_apic hot unplug which we don't currently support.

Eric

Re: [PATCH] x86_64 irq: reset more to default when clear irq_vector for destroy_irq

[Yinghai Lu]

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

revised version according to Eric. and it can be applied clearly to
current Linus's Tree.

Clear the irq releated entries in irq_vector, irq_domain and vector_irq
instead of clearing irq_vector only. So when new irq is created, it
could reuse that vector. (actually is the second loop scanning from
FIRST_DEVICE_VECTOR+8). This could avoid the vectors are used up
with enough module inserting and removing

Cc: Eric W. Biedierman <ebiederm@xmission.com>
Cc: Muli Ben-Yehuda <muli@il.ibm.com>
Signed-off-By: Yinghai Lu <yinghai.lu@amd.com>

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

diff --git a/arch/x86_64/kernel/io_apic.c b/arch/x86_64/kernel/io_apic.c
index fe429e5..976634c 100644
--- a/arch/x86_64/kernel/io_apic.c
+++ b/arch/x86_64/kernel/io_apic.c
@@ -684,6 +684,22 @@ static int assign_irq_vector(int irq, cp
 	return vector;
 }
 
+static void __clear_irq_vector(int irq)
+{
+	cpumask_t mask;
+	int cpu, vector;
+
+	BUG_ON(!irq_vector[irq]);
+
+	vector = irq_vector[irq];
+	cpus_and(mask, irq_domain[irq], cpu_online_map);
+	for_each_cpu_mask(cpu, mask)
+		per_cpu(vector_irq, cpu)[vector] = -1;
+
+	irq_vector[irq] = 0;
+	irq_domain[irq] = CPU_MASK_NONE;
+}
+
 void __setup_vector_irq(int cpu)
 {
 	/* Initialize vector_irq on a new cpu */
@@ -1771,7 +1787,7 @@ void destroy_irq(unsigned int irq)
 	dynamic_irq_cleanup(irq);
 
 	spin_lock_irqsave(&vector_lock, flags);
-	irq_vector[irq] = 0;
+	__clear_irq_vector(irq);
 	spin_unlock_irqrestore(&vector_lock, flags);
 }
 

Re: [PATCH] x86_64 irq: reset more to default when clear irq_vector for destroy_irq

[Oleg Verych]

Again, mister Yinghai Lu, bother to do patches, but you
"have to use outlook in [] office to send email".?

Anyway, _bother_ apply Documentation/SubmittingPatches.6, *please*.
,-
|6) No MIME, no links, no compression, no attachments.  Just plain text.
`-

> ------=_Part_69251_21055565.1162014276979
> Content-Type: text/x-patch; name=io_apic_clear_irq_vector_1027.diff; 
> 	charset=ANSI_X3.4-1968
> Content-Transfer-Encoding: base64
> X-Attachment-Id: f_ettl4wte
> Content-Disposition: attachment; filename="io_apic_clear_irq_vector_1027.diff"
>
> ZGlmZiAtLWdpdCBhL2FyY2gveDg2XzY0L2tlcm5lbC9pb19hcGljLmMgYi9hcmNoL3g4Nl82NC9r
> ZXJuZWwvaW9fYXBpYy5jCmluZGV4IGZlNDI5ZTUuLjk3NjYzNGMgMTAwNjQ0Ci0tLSBhL2FyY2gv
> eDg2XzY0L2tlcm5lbC9pb19hcGljLmMKKysrIGIvYXJjaC94ODZfNjQva2VybmVsL2lvX2FwaWMu
[]
____

Re: [PATCH] x86_64 irq: reset more to default when clear irq_vector for destroy_irq

[Andi Kleen]

On Fri, Oct 27, 2006 at 10:44:36PM -0700, Yinghai Lu wrote:
> revised version according to Eric. and it can be applied clearly to
> current Linus's Tree.
> 
> Clear the irq releated entries in irq_vector, irq_domain and vector_irq
> instead of clearing irq_vector only. So when new irq is created, it
> could reuse that vector. (actually is the second loop scanning from
> FIRST_DEVICE_VECTOR+8). This could avoid the vectors are used up
> with enough module inserting and removing

Added thanks.

Does i386 need a similar patch?

-Andi

Re: [PATCH] x86_64 irq: reset more to default when clear irq_vector for destroy_irq

[Eric W. Biederman]

Andi Kleen <ak@muc.de> writes:

> On Fri, Oct 27, 2006 at 10:44:36PM -0700, Yinghai Lu wrote:
>> revised version according to Eric. and it can be applied clearly to
>> current Linus's Tree.
>> 
>> Clear the irq releated entries in irq_vector, irq_domain and vector_irq
>> instead of clearing irq_vector only. So when new irq is created, it
>> could reuse that vector. (actually is the second loop scanning from
>> FIRST_DEVICE_VECTOR+8). This could avoid the vectors are used up
>> with enough module inserting and removing
>
> Added thanks.
>
> Does i386 need a similar patch?

i386 is good, as it doesn't deal with per cpu vectors, so it doesn't
have the additional data structures.

Eric

[PATCH] x86_64 irq: reset more to default when clear irq_vector for destroy_irq

[Lu, Yinghai]

 
Clear the irq releated entries in irq_vector, irq_domain and vector_irq 
instead of clearing irq_vector only. So when new irq is created, it 
could get that vector.

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

--- linux-2.6/arch/x86_64/kernel/io_apic.c	2006-10-24
13:40:48.000000000 -0700
+++ linux-2.6.xx/arch/x86_64/kernel/io_apic.c	2006-10-24
14:03:08.000000000 -0700
@@ -716,6 +716,22 @@
 	return vector;
 }
 
+static void __clear_irq_vector(int irq)
+{
+	int old_vector = -1;
+	if (irq_vector[irq] > 0)
+		old_vector = irq_vector[irq];
+	if (old_vector >= 0) {
+		cpumask_t old_mask;
+		int old_cpu;
+		cpus_and(old_mask, irq_domain[irq], cpu_online_map);
+		for_each_cpu_mask(old_cpu, old_mask)
+			per_cpu(vector_irq, old_cpu)[old_vector] = -1;
+	}
+	irq_vector[irq] = 0;
+	irq_domain[irq] = CPU_MASK_NONE;
+}
+
 void __setup_vector_irq(int cpu)
 {
 	/* Initialize vector_irq on a new cpu */
@@ -1803,7 +1819,7 @@
 	dynamic_irq_cleanup(irq);
 
 	spin_lock_irqsave(&vector_lock, flags);
-	irq_vector[irq] = 0;
+	__clear_irq_vector(irq);
 	spin_unlock_irqrestore(&vector_lock, flags);
 }
 

Re: [PATCH] x86_64 irq: reset more to default when clear irq_vector for destroy_irq

[Josef Sipek]

On Tue, Oct 24, 2006 at 02:33:08PM -0700, Lu, Yinghai wrote:
>  
> Clear the irq releated entries in irq_vector, irq_domain and vector_irq 
> instead of clearing irq_vector only. So when new irq is created, it 
> could get that vector.
> 
> Signed-off-By: Yinghai Lu <yinghai.lu@amd.com>
> 
> --- linux-2.6/arch/x86_64/kernel/io_apic.c	2006-10-24
> 13:40:48.000000000 -0700
> +++ linux-2.6.xx/arch/x86_64/kernel/io_apic.c	2006-10-24
> 14:03:08.000000000 -0700
> @@ -716,6 +716,22 @@

Your patch got mangled up.

Josef "Jeff" Sipek.

-- 
Linux, n.:
  Generous programmers from around the world all join forces to help
  you shoot yourself in the foot for free. 

Re: [PATCH] x86_64 irq: reset more to default when clear irq_vector for destroy_irq

[Yinghai Lu]

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

resend with gmail.

Clear the irq releated entries in irq_vector, irq_domain and vector_irq
instead of clearing irq_vector only. So when new irq is created, it
could reuse that vector. (actually is the second loop scanning from
FIRST_DEVICE_VECTOR+8). This could avoid the vectors are used up
with enough module inserting and removing

Cc: Eric W. Biedierman <ebiederm@xmission.com>
Signed-off-By: Yinghai Lu <yinghai.lu@amd.com>

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

--- linux-2.6/arch/x86_64/kernel/io_apic.c	2006-10-24 13:40:48.000000000 -0700
+++ linux-2.6.xx/arch/x86_64/kernel/io_apic.c	2006-10-24 14:03:08.000000000 -0700
@@ -716,6 +716,22 @@
 	return vector;
 }
 
+static void __clear_irq_vector(int irq)
+{
+	int old_vector = -1;
+	if (irq_vector[irq] > 0)
+		old_vector = irq_vector[irq];
+	if (old_vector >= 0) {
+		cpumask_t old_mask;
+		int old_cpu;
+		cpus_and(old_mask, irq_domain[irq], cpu_online_map);
+		for_each_cpu_mask(old_cpu, old_mask)
+			per_cpu(vector_irq, old_cpu)[old_vector] = -1;
+	}
+	irq_vector[irq] = 0;
+	irq_domain[irq] = CPU_MASK_NONE;
+}
+
 void __setup_vector_irq(int cpu)
 {
 	/* Initialize vector_irq on a new cpu */
@@ -1803,7 +1819,7 @@
 	dynamic_irq_cleanup(irq);
 
 	spin_lock_irqsave(&vector_lock, flags);
-	irq_vector[irq] = 0;
+	__clear_irq_vector(irq);
 	spin_unlock_irqrestore(&vector_lock, flags);
 }
 

Re: [PATCH] x86_64 irq: reset more to default when clear irq_vector for destroy_irq

[Yinghai Lu]

Andi,

This could get into your tree.

YH

On 10/24/06, Yinghai Lu <yinghai.lu@amd.com> wrote:
> resend with gmail.
>
> Clear the irq releated entries in irq_vector, irq_domain and vector_irq
> instead of clearing irq_vector only. So when new irq is created, it
> could reuse that vector. (actually is the second loop scanning from
> FIRST_DEVICE_VECTOR+8). This could avoid the vectors are used up
> with enough module inserting and removing
>
> Cc: Eric W. Biedierman <ebiederm@xmission.com>
> Signed-off-By: Yinghai Lu <yinghai.lu@amd.com>
>
>
>

Re: [PATCH] x86_64 irq: reset more to default when clear irq_vector for destroy_irq

[David Rientjes]

On Tue, 24 Oct 2006, Yinghai Lu wrote:

> --- linux-2.6/arch/x86_64/kernel/io_apic.c	2006-10-24 13:40:48.000000000 -0700
> +++ linux-2.6.xx/arch/x86_64/kernel/io_apic.c	2006-10-24 14:03:08.000000000 -0700
> @@ -716,6 +716,22 @@
>  	return vector;
>  }
>  
> +static void __clear_irq_vector(int irq)
> +{
> +	int old_vector = -1;
> +	if (irq_vector[irq] > 0)
> +		old_vector = irq_vector[irq];
> +	if (old_vector >= 0) {
> +		cpumask_t old_mask;
> +		int old_cpu;
> +		cpus_and(old_mask, irq_domain[irq], cpu_online_map);
> +		for_each_cpu_mask(old_cpu, old_mask)
> +			per_cpu(vector_irq, old_cpu)[old_vector] = -1;
> +	}
> +	irq_vector[irq] = 0;
> +	irq_domain[irq] = CPU_MASK_NONE;
> +}
> +

The two conditionals don't complement each other; in fact, only one 
conditional is required since the test for old_vector equality to zero 
will never be satisfied.  Also note that irq_vectors are u8 on x86_64 and 
not ints.

	static void __clear_irq_vector(int irq)
	{
		u8 old_vector = irq_vector[irq];
		if (old_vector > 0) {
			cpumask_t old_mask;
			int old_cpu;
			cpus_and(old_mask, irq_domain[irq], cpu_online_map);
			for_each_cpu_mask(old_cpu, old_mask)
				per_cpu(vector_irq, old_cpu)[old_vector] = -1;
		}
		irq_vector[irq] = 0;
		irq_domain[irq] = CPU_MASK_NONE;
	}

Re: [PATCH] x86_64 irq: reset more to default when clear irq_vector for destroy_irq

[yhlu]

I cut the code paste the lines from __assign_irq_vector().

It seems we need to change
    if (old_vector >= 0) {
in __assign_irq_vector with this one to
    if (old_vector > 0) {
later.   Eric?

YH




> The two conditionals don't complement each other; in fact, only one
> conditional is required since the test for old_vector equality to zero
> will never be satisfied.  Also note that irq_vectors are u8 on x86_64 and
> not ints.
>
>         static void __clear_irq_vector(int irq)
>         {
>                 u8 old_vector = irq_vector[irq];
>                 if (old_vector > 0) {
>                         cpumask_t old_mask;
>                         int old_cpu;
>                         cpus_and(old_mask, irq_domain[irq], cpu_online_map);
>                         for_each_cpu_mask(old_cpu, old_mask)
>                                 per_cpu(vector_irq, old_cpu)[old_vector] = -1;
>                 }
>                 irq_vector[irq] = 0;
>                 irq_domain[irq] = CPU_MASK_NONE;
>         }
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [PATCH] x86_64 irq: reset more to default when clear irq_vector for destroy_irq

[Muli Ben-Yehuda]

On Tue, Oct 24, 2006 at 08:46:31PM -0700, Yinghai Lu wrote:
> resend with gmail.
> 
> Clear the irq releated entries in irq_vector, irq_domain and vector_irq
> instead of clearing irq_vector only. So when new irq is created, it
> could reuse that vector. (actually is the second loop scanning from
> FIRST_DEVICE_VECTOR+8). This could avoid the vectors are used up
> with enough module inserting and removing
> 
> Cc: Eric W. Biedierman <ebiederm@xmission.com>
> Signed-off-By: Yinghai Lu <yinghai.lu@amd.com>

I hope I'm testing the right patch... this one boots and works fine.

Cheers,
Muli