Re: [PATCH 1/2] irqdomain: Fix up linear revmap for non-zero hwirq displacement.

[Grant Likely <grant.likely@secretlab.ca>]

On Thu, 21 Jun 2012 10:19:40 +0900, Paul Mundt <lethal@linux-sh.org> wrote:
> On Sat, Jun 16, 2012 at 08:14:19AM +0900, Paul Mundt wrote:
> > On Fri, Jun 15, 2012 at 12:25:40PM -0600, Grant Likely wrote:
> > > On Wed, 13 Jun 2012 16:34:00 +0900, Paul Mundt <lethal@linux-sh.org> wrote:
> > > > Presently the linear revmap code assumes that all hwirqs start at 0,
> > > > using the hwirq directly as an index value for the lookup. In the case of
> > > > legacy revmaps this isn't necessarily the case, as the first_hwirq value
> > > > passed in can be non-zero, causing those types of users to silently have
> > > > their IRQs placed in the radix tree instead.
> > > > 
> > > > With this change, hwirq displacement is factored in at association time
> > > > directly. This also makes it possible for non-legacy users to use linear
> > > > revmaps regardless of hwirq base position. This could potentially lead to
> > > > a bug if there's an attempt to associate multiple times in to the linear
> > > > map in a nonsensical and non-linear order, but at that point being
> > > > silently punted to the radix tree is likely to be the least of your
> > > > concerns (in such a case it's fairly trivial to simply extend
> > > > irq_domain_add_linear() to take a hwirq base and move the linear base
> > > > assignment there).
> > > 
> > > I actually hoped to be rid of the whole hwirq start offset thing.
> > > Doing without it simplifies the code, is slightly faster.  I suspect
> > > very few controllers actually need it, and for those that do I'm
> > > hoping the wasted space is in the order of 0-32 words.
> > > 
> > > Instead of this, can we change the affected controllers to use the
> > > maximum hwirq number when setting the size of the linear map?
> > > 
> > > Do you have hardware where the first hwirq is a >32 number?
> > > 
> > Yes. On the CPU I was just working on I have two linear ranges and a
> > tree, one of the linear ranges begins at hwirq 56. On other CPUs we have
> > linear ranges that begin at 64, 72, etc. most of which are fairly low in
> > the space. On newer parts on the ARM side there are also controllers with
> > ranges that begin > 400.
> > 
> > I don't particularly care for the linear_start hack myself either, but I
> > couldn't think of any cleaner approach for it. The simplest might be if
> > we can just bury these details in a domain-specific canonicalization op
> > (distinctly different from xlate), and plug it in for the few cases that
> > need a non-zero hwirq base. I don't mind hacking that up if you're more
> > agreeable to that approach.
> 
> Ping?
> 
> We can't do away with the first_irq thing in the legacy->linear merge
> without at least having a strategy for getting existing users off of it.
> Requiring the linear revmap to always begin at 0 seems like a significant
> regression in functionality for marginal performance gain, so if you're
> not willing to have the linear_start factored in we do need some other
> alternative. I've proposed several, if you don't like any of those you
> are welcome to propose an alternative. I don't mind doing the work one
> way or the other, but I do mind losing the functionality.

From another perspective, even if irqs do start at 400, that is wasted
space of 1600 bytes.  Less than half a page.  It still isn't a huge
amount.  The choice to use a linear vs. radix map is a choice between
speed and sparse flexability.  Considering that one of Ben's concerns is
preserving the fastest lookup path possible, I greatly prefer the
simplicity of a single offset between hwirq and irq.  If you want to
avoid it in your driver, I won't object, but it seems to me a case of
premature optimization.

However, I do agree that allocating 400 unused irq_descs would be a
problem, but the patches don't work that way.  irq_domain_add_legacy()
only calls irq_domain_associate_many() on the requested range of hwirqs
by using the value of hwirq_base passed in.

> Punishing legacy users with leading gaps in their revmap is likewise
> undesirable, especially as that most of the in-tree users (regmap-irq
> especially) of this functionality are using this legacy behaviour without
> incident.

Hardly punishment.  It is a different optimization decision.  The vast
majority of _add_legacy calls use 0 for hwirq_base, and the ones that
don't use a very small number.

...ummm are we talking about the same things?  You mention regmap-irq
specifically, but regmap irq also uses 0 for hwirq_start, so there is no
leading gap.

g.


-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.