NR_PIRQS vs. NR_IRQS

NR_PIRQS vs. NR_IRQS

[Jan Beulich]

I'm having some difficulty understanding why these two need to be
distinguished: Depending on the code location, an IRQ passed in from the
guest may be checked against NR_PIRQS (map_domain_pirq() as called
from PHYSDEVOP_alloc_irq_vector) or NR_IRQS (PHYSDEVOP_irq_status_query,
PHYSDEVOP_map_pirq), despite it having the same source.

Also, tying NR_IRQS to NR_VECTORS seems bogus - even with current
code I can't see why we shouldn't be able to support a higher NR_IRQS
without immediately doing the more involved code changes needed to
also grow NR_VECTORS. After all, NR_IRQS is directly tied to the number
of IO-APIC pins we can support - in order to support a device, its
cumulative pin number (being the irq) must be below NR_IRQS. But since
very likely not all pins are connected to devices, NR_VECTORS is much
less of a limiting factor.

Thanks, Jan

Re: NR_PIRQS vs. NR_IRQS

[Keir Fraser]

PIRQs are actually a different namespace. They aren't necessarily 1:1
mapped. Hence NR_PIRQS and NR_IRQS not really same thing.

Yes, I'm sure with a bit of finessing we could have NR_IRQS != NR_VECTORS.
I'm sure there'll be some barking NUMA box down the road that will require
something like that, but thankfully not so far.

 -- Keir

On 13/11/08 16:59, "Jan Beulich" <jbeulich@novell.com> wrote:

> I'm having some difficulty understanding why these two need to be
> distinguished: Depending on the code location, an IRQ passed in from the
> guest may be checked against NR_PIRQS (map_domain_pirq() as called
> from PHYSDEVOP_alloc_irq_vector) or NR_IRQS (PHYSDEVOP_irq_status_query,
> PHYSDEVOP_map_pirq), despite it having the same source.
> 
> Also, tying NR_IRQS to NR_VECTORS seems bogus - even with current
> code I can't see why we shouldn't be able to support a higher NR_IRQS
> without immediately doing the more involved code changes needed to
> also grow NR_VECTORS. After all, NR_IRQS is directly tied to the number
> of IO-APIC pins we can support - in order to support a device, its
> cumulative pin number (being the irq) must be below NR_IRQS. But since
> very likely not all pins are connected to devices, NR_VECTORS is much
> less of a limiting factor.
> 
> Thanks, Jan
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: NR_PIRQS vs. NR_IRQS

[Keir Fraser]

On 13/11/08 18:41, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote:

> PIRQs are actually a different namespace. They aren't necessarily 1:1
> mapped. Hence NR_PIRQS and NR_IRQS not really same thing.

However I'll agree that in some cases it's assumed that the namespaces are
the same size (PHYSDEVOP_alloc_irq_vector pretty much does that). And they
*are* being used rather interchangeably already... So yes, actually perhaps
we should kill off NR_PIRQS. It seems not worth cleaning up usage of NR_IRQS
vs NR_PIRQS to make the distinction clean and correct.

> Yes, I'm sure with a bit of finessing we could have NR_IRQS != NR_VECTORS.
> I'm sure there'll be some barking NUMA box down the road that will require
> something like that, but thankfully not so far.

I agree with keeping this naming distinction of course, although I think
allowing NR_IRQS > NR_VECTORS right now is not very useful. But maybe you
have a box in mind that needs it?

 -- Keir

>  -- Keir
> 
> On 13/11/08 16:59, "Jan Beulich" <jbeulich@novell.com> wrote:
> 
>> I'm having some difficulty understanding why these two need to be
>> distinguished: Depending on the code location, an IRQ passed in from the
>> guest may be checked against NR_PIRQS (map_domain_pirq() as called
>> from PHYSDEVOP_alloc_irq_vector) or NR_IRQS (PHYSDEVOP_irq_status_query,
>> PHYSDEVOP_map_pirq), despite it having the same source.
>> 
>> Also, tying NR_IRQS to NR_VECTORS seems bogus - even with current
>> code I can't see why we shouldn't be able to support a higher NR_IRQS
>> without immediately doing the more involved code changes needed to
>> also grow NR_VECTORS. After all, NR_IRQS is directly tied to the number
>> of IO-APIC pins we can support - in order to support a device, its
>> cumulative pin number (being the irq) must be below NR_IRQS. But since
>> very likely not all pins are connected to devices, NR_VECTORS is much
>> less of a limiting factor.
>> 
>> Thanks, Jan
>> 
>> 
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: NR_PIRQS vs. NR_IRQS

[Jan Beulich]

