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 <stefa...@gmail.com> wrote:
> > 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."
> >
> 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.
