Re: [PATCH] powerpc - Initialize the irq radix tree earlier

[Sebastien Dugue]

  Hi Michael,

On Thu, 31 Jul 2008 21:40:56 +1000 Michael Ellerman <michael@xxxxxxxxxxxxxx> wrote:

> On Thu, 2008-07-31 at 11:40 +0200, Sebastien Dugue wrote:
> > The radix tree used for fast irq reverse mapping by the XICS is initialized
> > late in the boot process, after the first interrupt (IPI) gets registered
> > and after the first IPI is received.
> > 
> >   This patch moves the initialization of the XICS radix tree earlier into
> > the boot process in smp_xics_probe() (the mm is already up but no interrupts
> > have been registered at that point) to avoid having to insert a mapping into
> > the tree in interrupt context. This will help in simplifying the locking
> > constraints and move to a lockless radix tree in subsequent patches.
> > 
> >   As a nice side effect, there is no need any longer to check for
> > (host->revmap_data.tree.gfp_mask != 0) to know if the tree have been
> > initialized.
> 
> Hi Sebastien,
> 
> This is a nice cleanup, I think :)

  Thanks.

> 
> > diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
> > index 6ac8612..0a1445c 100644
> > --- a/arch/powerpc/kernel/irq.c
> > +++ b/arch/powerpc/kernel/irq.c
> > @@ -893,28 +890,28 @@ unsigned int irq_find_mapping(struct irq_host *host,
> >  }
> >  EXPORT_SYMBOL_GPL(irq_find_mapping);
> >  
> > +void __init irq_radix_revmap_init(void)
> > +{
> > + 	struct irq_host *h;
> > +
> > +	list_for_each_entry(h, &irq_hosts, link) {
> > +		if (h->revmap_type == IRQ_HOST_MAP_TREE)
> > +			INIT_RADIX_TREE(&h->revmap_data.tree, GFP_ATOMIC);
> > +	}
> > +}
> 
> Note irq_radix_revmap_init() loops over all irq_hosts ...

  Yep, but there's only one host (xics)

> 
> > diff --git a/arch/powerpc/platforms/pseries/smp.c b/arch/powerpc/platforms/pseries/smp.c
> > index 9d8f8c8..b143fe7 100644
> > --- a/arch/powerpc/platforms/pseries/smp.c
> > +++ b/arch/powerpc/platforms/pseries/smp.c
> > @@ -130,6 +130,7 @@ static void smp_xics_message_pass(int target, int msg)
> >  
> >  static int __init smp_xics_probe(void)
> >  {
> > +	irq_radix_revmap_init();
> >  	xics_request_IPIs();
> 
> But now it's only called from the xics setup code.
> 
> Which seems a bit ugly. In practice it doesn't matter because at the
> moment xics is the only user of the radix revmap. But if we're going to
> switch to this sort of initialisation I think xics should only be
> init'ing the revmap for itself.

  You're right, that's what I intended to do from the beginning but
stumbled upon ... hmm, can't remember what, that made me change
my mind. But I agree, I'm not particularly proud of that. Will look
again into that.

> 
> 
> This boot ordering stuff is pretty hairy, so I might have missed
> something, but this is how the code is ordered AFAICT:
> ï
> start_kernel()
> 	init_IRQ()
> 	...
> 	local_irq_enable()
> 	...
> 	rest_init()
> 		kernel_thread()
> 			kernel_init()
> 				smp_prepare_cpus()
> 					smp_xics_probe()	(via smp_ops->probe())
> 
> 
> What's stopping us from taking an irq between local_irq_enable() and
> smp_xics_probe() ?  Is it just that no one's request_irq()'ed them yet?

  It's hairy, I agree, but as you've mentioned no one has done a request_irq()
at that point. The first one to do it is smp_xics_probe() for the IPI.

  Thanks for your comments.

  Sebastien.
--
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/