Re: [tip:irq/core] genirq: Make nr_irqs runtime expandable

Re: [tip:irq/core] genirq: Make nr_irqs runtime expandable

[Yinghai Lu]

On 02/19/2011 04:15 AM, tip-bot for Thomas Gleixner wrote:
> Commit-ID:  e7bcecb7b1d29b9ad5af939149a945658620ca8f
> Gitweb:     http://git.kernel.org/tip/e7bcecb7b1d29b9ad5af939149a945658620ca8f
> Author:     Thomas Gleixner <tglx@linutronix.de>
> AuthorDate: Wed, 16 Feb 2011 17:12:57 +0100
> Committer:  Thomas Gleixner <tglx@linutronix.de>
> CommitDate: Sat, 19 Feb 2011 12:58:06 +0100
> 
> genirq: Make nr_irqs runtime expandable
> 
> We face more and more the requirement to expand nr_irqs at
> runtime. The reason are irq expanders which can not be detected in the
> early boot stage. So we speculate nr_irqs to have enough room. Further
> Xen needs extra irq numbers and we really want to avoid adding more
> "detection" code into the early boot. There is no real good reason why
> we need to limit nr_irqs at early boot.
> 
> Allow the allocation code to expand nr_irqs. We have already 8k extra
> number space in the allocation bitmap, so lets use it.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  kernel/irq/irqdesc.c |   22 +++++++++++++++++++---
>  1 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
> index a250d3a..6f6644f 100644
> --- a/kernel/irq/irqdesc.c
> +++ b/kernel/irq/irqdesc.c
> @@ -206,6 +206,14 @@ struct irq_desc * __ref irq_to_desc_alloc_node(unsigned int irq, int node)
>  	return NULL;
>  }
>  
> +static int irq_expand_nr_irqs(unsigned int cnt)
> +{
> +	if (nr_irqs + cnt > IRQ_BITMAP_BITS)
> +		return -ENOMEM;
> +	nr_irqs += cnt;
> +	return 0;
> +}
> +
>  int __init early_irq_init(void)
>  {
>  	int i, initcnt, node = first_online_node;
> @@ -287,6 +295,12 @@ static inline int alloc_descs(unsigned int start, unsigned int cnt, int node)
>  {
>  	return start;
>  }
> +
> +static int irq_expand_nr_irqs(unsigned int cnt)
> +{
> +	return -ENOMEM;
> +}
> +
>  #endif /* !CONFIG_SPARSE_IRQ */
>  
>  /* Dynamic interrupt handling */
> @@ -335,9 +349,11 @@ irq_alloc_descs(int irq, unsigned int from, unsigned int cnt, int node)
>  	if (irq >=0 && start != irq)
>  		goto err;
>  
> -	ret = -ENOMEM;
> -	if (start >= nr_irqs)
> -		goto err;
> +	if (start >= nr_irqs) {
> +		ret = irq_expand_nr_irqs(cnt);
> +		if (ret)
> +			goto err;
> +	}

may need following patch.

[PATCH] genirq: use IRQ_BITMAP_BITS as search size

instead of nr_irqs.

Otherwise bitmap_find_next_area could exit with larger start and in extreme
case we could fail to get wrong irqs return.

For example:
IRQ_BITMAP_BITS=10240
nr_irqs=8192
cnt=2048

and bit 0 to bit 8190 are set already.

before patch start from bit_find_next_area() will be 8191+2048.
later irq_expand_nr_irqs will set nr_irqs 10240.
finally irq_alloc_descs will return [8191+2048, 8191+2048+2047] happily..

with this patch, will get correct [8191, 8191+2047]

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 kernel/irq/irqdesc.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Index: linux-2.6/kernel/irq/irqdesc.c
===================================================================
--- linux-2.6.orig/kernel/irq/irqdesc.c
+++ linux-2.6/kernel/irq/irqdesc.c
@@ -346,12 +346,13 @@ irq_alloc_descs(int irq, unsigned int fr
 
 	mutex_lock(&sparse_irq_lock);
 
-	start = bitmap_find_next_zero_area(allocated_irqs, nr_irqs, from, cnt, 0);
+	start = bitmap_find_next_zero_area(allocated_irqs, IRQ_BITMAP_BITS,
+						 from, cnt, 0);
 	ret = -EEXIST;
 	if (irq >=0 && start != irq)
 		goto err;
 
-	if (start >= nr_irqs) {
+	if (start + cnt > nr_irqs) {
 		ret = irq_expand_nr_irqs(cnt);
 		if (ret)
 			goto err;

Re: [tip:irq/core] genirq: Make nr_irqs runtime expandable

[Thomas Gleixner]

On Sat, 19 Feb 2011, Yinghai Lu wrote:
> On 02/19/2011 04:15 AM, tip-bot for Thomas Gleixner wrote:
> [PATCH] genirq: use IRQ_BITMAP_BITS as search size
> 
> instead of nr_irqs.
> 
> Otherwise bitmap_find_next_area could exit with larger start and in extreme
> case we could fail to get wrong irqs return.
> 
> For example:
> IRQ_BITMAP_BITS=10240
> nr_irqs=8192
> cnt=2048
> 
> and bit 0 to bit 8190 are set already.
> 
> before patch start from bit_find_next_area() will be 8191+2048.
> later irq_expand_nr_irqs will set nr_irqs 10240.
> finally irq_alloc_descs will return [8191+2048, 8191+2048+2047] happily..
> 
> with this patch, will get correct [8191, 8191+2047]

What a horrible changelog.
 
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> 
> ---
>  kernel/irq/irqdesc.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> Index: linux-2.6/kernel/irq/irqdesc.c
> ===================================================================
> --- linux-2.6.orig/kernel/irq/irqdesc.c
> +++ linux-2.6/kernel/irq/irqdesc.c
> @@ -346,12 +346,13 @@ irq_alloc_descs(int irq, unsigned int fr
>  
>  	mutex_lock(&sparse_irq_lock);
>  
> -	start = bitmap_find_next_zero_area(allocated_irqs, nr_irqs, from, cnt, 0);
> +	start = bitmap_find_next_zero_area(allocated_irqs, IRQ_BITMAP_BITS,
> +						 from, cnt, 0);
>  	ret = -EEXIST;
>  	if (irq >=0 && start != irq)
>  		goto err;
>  
> -	if (start >= nr_irqs) {
> +	if (start + cnt > nr_irqs) {

One off. That needs to be  >= nr_irqs. Good catch otherwise.

Thanks,

	tglx

Re: [tip:irq/core] genirq: Make nr_irqs runtime expandable

[Thomas Gleixner]

On Mon, 21 Feb 2011, Thomas Gleixner wrote:
> On Sat, 19 Feb 2011, Yinghai Lu wrote:
> > -	if (start >= nr_irqs) {
> > +	if (start + cnt > nr_irqs) {
> 
> One off. That needs to be  >= nr_irqs. Good catch otherwise.

Confused myself. Check is correct as is.

Re: [tip:irq/core] genirq: Make nr_irqs runtime expandable

[Lars-Peter Clausen]

On 02/19/2011 08:07 PM, Yinghai Lu wrote:
> On 02/19/2011 04:15 AM, tip-bot for Thomas Gleixner wrote:
>> Commit-ID:  e7bcecb7b1d29b9ad5af939149a945658620ca8f
>> Gitweb:     http://git.kernel.org/tip/e7bcecb7b1d29b9ad5af939149a945658620ca8f
>> Author:     Thomas Gleixner <tglx@linutronix.de>
>> AuthorDate: Wed, 16 Feb 2011 17:12:57 +0100
>> Committer:  Thomas Gleixner <tglx@linutronix.de>
>> CommitDate: Sat, 19 Feb 2011 12:58:06 +0100
>>
>> genirq: Make nr_irqs runtime expandable
>>
>> We face more and more the requirement to expand nr_irqs at
>> runtime. The reason are irq expanders which can not be detected in the
>> early boot stage. So we speculate nr_irqs to have enough room. Further
>> Xen needs extra irq numbers and we really want to avoid adding more
>> "detection" code into the early boot. There is no real good reason why
>> we need to limit nr_irqs at early boot.
>>
>> Allow the allocation code to expand nr_irqs. We have already 8k extra
>> number space in the allocation bitmap, so lets use it.
>>
>> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
>> ---
>>  kernel/irq/irqdesc.c |   22 +++++++++++++++++++---
>>  1 files changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
>> index a250d3a..6f6644f 100644
>> --- a/kernel/irq/irqdesc.c
>> +++ b/kernel/irq/irqdesc.c
>> @@ -206,6 +206,14 @@ struct irq_desc * __ref irq_to_desc_alloc_node(unsigned int irq, int node)
>>  	return NULL;
>>  }
>>  
>> +static int irq_expand_nr_irqs(unsigned int cnt)
>> +{
>> +	if (nr_irqs + cnt > IRQ_BITMAP_BITS)
>> +		return -ENOMEM;
>> +	nr_irqs += cnt;
>> +	return 0;
>> +}
>> +
>>  int __init early_irq_init(void)
>>  {
>>  	int i, initcnt, node = first_online_node;
>> @@ -287,6 +295,12 @@ static inline int alloc_descs(unsigned int start, unsigned int cnt, int node)
>>  {
>>  	return start;
>>  }
>> +
>> +static int irq_expand_nr_irqs(unsigned int cnt)
>> +{
>> +	return -ENOMEM;
>> +}
>> +
>>  #endif /* !CONFIG_SPARSE_IRQ */
>>  
>>  /* Dynamic interrupt handling */
>> @@ -335,9 +349,11 @@ irq_alloc_descs(int irq, unsigned int from, unsigned int cnt, int node)
>>  	if (irq >=0 && start != irq)
>>  		goto err;
>>  
>> -	ret = -ENOMEM;
>> -	if (start >= nr_irqs)
>> -		goto err;
>> +	if (start >= nr_irqs) {
>> +		ret = irq_expand_nr_irqs(cnt);
>> +		if (ret)
>> +			goto err;
>> +	}
> 
> may need following patch.
> 
> [PATCH] genirq: use IRQ_BITMAP_BITS as search size
> 
> instead of nr_irqs.
> 
> Otherwise bitmap_find_next_area could exit with larger start and in extreme
> case we could fail to get wrong irqs return.
> 
> For example:
> IRQ_BITMAP_BITS=10240
> nr_irqs=8192
> cnt=2048
> 
> and bit 0 to bit 8190 are set already.
> 
> before patch start from bit_find_next_area() will be 8191+2048.
> later irq_expand_nr_irqs will set nr_irqs 10240.
> finally irq_alloc_descs will return [8191+2048, 8191+2048+2047] happily..
> 
> with this patch, will get correct [8191, 8191+2047]
> 
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> 
> ---
>  kernel/irq/irqdesc.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> Index: linux-2.6/kernel/irq/irqdesc.c
> ===================================================================
> --- linux-2.6.orig/kernel/irq/irqdesc.c
> +++ linux-2.6/kernel/irq/irqdesc.c
> @@ -346,12 +346,13 @@ irq_alloc_descs(int irq, unsigned int fr
>  
>  	mutex_lock(&sparse_irq_lock);
>  
> -	start = bitmap_find_next_zero_area(allocated_irqs, nr_irqs, from, cnt, 0);
> +	start = bitmap_find_next_zero_area(allocated_irqs, IRQ_BITMAP_BITS,
> +						 from, cnt, 0);
>  	ret = -EEXIST;
>  	if (irq >=0 && start != irq)
>  		goto err;
>  
> -	if (start >= nr_irqs) {
> +	if (start + cnt > nr_irqs) {
>  		ret = irq_expand_nr_irqs(cnt);
Just a minor thing, but if there are still unused irqs available at the end of
the current range, you'll end up expanding the range more then you need to.
So either do
	irq_expand_nr_irqs(nr_irqs - start + cnt);
or change irq_expand_nr_irqs to let it take the new total number of irqs.

Btw., with this patch in place does it make sense to initialize nr_irqs to
anything else then initcnt in early_irq_init?


>  		if (ret)
>  			goto err;
> --
> 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: [tip:irq/core] genirq: Make nr_irqs runtime expandable

[Yinghai Lu]

On 02/21/2011 07:56 AM, Lars-Peter Clausen wrote:
> On 02/19/2011 08:07 PM, Yinghai Lu wrote:
...

>> [PATCH] genirq: use IRQ_BITMAP_BITS as search size
>>
>> instead of nr_irqs.
>>
>> Otherwise bitmap_find_next_area could exit with larger start and in extreme
>> case we could fail to get wrong irqs return.
>>
>> For example:
>> IRQ_BITMAP_BITS=10240
>> nr_irqs=8192
>> cnt=2048
>>
>> and bit 0 to bit 8190 are set already.
>>
>> before patch start from bit_find_next_area() will be 8191+2048.
>> later irq_expand_nr_irqs will set nr_irqs 10240.
>> finally irq_alloc_descs will return [8191+2048, 8191+2048+2047] happily..
>>
>> with this patch, will get correct [8191, 8191+2047]
>>
>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>>
>> ---
>>  kernel/irq/irqdesc.c |    5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> Index: linux-2.6/kernel/irq/irqdesc.c
>> ===================================================================
>> --- linux-2.6.orig/kernel/irq/irqdesc.c
>> +++ linux-2.6/kernel/irq/irqdesc.c
>> @@ -346,12 +346,13 @@ irq_alloc_descs(int irq, unsigned int fr
>>  
>>  	mutex_lock(&sparse_irq_lock);
>>  
>> -	start = bitmap_find_next_zero_area(allocated_irqs, nr_irqs, from, cnt, 0);
>> +	start = bitmap_find_next_zero_area(allocated_irqs, IRQ_BITMAP_BITS,
>> +						 from, cnt, 0);
>>  	ret = -EEXIST;
>>  	if (irq >=0 && start != irq)
>>  		goto err;
>>  
>> -	if (start >= nr_irqs) {
>> +	if (start + cnt > nr_irqs) {
>>  		ret = irq_expand_nr_irqs(cnt);
> Just a minor thing, but if there are still unused irqs available at the end of
> the current range, you'll end up expanding the range more then you need to.
> So either do
> 	irq_expand_nr_irqs(nr_irqs - start + cnt);
> or change irq_expand_nr_irqs to let it take the new total number of irqs.

yes. please check second way that you suggested.

[PATCH -v2] genirq: use IRQ_BITMAP_BITS as search size

Otherwise bitmap_find_next_area could exit with larger start and in extreme
case we could fail to get wrong irqs return.

For example:
IRQ_BITMAP_BITS=10240
nr_irqs=8192
cnt=2048

and bit 0 to bit 8190 are set already.

before patch start from bit_find_next_area() will be 8191+2048.
later irq_expand_nr_irqs will set nr_irqs 10240.
finally irq_alloc_descs will return [8191+2048, 8191+2048+2047] happily..

with this patch, will get correct [8191, 8191+2047]

-v2: let irq_expand_nr_irqs take new nr_irqs instead of cnt, so do not
	increase nr_irqs too much extra.
	Suggested by "Lars-Peter" Clausen <lars@metafoo.de>

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 kernel/irq/irqdesc.c |   15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

Index: linux-2.6/kernel/irq/irqdesc.c
===================================================================
--- linux-2.6.orig/kernel/irq/irqdesc.c
+++ linux-2.6/kernel/irq/irqdesc.c
@@ -207,11 +207,11 @@ struct irq_desc * __ref irq_to_desc_allo
 	return NULL;
 }
 
-static int irq_expand_nr_irqs(unsigned int cnt)
+static int irq_expand_nr_irqs(int new_nr_irqs)
 {
-	if (nr_irqs + cnt > IRQ_BITMAP_BITS)
+	if (new_nr_irqs > IRQ_BITMAP_BITS)
 		return -ENOMEM;
-	nr_irqs += cnt;
+	nr_irqs = new_nr_irqs;
 	return 0;
 }
 
@@ -298,7 +298,7 @@ static inline int alloc_descs(unsigned i
 	return start;
 }
 
-static int irq_expand_nr_irqs(unsigned int cnt)
+static int irq_expand_nr_irqs(int new_nr_irqs)
 {
 	return -ENOMEM;
 }
@@ -346,13 +346,14 @@ irq_alloc_descs(int irq, unsigned int fr
 
 	mutex_lock(&sparse_irq_lock);
 
-	start = bitmap_find_next_zero_area(allocated_irqs, nr_irqs, from, cnt, 0);
+	start = bitmap_find_next_zero_area(allocated_irqs, IRQ_BITMAP_BITS,
+						 from, cnt, 0);
 	ret = -EEXIST;
 	if (irq >=0 && start != irq)
 		goto err;
 
-	if (start >= nr_irqs) {
-		ret = irq_expand_nr_irqs(cnt);
+	if (start + cnt > nr_irqs) {
+		ret = irq_expand_nr_irqs(start + cnt);
 		if (ret)
 			goto err;
 	}


> 
> Btw., with this patch in place does it make sense to initialize nr_irqs to
> anything else then initcnt in early_irq_init?

i have one for x86 and it will set nr_irqs to nr_irqs_gsi

[PATCH] x86, irq: keep nr_irqs as low as possible

For sparseirq, We can expand nr_irqs later if needed, so could
keep it from small for beginning.

Assign it to  minium value of vectors of all cpus and gsi pins


Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 arch/x86/kernel/apic/io_apic.c |   13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

Index: linux-2.6/arch/x86/kernel/apic/io_apic.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/apic/io_apic.c
+++ linux-2.6/arch/x86/kernel/apic/io_apic.c
@@ -3642,16 +3642,11 @@ int __init arch_probe_nr_irqs(void)
 {
 	int nr;
 
-	if (nr_irqs > (NR_VECTORS * nr_cpu_ids))
-		nr_irqs = NR_VECTORS * nr_cpu_ids;
+	nr = NR_VECTORS * nr_cpu_ids;
+	if (nr < nr_irqs)
+		nr_irqs = nr;
 
-	nr = nr_irqs_gsi + 8 * nr_cpu_ids;
-#if defined(CONFIG_PCI_MSI) || defined(CONFIG_HT_IRQ)
-	/*
-	 * for MSI and HT dyn irq
-	 */
-	nr += nr_irqs_gsi * 16;
-#endif
+	nr = nr_irqs_gsi;
 	if (nr < nr_irqs)
 		nr_irqs = nr;
 

Re: [tip:irq/core] genirq: Make nr_irqs runtime expandable

[Thomas Gleixner]

On Mon, 21 Feb 2011, Lars-Peter Clausen wrote:
> On 02/19/2011 08:07 PM, Yinghai Lu wrote:
> > On 02/19/2011 04:15 AM, tip-bot for Thomas Gleixner wrote:
> > -	if (start >= nr_irqs) {
> > +	if (start + cnt > nr_irqs) {
> >  		ret = irq_expand_nr_irqs(cnt);
> Just a minor thing, but if there are still unused irqs available at the end of
> the current range, you'll end up expanding the range more then you need to.
> So either do
> 	irq_expand_nr_irqs(nr_irqs - start + cnt);
> or change irq_expand_nr_irqs to let it take the new total number of irqs.

Fun, I already changed YH's patch in that way :)
 
> Btw., with this patch in place does it make sense to initialize nr_irqs to
> anything else then initcnt in early_irq_init?

Probably not.

Thanks,

	tglx

[tip:irq/core] genirq: Use IRQ_BITMAP_BITS as search size in irq_alloc_descs()

[tip-bot for Yinghai Lu]

Commit-ID:  ed4dea6e0e33a3e58d8b77b775a8f0e433e7a005
Gitweb:     http://git.kernel.org/tip/ed4dea6e0e33a3e58d8b77b775a8f0e433e7a005
Author:     Yinghai Lu <yinghai@kernel.org>
AuthorDate: Sat, 19 Feb 2011 11:07:37 -0800
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Mon, 21 Feb 2011 21:20:00 +0100

genirq: Use IRQ_BITMAP_BITS as search size in irq_alloc_descs()

The runtime expansion of nr_irqs does not take into account that
bitmap_find_next_zero_area() returns "start" + size in case the search
for an matching zero area fails. That results in a start value which
can be completely off and is not covered by the following
expand_nr_irqs() and possibly outside of the absolute limit. But we
use it without further checking.

Use IRQ_BITMAP_BITS as the limit for the bitmap search and expand
nr_irqs when the start bit is beyond nr_irqs. So start is always
pointing to the correct area in the bitmap. nr_irqs is just the limit
for irq enumerations, not the real limit for the irq space.

[ tglx: Let irq_expand_nr_irqs() take the new upper end so we do not
  	expand nr_irqs more than necessary. Made changelog readable ]

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
LKML-Reference: <4D6014F9.8040605@kernel.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/irq/irqdesc.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 394ab6a..dbccc79 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -207,11 +207,11 @@ struct irq_desc * __ref irq_to_desc_alloc_node(unsigned int irq, int node)
 	return NULL;
 }
 
-static int irq_expand_nr_irqs(unsigned int cnt)
+static int irq_expand_nr_irqs(unsigned int nr)
 {
-	if (nr_irqs + cnt > IRQ_BITMAP_BITS)
+	if (nr > IRQ_BITMAP_BITS)
 		return -ENOMEM;
-	nr_irqs += cnt;
+	nr_irqs = nr;
 	return 0;
 }
 
@@ -298,7 +298,7 @@ static inline int alloc_descs(unsigned int start, unsigned int cnt, int node)
 	return start;
 }
 
-static int irq_expand_nr_irqs(unsigned int cnt)
+static int irq_expand_nr_irqs(unsigned int nr)
 {
 	return -ENOMEM;
 }
@@ -346,13 +346,14 @@ irq_alloc_descs(int irq, unsigned int from, unsigned int cnt, int node)
 
 	mutex_lock(&sparse_irq_lock);
 
-	start = bitmap_find_next_zero_area(allocated_irqs, nr_irqs, from, cnt, 0);
+	start = bitmap_find_next_zero_area(allocated_irqs, IRQ_BITMAP_BITS,
+					   from, cnt, 0);
 	ret = -EEXIST;
 	if (irq >=0 && start != irq)
 		goto err;
 
-	if (start >= nr_irqs) {
-		ret = irq_expand_nr_irqs(cnt);
+	if (start + cnt > nr_irqs) {
+		ret = irq_expand_nr_irqs(start + cnt);
 		if (ret)
 			goto err;
 	}