[11/16] memory: Add address_space_init_shareable()

From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

This will either create a new AS or return a pointer to an
already existing equivalent one, if we have already created
an AS for the specified root memory region.

The motivation is to reuse address spaces as much as possible.
It's going to be quite common that bus masters out in device land
have pointers to the same memory region for their mastering yet
each will need to create its own address space. Let the memory
API implement sharing for them.

Aside from the perf optimisations, this should reduce the amount
of redundant output on info mtree as well.

Thee returned value will be malloced, but the malloc will be
automatically freed when the AS runs out of refs.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
[PMM: dropped check for NULL root as unused; added doc-comment;
 squashed Peter C's reference-counting patch into this one;
 don't compare name string when deciding if we can share ASes;
 read as->malloced before the unref of as->root to avoid possible
 read-after-free if as->root was the owner of as]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/exec/memory.h | 18 ++++++++++++++++++
 memory.c              | 27 +++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)

On Thu, Nov 05, 2015 at 06:15:53PM +0000, Peter Maydell wrote:
> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> 
> This will either create a new AS or return a pointer to an
> already existing equivalent one, if we have already created
> an AS for the specified root memory region.
> 
> The motivation is to reuse address spaces as much as possible.
> It's going to be quite common that bus masters out in device land
> have pointers to the same memory region for their mastering yet
> each will need to create its own address space. Let the memory
> API implement sharing for them.
> 
> Aside from the perf optimisations, this should reduce the amount
> of redundant output on info mtree as well.
> 
> Thee returned value will be malloced, but the malloc will be
> automatically freed when the AS runs out of refs.
> 
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> [PMM: dropped check for NULL root as unused; added doc-comment;
>  squashed Peter C's reference-counting patch into this one;
>  don't compare name string when deciding if we can share ASes;
>  read as->malloced before the unref of as->root to avoid possible
>  read-after-free if as->root was the owner of as]
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/exec/memory.h | 18 ++++++++++++++++++
>  memory.c              | 27 +++++++++++++++++++++++++++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index e5d98f8..1d1740a 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -236,6 +236,8 @@ struct AddressSpace {
>      struct rcu_head rcu;
>      char *name;
>      MemoryRegion *root;
> +    int ref_count;
> +    bool malloced;
>  
>      /* Accessed via RCU.  */
>      struct FlatView *current_map;
> @@ -1167,6 +1169,22 @@ MemTxResult memory_region_dispatch_write(MemoryRegion *mr,
>   */
>  void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name);
>  
> +/**
> + * address_space_init_shareable: return an address space for a memory region,
> + *                               creating it if it does not already exist
> + *
> + * @root: a #MemoryRegion that routes addresses for the address space
> + * @name: an address space name.  The name is only used for debugging
> + *        output.
> + *
> + * This function will return a pointer to an existing AddressSpace
> + * which was initialized with the specified MemoryRegion, or it will
> + * create and initialize one if it does not already exist. The ASes
> + * are reference-counted, so the memory will be freed automatically
> + * when the AddressSpace is destroyed via address_space_destroy.
> + */
> +AddressSpace *address_space_init_shareable(MemoryRegion *root,
> +                                           const char *name);
>  
>  /**
>   * address_space_destroy: destroy an address space
> diff --git a/memory.c b/memory.c
> index c435c88..6c98a71 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -2101,7 +2101,9 @@ void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name)
>  {
>      memory_region_ref(root);
>      memory_region_transaction_begin();
> +    as->ref_count = 1;
>      as->root = root;
> +    as->malloced = false;
>      as->current_map = g_new(FlatView, 1);
>      flatview_init(as->current_map);
>      as->ioeventfd_nb = 0;
> @@ -2116,6 +2118,7 @@ void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name)
>  static void do_address_space_destroy(AddressSpace *as)
>  {
>      MemoryListener *listener;
> +    bool do_free = as->malloced;
>  
>      address_space_destroy_dispatch(as);
>  
> @@ -2127,12 +2130,36 @@ static void do_address_space_destroy(AddressSpace *as)
>      g_free(as->name);
>      g_free(as->ioeventfds);
>      memory_region_unref(as->root);
> +    if (do_free) {
> +        g_free(as);
> +    }
> +}
> +
> +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->ref_count++;
> +            return as;
> +        }
> +    }
> +
> +    as = g_malloc0(sizeof *as);
> +    address_space_init(as, root, name);

Nit-pick but IIUC, address_space_init does not need AS to be zeroed
so this could be changed to a g_malloc().

either-way:

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>



> +    as->malloced = true;
> +    return as;
>  }
>  
>  void address_space_destroy(AddressSpace *as)
>  {
>      MemoryRegion *root = as->root;
>  
> +    as->ref_count--;
> +    if (as->ref_count) {
> +        return;
> +    }
>      /* Flush out anything from MemoryListeners listening in on this */
>      memory_region_transaction_begin();
>      as->root = NULL;
> -- 
> 1.9.1
>

On 6 November 2015 at 14:29, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> On Thu, Nov 05, 2015 at 06:15:53PM +0000, Peter Maydell wrote:

>> +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->ref_count++;
>> +            return as;
>> +        }
>> +    }
>> +
>> +    as = g_malloc0(sizeof *as);
>> +    address_space_init(as, root, name);
>
> Nit-pick but IIUC, address_space_init does not need AS to be zeroed
> so this could be changed to a g_malloc().

