On Mon, Apr 01, 2013 at 04:20:31PM +0800, Liu Ping Fan wrote:
> From: Liu Ping Fan <pingf...@linux.vnet.ibm.com>
>
> 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 <pingf...@linux.vnet.ibm.com>
> ---
> 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