>>> Keir Fraser <keir.fraser@eu.citrix.com> 13.11.08 20:19 >>>
>On 13/11/08 18:41, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote:
>> Yes, I'm sure with a bit of finessing we could have NR_IRQS != NR_VECTORS.
>> I'm sure there'll be some barking NUMA box down the road that will require
>> something like that, but thankfully not so far.
>
>I agree with keeping this naming distinction of course, although I think
>allowing NR_IRQS > NR_VECTORS right now is not very useful. But maybe you
>have a box in mind that needs it?

I had sent a mail a few days ago on this, where IBM was testing 96 CPU
support (4-node system), and it crashing because of a PIRQ ending up in
DYNIRQ space (kernel perspective), because there being 300+ IO-APIC
pins. While the crash ought to be fixed with the subsequent patch, it's
clear that none of the devices with an accumulated pin number greater
than 255 will actually work on that system.

Jan

Re: NR_PIRQS vs. NR_IRQS

[Keir Fraser]

On 14/11/08 07:48, "Jan Beulich" <jbeulich@novell.com> wrote:

>>> Yes, I'm sure with a bit of finessing we could have NR_IRQS != NR_VECTORS.
>>> I'm sure there'll be some barking NUMA box down the road that will require
>>> something like that, but thankfully not so far.
>> 
>> I agree with keeping this naming distinction of course, although I think
>> allowing NR_IRQS > NR_VECTORS right now is not very useful. But maybe you
>> have a box in mind that needs it?
> 
> I had sent a mail a few days ago on this, where IBM was testing 96 CPU
> support (4-node system), and it crashing because of a PIRQ ending up in
> DYNIRQ space (kernel perspective), because there being 300+ IO-APIC
> pins. While the crash ought to be fixed with the subsequent patch, it's
> clear that none of the devices with an accumulated pin number greater
> than 255 will actually work on that system.

Oh dear. :-D

 -- Keir

Re: NR_PIRQS vs. NR_IRQS

[Keir Fraser]

On 14/11/08 07:54, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote:

>>> I agree with keeping this naming distinction of course, although I think
>>> allowing NR_IRQS > NR_VECTORS right now is not very useful. But maybe you
>>> have a box in mind that needs it?
>> 
>> I had sent a mail a few days ago on this, where IBM was testing 96 CPU
>> support (4-node system), and it crashing because of a PIRQ ending up in
>> DYNIRQ space (kernel perspective), because there being 300+ IO-APIC
>> pins. While the crash ought to be fixed with the subsequent patch, it's
>> clear that none of the devices with an accumulated pin number greater
>> than 255 will actually work on that system.
> 
> Oh dear. :-D

Is fixing this actually any harder than just bumping NR_IRQS/NR_PIRQS in Xen
and NR_PIRQS in Linux? Have IRQS and VECTORS got somehow accidentally tied
together in Xen?

These parameters should probably be build-time configurable.

 -- Keir

Re: NR_PIRQS vs. NR_IRQS

[Jan Beulich]

>>> Keir Fraser <keir.fraser@eu.citrix.com> 14.11.08 09:00 >>>
>On 14/11/08 07:54, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote:
>
>>>> I agree with keeping this naming distinction of course, although I think
>>>> allowing NR_IRQS > NR_VECTORS right now is not very useful. But maybe you
>>>> have a box in mind that needs it?
>>> 
>>> I had sent a mail a few days ago on this, where IBM was testing 96 CPU
>>> support (4-node system), and it crashing because of a PIRQ ending up in
>>> DYNIRQ space (kernel perspective), because there being 300+ IO-APIC
>>> pins. While the crash ought to be fixed with the subsequent patch, it's
>>> clear that none of the devices with an accumulated pin number greater
>>> than 255 will actually work on that system.
>> 
>> Oh dear. :-D
>
>Is fixing this actually any harder than just bumping NR_IRQS/NR_PIRQS in Xen
>and NR_PIRQS in Linux? Have IRQS and VECTORS got somehow accidentally tied
>together in Xen?

Perhaps not, but I only started checking (on the Xen side - the kernel side
has no issues, already bumped the value there). I'll continue as time permits.

>These parameters should probably be build-time configurable.

That'd certainly be nice for NR_IRQS (it seems we agreed to get rid of
NR_PIRQS). I can't the same being valid for NR_VECTORS, though.

Jan

Re: NR_PIRQS vs. NR_IRQS

[Keir Fraser]

On 14/11/08 08:19, "Jan Beulich" <jbeulich@novell.com> wrote:

>> Is fixing this actually any harder than just bumping NR_IRQS/NR_PIRQS in Xen
>> and NR_PIRQS in Linux? Have IRQS and VECTORS got somehow accidentally tied
>> together in Xen?
> 
> Perhaps not, but I only started checking (on the Xen side - the kernel side
> has no issues, already bumped the value there). I'll continue as time permits.
> 
>> These parameters should probably be build-time configurable.
> 
> That'd certainly be nice for NR_IRQS (it seems we agreed to get rid of
> NR_PIRQS). I can't the same being valid for NR_VECTORS, though.

