[PATCH] X86: Change the default value of nr_irqs from 32 to NR_IRQs

[PATCH] X86: Change the default value of nr_irqs from 32 to NR_IRQs

[Alex Nixon]

From: Alex Nixon <alex.nixon@citrix.com>
Date: Tue, 19 Aug 2008 17:49:37 +0100
Subject: [PATCH] X86:  Change the default value of nr_irqs from 32 to NR_IRQs

If the number of discovered IRQs is suspiciously low, this patch causes the number reported to default to NR_IRQS, rather than 32.  NR_IRQS has already been defined to be a sensible value for the current system (in particular, at least 224 when paravirtualisation is involved).

Signed-off-by: Alex Nixon <alex.nixon@citrix.com>
---
 arch/x86/kernel/io_apic.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/io_apic.c b/arch/x86/kernel/io_apic.c
index 30ff794..11bffa1 100644
--- a/arch/x86/kernel/io_apic.c
+++ b/arch/x86/kernel/io_apic.c
@@ -3632,7 +3632,7 @@ int __init probe_nr_irqs(void)
 
 	/* something wrong ? */
 	if (nr < 32)
-		nr = 32;
+		nr = NR_IRQS;
 
 	return nr;
 }
-- 
1.5.4.3

Re: [PATCH] X86: Change the default value of nr_irqs from 32 to NR_IRQs

[Yinghai Lu]

On Tue, Aug 19, 2008 at 9:55 AM, Alex Nixon <alex.nixon@citrix.com> wrote:
> From: Alex Nixon <alex.nixon@citrix.com>
> Date: Tue, 19 Aug 2008 17:49:37 +0100
> Subject: [PATCH] X86:  Change the default value of nr_irqs from 32 to NR_IRQs
>
> If the number of discovered IRQs is suspiciously low, this patch causes the number reported to default to NR_IRQS, rather than 32.  NR_IRQS has already been defined to be a sensible value for the current system (in particular, at least 224 when paravirtualisation is involved).
>
> Signed-off-by: Alex Nixon <alex.nixon@citrix.com>
> ---
>  arch/x86/kernel/io_apic.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/arch/x86/kernel/io_apic.c b/arch/x86/kernel/io_apic.c
> index 30ff794..11bffa1 100644
> --- a/arch/x86/kernel/io_apic.c
> +++ b/arch/x86/kernel/io_apic.c
> @@ -3632,7 +3632,7 @@ int __init probe_nr_irqs(void)
>
>        /* something wrong ? */
>        if (nr < 32)
> -               nr = 32;
> +               nr = NR_IRQS;
>
>        return nr;
>  }
> --
if only one ioapic, nr will be 24<<1, you will get 48. Does pv has io apic ?

YH

Re: [PATCH] X86: Change the default value of nr_irqs from 32 to NR_IRQs

[Yinghai Lu]

On Tue, Aug 19, 2008 at 11:13 AM, Alex Nixon (Intern)
<Alex.Nixon@citrix.com> wrote:
> Yinghai Lu <yhlu.kernel@gmail.com> wrote:
>>On Tue, Aug 19, 2008 at 9:55 AM, Alex Nixon <alex.nixon@citrix.com> wrote:
>>>
>>> If the number of discovered IRQs is suspiciously low, this patch causes
>>> the number reported to default to NR_IRQS, rather than 32.  NR_IRQS has
>>> already been defined to be a >sensible value for the current system (in
>>> particular, at least 224 when paravirtualisation is involved).
>>>
>>if only one ioapic, nr will be 24<<1, you will get 48. Does pv has io apic
>> ?
>>
>>YH
>>
> I'm not sure about the general case, but Xen does not (Jeremy correct me if
> I'm wrong).
>
> Unless I'm missing something (which I may well be; I'm new to this area of
> code), it seems more logical anyway to default back to the calculated
> system-specific value (NR_IRQS), instead of 32, which seems rather
> arbitrary.

can you try !CONFIG_HAVE_SPARSE_IRQ and CONFIG_HAVE_SPARSE_IRQ ?

YH

Re: [PATCH] X86: Change the default value of nr_irqs from 32 to NR_IRQs

[Alex Nixon]

