Re: [patch 00/47] Sparse irq rework

[Thomas Gleixner]

On Sun, 3 Oct 2010, Eric W. Biederman wrote:
> Thomas Gleixner <tglx@xxxxxxxxxxxxx> writes:
> >> I believe you have distored the design when aiming for migration
> >> of active irq descriptors (which you have not even implemented yet).
> >> 
> >> How do you plan to remove the radix tree lookup from the irq
> >> handling path?
> >
> > Not at all and it's not even even a requirement to remove the lookup
> > for implementing live migration.
> 
> It sounds like it is a requirement to *keep* the lookup for supporting
> live migration.  *Keeping* the lookup I see as a serious problem.  If we
> do this right the only users of the radix tree will be drivers using the
> functions in interrupt.h.

Oh well. I said that it's not a requirement to remove the lookup from
the entry code, but we can remove it for optimizaiton reasons.

That is the same problem vs. migration as we have a reference to
irq_desc either by lookup or by storage in vector data on x86.

> >> Those files provide the genirq irq chip implementation especially
> >> drivers/pci/msi.c.  Of course they will do what every other irq_chip
> >> implementation does to get access to data.  There is an unpleasant
> >> difference between which generic irq data field htirq.c uses and msi.c
> >> which may be worth cleaning up.  But otherwise I don't see any
> >> fundamental problems.
> >
> > The fundamental problem I hit, was the hack which handed down irq_desc
> > to avoid the lookup. If it had been msi_desc in the first place, then
> > I would not even need to touch the msi code to cleanup x86.
> 
> Just because you intend to rename the irq_desc irq_data...

No, I'm not renaming it. I'm cleaning up the mess which was created
with references to irq_desc all over the place. I need to change core
code and I cant w/o breaking the world and some more, just because
everyone fiddles in irq_desc directly. So I want to hand down irq_data
which is right now inside of irq_desc until the irq_desc users outside
of kernel/irq are gone. Then irq_data is not necessarily a part of
irq_desc anymore.
 
> It isn't a hack for an irq method to look at irq_desc.  At least not
> until your irq_data changes go through.  This has nothing to do with
> how x86 is structured and everything to do with your irq_data
> ``cleanup'' which appears to be mostly about code churn, for very little
> apparent benefit.

I tend to disagree. The sparse_irq optimizations to get rid of the
redundant irq_desc lookups to gain access to the irq_data should have
been done in exactly that way.

And I call something which removes 500 lines of code hardly useless
code churn.

> In the current state of the kernel I find it very hard to swallow that
> having a genirq client using irq_desc (which is the only way to
> implement somethings) is a hack.

This could have been done 2 years ago by those who pushed sparse_irq
including the resulting "optimization".

Thanks,

	tglx
--
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/