Message-ID: <20080731141029.0a9dd4cf@bull.net>
Date:	Thu, 31 Jul 2008 14:10:29 +0200
From:	Sebastien Dugue <sebastien.dugue@...l.net>
To:	Sebastien Dugue <sebastien.dugue@...l.net>
Cc:	michael@...erman.id.au, tinytim@...ibm.com,
	linux-rt-users@...r.kernel.org, jean-pierre.dion@...l.net,
	rostedt@...dmis.org, linux-kernel@...r.kernel.org,
	linuxppc-dev@...abs.org, paulus@...ba.org,
	gilles.carry@....bull.net, tglx@...utronix.de
Subject: Re: [PATCH] powerpc - Initialize the irq radix tree earlier

On Thu, 31 Jul 2008 14:00:02 +0200 Sebastien Dugue <sebastien.dugue@...l.net> wrote:

> 
>   Hi Michael,
> 
> On Thu, 31 Jul 2008 21:40:56 +1000 Michael Ellerman <michael@...erman.id.au> 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.

  Ah yes, I wanted to do it from xics_init_host() but backed off
because at that point the mm is not up. But it does not make a difference
as the first request_irq() happens after the mm is up. A bit shaky I
concede.

> 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.
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@...abs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists