[5/9] exec: protect mru_block with RCU

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(-)

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
> 
>

On 05/02/2015 07:23, Fam Zheng wrote:
>> > @@ -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

Because writes are protected by the ramlist lock.  atomic_inc is more
expensive.

> (or why is atomic_rcu_set necessary here)?

I probably should move it to patch 9; it is needed to update the list
before ram_list.version.

If you prefer I can change it to

	smp_wmb();
	atomic_set(&ram_list.version, ram_list.version + 1);

?

Paolo

On Thu, 02/05 09:37, Paolo Bonzini wrote:
> 
> 
> On 05/02/2015 07:23, Fam Zheng wrote:
> >> > @@ -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
> 
> Because writes are protected by the ramlist lock.  atomic_inc is more
> expensive.

OK!

> 
> > (or why is atomic_rcu_set necessary here)?
> 
> I probably should move it to patch 9; it is needed to update the list
> before ram_list.version.
> 
> If you prefer I can change it to
> 
> 	smp_wmb();
> 	atomic_set(&ram_list.version, ram_list.version + 1);
> 
> ?
> 

Yes, this looks more obvious :)

Fam