Re: [Qemu-devel] [PATCH v1 2/2] reduce qemu's heap Rss size from 12252kB to 2752KB

[Paolo Bonzini <pbonzini@redhat.com>]

On 10/03/2017 16:14, Yang Zhong wrote:
> There is no need to delete subregion and do memory begin/commit for
> subpage in memory_region_finalize().
> 
> This patch is from Anthony Xu <anthony.xu@intel.com>.
> 
> Signed-off-by: Yang Zhong <yang.zhong@intel.com>
> ---
>  memory.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/memory.c b/memory.c
> index 284894b..3e9bfff 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1505,13 +1505,14 @@ static void memory_region_finalize(Object *obj)
>       * and cause an infinite loop.
>       */
>      mr->enabled = false;
> -    memory_region_transaction_begin();
> -    while (!QTAILQ_EMPTY(&mr->subregions)) {
> -        MemoryRegion *subregion = QTAILQ_FIRST(&mr->subregions);
> -        memory_region_del_subregion(mr, subregion);
> +    if (!mr->subpage) {
> +        memory_region_transaction_begin();
> +        while (!QTAILQ_EMPTY(&mr->subregions)) {
> +            MemoryRegion *subregion = QTAILQ_FIRST(&mr->subregions);
> +            memory_region_del_subregion(mr, subregion);
> +        }
> +        memory_region_transaction_commit();

Subpages never have subregions, so the loop never runs.  The
begin/commit pair then becomes:

    ++memory_region_transaction_depth;
    --memory_region_transaction_depth;
    if (!memory_region_transaction_depth) {
        if (memory_region_update_pending) {
            ...
        } else if (ioeventfd_update_pending) {
            ...
        }
        // memory_region_clear_pending()
        memory_region_update_pending = false;
        ioeventfd_update_pending = false;
   }

If memory_region_transaction_depth is > 0 the begin/commit pair does
nothing.

But if memory_region_transaction_depth is == 0, there should be no
update pending because the loop has never run.  So I don't see what your
patch can change.

Of course there could be an update pending because of a bug elsewhere,
but I tried adding this patch:

diff --git a/memory.c b/memory.c
index 284894b..2208a21 100644
--- a/memory.c
+++ b/memory.c
@@ -903,6 +903,10 @@ static void
address_space_update_topology(AddressSpace *as)
 void memory_region_transaction_begin(void)
 {
     qemu_flush_coalesced_mmio_buffer();
+    if (!memory_region_transaction_depth) {
+        assert(!memory_region_update_pending);
+        assert(!ioeventfd_update_pending);
+    }
     ++memory_region_transaction_depth;
 }

and at least a basic qemu-system-x86_64 run started just fine.  So why
does this patch make a difference?

Paolo

>      }
> -    memory_region_transaction_commit();
> -