Patchwork [1/5] memory: add ref/unref interface for MemroyRegionOps

login
register
mail settings
Submitter pingfan liu
Date April 1, 2013, 8:20 a.m.
Message ID <1364804434-7980-2-git-send-email-qemulist@gmail.com>
Download mbox | patch
Permalink /patch/232675/
State New
Headers show

Comments

pingfan liu - April 1, 2013, 8:20 a.m.
From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>

This pair of interface are optinal, except for those device which is
used outside the biglock's protection for hot unplug. Currently,
HostMem used by virtio-blk dataplane is outside biglock, so the RAM
device should implement this.

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 include/exec/memory.h |   10 ++++++++++
 memory.c              |   18 ++++++++++++++++++
 2 files changed, 28 insertions(+), 0 deletions(-)
Stefan Hajnoczi - April 11, 2013, 9:49 a.m.
On Mon, Apr 01, 2013 at 04:20:30PM +0800, Liu Ping Fan wrote:
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> 
> This pair of interface are optinal, except for those device which is
> used outside the biglock's protection for hot unplug.

Not sure if this comment is true.  Memory unplug safety is not about the
big lock, it's about whether a reference to memory is held *across* a
hot unplug operation.

So even code that is under the big lock can use a guest RAM buffer
across the event loop, and therefore be exposed to a RAM unplug!

Therefore inc/dec must be used if guest RAM is held across event loop
handler calls.  If the guest RAM access happens completely inside a
handler function, then it is not affected by hot plug and doesn't need
to do inc/dec.
pingfan liu - April 12, 2013, 4:12 a.m.
On Thu, Apr 11, 2013 at 5:49 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Mon, Apr 01, 2013 at 04:20:30PM +0800, Liu Ping Fan wrote:
>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>
>> This pair of interface are optinal, except for those device which is
>> used outside the biglock's protection for hot unplug.
>
> Not sure if this comment is true.  Memory unplug safety is not about the
> big lock, it's about whether a reference to memory is held *across* a
> hot unplug operation.
>
What I exactly mean is DeviceX unplug is under biglock, so if
operations on DeviceX are all within the biglock, they are safe. But
that is not the trueth with RAM. So using ref/unref to manage the
reference to memory

> So even code that is under the big lock can use a guest RAM buffer
> across the event loop, and therefore be exposed to a RAM unplug!
>
> Therefore inc/dec must be used if guest RAM is held across event loop

The ref/unref interface wrapper the inc/dec, exposes them from MemoryRegionOps.

> handler calls.  If the guest RAM access happens completely inside a
> handler function, then it is not affected by hot plug and doesn't need
> to do inc/dec.

Yes, that is truth. And that is why only calling ref/unref in
HostMemListener region_add, but not in mem_add().

Patch

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 2322732..4289e62 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -55,6 +55,12 @@  struct MemoryRegionIORange {
  * Memory region callbacks
  */
 struct MemoryRegionOps {
+
+    /* ref/unref pair is optional;  ref.
+     * inc refcnt of object who store MemoryRegion
+     */
+    void (*ref)(void);
+    void (*unref)(void);
     /* Read from the memory region. @addr is relative to @mr; @size is
      * in bytes. */
     uint64_t (*read)(void *opaque,
@@ -228,6 +234,10 @@  struct MemoryListener {
     QTAILQ_ENTRY(MemoryListener) link;
 };
 
+/**/
+bool memory_region_ref(MemoryRegion *mr);
+bool memory_region_unref(MemoryRegion *mr);
+
 /**
  * memory_region_init: Initialize a memory region
  *
diff --git a/memory.c b/memory.c
index 75ca281..c29998d 100644
--- a/memory.c
+++ b/memory.c
@@ -786,6 +786,24 @@  static bool memory_region_wrong_endianness(MemoryRegion *mr)
 #endif
 }
 
+bool memory_region_ref(MemoryRegion *mr)
+{
+    if (mr->ops && mr->ops->ref) {
+        mr->ops->ref();
+        return true;
+    }
+    return false;
+}
+
+bool memory_region_unref(MemoryRegion *mr)
+{
+    if (mr->ops && mr->ops->unref) {
+        mr->ops->unref();
+        return true;
+    }
+    return false;
+}
+
 void memory_region_init(MemoryRegion *mr,
                         const char *name,
                         uint64_t size)