From: "Paul E. McKenney" <paulmck@linux•vnet.ibm.com>
To: Milton Miller <miltonm@bga•com>
Cc: linuxppc-dev@lists•ozlabs.org
Subject: Re: [PATCH 7/8] powerpc irq: protect irq_radix_revmap_lookup against irq_free_virt
Date: Wed, 25 May 2011 14:31:12 -0700 [thread overview]
Message-ID: <20110525213112.GL2341@linux.vnet.ibm.com> (raw)
In-Reply-To: <rcu-lock-radix-lookup-ppc@mdm.bga.com>
On Wed, May 25, 2011 at 01:34:18AM -0500, Milton Miller wrote:
> The radix-tree code uses call_rcu when freeing internal elements.
> We must protect against the elements being freed while we traverse
> the tree, even if the returned pointer will still be valid.
>
> While preparing a patch to expand the context in which
> irq_radix_revmap_lookup will be called, I realized that the
> radix tree was not locked.
>
> When asked
>
> For a normal call_rcu usage, is it allowed to read the structure in
> irq_enter / irq_exit, without additional rcu_read_lock? Could an
> element freed with call_rcu advance with the cpu still between
> irq_enter/irq_exit (and irq_disabled())?
>
> Paul McKenney replied:
>
> Absolutely illegal to do so. OK for call_rcu_sched(), but a
> flaming bug for call_rcu().
>
> And thank you very much for finding this!!!
>
> Further analysis:
>
> In the current CONFIG_TREE_RCU implementation. CONFIG_TREE_PREEMPT_RCU
> (and CONFIG_TINY_PREEMPT_RCU) uses explicit counters.
>
> These counters are reflected from per-CPU to global in the
> scheduling-clock-interrupt handler, so disabling irq does prevent the
> grace period from completing. But there are real-time implementations
> (such as the one use by the Concurrent guys) where disabling irq
> does -not- prevent the grace period from completing.
>
>
> While an alternative fix would be to switch radix-tree to rcu_sched, I
> don't want to audit the other users of radix trees (nor put alternative
> freeing in the library). The normal overhead for rcu_read_lock and
> unlock are a local counter increment and decrement.
>
> This does not show up in the rcu lockdep because in 2.6.34 commit
> 2676a58c98 (radix-tree: Disable RCU lockdep checking in radix tree)
> deemed it too hard to pass the condition of the protecting lock
> to the library.
Reviewed-by: Paul E. McKenney <paulmck@linux•vnet.ibm.com>
> Signed-off-by: Milton Miller <miltonm@bga•com>
>
> Index: work.git/arch/powerpc/kernel/irq.c
> ===================================================================
> --- work.git.orig/arch/powerpc/kernel/irq.c 2011-05-24 21:14:30.860096118 -0500
> +++ work.git/arch/powerpc/kernel/irq.c 2011-05-24 21:15:55.350096024 -0500
> @@ -893,10 +893,13 @@ unsigned int irq_radix_revmap_lookup(str
> return irq_find_mapping(host, hwirq);
>
> /*
> - * No rcu_read_lock(ing) needed, the ptr returned can't go under us
> - * as it's referencing an entry in the static irq_map table.
> + * The ptr returned references the static global irq_map.
> + * but freeing an irq can delete nodes along the path to
> + * do the lookup via call_rcu.
> */
> + rcu_read_lock();
> ptr = radix_tree_lookup(&host->revmap_data.tree, hwirq);
> + rcu_read_unlock();
>
> /*
> * If found in radix tree, then fine.
prev parent reply other threads:[~2011-05-25 21:31 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-05-25 6:34 [PATCH 0/8] ipi and irq cleanups and fixes Milton Miller
2011-05-25 6:34 ` [PATCH 6/8] powerpc 8xx: cascade eoi will be performed by generic_handle_irq handler Milton Miller
2011-05-26 3:32 ` Benjamin Herrenschmidt
2011-05-26 11:19 ` Milton Miller
2011-05-25 6:34 ` [PATCH 4/8] powerpc irq: always free duplicate IRQ_LEGACY hosts Milton Miller
2011-05-25 6:34 ` [PATCH 8/8] powerpc: fix irq_free_virt by adjusting bounds before loop Milton Miller
2011-05-25 6:34 ` [PATCH 3/8] powerpc irq: remove stale and misleading comment Milton Miller
2011-05-25 6:34 ` [PATCH 5/8] powerpc: check desc in handle_one_irq and expand generic_handle_irq Milton Miller
2011-05-25 6:34 ` [PATCH 2/8] powerpc cell: rename ipi functions to match current abstractions Milton Miller
2011-05-25 6:34 ` [PATCH 7/8] powerpc irq: protect irq_radix_revmap_lookup against irq_free_virt Milton Miller
2011-05-25 21:31 ` Paul E. McKenney [this message]
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=20110525213112.GL2341@linux.vnet.ibm.com \
--to=paulmck@linux$(echo .)vnet.ibm.com \
--cc=linuxppc-dev@lists$(echo .)ozlabs.org \
--cc=miltonm@bga$(echo .)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