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

PATCH In review Alex Nixon 3 participants 12 messages Aug 19, 2008 – Aug 21, 2008 1 message not in the archive x86

Share & tools

Alex Nixon

author

2008-08-19 16:55 UTC #0 lore raw

PATCH 1/1 X86: Change the default value of nr_irqs from 32 to NR_IRQs

From: Alex Nixon <[email protected]>
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 <[email protected]>

---
 arch/x86/kernel/io_apic.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

arch/x86/kernel/io_apic.c +1 -1

index 30ff794..11bffa1 100644
@@ -3632,7 +3632,7 @@ int __init probe_nr_irqs(void)
 
 	/* something wrong ? */
 	if (nr < 32)
-		nr = 32;
+		nr = NR_IRQS;
 
 	return nr;
 }

Signature · 1 line

1.5.4.3

Yinghai Lu

2008-08-19 17:44 UTC #1 lore raw

Quoted 16 lines · Alex Nixon

From: Alex Nixon <[email protected]>
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 <[email protected]>
---
 arch/x86/kernel/io_apic.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
arch/x86/kernel/io_apic.c +0 -0
index 30ff794..11bffa1 100644

arch/x86/kernel/io_apic.c int __init probe_nr_irqs(void)

Quoted 9 lines

arch/x86/kernel/io_apic.c +1 -2

@@ -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 ?

Yinghai Lu

2008-08-19 18:25 UTC #3 lore raw

In reply to a message not in the archive

Quoted 13 lines · Alex Nixon (Intern)

Quoted 7 lines · Yinghai Lu
Quoted 6 lines · Alex Nixon
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).
Quoted 5 lines
if only one ioapic, nr will be 24<<1, you will get 48. Does pv has io apic ?
YH

Quoted 7 lines

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 ?

Yinghai Lu

2008-08-19 19:00 UTC #5 lore raw

In reply to Alex Nixon

Quoted 31 lines · Alex Nixon

Quoted 30 lines · Yinghai Lu
Quoted 26 lines · Alex Nixon (Intern)
Quoted 9 lines · Yinghai Lu
Quoted 6 lines · Alex Nixon
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).
Quoted 6 lines
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.

Quoted 6 lines

Quoted 3 lines
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

Alex Nixon

author

2008-08-19 19:01 UTC #4 lore raw

In reply to Yinghai Lu

Quoted 17 lines · Yinghai Lu

Quoted 15 lines · Alex Nixon (Intern)
Quoted 11 lines · Yinghai Lu
Quoted 5 lines · Alex Nixon
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).

Quoted 8 lines

Quoted 4 lines
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

Alex Nixon

author

2008-08-19 19:51 UTC #6 lore raw

In reply to Yinghai Lu

Quoted 29 lines · Yinghai Lu

Quoted 28 lines · Alex Nixon
Quoted 27 lines · Yinghai Lu
Quoted 18 lines · Alex Nixon (Intern)
Quoted 7 lines · Yinghai Lu
Quoted 5 lines · Alex Nixon
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).
Quoted 6 lines
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).
Quoted 5 lines
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 ?

Quoted 8 lines

Quoted 3 lines
Quoted 1 line
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

Yinghai Lu

2008-08-19 20:52 UTC #7 lore raw

Quoted 50 lines · Alex Nixon

Quoted 49 lines · Yinghai Lu
Quoted 36 lines · Alex Nixon
Quoted 34 lines · Yinghai Lu
Quoted 30 lines · Alex Nixon (Intern)
Quoted 12 lines · Yinghai Lu
Quoted 9 lines · Alex Nixon
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).
Quoted 6 lines
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.
Quoted 6 lines
Quoted 3 lines
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

Quoted 7 lines

Quoted 1 line
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.

Alex Nixon

author

2008-08-19 23:20 UTC #8 lore raw

Quoted 5 lines · Yinghai Lu

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

Jeremy Fitzhardinge

2008-08-20 23:23 UTC #12 lore raw

In reply to a message not in the archive

Quoted 3 lines · Alex Nixon (Intern)

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.

Quoted 5 lines

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).

Alex Nixon

author

2008-08-20 23:24 UTC #9 lore raw

In reply to Alex Nixon

Quoted 7 lines · Alex Nixon

Quoted 5 lines · Yinghai Lu
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

Quoted 8 lines

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

Yinghai Lu

2008-08-20 23:47 UTC #10 lore raw

Quoted 20 lines · Alex Nixon

Quoted 19 lines · Alex Nixon
Quoted 6 lines · Yinghai Lu
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)?

Quoted 7 lines

Quoted 2 lines
- 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

Jeremy Fitzhardinge

2008-08-21 23:22 UTC #11 lore raw

Quoted 28 lines · Yinghai Lu

Quoted 26 lines · Alex Nixon
Quoted 13 lines · Alex Nixon
Quoted 6 lines · Yinghai Lu
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)?
Quoted 8 lines
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.

Quoted 8 lines

Quoted 2 lines
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?

Quoted 2 lines

#ifdef CONFIG_XEN

I assume you mean ifndef here?

Quoted 22 lines

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;
}

Quoted 2 lines

#endif