Yinghai Lu wrote:
> On Tue, Aug 19, 2008 at 11:13 AM, Alex Nixon (Intern)
> <Alex.Nixon@citrix.com> wrote:
>> Yinghai Lu <yhlu.kernel@gmail.com> wrote:
>>> On Tue, Aug 19, 2008 at 9:55 AM, Alex Nixon <alex.nixon@citrix.com> wrote:
>>>> If the number of discovered IRQs is suspiciously low, this patch causes
>>>> the number reported to default to NR_IRQS, rather than 32.  NR_IRQS has
>>>> already been defined to be a >sensible value for the current system (in
>>>> particular, at least 224 when paravirtualisation is involved).
>>>>
>>> if only one ioapic, nr will be 24<<1, you will get 48. Does pv has io apic
>>> ?
>>>
>>> YH
>>>
>> I'm not sure about the general case, but Xen does not (Jeremy correct me if
>> I'm wrong).
>>
>> Unless I'm missing something (which I may well be; I'm new to this area of
>> code), it seems more logical anyway to default back to the calculated
>> system-specific value (NR_IRQS), instead of 32, which seems rather
>> arbitrary.
> 
> can you try !CONFIG_HAVE_SPARSE_IRQ and CONFIG_HAVE_SPARSE_IRQ ?
> 
> YH

Sorry I should have mentioned originally - the bug occurs both with 
CONFIG_HAVE_SPARSE_IRQ enabled, and disabled.

- Alex

Re: [PATCH] X86: Change the default value of nr_irqs from 32 to NR_IRQs

[Yinghai Lu]

On Tue, Aug 19, 2008 at 11:32 AM, Alex Nixon <alex.nixon@citrix.com> wrote:
> Yinghai Lu wrote:
>>
>> On Tue, Aug 19, 2008 at 11:13 AM, Alex Nixon (Intern)
>> <Alex.Nixon@citrix.com> wrote:
>>>
>>> Yinghai Lu <yhlu.kernel@gmail.com> wrote:
>>>>
>>>> On Tue, Aug 19, 2008 at 9:55 AM, Alex Nixon <alex.nixon@citrix.com>
>>>> wrote:
>>>>>
>>>>> If the number of discovered IRQs is suspiciously low, this patch causes
>>>>> the number reported to default to NR_IRQS, rather than 32.  NR_IRQS has
>>>>> already been defined to be a >sensible value for the current system (in
>>>>> particular, at least 224 when paravirtualisation is involved).
>>>>>
>>>> if only one ioapic, nr will be 24<<1, you will get 48. Does pv has io
>>>> apic
>>>> ?
>>>>
>>>> YH
>>>>
>>> I'm not sure about the general case, but Xen does not (Jeremy correct me
>>> if
>>> I'm wrong).
>>>
>>> Unless I'm missing something (which I may well be; I'm new to this area
>>> of
>>> code), it seems more logical anyway to default back to the calculated
>>> system-specific value (NR_IRQS), instead of 32, which seems rather
>>> arbitrary.
>>
>> can you try !CONFIG_HAVE_SPARSE_IRQ and CONFIG_HAVE_SPARSE_IRQ ?
>>
>> YH
>
> Sorry I should have mentioned originally - the bug occurs both with
> CONFIG_HAVE_SPARSE_IRQ enabled, and disabled.

maybe we need special probe_nr_irqs for PV or not call that in
setup_arch for xen -- it will leave nr_irqs == NR_IRQS

YH

Re: [PATCH] X86: Change the default value of nr_irqs from 32 to NR_IRQs

[Alex Nixon]

