On Wed, Aug 12, 2015 at 18:41:00 +0200, Paolo Bonzini wrote:
> page_find is reading the radix tree outside all locks, so it has to
> use the RCU primitives. It does not need RCU critical sections
> because the PageDescs are never removed, so there is never a need
> to wait for the end of code sections that use a PageDesc.
Note that rcu_find_alloc might end up writing to the tree, see below.
BTW the fact that there are no removals makes the use of RCU unnecessary.
> Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
> ---
> translate-all.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/translate-all.c b/translate-all.c
> index 7727091..78a787d 100644
> --- a/translate-all.c
> +++ b/translate-all.c
> @@ -437,14 +437,14 @@ static PageDesc *page_find_alloc(tb_page_addr_t index,
> int alloc)
>
> /* Level 2..N-1. */
> for (i = V_L1_SHIFT / V_L2_BITS - 1; i > 0; i--) {
> - void **p = *lp;
> + void **p = atomic_rcu_read(lp);
>
> if (p == NULL) {
> if (!alloc) {
> return NULL;
> }
> p = g_new0(void *, V_L2_SIZE);
> - *lp = p;
> + atomic_rcu_set(lp, p);
> }
>
> lp = p + ((index >> (i * V_L2_BITS)) & (V_L2_SIZE - 1));
> @@ -456,7 +456,7 @@ static PageDesc *page_find_alloc(tb_page_addr_t index,
> int alloc)
> return NULL;
> }
> pd = g_new0(PageDesc, V_L2_SIZE);
> - *lp = pd;
> + atomic_rcu_set(lp, pd);
rcu_set is not enough; a cmpxchg with a fail path would be needed here, since
if the find_alloc is called without any locks held (as described in the commit
message)
several threads could concurrently write to the same node, corrupting the tree.
I argue however that it is better to call page_find/_alloc with a mutex held,
since otherwise we'd have to add per-PageDesc locks (it's very common to
call page_find and then update the PageDesc). I have an RFC patchset for
multithreaded tcg
that deals with this, will submit once I bring it up to date with upstream.
Emilio
