On Tue, Aug 19, 2008 at 11:13 AM, Alex Nixon (Intern)
<[email protected]> wrote:
> Yinghai Lu <[email protected]> wrote:
>>On Tue, Aug 19, 2008 at 9:55 AM, Alex Nixon <[email protected]> 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
On Tue, Aug 19, 2008 at 11:32 AM, Alex Nixon <[email protected]> wrote:
> Yinghai Lu wrote:
>>
>> On Tue, Aug 19, 2008 at 11:13 AM, Alex Nixon (Intern)
>> <[email protected]> wrote:
>>>
>>> Yinghai Lu <[email protected]> wrote:
>>>>
>>>> On Tue, Aug 19, 2008 at 9:55 AM, Alex Nixon <[email protected]>
>>>> 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
Yinghai Lu wrote:
> On Tue, Aug 19, 2008 at 11:13 AM, Alex Nixon (Intern)
> <[email protected]> wrote:
>> Yinghai Lu <[email protected]> wrote:
>>> On Tue, Aug 19, 2008 at 9:55 AM, Alex Nixon <[email protected]> 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
Yinghai Lu wrote:
> On Tue, Aug 19, 2008 at 11:32 AM, Alex Nixon <[email protected]> wrote:
>> Yinghai Lu wrote:
>>> On Tue, Aug 19, 2008 at 11:13 AM, Alex Nixon (Intern)
>>> <[email protected]> wrote:
>>>> Yinghai Lu <[email protected]> wrote:
>>>>> On Tue, Aug 19, 2008 at 9:55 AM, Alex Nixon <[email protected]>
>>>>> 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
On Tue, Aug 19, 2008 at 12:50 PM, Alex Nixon <[email protected]> wrote:
> Yinghai Lu wrote:
>>
>> On Tue, Aug 19, 2008 at 11:32 AM, Alex Nixon <[email protected]>
>> wrote:
>>>
>>> Yinghai Lu wrote:
>>>>
>>>> On Tue, Aug 19, 2008 at 11:13 AM, Alex Nixon (Intern)
>>>> <[email protected]> wrote:
>>>>>
>>>>> Yinghai Lu <[email protected]> wrote:
>>>>>>
>>>>>> On Tue, Aug 19, 2008 at 9:55 AM, Alex Nixon <[email protected]>
>>>>>> 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
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
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
On Wed, Aug 20, 2008 at 4:23 PM, Alex Nixon <[email protected]> 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
Yinghai Lu wrote:
> On Wed, Aug 20, 2008 at 4:23 PM, Alex Nixon <[email protected]> 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