Yinghai Lu wrote:
> On Tue, Aug 19, 2008 at 11:32 AM, Alex Nixon <alex.nixon@citrix.com> wrote:
>> Yinghai Lu wrote:
>>> On Tue, Aug 19, 2008 at 11:13 AM, Alex Nixon (Intern)
>>> <Alex.Nixon@citrix.com> wrote:
>>>> Yinghai Lu <yhlu.kernel@gmail.com> wrote:
>>>>> On Tue, Aug 19, 2008 at 9:55 AM, Alex Nixon <alex.nixon@citrix.com>
>>>>> wrote:
>>>>>> If the number of discovered IRQs is suspiciously low, this patch causes
>>>>>> the number reported to default to NR_IRQS, rather than 32.  NR_IRQS has
>>>>>> already been defined to be a >sensible value for the current system (in
>>>>>> particular, at least 224 when paravirtualisation is involved).
>>>>>>
>>>>> if only one ioapic, nr will be 24<<1, you will get 48. Does pv has io
>>>>> apic
>>>>> ?
>>>>>
>>>>> YH
>>>>>
>>>> I'm not sure about the general case, but Xen does not (Jeremy correct me
>>>> if
>>>> I'm wrong).
>>>>
>>>> Unless I'm missing something (which I may well be; I'm new to this area
>>>> of
>>>> code), it seems more logical anyway to default back to the calculated
>>>> system-specific value (NR_IRQS), instead of 32, which seems rather
>>>> arbitrary.
>>> can you try !CONFIG_HAVE_SPARSE_IRQ and CONFIG_HAVE_SPARSE_IRQ ?
>>>
>>> YH
>> Sorry I should have mentioned originally - the bug occurs both with
>> CONFIG_HAVE_SPARSE_IRQ enabled, and disabled.
> 
> maybe we need special probe_nr_irqs for PV or not call that in
> setup_arch for xen -- it will leave nr_irqs == NR_IRQS
> 
> YH

That would be one solution, but would be more involved than this trivial 
patch (although if considered more 'correct' then it is of course worth 
the effort).
   But attempting to keep things simple, is there a reason it's 
preferable to fall back to 32 rather NR_IRQS?

- Alex

Re: [PATCH] X86: Change the default value of nr_irqs from 32 to NR_IRQs

[Yinghai Lu]

On Tue, Aug 19, 2008 at 12:50 PM, Alex Nixon <alex.nixon@citrix.com> wrote:
> Yinghai Lu wrote:
>>
>> On Tue, Aug 19, 2008 at 11:32 AM, Alex Nixon <alex.nixon@citrix.com>
>> wrote:
>>>
>>> Yinghai Lu wrote:
>>>>
>>>> On Tue, Aug 19, 2008 at 11:13 AM, Alex Nixon (Intern)
>>>> <Alex.Nixon@citrix.com> wrote:
>>>>>
>>>>> Yinghai Lu <yhlu.kernel@gmail.com> wrote:
>>>>>>
>>>>>> On Tue, Aug 19, 2008 at 9:55 AM, Alex Nixon <alex.nixon@citrix.com>
>>>>>> wrote:
>>>>>>>
>>>>>>> If the number of discovered IRQs is suspiciously low, this patch
>>>>>>> causes
>>>>>>> the number reported to default to NR_IRQS, rather than 32.  NR_IRQS
>>>>>>> has
>>>>>>> already been defined to be a >sensible value for the current system
>>>>>>> (in
>>>>>>> particular, at least 224 when paravirtualisation is involved).
>>>>>>>
>>>>>> if only one ioapic, nr will be 24<<1, you will get 48. Does pv has io
>>>>>> apic
>>>>>> ?
>>>>>>
>>>>>> YH
>>>>>>
>>>>> I'm not sure about the general case, but Xen does not (Jeremy correct
>>>>> me
>>>>> if
>>>>> I'm wrong).
>>>>>
>>>>> Unless I'm missing something (which I may well be; I'm new to this area
>>>>> of
>>>>> code), it seems more logical anyway to default back to the calculated
>>>>> system-specific value (NR_IRQS), instead of 32, which seems rather
>>>>> arbitrary.
>>>>
>>>> can you try !CONFIG_HAVE_SPARSE_IRQ and CONFIG_HAVE_SPARSE_IRQ ?
>>>>
>>>> YH
>>>
>>> Sorry I should have mentioned originally - the bug occurs both with
>>> CONFIG_HAVE_SPARSE_IRQ enabled, and disabled.
>>
>> maybe we need special probe_nr_irqs for PV or not call that in
>> setup_arch for xen -- it will leave nr_irqs == NR_IRQS
>>
>> YH
>
> That would be one solution, but would be more involved than this trivial
> patch (although if considered more 'correct' then it is of course worth the
> effort).
>  But attempting to keep things simple, is there a reason it's preferable to
> fall back to 32 rather NR_IRQS?

