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

[liu ping fan]

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()?
> 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