[v1,6/9] softmmu/physmem: Don't use atomic operations in ram_block_discard_(disable|require)

Commit Message

David Hildenbrand Nov. 19, 2020, 3:39 p.m. UTC

We have users in migration context that don't hold the BQL (when
finishing migration). To prepare for further changes, use a dedicated mutex
instead of atomic operations. Keep using qatomic_read ("READ_ONCE") for the
functions that only extract the current state (e.g., used by
virtio-balloon), locking isn't necessary.

While at it, split up the counter into two variables to make it easier
to understand.

Suggested-by: Peter Xu <[e-mail]>
Cc: Paolo Bonzini <[e-mail]>
Cc: "Michael S. Tsirkin" <[e-mail]>
Cc: Alex Williamson <[e-mail]>
Cc: Dr. David Alan Gilbert <[e-mail]>
Cc: Igor Mammedov <[e-mail]>
Cc: Pankaj Gupta <[e-mail]>
Cc: Peter Xu <[e-mail]>
Cc: Auger Eric <[e-mail]>
Cc: Wei Yang <[e-mail]>
Cc: teawater <[e-mail]>
Cc: Marek Kedzierski <[e-mail]>
Signed-off-by: David Hildenbrand <[e-mail]>
---
 softmmu/physmem.c | 70 ++++++++++++++++++++++++++---------------------
 1 file changed, 39 insertions(+), 31 deletions(-)

Comments

Peter Xu Nov. 19, 2020, 8:34 p.m. UTC | #1

On Thu, Nov 19, 2020 at 04:39:15PM +0100, David Hildenbrand wrote:
>  int ram_block_discard_disable(bool state)
>  {
> -    int old;
> +    int ret = 0;
>  
> +    ram_block_discard_disable_mutex_lock();
>      if (!state) {
> -        qatomic_dec(&ram_block_discard_disabled);
> -        return 0;
> +        ram_block_discard_disablers--;
> +    } else if (!ram_block_discard_requirers) {
> +        ram_block_discard_disablers++;
> +    } else {
> +        ret = -EBUSY;
>      }

I would make things even easier by:

  if (ram_block_discard_is_required()) {
    return -EBUSY;
  }

  if (state) {
    ram_block_discard_disablers++;
  } else {
    ram_block_discard_disablers--;
  }

But I think it's kind of nitpicking. :)

Reviewed-by: Peter Xu <[e-mail]>

Thanks for writing this patch.

Patch

diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 3027747c03..448e4e8c86 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -3650,56 +3650,64 @@  void mtree_print_dispatch(AddressSpaceDispatch *d, MemoryRegion *root)
     }
 }
 
-/*
- * If positive, discarding RAM is disabled. If negative, discarding RAM is
- * required to work and cannot be disabled.
- */
-static int ram_block_discard_disabled;
+static unsigned int ram_block_discard_requirers;
+static unsigned int ram_block_discard_disablers;
+static QemuMutex ram_block_discard_disable_mutex;
+
+static void ram_block_discard_disable_mutex_lock(void)
+{
+    static gsize initialized;
+
+    if (g_once_init_enter(&initialized)) {
+        qemu_mutex_init(&ram_block_discard_disable_mutex);
+        g_once_init_leave(&initialized, 1);
+    }
+    qemu_mutex_lock(&ram_block_discard_disable_mutex);
+}
+
+static void ram_block_discard_disable_mutex_unlock(void)
+{
+    qemu_mutex_unlock(&ram_block_discard_disable_mutex);
+}
 
 int ram_block_discard_disable(bool state)
 {
-    int old;
+    int ret = 0;
 
+    ram_block_discard_disable_mutex_lock();
     if (!state) {
-        qatomic_dec(&ram_block_discard_disabled);
-        return 0;
+        ram_block_discard_disablers--;
+    } else if (!ram_block_discard_requirers) {
+        ram_block_discard_disablers++;
+    } else {
+        ret = -EBUSY;
     }
-
-    do {
-        old = qatomic_read(&ram_block_discard_disabled);
-        if (old < 0) {
-            return -EBUSY;
-        }
-    } while (qatomic_cmpxchg(&ram_block_discard_disabled,
-                             old, old + 1) != old);
-    return 0;
+    ram_block_discard_disable_mutex_unlock();
+    return ret;
 }
 
 int ram_block_discard_require(bool state)
 {
-    int old;
+    int ret = 0;
 
+    ram_block_discard_disable_mutex_lock();
     if (!state) {
-        qatomic_inc(&ram_block_discard_disabled);
-        return 0;
+        ram_block_discard_requirers--;
+    } else if (!ram_block_discard_disablers) {
+        ram_block_discard_requirers++;
+    } else {
+        ret = -EBUSY;
     }
-
-    do {
-        old = qatomic_read(&ram_block_discard_disabled);
-        if (old > 0) {
-            return -EBUSY;
-        }
-    } while (qatomic_cmpxchg(&ram_block_discard_disabled,
-                             old, old - 1) != old);
-    return 0;
+    ram_block_discard_disable_mutex_unlock();
+    return ret;
 }
 
 bool ram_block_discard_is_disabled(void)
 {
-    return qatomic_read(&ram_block_discard_disabled) > 0;
+    return qatomic_read(&ram_block_discard_disablers);
 }
 
 bool ram_block_discard_is_required(void)
 {
-    return qatomic_read(&ram_block_discard_disabled) < 0;
+    return qatomic_read(&ram_block_discard_requirers);
 }