when !CONFIG_HAVE_SPARSE_IRQ, with dyn_array, could allocate irq_desc
and etc as less as possible.
when CONFIG_HAVE_SPARESE_IRQ, no actually meaning for nr_irqs.

YH

Re: [PATCH] X86: Change the default value of nr_irqs from 32 to NR_IRQs

[Alex Nixon]

Yinghai Lu wrote:
> when !CONFIG_HAVE_SPARSE_IRQ, with dyn_array, could allocate irq_desc
> and etc as less as possible.
> when CONFIG_HAVE_SPARESE_IRQ, no actually meaning for nr_irqs.
> 
> YH

So I believe the only case this affects is !CONFIG_HAVE_SPARSE_IRQ

The worry is that with CONFIG_HAVE_DYN_ARRAY we may waste memory by 
pre-allocating more irq_descs than may be necessary (NR_IRQs vs 32)?

With !CONFIG_HAVE_DYN_ARRAY however, a static array of size NR_IRQS is 
allocated instead - so doesn't defaulting nr_irqs back to NR_IRQS just 
revert to the old behaviour (with the exception of the irq_descs being 
allocated in pre_alloc_dyn_array instead)?

- Alex

Re: [PATCH] X86: Change the default value of nr_irqs from 32 to NR_IRQs

[Alex Nixon]

Alex Nixon wrote:
> Yinghai Lu wrote:
>> when !CONFIG_HAVE_SPARSE_IRQ, with dyn_array, could allocate irq_desc
>> and etc as less as possible.
>> when CONFIG_HAVE_SPARESE_IRQ, no actually meaning for nr_irqs.
>>
>> YH
> 
> So I believe the only case this affects is !CONFIG_HAVE_SPARSE_IRQ
> 
> The worry is that with CONFIG_HAVE_DYN_ARRAY we may waste memory by 
> pre-allocating more irq_descs than may be necessary (NR_IRQs vs 32)?
> 
> With !CONFIG_HAVE_DYN_ARRAY however, a static array of size NR_IRQS is 
> allocated instead - so doesn't defaulting nr_irqs back to NR_IRQS just 
> revert to the old behaviour (with the exception of the irq_descs being 
> allocated in pre_alloc_dyn_array instead)?
> 
> - Alex
> 

Sorry to pester you Yinghai, but I'd like to get a patch for this out 
one way or another as Xen is _completely_ unusable with 5 or more VCPUs.

Can you explain more clearly what the problem with the patch is?

I have a different patch set which solves the problem by adding in a pv 
hook for probe_nr_irqs, but it's by far less clean.

Or alternatively, we could revert your patch b2e5f326bb

Cheers,
- Alex

Re: [PATCH] X86: Change the default value of nr_irqs from 32 to NR_IRQs

[Yinghai Lu]

On Wed, Aug 20, 2008 at 4:23 PM, Alex Nixon <alex.nixon@citrix.com> wrote:
> Alex Nixon wrote:
>>
>> Yinghai Lu wrote:
>>>
>>> when !CONFIG_HAVE_SPARSE_IRQ, with dyn_array, could allocate irq_desc
>>> and etc as less as possible.
>>> when CONFIG_HAVE_SPARESE_IRQ, no actually meaning for nr_irqs.
>>>
>>> YH
>>
>> So I believe the only case this affects is !CONFIG_HAVE_SPARSE_IRQ
>>
>> The worry is that with CONFIG_HAVE_DYN_ARRAY we may waste memory by
>> pre-allocating more irq_descs than may be necessary (NR_IRQs vs 32)?
>>
>> With !CONFIG_HAVE_DYN_ARRAY however, a static array of size NR_IRQS is
>> allocated instead - so doesn't defaulting nr_irqs back to NR_IRQS just
>> revert to the old behaviour (with the exception of the irq_descs being
>> allocated in pre_alloc_dyn_array instead)?
>>
>> - Alex
>>
>
> Sorry to pester you Yinghai, but I'd like to get a patch for this out one
> way or another as Xen is _completely_ unusable with 5 or more VCPUs.
>
> Can you explain more clearly what the problem with the patch is?

