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

[Stefan Hajnoczi]

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 <address@hidden> wrote:
> > 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."
> >
> 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.