diff mbox

[v2,4/6] hostmem: hostmem listener pin RAM-Device by refcnt

Message ID 1367549122-2948-5-git-send-email-qemulist@gmail.com
State New
Headers show

Commit Message

pingfan liu May 3, 2013, 2:45 a.m. UTC 
From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>

With ref()/unref() interface of MemoryRegion, we can pin RAM-Device
when using its memory, and release it when done.

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 hw/virtio/dataplane/hostmem.c         |   44 +++++++++++++++++++++++++++-----
 hw/virtio/dataplane/vring.c           |    8 +++---
 include/hw/virtio/dataplane/hostmem.h |    4 ++-
 3 files changed, 44 insertions(+), 12 deletions(-)

Comments

Stefan Hajnoczi May 3, 2013, 9:35 a.m. UTC | #1 
On Fri, May 03, 2013 at 10:45:20AM +0800, Liu Ping Fan wrote:
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> 
> With ref()/unref() interface of MemoryRegion, we can pin RAM-Device
> when using its memory, and release it when done.
> 
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  hw/virtio/dataplane/hostmem.c         |   44 +++++++++++++++++++++++++++-----
>  hw/virtio/dataplane/vring.c           |    8 +++---
>  include/hw/virtio/dataplane/hostmem.h |    4 ++-
>  3 files changed, 44 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/virtio/dataplane/hostmem.c b/hw/virtio/dataplane/hostmem.c
> index 1fd3e06..0e28dfc 100644
> --- a/hw/virtio/dataplane/hostmem.c
> +++ b/hw/virtio/dataplane/hostmem.c
> @@ -42,18 +42,28 @@ static void hostmem_ref(HostMem *hostmem)
>  
>  static void hostmem_unref(HostMem *hostmem)
>  {
> -    int t;
> +    int i, t;
> +    HostMemRegion *hmr;
>  
>      t = __sync_sub_and_fetch(&hostmem->ref, 1);
>      assert(t >= 0);
>      if (!t) {
> +        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();
> +            }

This patch should use memory_region_ref()/unref() which you introduced
in the previous patch instead of open-coding this.

> @@ -158,6 +176,18 @@ static void hostmem_listener_append_region(MemoryListener *listener,
>      hostmem_append_new_region(as_mem->next_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();
> +    }
> +}

Why does append increment the refcount while nop does not?

It seems that we always need to increment the memory region refcount
since we're building a completely new hostmem.  The refcount ownership
is not passed from the current hostmen to the next hostmem, all memory
regions are released in hostmem_unref().
pingfan liu May 6, 2013, 1:45 a.m. UTC | #2 
On Fri, May 3, 2013 at 5:35 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Fri, May 03, 2013 at 10:45:20AM +0800, Liu Ping Fan wrote:
>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>
>> With ref()/unref() interface of MemoryRegion, we can pin RAM-Device
>> when using its memory, and release it when done.
>>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>>  hw/virtio/dataplane/hostmem.c         |   44 +++++++++++++++++++++++++++-----
>>  hw/virtio/dataplane/vring.c           |    8 +++---
>>  include/hw/virtio/dataplane/hostmem.h |    4 ++-
>>  3 files changed, 44 insertions(+), 12 deletions(-)
>>
>> diff --git a/hw/virtio/dataplane/hostmem.c b/hw/virtio/dataplane/hostmem.c
>> index 1fd3e06..0e28dfc 100644
>> --- a/hw/virtio/dataplane/hostmem.c
>> +++ b/hw/virtio/dataplane/hostmem.c
>> @@ -42,18 +42,28 @@ static void hostmem_ref(HostMem *hostmem)
>>
>>  static void hostmem_unref(HostMem *hostmem)
>>  {
>> -    int t;
>> +    int i, t;
>> +    HostMemRegion *hmr;
>>
>>      t = __sync_sub_and_fetch(&hostmem->ref, 1);
>>      assert(t >= 0);
>>      if (!t) {
>> +        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();
>> +            }
>
> This patch should use memory_region_ref()/unref() which you introduced
> in the previous patch instead of open-coding this.
>
Yes, reasonable.
>> @@ -158,6 +176,18 @@ static void hostmem_listener_append_region(MemoryListener *listener,
>>      hostmem_append_new_region(as_mem->next_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();
>> +    }
>> +}
>
> Why does append increment the refcount while nop does not?
>
> It seems that we always need to increment the memory region refcount
> since we're building a completely new hostmem.  The refcount ownership
> is not passed from the current hostmen to the next hostmem, all memory
> regions are released in hostmem_unref().

Yes, you are right.  Will fix it.

Thanks,
Pingfan
diff mbox

Patch

diff --git a/hw/virtio/dataplane/hostmem.c b/hw/virtio/dataplane/hostmem.c
index 1fd3e06..0e28dfc 100644
--- a/hw/virtio/dataplane/hostmem.c
+++ b/hw/virtio/dataplane/hostmem.c
@@ -42,18 +42,28 @@  static void hostmem_ref(HostMem *hostmem)
 
 static void hostmem_unref(HostMem *hostmem)
 {
-    int t;
+    int i, t;
+    HostMemRegion *hmr;
 
     t = __sync_sub_and_fetch(&hostmem->ref, 1);
     assert(t >= 0);
     if (!t) {
+        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);
     }
 }
 
 static void *address_space_mem_lookup(AddrSpaceMem *as_mem, hwaddr phys,
-    hwaddr len, bool is_write)
+    hwaddr len, MemoryRegion **mr, bool is_write)
 {
     HostMemRegion *region;
     void *host_addr = NULL;
@@ -65,6 +75,9 @@  static void *address_space_mem_lookup(AddrSpaceMem *as_mem, hwaddr phys,
     hostmem_ref(hostmem);
     qemu_mutex_unlock(&as_mem->cur_lock);
 
+    if (mr) {
+        *mr = NULL;
+    }
     region = bsearch(&phys, hostmem->current_regions,
                      hostmem->num_current_regions,
                      sizeof(hostmem->current_regions[0]),
@@ -79,7 +92,10 @@  static void *address_space_mem_lookup(AddrSpaceMem *as_mem, hwaddr phys,
     if (len <= region->size - offset_within_region) {
         host_addr = region->host_addr + offset_within_region;
     }
-
+    if (mr) {
+        *mr = region->mr;
+        memory_region_ref(*mr);
+    }
 out:
     hostmem_unref(hostmem);
     return host_addr;
@@ -88,9 +104,10 @@  out:
 /**
  * Map guest physical address to host pointer
  */
-void *hostmem_lookup(hwaddr phys, hwaddr len, bool is_write)
+void *hostmem_lookup(hwaddr phys, hwaddr len, MemoryRegion **mr,
+    bool is_write)
 {
-    return address_space_mem_lookup(system_mem, phys, len, is_write);
+    return address_space_mem_lookup(system_mem, phys, len, mr, is_write);
 }
 
 static void hostmem_listener_begin(MemoryListener *listener)
@@ -134,13 +151,14 @@  static void hostmem_append_new_region(HostMem *hostmem,
     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_current_regions++;
 }
 
-static void hostmem_listener_append_region(MemoryListener *listener,
+static void hostmem_listener_nop_region(MemoryListener *listener,
                                            MemoryRegionSection *section)
 {
     AddrSpaceMem *as_mem = container_of(listener, AddrSpaceMem, listener);
@@ -158,6 +176,18 @@  static void hostmem_listener_append_region(MemoryListener *listener,
     hostmem_append_new_region(as_mem->next_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)
 {
@@ -191,7 +221,7 @@  static AddrSpaceMem *address_space_mem_create(AddressSpace *as)
         .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,
diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c
index 4d6d735..e3c3afb 100644
--- a/hw/virtio/dataplane/vring.c
+++ b/hw/virtio/dataplane/vring.c
@@ -27,7 +27,7 @@  bool vring_setup(Vring *vring, VirtIODevice *vdev, int n)
 
     vring->broken = false;
 
-    vring_ptr = hostmem_lookup(vring_addr, vring_size, true);
+    vring_ptr = hostmem_lookup(vring_addr, vring_size, NULL, true);
     if (!vring_ptr) {
         error_report("Failed to map vring "
                      "addr %#" HWADDR_PRIx " size %" HWADDR_PRIu,
@@ -137,7 +137,7 @@  static int get_indirect(Vring *vring,
 
         /* Translate indirect descriptor */
         desc_ptr = hostmem_lookup(indirect->addr + found * sizeof(desc),
-                                  sizeof(desc), false);
+                                  sizeof(desc), NULL, false);
         if (!desc_ptr) {
             error_report("Failed to map indirect descriptor "
                          "addr %#" PRIx64 " len %zu",
@@ -169,7 +169,7 @@  static int get_indirect(Vring *vring,
             return -ENOBUFS;
         }
 
-        iov->iov_base = hostmem_lookup(desc.addr, desc.len,
+        iov->iov_base = hostmem_lookup(desc.addr, desc.len, NULL,
                                        desc.flags & VRING_DESC_F_WRITE);
         if (!iov->iov_base) {
             error_report("Failed to map indirect descriptor"
@@ -297,7 +297,7 @@  int vring_pop(VirtIODevice *vdev, Vring *vring,
         }
 
         /* TODO handle non-contiguous memory across region boundaries */
-        iov->iov_base = hostmem_lookup(desc.addr, desc.len,
+        iov->iov_base = hostmem_lookup(desc.addr, desc.len, NULL,
                                        desc.flags & VRING_DESC_F_WRITE);
         if (!iov->iov_base) {
             error_report("Failed to map vring desc addr %#" PRIx64 " len %u",
diff --git a/include/hw/virtio/dataplane/hostmem.h b/include/hw/virtio/dataplane/hostmem.h
index 51afa8f..20289c8 100644
--- a/include/hw/virtio/dataplane/hostmem.h
+++ b/include/hw/virtio/dataplane/hostmem.h
@@ -20,6 +20,7 @@ 
 typedef struct {
     void *host_addr;
     hwaddr guest_addr;
+    MemoryRegion *mr;
     uint64_t size;
     bool readonly;
 } HostMemRegion;
@@ -51,6 +52,7 @@  typedef struct {
  * can be done with other mechanisms like bdrv_drain_all() that quiesce
  * in-flight I/O.
  */
-void *hostmem_lookup(hwaddr phys, hwaddr len, bool is_write);
+void *hostmem_lookup(hwaddr phys, hwaddr len, MemoryRegion **mr,
+    bool is_write);
 
 #endif /* HOSTMEM_H */