From: Sebastien Dugue <sebastien.dugue@bull•net>
To: benh@kernel•crashing.org
Cc: tinytim@us•ibm.com, linux-rt-users@vger•kernel.org,
linux-kernel@vger•kernel.org, rostedt@goodmis•org,
jean-pierre.dion@bull•net, linuxppc-dev@ozlabs•org,
paulus@samba•org, gilles.carry@ext•bull.net, tglx@linutronix•de
Subject: Re: [PATCH 1/3] powerpc - Initialize the irq radix tree earlier
Date: Tue, 5 Aug 2008 10:26:36 +0200 [thread overview]
Message-ID: <20080805102636.74d7ab12@bull.net> (raw)
In-Reply-To: <1217898226.24157.120.camel@pasglop>
Hi Benjamin,
On Tue, 05 Aug 2008 11:03:46 +1000 Benjamin Herrenschmidt <benh@kernel•crashing.org> 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 <sebastien.dugue@bull•net>
> > Cc: Paul Mackerras <paulus@samba•org>
> > Cc: Benjamin Herrenschmidt <benh@kernel•crashing.org>
> > Cc: Michael Ellerman <michael@ellerman•id.au>
> > ---
> > 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);
> >
>
>
next prev parent reply other threads:[~2008-08-05 9:31 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2008-08-04 11:08 [PATCH 0/3 V2] powerpc - Make the irq reverse mapping tree lockless Sebastien Dugue
2008-08-04 11:08 ` [PATCH 1/3] powerpc - Initialize the irq radix tree earlier Sebastien Dugue
2008-08-05 1:03 ` Benjamin Herrenschmidt
2008-08-05 1:05 ` Benjamin Herrenschmidt
2008-08-05 8:27 ` Sebastien Dugue
2008-08-05 8:26 ` Sebastien Dugue [this message]
2008-08-04 11:08 ` [PATCH 2/3] powerpc - Separate the irq radix tree insertion and lookup Sebastien Dugue
2008-08-04 11:08 ` [PATCH 3/3] powerpc - Make the irq reverse mapping radix tree lockless Sebastien Dugue
2008-08-04 16:31 ` Daniel Walker
2008-08-05 8:28 ` Sebastien Dugue
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20080805102636.74d7ab12@bull.net \
--to=sebastien.dugue@bull$(echo .)net \
--cc=benh@kernel$(echo .)crashing.org \
--cc=gilles.carry@ext$(echo .)bull.net \
--cc=jean-pierre.dion@bull$(echo .)net \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=linux-rt-users@vger$(echo .)kernel.org \
--cc=linuxppc-dev@ozlabs$(echo .)org \
--cc=paulus@samba$(echo .)org \
--cc=rostedt@goodmis$(echo .)org \
--cc=tglx@linutronix$(echo .)de \
--cc=tinytim@us$(echo .)ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox