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

[Sebastien Dugue <sebastien.dugue@bull.net>]

  Hi Michael,

On Thu, 31 Jul 2008 21:40:56 +1000 Michael Ellerman <michael@ellerman.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 initial=
ized
> > late in the boot process, after the first interrupt (IPI) gets register=
ed
> > and after the first IPI is received.
> >=20
> >   This patch moves the initialization of the XICS radix tree earlier in=
to
> > the boot process in smp_xics_probe() (the mm is already up but no inter=
rupts
> > 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.
> >=20
> >   As a nice side effect, there is no need any longer to check for
> > (host->revmap_data.tree.gfp_mask !=3D 0) to know if the tree have been
> > initialized.
>=20
> Hi Sebastien,
>=20
> This is a nice cleanup, I think :)

  Thanks.

>=20
> > 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 *ho=
st,
> >  }
> >  EXPORT_SYMBOL_GPL(irq_find_mapping);
> > =20
> > +void __init irq_radix_revmap_init(void)
> > +{
> > + 	struct irq_host *h;
> > +
> > +	list_for_each_entry(h, &irq_hosts, link) {
> > +		if (h->revmap_type =3D=3D IRQ_HOST_MAP_TREE)
> > +			INIT_RADIX_TREE(&h->revmap_data.tree, GFP_ATOMIC);
> > +	}
> > +}
>=20
> Note irq_radix_revmap_init() loops over all irq_hosts ...

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

>=20
> > diff --git a/arch/powerpc/platforms/pseries/smp.c b/arch/powerpc/platfo=
rms/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 m=
sg)
> > =20
> >  static int __init smp_xics_probe(void)
> >  {
> > +	irq_radix_revmap_init();
> >  	xics_request_IPIs();
>=20
> But now it's only called from the xics setup code.
>=20
> 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.

>=20
>=20
> This boot ordering stuff is pretty hairy, so I might have missed
> something, but this is how the code is ordered AFAICT:
> =EF=BB=BF
> start_kernel()
> 	init_IRQ()
> 	...
> 	local_irq_enable()
> 	...
> 	rest_init()
> 		kernel_thread()
> 			kernel_init()
> 				smp_prepare_cpus()
> 					smp_xics_probe()	(via smp_ops->probe())
>=20
>=20
> 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_ir=
q()
at that point. The first one to do it is smp_xics_probe() for the IPI.

  Thanks for your comments.

  Sebastien.