Re: [Qemu-devel] [PATCH 5/9] exec: protect mru_block with RCU

[Fam Zheng <famz@redhat.com>]

On Tue, 02/03 13:52, Paolo Bonzini wrote:
> Hence, freeing a RAMBlock has to be switched to call_rcu.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  exec.c                 | 60 +++++++++++++++++++++++++++++++++++---------------
>  include/exec/cpu-all.h |  2 ++
>  2 files changed, 44 insertions(+), 18 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index a423def..05c5b44 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -811,7 +811,7 @@ static RAMBlock *qemu_get_ram_block(ram_addr_t addr)
>      RAMBlock *block;
>  
>      /* The list is protected by the iothread lock here.  */
> -    block = ram_list.mru_block;
> +    block = atomic_rcu_read(&ram_list.mru_block);
>      if (block && addr - block->offset < block->max_length) {
>          goto found;
>      }
> @@ -825,6 +825,22 @@ static RAMBlock *qemu_get_ram_block(ram_addr_t addr)
>      abort();
>  
>  found:
> +    /* It is safe to write mru_block outside the iothread lock.  This
> +     * is what happens:
> +     *
> +     *     mru_block = xxx
> +     *     rcu_read_unlock()
> +     *                                        xxx removed from list
> +     *                  rcu_read_lock()
> +     *                  read mru_block
> +     *                                        mru_block = NULL;
> +     *                                        call_rcu(reclaim_ramblock, xxx);
> +     *                  rcu_read_unlock()
> +     *
> +     * atomic_rcu_set is not needed here.  The block was already published
> +     * when it was placed into the list.  Here we're just making an extra
> +     * copy of the pointer.
> +     */
>      ram_list.mru_block = block;
>      return block;
>  }
> @@ -1381,14 +1397,16 @@ static ram_addr_t ram_block_add(RAMBlock *new_block, Error **errp)
>          QTAILQ_INSERT_TAIL(&ram_list.blocks, new_block, next);
>      }
>      ram_list.mru_block = NULL;
> +    atomic_rcu_set(&ram_list.version, ram_list.version + 1);
>  
> -    ram_list.version++;

Why is this not atomic_inc (or why is atomic_rcu_set necessary here)?

Fam

>      qemu_mutex_unlock_ramlist();
>  
>      new_ram_size = last_ram_offset() >> TARGET_PAGE_BITS;
>  
>      if (new_ram_size > old_ram_size) {
>          int i;
> +
> +        /* ram_list.dirty_memory[] is protected by the iothread lock.  */
>          for (i = 0; i < DIRTY_MEMORY_NUM; i++) {
>              ram_list.dirty_memory[i] =
>                  bitmap_zero_extend(ram_list.dirty_memory[i],
> @@ -1525,14 +1543,32 @@ void qemu_ram_free_from_ptr(ram_addr_t addr)
>          if (addr == block->offset) {
>              QTAILQ_REMOVE(&ram_list.blocks, block, next);
>              ram_list.mru_block = NULL;
> -            ram_list.version++;
> -            g_free(block);
> +            atomic_rcu_set(&ram_list.version, ram_list.version + 1);
> +            call_rcu(block, (void (*)(struct RAMBlock *))g_free, rcu);
>              break;
>          }
>      }
>      qemu_mutex_unlock_ramlist();
>  }
>  
> +static void reclaim_ramblock(RAMBlock *block)
> +{
> +    if (block->flags & RAM_PREALLOC) {
> +        ;
> +    } else if (xen_enabled()) {
> +        xen_invalidate_map_cache_entry(block->host);
> +#ifndef _WIN32
> +    } else if (block->fd >= 0) {
> +        munmap(block->host, block->max_length);
> +        close(block->fd);
> +#endif
> +    } else {
> +        qemu_anon_ram_free(block->host, block->max_length);
> +    }
> +    g_free(block);
> +}
> +
> +/* Called with the iothread lock held */
>  void qemu_ram_free(ram_addr_t addr)
>  {
>      RAMBlock *block;
> @@ -1543,20 +1579,8 @@ void qemu_ram_free(ram_addr_t addr)
>          if (addr == block->offset) {
>              QTAILQ_REMOVE(&ram_list.blocks, block, next);
>              ram_list.mru_block = NULL;
> -            ram_list.version++;
> -            if (block->flags & RAM_PREALLOC) {
> -                ;
> -            } else if (xen_enabled()) {
> -                xen_invalidate_map_cache_entry(block->host);
> -#ifndef _WIN32
> -            } else if (block->fd >= 0) {
> -                munmap(block->host, block->max_length);
> -                close(block->fd);
> -#endif
> -            } else {
> -                qemu_anon_ram_free(block->host, block->max_length);
> -            }
> -            g_free(block);
> +            atomic_rcu_set(&ram_list.version, ram_list.version + 1);
> +            call_rcu(block, reclaim_ramblock, rcu);
>              break;
>          }
>      }
> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
> index 2c48286..b8781d1 100644
> --- a/include/exec/cpu-all.h
> +++ b/include/exec/cpu-all.h
> @@ -24,6 +24,7 @@
>  #include "exec/memory.h"
>  #include "qemu/thread.h"
>  #include "qom/cpu.h"
> +#include "qemu/rcu.h"
>  
>  /* some important defines:
>   *
> @@ -268,6 +269,7 @@ CPUArchState *cpu_copy(CPUArchState *env);
>  typedef struct RAMBlock RAMBlock;
>  
>  struct RAMBlock {
> +    struct rcu_head rcu;
>      struct MemoryRegion *mr;
>      uint8_t *host;
>      ram_addr_t offset;
> -- 
> 1.8.3.1
> 
>