I can't really disagree regarding NR_VECTORS!

 -- Keir

Re: NR_PIRQS vs. NR_IRQS

[Jeremy Fitzhardinge]

Keir Fraser wrote:
> Is fixing this actually any harder than just bumping NR_IRQS/NR_PIRQS in Xen
> and NR_PIRQS in Linux? Have IRQS and VECTORS got somehow accidentally tied
> together in Xen?
>   

The pvops dom0 kernel allocates all irqs dynamically without having 
reserved ranges for particular classes of irq, so I think it all comes 
down to whether Xen can handle it.

    J

Re: NR_PIRQS vs. NR_IRQS

[Jan Beulich]

>>> Keir Fraser <keir.fraser@eu.citrix.com> 14.11.08 09:00 >>>
>On 14/11/08 07:54, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote:
>
>>>> I agree with keeping this naming distinction of course, although I think
>>>> allowing NR_IRQS > NR_VECTORS right now is not very useful. But maybe you
>>>> have a box in mind that needs it?
>>> 
>>> I had sent a mail a few days ago on this, where IBM was testing 96 CPU
>>> support (4-node system), and it crashing because of a PIRQ ending up in
>>> DYNIRQ space (kernel perspective), because there being 300+ IO-APIC
>>> pins. While the crash ought to be fixed with the subsequent patch, it's
>>> clear that none of the devices with an accumulated pin number greater
>>> than 255 will actually work on that system.
>> 
>> Oh dear. :-D
>
>Is fixing this actually any harder than just bumping NR_IRQS/NR_PIRQS in Xen
>and NR_PIRQS in Linux? Have IRQS and VECTORS got somehow accidentally tied
>together in Xen?

Unfortunately there was some mix-up. But I think I got this fixed (running
a NR_IRQS=1024 hypervisor at the moment). I'd prefer to submit these
patches only after your NR_PIRQS -> NR_IRQS change comes out of the
staging tree though, to make sure it applies (and works) correctly on top
of it.

>These parameters should probably be build-time configurable.

This too.

Jan

Re: NR_PIRQS vs. NR_IRQS

[Keir Fraser]

On 19/11/08 15:55, "Jan Beulich" <jbeulich@novell.com> wrote:

>> Is fixing this actually any harder than just bumping NR_IRQS/NR_PIRQS in Xen
>> and NR_PIRQS in Linux? Have IRQS and VECTORS got somehow accidentally tied
>> together in Xen?
> 
> Unfortunately there was some mix-up. But I think I got this fixed (running
> a NR_IRQS=1024 hypervisor at the moment). I'd prefer to submit these
> patches only after your NR_PIRQS -> NR_IRQS change comes out of the
> staging tree though, to make sure it applies (and works) correctly on top
> of it.

Okay, I manually synced the tree. I've been meaning to do so for a while.

 -- Keir

Re: NR_PIRQS vs. NR_IRQS

[Jan Beulich]

>>> Keir Fraser <keir.fraser@eu.citrix.com> 19.11.08 17:09 >>>
>On 19/11/08 15:55, "Jan Beulich" <jbeulich@novell.com> wrote:
>
>>> Is fixing this actually any harder than just bumping NR_IRQS/NR_PIRQS in Xen
>>> and NR_PIRQS in Linux? Have IRQS and VECTORS got somehow accidentally tied
>>> together in Xen?
>> 
>> Unfortunately there was some mix-up. But I think I got this fixed (running
>> a NR_IRQS=1024 hypervisor at the moment). I'd prefer to submit these
>> patches only after your NR_PIRQS -> NR_IRQS change comes out of the
>> staging tree though, to make sure it applies (and works) correctly on top
>> of it.
>
>Okay, I manually synced the tree. I've been meaning to do so for a while.

Is there anything in particular that's broken (and thus preventing it to
progress automatically) that one should be aware of?

Jan

Re: NR_PIRQS vs. NR_IRQS

[Keir Fraser]

On 19/11/08 16:23, "Jan Beulich" <jbeulich@novell.com> wrote:

>>> Unfortunately there was some mix-up. But I think I got this fixed (running
>>> a NR_IRQS=1024 hypervisor at the moment). I'd prefer to submit these
>>> patches only after your NR_PIRQS -> NR_IRQS change comes out of the
>>> staging tree though, to make sure it applies (and works) correctly on top
>>> of it.
>> 
>> Okay, I manually synced the tree. I've been meaning to do so for a while.
> 
> Is there anything in particular that's broken (and thus preventing it to
> progress automatically) that one should be aware of?

No, I think some part of the test infrastructure is down.

 -- Keir