Re: [PATCH] x86_64: resize NR_IRQS for large machines

[Eric W. Biederman]

On Wed, 2008-03-26 at 13:55 -0500, Alan Mayer wrote:
> On Wed, 26 Mar 2008, Linus Torvalds wrote:
> 
> > 
> > 
> > On Wed, 26 Mar 2008, Pavel Machek wrote:
> > > 
> > > This is very ugly. Why not include it unconditionally -- with guard in
> > > apicdef.h?
> > 
> > I do agree that it's ugly, but I think the ugliness is more serious than 
> > that.
> > 
> > What I think we should do is to make NR_IRQS no longer be a compile-time 
> > constant, but instead just do something like
> > 
> > 	unsigned int NR_IRQS __read_mostly;
> > 
> > and then just set it early in the boot sequence depending on the real CPU 
> > numbers etc.
> > 
> > I realize that this will require some changes to a few arrays that are 
> > statically allocated and depend on NR_IRQ's (notably "irq_desc"), but 
> > don't you guys think that this would be a cleaner thing?
> > 
> > [ I suspect that irq_desc[] itself could quite reasonably be a rather much 
> >   smaller __read_mostly hash-table of dynamically allocated entries - the 
> >   thing would be only modified at boot, so it should cache beautifully 
> >   even across hundreds of CPU's ]
> > 
> > Whatever. I'm not opposed to this whole static thing, but I do wonder if 
> > it's worth doing that way. There *may* be performance reasons for doing it 
> > the way we're doing it, but quite frankly, I think the #define is mostly 
> > purely historical, from when it was just a fixed number (originally 16!) 
> > and it made sense to think of it as a small static array.
> > 
> > 			Linus
> > 
> 
> Well, I was looking at it from that point of view.  But, when I found myself
> looking at code, particularly in drivers, that indexed into the irq_desc array
> and started modifying the descriptor in place and then calling setup_irq(),
> I realized that what was needed was a redesign of the whole mess from first
> principals.  I still think that's what needs to be done, but by some one with
> more experience and credibility than me.  Maybe in a year I'd be willing
> to attempt it, but not today.

Well I will agree with Linus and go one farther and say that NR_IRQS
needs to die.  I started on that once and x86 is just about ready to
accomodate it.

There is a size issue on small machines.  And there is very bad NUMA
affinity on large machines.  So the current structure really is not
optimal for anyone.  All of this gets especially bad for distro kernels
that try and support everything.

Also MAX_IOAPICS is very much the wrong factor to be using on large
machines to size the irq array.  New machines are moving towards MSI and
cards can have an unreasonable number of MSI irqs.  In practice the top
end I have seen is 20-30 per card but it is still a lot.    So I think
you may get a nasty surprise when you plug in a bunch of high
performance cards with multiple queues into a big box.  The 32*NR_CPUS
as a rule of thumb comes from IBM boxes that are a little better
balanced when it comes to compute vs. I/O capablility.  

For actual irq reception we have our per cpu array of vectors that point
to the irq_desc so even if the global list of irqs was a linked list we
should not have performance problems.

I need to do some sorting out of sysfs first but I will certainly see if
I can look at this again.  And I will very much be willing to work with
someone else who wants to work on this and has more time then I do at
the moment.

The basic idea is moving the generic irq apis to a point where we can
refer to irqs in the generic code with a struct irq * instead of by
number.  We really only the need the number for talking about irqs to
user space.

Eric


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/