[6/9] ioport: split deletion and destruction

Of the two functions portio_list_del and portio_list_destroy,
the latter is just freeing a memory area.  However, portio_list_del
is the logical equivalent of memory_region_del_subregion so
destruction of memory regions does not belong there.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 ioport.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

On Wed, Jul 30, 2014 at 8:27 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Of the two functions portio_list_del and portio_list_destroy,
> the latter is just freeing a memory area.  However, portio_list_del
> is the logical equivalent of memory_region_del_subregion so
> destruction of memory regions does not belong there.
>

So I found the original implementation made sense to me, in that _del
is the converse of _add and _destroy _init WRT to the MR ops.

Currently
_init = malloc array
_add = mr_init + mr_add_subregion
_del = mr_del_subregion + mr_destroy
_destory = free array

This delays mr_destory to _destroy breaking the symmetry.

With this change is is still possible to:

_init()
_add()
_del()
_add()
_del()
_destrory()

And not leak a ref, with the reinited memory region on second add?

Then again I may be misunderstanding, as apparently neither of these
API have any users so I'm having trouble getting a handle on intended
usage:

$ git grep portio_list_del
include/exec/ioport.h:void portio_list_del(PortioList *piolist);
ioport.c:void portio_list_del(PortioList *piolist)
$ git grep portio_list_destroy
include/exec/ioport.h:void portio_list_destroy(PortioList *piolist);
ioport.c:void portio_list_destroy(PortioList *piolist)

Regards,
Peter

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  ioport.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/ioport.c b/ioport.c
> index 3d91e79..dce94a3 100644
> --- a/ioport.c
> +++ b/ioport.c
> @@ -149,6 +149,14 @@ void portio_list_set_flush_coalesced(PortioList *piolist)
>
>  void portio_list_destroy(PortioList *piolist)
>  {
> +    MemoryRegionPortioList *mrpio;
> +    unsigned i;
> +
> +    for (i = 0; i < piolist->nr; ++i) {
> +        mrpio = container_of(piolist->regions[i], MemoryRegionPortioList, mr);
> +        memory_region_destroy(&mrpio->mr);
> +        g_free(mrpio);
> +    }
>      g_free(piolist->regions);
>  }
>
> @@ -291,8 +299,5 @@ void portio_list_del(PortioList *piolist)
>      for (i = 0; i < piolist->nr; ++i) {
>          mrpio = container_of(piolist->regions[i], MemoryRegionPortioList, mr);
>          memory_region_del_subregion(piolist->address_space, &mrpio->mr);
> -        memory_region_destroy(&mrpio->mr);
> -        g_free(mrpio);
> -        piolist->regions[i] = NULL;
>      }
>  }
> --
> 1.8.3.1
>
>
>

Il 31/07/2014 14:34, Peter Crosthwaite ha scritto:
> So I found the original implementation made sense to me, in that _del
> is the converse of _add and _destroy _init WRT to the MR ops.
> 
> Currently
> _init = malloc array
> _add = mr_init + mr_add_subregion
> _del = mr_del_subregion + mr_destroy
> _destory = free array
> 
> This delays mr_destory to _destroy breaking the symmetry.

Note that there is a fundamental difference between init and destroy:
when you do init, the MemoryRegion should not be providing references to
the MemoryRegion's owner.  So you can do it pretty much whenever you want.

However, after del_subregion the device models can have references to
the MemoryRegion's owner (via address_space_map/unmap) and destroy has
to be delayed to a point when these references have disappeared.  This
is why I'm keen on making memory_region_destroy happen automatically at
finalize time, as I do later in this series: last year I had tried doing
it manually with instance_finalize, and it was an unreviewable mess.

With this patch, you still would need manual portio_list_destroy calls
in instance_finalize, but PortioLists are used sparingly so it's much
simpler to manage them.

This difference is why in this patch I focused only on
portio_list_del/destroy.  However, I can try and move init to
portio_list_add; then the symmetry is restored.


> With this change is is still possible to:
> 
> _init()
> _add()
> _del()
> _add()
> _del()
> _destrory()
> 
> And not leak a ref, with the reinited memory region on second add?
> 
> Then again I may be misunderstanding, as apparently neither of these
> API have any users so I'm having trouble getting a handle on intended
> usage:

Yes, that's because these patches are mostly used by ISA devices or
VGAs, and currently neither is hot-unpluggable.

Paolo