[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 07/10] softmmu/physmem: Don't use atomic operations in ram
From: |
Pankaj Gupta |
Subject: |
Re: [PATCH v2 07/10] softmmu/physmem: Don't use atomic operations in ram_block_discard_(disable|require) |
Date: |
Thu, 10 Dec 2020 15:54:12 +0100 |
> 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 <peterx@redhat.com>
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
> Cc: Peter Xu <peterx@redhat.com>
> Cc: Auger Eric <eric.auger@redhat.com>
> Cc: Wei Yang <richard.weiyang@linux.alibaba.com>
> Cc: teawater <teawaterz@linux.alibaba.com>
> Cc: Marek Kedzierski <mkedzier@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> softmmu/physmem.c | 70 ++++++++++++++++++++++++++---------------------
> 1 file changed, 39 insertions(+), 31 deletions(-)
>
> 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);
> }
return value won't be bool?
>
> bool ram_block_discard_is_required(void)
> {
> - return qatomic_read(&ram_block_discard_disabled) < 0;
> + return qatomic_read(&ram_block_discard_requirers);
same here.
> }
> --
Apart from query above, looks good.
Reviewed-by: Pankaj Gupta <pankaj.gupta@cloud.ionos.com>
- Re: [PATCH v2 02/10] virtio-mem: Factor out traversing unplugged ranges, (continued)
- [PATCH v2 03/10] virtio-mem: Implement RamDiscardMgr interface, David Hildenbrand, 2020/12/08
- [PATCH v2 04/10] vfio: Query and store the maximum number of DMA mappings, David Hildenbrand, 2020/12/08
- [PATCH v2 09/10] virtio-mem: Require only coordinated discards, David Hildenbrand, 2020/12/08
- [PATCH v2 10/10] vfio: Disable only uncoordinated discards, David Hildenbrand, 2020/12/08
- [PATCH v2 05/10] vfio: Support for RamDiscardMgr in the !vIOMMU case, David Hildenbrand, 2020/12/08
- [PATCH v2 06/10] vfio: Support for RamDiscardMgr in the vIOMMU case, David Hildenbrand, 2020/12/08
- [PATCH v2 07/10] softmmu/physmem: Don't use atomic operations in ram_block_discard_(disable|require), David Hildenbrand, 2020/12/08
- Re: [PATCH v2 07/10] softmmu/physmem: Don't use atomic operations in ram_block_discard_(disable|require),
Pankaj Gupta <=
- [PATCH v2 08/10] softmmu/physmem: Extend ram_block_discard_(require|disable) by two discard types, David Hildenbrand, 2020/12/08