| Message ID | 1364804434-7980-3-git-send-email-qemulist@gmail.com |
|---|---|
| State | New |
| Headers | show |
在 2013-04-01一的 16:20 +0800，Liu Ping Fan写道： > From: Liu Ping Fan <pingfank@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 <pingfank@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); > +} > + > +void hostmem_unref(HostMem *hostmem) > +{ > + int i; > + HostMemRegion *hmr; > + > + if (!__sync_sub_and_fetch(&hostmem->ref, 1)) { maybe '__sync_sub_and_fetch(&hostmem->ref, 1) > 0' is more safer > + 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) > + */ > + 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) > { > 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); > 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); > > +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); > + 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; > } > > /** > @@ -83,23 +127,24 @@ static void hostmem_append_new_region(HostMem *hostmem, > MemoryRegionSection *section) > { > void *ram_ptr = memory_region_get_ram_ptr(section->mr); > - size_t num = hostmem->num_new_regions; > - size_t new_size = (num + 1) * sizeof(hostmem->new_regions[0]); > + size_t num = hostmem->num_current_regions; > + size_t new_size = (num + 1) * sizeof(hostmem->current_regions[0]); > > - hostmem->new_regions = g_realloc(hostmem->new_regions, new_size); > - hostmem->new_regions[num] = (HostMemRegion){ > + hostmem->current_regions = g_realloc(hostmem->current_regions, new_size); > + hostmem->current_regions[num] = (HostMemRegion){ > .host_addr = ram_ptr + section->offset_within_region, > .guest_addr = section->offset_within_address_space, > + .mr = section->mr, > .size = section->size, > .readonly = section->readonly, > }; > - hostmem->num_new_regions++; > + hostmem->num_current_regions++; > } > > -static void hostmem_listener_append_region(MemoryListener *listener, > +static void hostmem_listener_nop_region(MemoryListener *listener, > MemoryRegionSection *section) > { > - HostMem *hostmem = container_of(listener, HostMem, listener); > + HostMem *hostmem = next_hostmem; > > /* Ignore non-RAM regions, we may not be able to map them */ > if (!memory_region_is_ram(section->mr)) { > @@ -114,6 +159,18 @@ static void hostmem_listener_append_region(MemoryListener *listener, > hostmem_append_new_region(hostmem, section); > } > > +static void hostmem_listener_append_region(MemoryListener *listener, > + MemoryRegionSection *section) > +{ > + hostmem_listener_nop_region(listener, section); > + /* Fix me, when memory hotunplug implement > + * assert(section->mr->ops->ref) > + */ > + if (section->mr->ops && section->mr->ops->ref) { > + section->mr->ops->ref(); > + } > +} > + > /* We don't implement most MemoryListener callbacks, use these nop stubs */ > static void hostmem_listener_dummy(MemoryListener *listener) > { > @@ -137,18 +194,12 @@ static void hostmem_listener_coalesced_mmio_dummy(MemoryListener *listener, > { > } > > -void hostmem_init(HostMem *hostmem) > -{ > - memset(hostmem, 0, sizeof(*hostmem)); > - > - qemu_mutex_init(&hostmem->current_regions_lock); > - > - hostmem->listener = (MemoryListener){ > - .begin = hostmem_listener_dummy, > +static MemoryListener hostmem_listener = { > + .begin = hostmem_listener_begin, > .commit = hostmem_listener_commit, > .region_add = hostmem_listener_append_region, > .region_del = hostmem_listener_section_dummy, > - .region_nop = hostmem_listener_append_region, > + .region_nop = hostmem_listener_nop_region, > .log_start = hostmem_listener_section_dummy, > .log_stop = hostmem_listener_section_dummy, > .log_sync = hostmem_listener_section_dummy, > @@ -159,18 +210,19 @@ void hostmem_init(HostMem *hostmem) > .coalesced_mmio_add = hostmem_listener_coalesced_mmio_dummy, > .coalesced_mmio_del = hostmem_listener_coalesced_mmio_dummy, > .priority = 10, > - }; > +}; > > - memory_listener_register(&hostmem->listener, &address_space_memory); > - if (hostmem->num_new_regions > 0) { > - hostmem_listener_commit(&hostmem->listener); > - } > +void hostmem_init(void) > +{ > + qemu_mutex_init(&hostmem_lock); > + cur_hostmem = g_new0(HostMem, 1); > + cur_hostmem->ref = 1; > + memory_listener_register(&hostmem_listener, &address_space_memory); > } > > -void hostmem_finalize(HostMem *hostmem) > +void hostmem_finalize(void) > { > - memory_listener_unregister(&hostmem->listener); > - g_free(hostmem->new_regions); > - g_free(hostmem->current_regions); > - qemu_mutex_destroy(&hostmem->current_regions_lock); > + memory_listener_unregister(&hostmem_listener); > + hostmem_unref(cur_hostmem); > + qemu_mutex_destroy(&hostmem_lock); > } > diff --git a/hw/dataplane/hostmem.h b/hw/dataplane/hostmem.h > index b2cf093..883ba74 100644 > --- a/hw/dataplane/hostmem.h > +++ b/hw/dataplane/hostmem.h > @@ -20,29 +20,18 @@ > typedef struct { > void *host_addr; > hwaddr guest_addr; > + MemoryRegion *mr; > uint64_t size; > bool readonly; > } HostMemRegion; > > typedef struct { > - /* The listener is invoked when regions change and a new list of regions is > - * built up completely before they are installed. > - */ > - MemoryListener listener; > - HostMemRegion *new_regions; > - size_t num_new_regions; > - > - /* Current regions are accessed from multiple threads either to lookup > - * addresses or to install a new list of regions. The lock protects the > - * pointer and the regions. > - */ > - QemuMutex current_regions_lock; > + int ref; > HostMemRegion *current_regions; > size_t num_current_regions; > } HostMem; > > -void hostmem_init(HostMem *hostmem); > -void hostmem_finalize(HostMem *hostmem); > +void hostmem_unref(HostMem *hostmem); > > /** > * Map a guest physical address to a pointer > @@ -52,6 +41,6 @@ void hostmem_finalize(HostMem *hostmem); > * can be done with other mechanisms like bdrv_drain_all() that quiesce > * in-flight I/O. > */ > -void *hostmem_lookup(HostMem *hostmem, hwaddr phys, hwaddr len, bool is_write); > +void *hostmem_lookup(hwaddr phys, hwaddr len, MemoryRegion **mr, bool is_write); > > #endif /* HOSTMEM_H */
Il 01/04/2013 10:20, Liu Ping Fan ha scritto: > From: Liu Ping Fan <pingfank@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 <pingfank@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); > +} > + > +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) > + */ > + 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) > { > HostMemRegion *region; > void *host_addr = NULL; > hwaddr offset_within_region; > + HostMem *hostmem; > + > + assert(mr); > + *mr = NULL; I think it's simpler if you allow a NULL MemoryRegion. Also, it is better if you keep the HostMem type, because in principle different HostMems could be used for different AddressSpaces. Basically, what is now HostMem would become HostMemRegions, and the new HostMem type would have these fields: QemuMutex hostmem_lock; HostMemRegions *cur_regions; HostMemRegions *next_regions; matching your three globals. I like the rcu-ready design. Finally, please split this patch and patch 3 differently: 1) add HostMemRegions 2) add ref/unref of memory regions to hostmem 3) add MemoryRegion argument to hostmem_lookup, leave it NULL in vring 4) add ref/unref of memory regions to vring Paolo > + qemu_mutex_lock(&hostmem_lock); > + hostmem = cur_hostmem; > + hostmem_ref(hostmem); > + qemu_mutex_unlock(&hostmem_lock); > > - qemu_mutex_lock(&hostmem->current_regions_lock); > 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); > > +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); > + 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; > } > > /** > @@ -83,23 +127,24 @@ static void hostmem_append_new_region(HostMem *hostmem, > MemoryRegionSection *section) > { > void *ram_ptr = memory_region_get_ram_ptr(section->mr); > - size_t num = hostmem->num_new_regions; > - size_t new_size = (num + 1) * sizeof(hostmem->new_regions[0]); > + size_t num = hostmem->num_current_regions; > + size_t new_size = (num + 1) * sizeof(hostmem->current_regions[0]); > > - hostmem->new_regions = g_realloc(hostmem->new_regions, new_size); > - hostmem->new_regions[num] = (HostMemRegion){ > + hostmem->current_regions = g_realloc(hostmem->current_regions, new_size); > + hostmem->current_regions[num] = (HostMemRegion){ > .host_addr = ram_ptr + section->offset_within_region, > .guest_addr = section->offset_within_address_space, > + .mr = section->mr, > .size = section->size, > .readonly = section->readonly, > }; > - hostmem->num_new_regions++; > + hostmem->num_current_regions++; > } > > -static void hostmem_listener_append_region(MemoryListener *listener, > +static void hostmem_listener_nop_region(MemoryListener *listener, > MemoryRegionSection *section) > { > - HostMem *hostmem = container_of(listener, HostMem, listener); > + HostMem *hostmem = next_hostmem; > > /* Ignore non-RAM regions, we may not be able to map them */ > if (!memory_region_is_ram(section->mr)) { > @@ -114,6 +159,18 @@ static void hostmem_listener_append_region(MemoryListener *listener, > hostmem_append_new_region(hostmem, section); > } > > +static void hostmem_listener_append_region(MemoryListener *listener, > + MemoryRegionSection *section) > +{ > + hostmem_listener_nop_region(listener, section); > + /* Fix me, when memory hotunplug implement > + * assert(section->mr->ops->ref) > + */ > + if (section->mr->ops && section->mr->ops->ref) { > + section->mr->ops->ref(); > + } > +} > + > /* We don't implement most MemoryListener callbacks, use these nop stubs */ > static void hostmem_listener_dummy(MemoryListener *listener) > { > @@ -137,18 +194,12 @@ static void hostmem_listener_coalesced_mmio_dummy(MemoryListener *listener, > { > } > > -void hostmem_init(HostMem *hostmem) > -{ > - memset(hostmem, 0, sizeof(*hostmem)); > - > - qemu_mutex_init(&hostmem->current_regions_lock); > - > - hostmem->listener = (MemoryListener){ > - .begin = hostmem_listener_dummy, > +static MemoryListener hostmem_listener = { > + .begin = hostmem_listener_begin, > .commit = hostmem_listener_commit, > .region_add = hostmem_listener_append_region, > .region_del = hostmem_listener_section_dummy, > - .region_nop = hostmem_listener_append_region, > + .region_nop = hostmem_listener_nop_region, > .log_start = hostmem_listener_section_dummy, > .log_stop = hostmem_listener_section_dummy, > .log_sync = hostmem_listener_section_dummy, > @@ -159,18 +210,19 @@ void hostmem_init(HostMem *hostmem) > .coalesced_mmio_add = hostmem_listener_coalesced_mmio_dummy, > .coalesced_mmio_del = hostmem_listener_coalesced_mmio_dummy, > .priority = 10, > - }; > +}; > > - memory_listener_register(&hostmem->listener, &address_space_memory); > - if (hostmem->num_new_regions > 0) { > - hostmem_listener_commit(&hostmem->listener); > - } > +void hostmem_init(void) > +{ > + qemu_mutex_init(&hostmem_lock); > + cur_hostmem = g_new0(HostMem, 1); > + cur_hostmem->ref = 1; > + memory_listener_register(&hostmem_listener, &address_space_memory); > } > > -void hostmem_finalize(HostMem *hostmem) > +void hostmem_finalize(void) > { > - memory_listener_unregister(&hostmem->listener); > - g_free(hostmem->new_regions); > - g_free(hostmem->current_regions); > - qemu_mutex_destroy(&hostmem->current_regions_lock); > + memory_listener_unregister(&hostmem_listener); > + hostmem_unref(cur_hostmem); > + qemu_mutex_destroy(&hostmem_lock); > } > diff --git a/hw/dataplane/hostmem.h b/hw/dataplane/hostmem.h > index b2cf093..883ba74 100644 > --- a/hw/dataplane/hostmem.h > +++ b/hw/dataplane/hostmem.h > @@ -20,29 +20,18 @@ > typedef struct { > void *host_addr; > hwaddr guest_addr; > + MemoryRegion *mr; > uint64_t size; > bool readonly; > } HostMemRegion; > > typedef struct { > - /* The listener is invoked when regions change and a new list of regions is > - * built up completely before they are installed. > - */ > - MemoryListener listener; > - HostMemRegion *new_regions; > - size_t num_new_regions; > - > - /* Current regions are accessed from multiple threads either to lookup > - * addresses or to install a new list of regions. The lock protects the > - * pointer and the regions. > - */ > - QemuMutex current_regions_lock; > + int ref; > HostMemRegion *current_regions; > size_t num_current_regions; > } HostMem; > > -void hostmem_init(HostMem *hostmem); > -void hostmem_finalize(HostMem *hostmem); > +void hostmem_unref(HostMem *hostmem); > > /** > * Map a guest physical address to a pointer > @@ -52,6 +41,6 @@ void hostmem_finalize(HostMem *hostmem); > * can be done with other mechanisms like bdrv_drain_all() that quiesce > * in-flight I/O. > */ > -void *hostmem_lookup(HostMem *hostmem, hwaddr phys, hwaddr len, bool is_write); > +void *hostmem_lookup(hwaddr phys, hwaddr len, MemoryRegion **mr, bool is_write); > > #endif /* HOSTMEM_H */ >
On Mon, Apr 01, 2013 at 04:20:31PM +0800, Liu Ping Fan wrote: > From: Liu Ping Fan <pingfank@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 <pingfank@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
Il 11/04/2013 12:11, Stefan Hajnoczi ha scritto: > 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. We are already using GCC atomics elsewhere (spice, migration, vhost). They are really ugly to type and not complete, so we should have our own wrapper include, but they are fine. Paolo
On Thu, Apr 11, 2013 at 6:11 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote: > On Mon, Apr 01, 2013 at 04:20:31PM +0800, Liu Ping Fan wrote: >> From: Liu Ping Fan <pingfank@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 <pingfank@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." > So if needed, using abort()? > 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(). > Ok. > 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? > This is used to notice the developer that the ref/unref interface should be implemented for RAM device, since it is can be used during RAM unplug. And as you mentioned, here s/assert/abort/ Right? >> + 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. > The new API enforce a param "MemoryRegion **mr", and rely on the refcnt on it to survive the RAM unplug. The caller should unref this mr when done with it. But the old API can not provide this, and not easy to provide a wrapper. >> { >> 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. > The trick is the RCU. The lookup work will cur_hostmem, meanwhile if there is a updater, it changes next_hostmem. So there is no race between them. >> 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! > When hostmem_listener_append_region, we inc refcnt of mr, this will pin the mr. During the lookup, it will help us agaist unplug. Then after cur_hostmem retired to past, we will release the corresponding refcnt which it holds. >> >> +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? > Yes, protect. Supposed we have HostMem A, and it will become B. Then hostmem_lookup will either see A or B. If it see A, it should use A refcnt agaist hostmem_listener_commit to drop A. This refcnt has no relation with mr's object's refcnt. >> + 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 The main idea differ from origianl code is rcu, we have two version of HostMem, reader uses cur_hostmem, updater uses next_hostmem > 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? > Sorry for that, I will try to imrove it, and more document. Thanks and regards, Pingfan > Stefan
On Thu, Apr 11, 2013 at 6:09 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 01/04/2013 10:20, Liu Ping Fan ha scritto: >> From: Liu Ping Fan <pingfank@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 <pingfank@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); >> +} >> + >> +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) >> + */ >> + 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) >> { >> HostMemRegion *region; >> void *host_addr = NULL; >> hwaddr offset_within_region; >> + HostMem *hostmem; >> + >> + assert(mr); >> + *mr = NULL; > > I think it's simpler if you allow a NULL MemoryRegion. > But how can we guarantee the caller to release the refcnt? In fact, if @mr is not exposed to external component directly, it is still required internally, for example, cpu_physical_memory_map() ported onto HostMem, we will still rely on _unmap() to release the refcnt. > Also, it is better if you keep the HostMem type, because in principle > different HostMems could be used for different AddressSpaces. > Basically, what is now HostMem would become HostMemRegions, and the new > HostMem type would have these fields: > > QemuMutex hostmem_lock; > HostMemRegions *cur_regions; > HostMemRegions *next_regions; > > matching your three globals. I like the rcu-ready design. > What about AddressSpaceMemoryRegion { QemuMutex hostmem_lock; HostMem *cur_hostmem; HostMem *next_hostmem; MemoryListener listener; } So each AddressSpaceMemoryRegion can be bound with specific AddressSpace. > Finally, please split this patch and patch 3 differently: > > 1) add HostMemRegions > > 2) add ref/unref of memory regions to hostmem > > 3) add MemoryRegion argument to hostmem_lookup, leave it NULL in vring > As explained above, seem we can not leave it NULL. (In theory, vring seated on RAM can not be unplug, since it is kernel. But I think we can not rely on the guest's behavior) > 4) add ref/unref of memory regions to vring > Thank you very much :-). Will follow that, and arrange patches more clearly. Regards, Pingfan > Paolo > >> + qemu_mutex_lock(&hostmem_lock); >> + hostmem = cur_hostmem; >> + hostmem_ref(hostmem); >> + qemu_mutex_unlock(&hostmem_lock); >> >> - qemu_mutex_lock(&hostmem->current_regions_lock); >> 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); >> >> +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); >> + 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; >> } >> >> /** >> @@ -83,23 +127,24 @@ static void hostmem_append_new_region(HostMem *hostmem, >> MemoryRegionSection *section) >> { >> void *ram_ptr = memory_region_get_ram_ptr(section->mr); >> - size_t num = hostmem->num_new_regions; >> - size_t new_size = (num + 1) * sizeof(hostmem->new_regions[0]); >> + size_t num = hostmem->num_current_regions; >> + size_t new_size = (num + 1) * sizeof(hostmem->current_regions[0]); >> >> - hostmem->new_regions = g_realloc(hostmem->new_regions, new_size); >> - hostmem->new_regions[num] = (HostMemRegion){ >> + hostmem->current_regions = g_realloc(hostmem->current_regions, new_size); >> + hostmem->current_regions[num] = (HostMemRegion){ >> .host_addr = ram_ptr + section->offset_within_region, >> .guest_addr = section->offset_within_address_space, >> + .mr = section->mr, >> .size = section->size, >> .readonly = section->readonly, >> }; >> - hostmem->num_new_regions++; >> + hostmem->num_current_regions++; >> } >> >> -static void hostmem_listener_append_region(MemoryListener *listener, >> +static void hostmem_listener_nop_region(MemoryListener *listener, >> MemoryRegionSection *section) >> { >> - HostMem *hostmem = container_of(listener, HostMem, listener); >> + HostMem *hostmem = next_hostmem; >> >> /* Ignore non-RAM regions, we may not be able to map them */ >> if (!memory_region_is_ram(section->mr)) { >> @@ -114,6 +159,18 @@ static void hostmem_listener_append_region(MemoryListener *listener, >> hostmem_append_new_region(hostmem, section); >> } >> >> +static void hostmem_listener_append_region(MemoryListener *listener, >> + MemoryRegionSection *section) >> +{ >> + hostmem_listener_nop_region(listener, section); >> + /* Fix me, when memory hotunplug implement >> + * assert(section->mr->ops->ref) >> + */ >> + if (section->mr->ops && section->mr->ops->ref) { >> + section->mr->ops->ref(); >> + } >> +} >> + >> /* We don't implement most MemoryListener callbacks, use these nop stubs */ >> static void hostmem_listener_dummy(MemoryListener *listener) >> { >> @@ -137,18 +194,12 @@ static void hostmem_listener_coalesced_mmio_dummy(MemoryListener *listener, >> { >> } >> >> -void hostmem_init(HostMem *hostmem) >> -{ >> - memset(hostmem, 0, sizeof(*hostmem)); >> - >> - qemu_mutex_init(&hostmem->current_regions_lock); >> - >> - hostmem->listener = (MemoryListener){ >> - .begin = hostmem_listener_dummy, >> +static MemoryListener hostmem_listener = { >> + .begin = hostmem_listener_begin, >> .commit = hostmem_listener_commit, >> .region_add = hostmem_listener_append_region, >> .region_del = hostmem_listener_section_dummy, >> - .region_nop = hostmem_listener_append_region, >> + .region_nop = hostmem_listener_nop_region, >> .log_start = hostmem_listener_section_dummy, >> .log_stop = hostmem_listener_section_dummy, >> .log_sync = hostmem_listener_section_dummy, >> @@ -159,18 +210,19 @@ void hostmem_init(HostMem *hostmem) >> .coalesced_mmio_add = hostmem_listener_coalesced_mmio_dummy, >> .coalesced_mmio_del = hostmem_listener_coalesced_mmio_dummy, >> .priority = 10, >> - }; >> +}; >> >> - memory_listener_register(&hostmem->listener, &address_space_memory); >> - if (hostmem->num_new_regions > 0) { >> - hostmem_listener_commit(&hostmem->listener); >> - } >> +void hostmem_init(void) >> +{ >> + qemu_mutex_init(&hostmem_lock); >> + cur_hostmem = g_new0(HostMem, 1); >> + cur_hostmem->ref = 1; >> + memory_listener_register(&hostmem_listener, &address_space_memory); >> } >> >> -void hostmem_finalize(HostMem *hostmem) >> +void hostmem_finalize(void) >> { >> - memory_listener_unregister(&hostmem->listener); >> - g_free(hostmem->new_regions); >> - g_free(hostmem->current_regions); >> - qemu_mutex_destroy(&hostmem->current_regions_lock); >> + memory_listener_unregister(&hostmem_listener); >> + hostmem_unref(cur_hostmem); >> + qemu_mutex_destroy(&hostmem_lock); >> } >> diff --git a/hw/dataplane/hostmem.h b/hw/dataplane/hostmem.h >> index b2cf093..883ba74 100644 >> --- a/hw/dataplane/hostmem.h >> +++ b/hw/dataplane/hostmem.h >> @@ -20,29 +20,18 @@ >> typedef struct { >> void *host_addr; >> hwaddr guest_addr; >> + MemoryRegion *mr; >> uint64_t size; >> bool readonly; >> } HostMemRegion; >> >> typedef struct { >> - /* The listener is invoked when regions change and a new list of regions is >> - * built up completely before they are installed. >> - */ >> - MemoryListener listener; >> - HostMemRegion *new_regions; >> - size_t num_new_regions; >> - >> - /* Current regions are accessed from multiple threads either to lookup >> - * addresses or to install a new list of regions. The lock protects the >> - * pointer and the regions. >> - */ >> - QemuMutex current_regions_lock; >> + int ref; >> HostMemRegion *current_regions; >> size_t num_current_regions; >> } HostMem; >> >> -void hostmem_init(HostMem *hostmem); >> -void hostmem_finalize(HostMem *hostmem); >> +void hostmem_unref(HostMem *hostmem); >> >> /** >> * Map a guest physical address to a pointer >> @@ -52,6 +41,6 @@ void hostmem_finalize(HostMem *hostmem); >> * can be done with other mechanisms like bdrv_drain_all() that quiesce >> * in-flight I/O. >> */ >> -void *hostmem_lookup(HostMem *hostmem, hwaddr phys, hwaddr len, bool is_write); >> +void *hostmem_lookup(hwaddr phys, hwaddr len, MemoryRegion **mr, bool is_write); >> >> #endif /* HOSTMEM_H */ >> >
On Fri, Apr 12, 2013 at 02:46:41PM +0800, liu ping fan wrote: > On Thu, Apr 11, 2013 at 6:09 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > > Il 01/04/2013 10:20, Liu Ping Fan ha scritto: > > Finally, please split this patch and patch 3 differently: > > > > 1) add HostMemRegions > > > > 2) add ref/unref of memory regions to hostmem > > > > 3) add MemoryRegion argument to hostmem_lookup, leave it NULL in vring > > > As explained above, seem we can not leave it NULL. (In theory, vring > seated on RAM can not be unplug, since it is kernel. But I think we > can not rely on the guest's behavior) It should be possible to evacuate the vring. The guest needs to reset the device so the vring is unmapped. Then memory can be unplugged. Reinitializing the device will allocate a new vring and things work again. Stefan
On Fri, Apr 12, 2013 at 11:55:39AM +0800, liu ping fan wrote: > On Thu, Apr 11, 2013 at 6:11 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote: > > On Mon, Apr 01, 2013 at 04:20:31PM +0800, Liu Ping Fan wrote: > >> From: Liu Ping Fan <pingfank@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 <pingfank@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." > > > So if needed, using abort()? No, you can still use assert but don't put side-effects into the assertion: int old = __sync_add_and_fetch(&hostmem->ref, 1); assert(old > 0); > > 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(). > > > Ok. > > 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? > > > This is used to notice the developer that the ref/unref interface > should be implemented for RAM device, since it is can be used during > RAM unplug. And as you mentioned, here s/assert/abort/ Right? Ah, sorry. I misread the assertion, you want to require that ->unref() is implemented. The assertion is fine the way it is. > >> + 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. > > > The new API enforce a param "MemoryRegion **mr", and rely on the > refcnt on it to survive the RAM unplug. The caller should unref this > mr when done with it. But the old API can not provide this, and not > easy to provide a wrapper. I understand, MemoryRegion needs to be added to the function. But it would be nice to keep the hostmem_lock and cur_hostmem stuff separate to make this function easier to understand. In other words, keep the globals away from the code that operates on a HostMem. It makes the code easier to read and guarantees to the reader that you are not mixing in assumptions about global state. > >> { > >> 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. > > > The trick is the RCU. The lookup work will cur_hostmem, meanwhile if > there is a updater, it changes next_hostmem. So there is no race > between them. I see. Please document the nature of the cur/next variables in comments. You can make the code review process smoother by laying out patches in a way that makes them logical and easy to understand. Right now it feels like I have to reverse-engineer what you were thinking in order to understand the patches. > >> 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! > > > When hostmem_listener_append_region, we inc refcnt of mr, this will > pin the mr. During the lookup, it will help us agaist unplug. Then > after cur_hostmem retired to past, we will release the corresponding > refcnt which it holds. I see. This also explain why hostmem_ref()/hostmem_unref() a asymmetric: ref() just increments hostmem's refcount while unref() actually decrements refcounts for memory regions. It's something I wondered about when looking at those functions. > >> > >> +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? > > > Yes, protect. Supposed we have HostMem A, and it will become B. Then > hostmem_lookup will either see A or B. If it see A, it should use A > refcnt agaist hostmem_listener_commit to drop A. This refcnt has no > relation with mr's object's refcnt. My question is why you are accessing cur_hostmem outside hostmem_lock but then assigning it inside the lock on the next line... > >> + cur_hostmem = next_hostmem; ...here. If you want an atomic exchange then tmp = cur_hostmem should be inside the lock.
Il 12/04/2013 10:38, Stefan Hajnoczi ha scritto: >> > Yes, protect. Supposed we have HostMem A, and it will become B. Then >> > hostmem_lookup will either see A or B. If it see A, it should use A >> > refcnt agaist hostmem_listener_commit to drop A. This refcnt has no >> > relation with mr's object's refcnt. > My question is why you are accessing cur_hostmem outside hostmem_lock > but then assigning it inside the lock on the next line... > >>>> > >> + cur_hostmem = next_hostmem; > ...here. > > If you want an atomic exchange then tmp = cur_hostmem should be inside > the lock. It will work because readers will grab either the hostmem_lock or the BQL, while writers will grab both. A kind of local/global lock, but I'm not sure it was intentional. :) It's simpler to just move the read inside the lock. Paolo
On Fri, Apr 12, 2013 at 6:03 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 12/04/2013 10:38, Stefan Hajnoczi ha scritto: >>> > Yes, protect. Supposed we have HostMem A, and it will become B. Then >>> > hostmem_lookup will either see A or B. If it see A, it should use A >>> > refcnt agaist hostmem_listener_commit to drop A. This refcnt has no >>> > relation with mr's object's refcnt. >> My question is why you are accessing cur_hostmem outside hostmem_lock >> but then assigning it inside the lock on the next line... >> >>>>> > >> + cur_hostmem = next_hostmem; >> ...here. >> >> If you want an atomic exchange then tmp = cur_hostmem should be inside >> the lock. > > It will work because readers will grab either the hostmem_lock or the > BQL, while writers will grab both. A kind of local/global lock, but I'm No only hostmem_lock is used to protect readers from writers. While the writers are protected agaist each other by biglock. So leaving the "cur_hostmem = next_hostmem" outside to reflect the lock's rules. Regards, Pingfan > not sure it was intentional. :) It's simpler to just move the read > inside the lock. > > Paolo
Il 15/04/2013 03:42, liu ping fan ha scritto: > > It will work because readers will grab either the hostmem_lock or the > > BQL, while writers will grab both. A kind of local/global lock, but I'm > > No only hostmem_lock is used to protect readers from writers. While > the writers are protected agaist each other by biglock. So leaving > the "cur_hostmem = next_hostmem" outside to reflect the lock's rules. Doesn't help much if you do not document them... Paolo
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); +} + +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) + */ + 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) { 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); 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); +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); + 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; } /** @@ -83,23 +127,24 @@ static void hostmem_append_new_region(HostMem *hostmem, MemoryRegionSection *section) { void *ram_ptr = memory_region_get_ram_ptr(section->mr); - size_t num = hostmem->num_new_regions; - size_t new_size = (num + 1) * sizeof(hostmem->new_regions[0]); + size_t num = hostmem->num_current_regions; + size_t new_size = (num + 1) * sizeof(hostmem->current_regions[0]); - hostmem->new_regions = g_realloc(hostmem->new_regions, new_size); - hostmem->new_regions[num] = (HostMemRegion){ + hostmem->current_regions = g_realloc(hostmem->current_regions, new_size); + hostmem->current_regions[num] = (HostMemRegion){ .host_addr = ram_ptr + section->offset_within_region, .guest_addr = section->offset_within_address_space, + .mr = section->mr, .size = section->size, .readonly = section->readonly, }; - hostmem->num_new_regions++; + hostmem->num_current_regions++; } -static void hostmem_listener_append_region(MemoryListener *listener, +static void hostmem_listener_nop_region(MemoryListener *listener, MemoryRegionSection *section) { - HostMem *hostmem = container_of(listener, HostMem, listener); + HostMem *hostmem = next_hostmem; /* Ignore non-RAM regions, we may not be able to map them */ if (!memory_region_is_ram(section->mr)) { @@ -114,6 +159,18 @@ static void hostmem_listener_append_region(MemoryListener *listener, hostmem_append_new_region(hostmem, section); } +static void hostmem_listener_append_region(MemoryListener *listener, + MemoryRegionSection *section) +{ + hostmem_listener_nop_region(listener, section); + /* Fix me, when memory hotunplug implement + * assert(section->mr->ops->ref) + */ + if (section->mr->ops && section->mr->ops->ref) { + section->mr->ops->ref(); + } +} + /* We don't implement most MemoryListener callbacks, use these nop stubs */ static void hostmem_listener_dummy(MemoryListener *listener) { @@ -137,18 +194,12 @@ static void hostmem_listener_coalesced_mmio_dummy(MemoryListener *listener, { } -void hostmem_init(HostMem *hostmem) -{ - memset(hostmem, 0, sizeof(*hostmem)); - - qemu_mutex_init(&hostmem->current_regions_lock); - - hostmem->listener = (MemoryListener){ - .begin = hostmem_listener_dummy, +static MemoryListener hostmem_listener = { + .begin = hostmem_listener_begin, .commit = hostmem_listener_commit, .region_add = hostmem_listener_append_region, .region_del = hostmem_listener_section_dummy, - .region_nop = hostmem_listener_append_region, + .region_nop = hostmem_listener_nop_region, .log_start = hostmem_listener_section_dummy, .log_stop = hostmem_listener_section_dummy, .log_sync = hostmem_listener_section_dummy, @@ -159,18 +210,19 @@ void hostmem_init(HostMem *hostmem) .coalesced_mmio_add = hostmem_listener_coalesced_mmio_dummy, .coalesced_mmio_del = hostmem_listener_coalesced_mmio_dummy, .priority = 10, - }; +}; - memory_listener_register(&hostmem->listener, &address_space_memory); - if (hostmem->num_new_regions > 0) { - hostmem_listener_commit(&hostmem->listener); - } +void hostmem_init(void) +{ + qemu_mutex_init(&hostmem_lock); + cur_hostmem = g_new0(HostMem, 1); + cur_hostmem->ref = 1; + memory_listener_register(&hostmem_listener, &address_space_memory); } -void hostmem_finalize(HostMem *hostmem) +void hostmem_finalize(void) { - memory_listener_unregister(&hostmem->listener); - g_free(hostmem->new_regions); - g_free(hostmem->current_regions); - qemu_mutex_destroy(&hostmem->current_regions_lock); + memory_listener_unregister(&hostmem_listener); + hostmem_unref(cur_hostmem); + qemu_mutex_destroy(&hostmem_lock); } diff --git a/hw/dataplane/hostmem.h b/hw/dataplane/hostmem.h index b2cf093..883ba74 100644 --- a/hw/dataplane/hostmem.h +++ b/hw/dataplane/hostmem.h @@ -20,29 +20,18 @@ typedef struct { void *host_addr; hwaddr guest_addr; + MemoryRegion *mr; uint64_t size; bool readonly; } HostMemRegion; typedef struct { - /* The listener is invoked when regions change and a new list of regions is - * built up completely before they are installed. - */ - MemoryListener listener; - HostMemRegion *new_regions; - size_t num_new_regions; - - /* Current regions are accessed from multiple threads either to lookup - * addresses or to install a new list of regions. The lock protects the - * pointer and the regions. - */ - QemuMutex current_regions_lock; + int ref; HostMemRegion *current_regions; size_t num_current_regions; } HostMem; -void hostmem_init(HostMem *hostmem); -void hostmem_finalize(HostMem *hostmem); +void hostmem_unref(HostMem *hostmem); /** * Map a guest physical address to a pointer @@ -52,6 +41,6 @@ void hostmem_finalize(HostMem *hostmem); * can be done with other mechanisms like bdrv_drain_all() that quiesce * in-flight I/O. */ -void *hostmem_lookup(HostMem *hostmem, hwaddr phys, hwaddr len, bool is_write); +void *hostmem_lookup(hwaddr phys, hwaddr len, MemoryRegion **mr, bool is_write); #endif /* HOSTMEM_H */