[v3] memory: reduce heap Rss size around 3M

Hello Peter,

The cpu-memory and memory have same address space through info mtree tool.

address-space: memory
  0000000000000000-ffffffffffffffff (prio 0, i/o): system
    0000000000000000-000000007fffffff (prio 0, i/o): alias ram-below-4g @pc.ram 0000000000000000-000000007fffffff
    0000000000000000-ffffffffffffffff (prio -1, i/o): pci
      00000000000c0000-00000000000dffff (prio 1, ram): pc.rom
      00000000000e0000-00000000000fffff (prio 1, i/o): alias isa-bios @pc.bios 0000000000020000-000000000003ffff
      00000000fffc0000-00000000ffffffff (prio 0, rom): pc.bios

address-space: cpu-memory
  0000000000000000-ffffffffffffffff (prio 0, i/o): system
    0000000000000000-000000007fffffff (prio 0, i/o): alias ram-below-4g @pc.ram 0000000000000000-000000007fffffff
    0000000000000000-ffffffffffffffff (prio -1, i/o): pci
      00000000000c0000-00000000000dffff (prio 1, ram): pc.rom
      00000000000e0000-00000000000fffff (prio 1, i/o): alias isa-bios @pc.bios 0000000000020000-000000000003ffff
      00000000fffc0000-00000000ffffffff (prio 0, rom): pc.bios

So we can omit the cpu-memory address space allocated in the address_space_init_shareable(), which will same around 3M physical memory.

Maybe the below patch is much better, please help review, thanks!





-----Original Message-----
From: Peter Maydell [mailto:peter.maydell@linaro.org] 

Sent: Wednesday, March 15, 2017 6:42 PM
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: Zhong, Yang <yang.zhong@intel.com>; QEMU Developers <qemu-devel@nongnu.org>; Xu, Anthony <anthony.xu@intel.com>
Subject: Re: [PATCH v3] memory: reduce heap Rss size around 3M

On 15 March 2017 at 08:12, Paolo Bonzini <pbonzini@redhat.com> wrote:
>

>

> On 15/03/2017 14:39, Yang Zhong wrote:

>> Since cpu-memory and memory have same address space,one malloced 

>> memory is enough. This patch will skip memory malloc for memory 

>> address space,which will reduce around 3M physical memory in heap.

>>

>> Signed-off-by: Yang Zhong <yang.zhong@intel.com>

>> ---

>>  memory.c | 2 +-

>>  1 file changed, 1 insertion(+), 1 deletion(-)

>>

>> diff --git a/memory.c b/memory.c

>> index 64b0a60..0003b1e 100644

>> --- a/memory.c

>> +++ b/memory.c

>> @@ -2422,7 +2422,7 @@ AddressSpace *address_space_init_shareable(MemoryRegion *root, const char *name)

>>      AddressSpace *as;

>>

>>      QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {

>> -        if (root == as->root && as->malloced) {

>> +        if (root == as->root && as == &address_space_memory) {

>

> Of course you need to keep the as->malloced check, don't you?

>

>>              as->ref_count++;

>>              return as;

>>          }

>>

>

> This is not really beautiful, but compared to v1 and v2 it has the 

> advantage that it works (with the as->malloced check reintroduced).

> Peter, you introduced address_space_init_shareable, what do you think?


It looks wrong to me. address_space_memory is not allocated via address_space_init_shareable(), so it's not correct to treat it that way (we're implicitly relying on it never being destroyed). It works by accident, not by design.

If we want to support sharing of address spaces which are constant and exist for the lifetime of QEMU then we should probably do it with a new function something like
    address_space_init_static_shareable()
which marks the AS as being (1) shareable and (2) invalid to ever try to destroy. Then you could use that for both address_space_memory and address_space_io.

thanks
-- PMM

On 15 March 2017 at 12:49, Zhong, Yang <yang.zhong@intel.com> wrote:
> So we can omit the cpu-memory address space allocated in the address_space_init_shareable(), which will same around 3M physical memory.

I see what you want to do...

> Maybe the below patch is much better, please help review, thanks!
>
> diff --git a/memory.c b/memory.c
> index 64b0a60..230f2cb 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -2422,7 +2422,7 @@ AddressSpace *address_space_init_shareable(MemoryRegion *root, const char *name)
>      AddressSpace *as;
>
>      QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
> -        if (root == as->root && as->malloced) {
> +        if (root == as->root && (as->malloced || as == &address_space_memory)) {
>              as->ref_count++;
>              return as;
>          }

...but all the stuff I said below applies to this patch.

> It looks wrong to me. address_space_memory is not allocated via address_space_init_shareable(), so it's not correct to treat it that way (we're implicitly relying on it never being destroyed). It works by accident, not by design.
>
> If we want to support sharing of address spaces which are constant and exist for the lifetime of QEMU then we should probably do it with a new function something like
>     address_space_init_static_shareable()
> which marks the AS as being (1) shareable and (2) invalid to ever try to destroy. Then you could use that for both address_space_memory and address_space_io.

We should not be special casing address_space_memory here:
if you want a mechanism for allowing static "exists for
lifetime of QEMU" address spaces to be included in the
set which can be shared, then you need to create one.

thanks
-- PMM