I tend to prefer to be conservative about these things,
so I use zero-allocation unless I'm really sure it's
unnecessary.

> either-way:
>
> Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

thanks
-- PMM

On 05/11/2015 19:15, Peter Maydell wrote:
> +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->ref_count++;
> +            return as;
> +        }
> +    }
> +
> +    as = g_malloc0(sizeof *as);
> +    address_space_init(as, root, name);
> +    as->malloced = true;
> +    return as;
>  }

You shouldn't return a non-shareable address space here, I think,
because it might be contained into another object and that object might
disappear.  I haven't thought this through very much, but adding an " &&
as->malloced" to the conditional seems easy and safe.

Paolo

>  
>  void address_space_destroy(AddressSpace *as)
>  {
>      MemoryRegion *root = as->root;
>  
> +    as->ref_count--;
> +    if (as->ref_count) {
> +        return;
> +    }
>      /* Flush out anything from MemoryListeners listening in on this */
>      memory_region_transaction_begin();
>      as->root = NULL;
>

On 9 November 2015 at 10:55, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 05/11/2015 19:15, Peter Maydell wrote:
>> +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->ref_count++;
>> +            return as;
>> +        }
>> +    }
>> +
>> +    as = g_malloc0(sizeof *as);
>> +    address_space_init(as, root, name);
>> +    as->malloced = true;
>> +    return as;
>>  }
>
> You shouldn't return a non-shareable address space here, I think,
> because it might be contained into another object and that object might
> disappear.  I haven't thought this through very much, but adding an " &&
> as->malloced" to the conditional seems easy and safe.

That would prevent sharing the system address space, which is one
you really commonly want to share. I guess we could make that one
be alloced-shareable.

thanks
-- PMM

On 09/11/2015 11:59, Peter Maydell wrote:
>>> >> +    as = g_malloc0(sizeof *as);
>>> >> +    address_space_init(as, root, name);
>>> >> +    as->malloced = true;
>>> >> +    return as;
>>> >>  }
>> >
>> > You shouldn't return a non-shareable address space here, I think,
>> > because it might be contained into another object and that object might
>> > disappear.  I haven't thought this through very much, but adding an " &&
>> > as->malloced" to the conditional seems easy and safe.
> That would prevent sharing the system address space, which is one
> you really commonly want to share. I guess we could make that one
> be alloced-shareable.

You would only allocate one duplicate of the system address space
though.  The second call to address_space_init_shareable would not
create a new AS.

Paolo