small real system doesn't have MSI ioapic will have nr_irqs == 32.
your patch will increase that to 224 again.

sth like ?

#ifdef CONFIG_XEN
int __init probe_nr_irqs(void)
{
        int idx;
        int nr = 0;

        for (idx = 0; idx < nr_ioapics; idx++)
                nr += io_apic_get_redir_entries(idx);

        /* double it for hotplug and msi and nmi */
        nr <<= 1;

        /* something wrong ? */
        if (nr < 32)
                nr = 32;

        return nr;
}
#else
int __init probe_nr_irqs(void)
{
       return NR_IRQS;
}
#endif

Re: [PATCH] X86: Change the default value of nr_irqs from 32 to NR_IRQs

[Jeremy Fitzhardinge]

Yinghai Lu wrote:
> On Wed, Aug 20, 2008 at 4:23 PM, Alex Nixon <alex.nixon@citrix.com> wrote:
>   
>> Alex Nixon wrote:
>>     
>>> Yinghai Lu wrote:
>>>       
>>>> when !CONFIG_HAVE_SPARSE_IRQ, with dyn_array, could allocate irq_desc
>>>> and etc as less as possible.
>>>> when CONFIG_HAVE_SPARESE_IRQ, no actually meaning for nr_irqs.
>>>>
>>>> YH
>>>>         
>>> So I believe the only case this affects is !CONFIG_HAVE_SPARSE_IRQ
>>>
>>> The worry is that with CONFIG_HAVE_DYN_ARRAY we may waste memory by
>>> pre-allocating more irq_descs than may be necessary (NR_IRQs vs 32)?
>>>
>>> With !CONFIG_HAVE_DYN_ARRAY however, a static array of size NR_IRQS is
>>> allocated instead - so doesn't defaulting nr_irqs back to NR_IRQS just
>>> revert to the old behaviour (with the exception of the irq_descs being
>>> allocated in pre_alloc_dyn_array instead)?
>>>
>>> - Alex
>>>
>>>       
>> Sorry to pester you Yinghai, but I'd like to get a patch for this out one
>> way or another as Xen is _completely_ unusable with 5 or more VCPUs.
>>
>> Can you explain more clearly what the problem with the patch is?
>>     
>
> small real system doesn't have MSI ioapic will have nr_irqs == 32.
> your patch will increase that to 224 again.
>
> sth like ?
>   

No.  A Xen-capable kernel can also run native, so it must do whatever a
normal kernel would do when booting native.

At what point in the boot does nr_irqs need to be set?  Could we just
override it at some point?

> #ifdef CONFIG_XEN
>   

I assume you mean ifndef here?

> int __init probe_nr_irqs(void)
> {
>         int idx;
>         int nr = 0;
>
>         for (idx = 0; idx < nr_ioapics; idx++)
>                 nr += io_apic_get_redir_entries(idx);
>
>         /* double it for hotplug and msi and nmi */
>         nr <<= 1;
>
>         /* something wrong ? */
>         if (nr < 32)
>                 nr = 32;
>
>         return nr;
> }
> #else
> int __init probe_nr_irqs(void)
> {
>        return NR_IRQS;
> }
> #endif
>   

    J

Re: [PATCH] X86: Change the default value of nr_irqs from 32 to NR_IRQs

[Jeremy Fitzhardinge]

Alex Nixon (Intern) wrote:
> I'm not sure about the general case, but Xen does not (Jeremy correct
> me if I'm wrong).
>

No, no apics of any kind in the PV domU case.

> Unless I'm missing something (which I may well be; I'm new to this
> area of code), it seems more logical anyway to default back to the
> calculated system-specific value (NR_IRQS), instead of 32, which seems
> rather arbitrary.
>

I think the patch looks fine for now.  But now that we seem to have
unified the use of vectors (I haven't looked at the current code closely
yet), I'd like to move Xen to mapping event channels -> vectors rather
than IRQs, and have the per-cpu event channels share an irq.  That will
dramatically decrease the irq requirements for a PV domU (to 6 irqs +
devices, rather than 6/cpu + devices; a typical domain may only need 10
irqs overall).

    J