Hi Benjamin,
On Tue, 05 Aug 2008 11:03:46 +1000 Benjamin Herrenschmidt <[EMAIL PROTECTED]>
wrote:
> On Mon, 2008-08-04 at 13:08 +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.
>
> I'm not too happy with the moving of the radix tree init to platform
> code.
>
> The fact that the revmap code uses a radix tree should be mostly
> transparent to the XICS code. I don't like adding this explicit code
> from xics to initialize it.
OK, I'm fine with that.
>
> I think the tree should still be initialized from generic code and it
> can be done as late as we want as interrupts work without the tree being
> there, they are just a bit slower.
>
> I believe the right approach is:
>
> - Remove the populating of the tree from the revmap function as
> you already do
> - Move it to irq_create_mapping() for the normal case
Agreed.
> - For pre-existing interrupt, have the generic code that initializes
> the radix tree walk through all interrupts and setup the revmap for
> them. If that needs locking vs. concurrent irq_create_mapping, it's
> easy to use one of the available spinlocks for that.
That's what I wanted to avoid to not add some new complexity, but I can
see that my approach makes some assumptions about initializations order
that I'm no longer comfortable with. Will do as you suggest as it's
way more explicit.
Thanks a lot for your comments.
Sebastien.
>
> Cheers,
> Ben.
>
>
> > Signed-off-by: Sebastien Dugue <[EMAIL PROTECTED]>
> > Cc: Paul Mackerras <[EMAIL PROTECTED]>
> > Cc: Benjamin Herrenschmidt <[EMAIL PROTECTED]>
> > Cc: Michael Ellerman <[EMAIL PROTECTED]>
> > ---
> > arch/powerpc/kernel/irq.c | 17 -----------------
> > arch/powerpc/platforms/pseries/smp.c | 1 +
> > arch/powerpc/platforms/pseries/xics.c | 5 +++++
> > arch/powerpc/platforms/pseries/xics.h | 1 +
> > 4 files changed, 7 insertions(+), 17 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
> > index d972dec..9bef9f3 100644
> > --- a/arch/powerpc/kernel/irq.c
> > +++ b/arch/powerpc/kernel/irq.c
> > @@ -1016,23 +1016,6 @@ void irq_early_init(void)
> > get_irq_desc(i)->status |= IRQ_NOREQUEST;
> > }
> >
> > -/* We need to create the radix trees late */
> > -static int irq_late_init(void)
> > -{
> > - struct irq_host *h;
> > - unsigned long flags;
> > -
> > - irq_radix_wrlock(&flags);
> > - 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);
> > - }
> > - irq_radix_wrunlock(flags);
> > -
> > - return 0;
> > -}
> > -arch_initcall(irq_late_init);
> > -
> > #ifdef CONFIG_VIRQ_DEBUG
> > static int virq_debug_show(struct seq_file *m, void *private)
> > {
> > diff --git a/arch/powerpc/platforms/pseries/smp.c
> > b/arch/powerpc/platforms/pseries/smp.c
> > index 9d8f8c8..3d4429a 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)
> > {
> > + xics_radix_revmap_init();
> > xics_request_IPIs();
> >
> > return cpus_weight(cpu_possible_map);
> > diff --git a/arch/powerpc/platforms/pseries/xics.c
> > b/arch/powerpc/platforms/pseries/xics.c
> > index 0fc830f..d6e28f9 100644
> > --- a/arch/powerpc/platforms/pseries/xics.c
> > +++ b/arch/powerpc/platforms/pseries/xics.c
> > @@ -556,6 +556,11 @@ static struct irq_host_ops xics_host_ops = {
> > .xlate = xics_host_xlate,
> > };
> >
> > +void __init xics_radix_revmap_init(void)
> > +{
> > + INIT_RADIX_TREE(&xics_host->revmap_data.tree, GFP_ATOMIC);
> > +}
> > +
> > static void __init xics_init_host(void)
> > {
> > if (firmware_has_feature(FW_FEATURE_LPAR))
> > diff --git a/arch/powerpc/platforms/pseries/xics.h
> > b/arch/powerpc/platforms/pseries/xics.h
> > index 1c5321a..11490be 100644
> > --- a/arch/powerpc/platforms/pseries/xics.h
> > +++ b/arch/powerpc/platforms/pseries/xics.h
> > @@ -19,6 +19,7 @@ extern void xics_setup_cpu(void);
> > extern void xics_teardown_cpu(void);
> > extern void xics_kexec_teardown_cpu(int secondary);
> > extern void xics_cause_IPI(int cpu);
> > +extern void xics_radix_revmap_init(void);
> > extern void xics_request_IPIs(void);
> > extern void xics_migrate_irqs_away(void);
> >
>
>
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
