Re: [Qemu-devel] [PATCH 2/5] hostmem: make hostmem global and RAM hotunplg safe

[Stefan Hajnoczi]

On Mon, Apr 01, 2013 at 04:20:31PM +0800, Liu Ping Fan wrote:
> From: Liu Ping Fan <address@hidden>
> 
> The hostmem listener will translate gpa to hva quickly. Make it
> global, so other component can use it.
> Also this patch adopt MemoryRegionOps's ref/unref interface to
> make it survive the RAM hotunplug.
> 
> Signed-off-by: Liu Ping Fan <address@hidden>
> ---
>  hw/dataplane/hostmem.c |  130 +++++++++++++++++++++++++++++++++--------------
>  hw/dataplane/hostmem.h |   19 ++------
>  2 files changed, 95 insertions(+), 54 deletions(-)
> 
> diff --git a/hw/dataplane/hostmem.c b/hw/dataplane/hostmem.c
> index 380537e..86c02cd 100644
> --- a/hw/dataplane/hostmem.c
> +++ b/hw/dataplane/hostmem.c
> @@ -13,6 +13,12 @@
>  
>  #include "exec/address-spaces.h"
>  #include "hostmem.h"
> +#include "qemu/main-loop.h"
> +
> +/* lock to protect the access to cur_hostmem */
> +static QemuMutex hostmem_lock;
> +static HostMem *cur_hostmem;
> +static HostMem *next_hostmem;
>  
>  static int hostmem_lookup_cmp(const void *phys_, const void *region_)
>  {
> @@ -28,16 +34,49 @@ static int hostmem_lookup_cmp(const void *phys_, const 
> void *region_)
>      }
>  }
>  
> +static void hostmem_ref(HostMem *hostmem)
> +{
> +    assert(__sync_add_and_fetch(&hostmem->ref, 1) > 0);

man 3 assert:
"If  the macro NDEBUG was defined at the moment <assert.h> was last
included, the macro assert() generates no code, and hence does nothing
at all."

Never rely on a side-effect within an assert() call.  Store the return
value into a local variable and test the local in the assert().

Also, these sync gcc builtins are not available on all platforms or gcc
versions.  We need to be a little careful to avoid breaking builds here.
Maybe __sync_add_and_fetch() is fine but I wanted to mention it because
I've had trouble with these in the past.

I suggest using glib atomics instead, if possible.

> +}
> +
> +void hostmem_unref(HostMem *hostmem)
> +{
> +    int i;
> +    HostMemRegion *hmr;
> +
> +    if (!__sync_sub_and_fetch(&hostmem->ref, 1)) {
> +        for (i = 0; i < hostmem->num_current_regions; i++) {
> +            hmr = &hostmem->current_regions[i];
> +            /* Fix me, when memory hotunplug implement
> +             * assert(hmr->mr_ops->unref)
> +             */

Why this fixme?  Can you resolve it by making ->unref() return bool from
the start of this patch series?

> +            if (hmr->mr->ops && hmr->mr->ops->unref) {
> +                hmr->mr->ops->unref();
> +            }
> +        }
> +        g_free(hostmem->current_regions);
> +        g_free(hostmem);
> +    }
> +}
> +
>  /**
>   * Map guest physical address to host pointer
> + * also inc refcnt of *mr, caller need to unref later
>   */
> -void *hostmem_lookup(HostMem *hostmem, hwaddr phys, hwaddr len, bool 
> is_write)
> +void *hostmem_lookup(hwaddr phys, hwaddr len, MemoryRegion **mr, bool 
> is_write)

A cleaner approach is to keep the hostmem_foo(HostMem *) functions and
have a global interface without the HostMem*.  That way the global
wrapper focusses on acquiring cur_hostmem while the existing functions
stay unchanged and focus on performing the actual operation.

>  {
>      HostMemRegion *region;
>      void *host_addr = NULL;
>      hwaddr offset_within_region;
> +    HostMem *hostmem;
> +
> +    assert(mr);
> +    *mr = NULL;
> +    qemu_mutex_lock(&hostmem_lock);
> +    hostmem = cur_hostmem;
> +    hostmem_ref(hostmem);
> +    qemu_mutex_unlock(&hostmem_lock);
>  
> -    qemu_mutex_lock(&hostmem->current_regions_lock);

Why is it safe to drop this lock?  The memory API could invoke callbacks
in another thread which causes us to update regions.

>      region = bsearch(&phys, hostmem->current_regions,
>                       hostmem->num_current_regions,
>                       sizeof(hostmem->current_regions[0]),
> @@ -52,28 +91,33 @@ void *hostmem_lookup(HostMem *hostmem, hwaddr phys, 
> hwaddr len, bool is_write)
>      if (len <= region->size - offset_within_region) {
>          host_addr = region->host_addr + offset_within_region;
>      }
> -out:
> -    qemu_mutex_unlock(&hostmem->current_regions_lock);
> +    *mr = region->mr;
> +    memory_region_ref(*mr);

How does this protect against the QEMU thread hot unplugging while we
are searching hostmem->current_regions[]?  Our mr would be stale and the
ref operation would corrupt memory if mr has already been freed!

>  
> +out:
> +    hostmem_unref(hostmem);
>      return host_addr;
>  }
>  
> +static void hostmem_listener_begin(MemoryListener *listener)
> +{
> +    next_hostmem = g_new0(HostMem, 1);
> +    next_hostmem->ref = 1;
> +}
> +
>  /**
>   * Install new regions list
>   */
>  static void hostmem_listener_commit(MemoryListener *listener)
>  {
> -    HostMem *hostmem = container_of(listener, HostMem, listener);
> +    HostMem *tmp;
>  
> -    qemu_mutex_lock(&hostmem->current_regions_lock);
> -    g_free(hostmem->current_regions);
> -    hostmem->current_regions = hostmem->new_regions;
> -    hostmem->num_current_regions = hostmem->num_new_regions;
> -    qemu_mutex_unlock(&hostmem->current_regions_lock);
> +    tmp = cur_hostmem;
> +    qemu_mutex_lock(&hostmem_lock);

In hostmem_lookup() you accessed cur_hostmem inside the lock.  Does the
lock protect cur_hostmem or not?

> +    cur_hostmem = next_hostmem;
> +    qemu_mutex_unlock(&hostmem_lock);
> +    hostmem_unref(tmp);
>  
> -    /* Reset new regions list */
> -    hostmem->new_regions = NULL;
> -    hostmem->num_new_regions = 0;
>  }
>  
>  /**

I gave up here.  The approach you are trying to take isn't clear in this
patch.  I've pointed out some inconsistencies but they make it hard to
review more since I don't understand what you're trying to do.

Please split patches into logical steps and especially document
locks/refcounts to explain their scope - what do they protect?

Stefan