Patchwork [v2,03/11] exec: simplify destruction of the phys map

login
register
mail settings
Submitter Paolo Bonzini
Date June 28, 2013, 4:58 p.m.
Message ID <1372438702-20491-4-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/255481/
State New
Headers show

Comments

Paolo Bonzini - June 28, 2013, 4:58 p.m.
Do not bother visiting the radix tree when an address space is destroyed.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 exec.c | 34 +---------------------------------
 1 file changed, 1 insertion(+), 33 deletions(-)
Jan Kiszka - July 1, 2013, 2:23 p.m.
On 2013-06-28 18:58, Paolo Bonzini wrote:
> Do not bother visiting the radix tree when an address space is destroyed.

The "because" is missing, see my comment on the previous patch. If you
can clarify this, the rest is fine.

Jan

> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  exec.c | 34 +---------------------------------
>  1 file changed, 1 insertion(+), 33 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index e7eadf5..9c77285 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -154,12 +154,6 @@ static uint16_t phys_map_node_alloc(void)
>      return ret;
>  }
>  
> -static void phys_map_nodes_reset(void)
> -{
> -    phys_map_nodes_nb = 0;
> -}
> -
> -
>  static void phys_page_set_level(PhysPageEntry *lp, hwaddr *index,
>                                  hwaddr *nb, uint16_t leaf,
>                                  int level)
> @@ -763,31 +757,6 @@ static int subpage_register (subpage_t *mmio, uint32_t start, uint32_t end,
>                               uint16_t section);
>  static subpage_t *subpage_init(AddressSpace *as, hwaddr base);
>  
> -static void destroy_l2_mapping(PhysPageEntry *lp, unsigned level)
> -{
> -    unsigned i;
> -    PhysPageEntry *p;
> -
> -    if (lp->ptr == PHYS_MAP_NODE_NIL) {
> -        return;
> -    }
> -
> -    p = phys_map_nodes[lp->ptr];
> -    for (i = 0; i < L2_SIZE; ++i) {
> -        if (!p[i].is_leaf) {
> -            destroy_l2_mapping(&p[i], level - 1);
> -        }
> -    }
> -    lp->is_leaf = 0;
> -    lp->ptr = PHYS_MAP_NODE_NIL;
> -}
> -
> -static void destroy_all_mappings(AddressSpaceDispatch *d)
> -{
> -    destroy_l2_mapping(&d->phys_map, P_L2_LEVELS - 1);
> -    phys_map_nodes_reset();
> -}
> -
>  static uint16_t phys_section_add(MemoryRegionSection *section)
>  {
>      /* The physical section number is ORed with a page-aligned
> @@ -820,6 +789,7 @@ static void phys_sections_clear(void)
>          MemoryRegionSection *section = &phys_sections[--phys_sections_nb];
>          phys_section_destroy(section->mr);
>      }
> +    phys_map_nodes_nb = 0;
>  }
>  
>  static void register_subpage(AddressSpaceDispatch *d, MemoryRegionSection *section)
> @@ -1724,7 +1694,6 @@ static void mem_begin(MemoryListener *listener)
>  {
>      AddressSpaceDispatch *d = container_of(listener, AddressSpaceDispatch, listener);
>  
> -    destroy_all_mappings(d);
>      d->phys_map.ptr = PHYS_MAP_NODE_NIL;
>  }
>  
> @@ -1791,7 +1760,6 @@ void address_space_destroy_dispatch(AddressSpace *as)
>      AddressSpaceDispatch *d = as->dispatch;
>  
>      memory_listener_unregister(&d->listener);
> -    destroy_l2_mapping(&d->phys_map, P_L2_LEVELS - 1);
>      g_free(d);
>      as->dispatch = NULL;
>  }
>
Paolo Bonzini - July 1, 2013, 2:33 p.m.
Il 01/07/2013 16:23, Jan Kiszka ha scritto:
> On 2013-06-28 18:58, Paolo Bonzini wrote:
>> Do not bother visiting the radix tree when an address space is destroyed.
> 
> The "because" is missing, see my comment on the previous patch. If you
> can clarify this, the rest is fine.

After the previous patch, the "because" is that this is has become a
pointless exercise: all you're doing is zeroing out a structure that
will be freed as soon as you come back.

I'll add the justification to both patches.

Paolo

> Jan
> 
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  exec.c | 34 +---------------------------------
>>  1 file changed, 1 insertion(+), 33 deletions(-)
>>
>> diff --git a/exec.c b/exec.c
>> index e7eadf5..9c77285 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -154,12 +154,6 @@ static uint16_t phys_map_node_alloc(void)
>>      return ret;
>>  }
>>  
>> -static void phys_map_nodes_reset(void)
>> -{
>> -    phys_map_nodes_nb = 0;
>> -}
>> -
>> -
>>  static void phys_page_set_level(PhysPageEntry *lp, hwaddr *index,
>>                                  hwaddr *nb, uint16_t leaf,
>>                                  int level)
>> @@ -763,31 +757,6 @@ static int subpage_register (subpage_t *mmio, uint32_t start, uint32_t end,
>>                               uint16_t section);
>>  static subpage_t *subpage_init(AddressSpace *as, hwaddr base);
>>  
>> -static void destroy_l2_mapping(PhysPageEntry *lp, unsigned level)
>> -{
>> -    unsigned i;
>> -    PhysPageEntry *p;
>> -
>> -    if (lp->ptr == PHYS_MAP_NODE_NIL) {
>> -        return;
>> -    }
>> -
>> -    p = phys_map_nodes[lp->ptr];
>> -    for (i = 0; i < L2_SIZE; ++i) {
>> -        if (!p[i].is_leaf) {
>> -            destroy_l2_mapping(&p[i], level - 1);
>> -        }
>> -    }
>> -    lp->is_leaf = 0;
>> -    lp->ptr = PHYS_MAP_NODE_NIL;
>> -}
>> -
>> -static void destroy_all_mappings(AddressSpaceDispatch *d)
>> -{
>> -    destroy_l2_mapping(&d->phys_map, P_L2_LEVELS - 1);
>> -    phys_map_nodes_reset();
>> -}
>> -
>>  static uint16_t phys_section_add(MemoryRegionSection *section)
>>  {
>>      /* The physical section number is ORed with a page-aligned
>> @@ -820,6 +789,7 @@ static void phys_sections_clear(void)
>>          MemoryRegionSection *section = &phys_sections[--phys_sections_nb];
>>          phys_section_destroy(section->mr);
>>      }
>> +    phys_map_nodes_nb = 0;
>>  }
>>  
>>  static void register_subpage(AddressSpaceDispatch *d, MemoryRegionSection *section)
>> @@ -1724,7 +1694,6 @@ static void mem_begin(MemoryListener *listener)
>>  {
>>      AddressSpaceDispatch *d = container_of(listener, AddressSpaceDispatch, listener);
>>  
>> -    destroy_all_mappings(d);
>>      d->phys_map.ptr = PHYS_MAP_NODE_NIL;
>>  }
>>  
>> @@ -1791,7 +1760,6 @@ void address_space_destroy_dispatch(AddressSpace *as)
>>      AddressSpaceDispatch *d = as->dispatch;
>>  
>>      memory_listener_unregister(&d->listener);
>> -    destroy_l2_mapping(&d->phys_map, P_L2_LEVELS - 1);
>>      g_free(d);
>>      as->dispatch = NULL;
>>  }
>>
>

Patch

diff --git a/exec.c b/exec.c
index e7eadf5..9c77285 100644
--- a/exec.c
+++ b/exec.c
@@ -154,12 +154,6 @@  static uint16_t phys_map_node_alloc(void)
     return ret;
 }
 
-static void phys_map_nodes_reset(void)
-{
-    phys_map_nodes_nb = 0;
-}
-
-
 static void phys_page_set_level(PhysPageEntry *lp, hwaddr *index,
                                 hwaddr *nb, uint16_t leaf,
                                 int level)
@@ -763,31 +757,6 @@  static int subpage_register (subpage_t *mmio, uint32_t start, uint32_t end,
                              uint16_t section);
 static subpage_t *subpage_init(AddressSpace *as, hwaddr base);
 
-static void destroy_l2_mapping(PhysPageEntry *lp, unsigned level)
-{
-    unsigned i;
-    PhysPageEntry *p;
-
-    if (lp->ptr == PHYS_MAP_NODE_NIL) {
-        return;
-    }
-
-    p = phys_map_nodes[lp->ptr];
-    for (i = 0; i < L2_SIZE; ++i) {
-        if (!p[i].is_leaf) {
-            destroy_l2_mapping(&p[i], level - 1);
-        }
-    }
-    lp->is_leaf = 0;
-    lp->ptr = PHYS_MAP_NODE_NIL;
-}
-
-static void destroy_all_mappings(AddressSpaceDispatch *d)
-{
-    destroy_l2_mapping(&d->phys_map, P_L2_LEVELS - 1);
-    phys_map_nodes_reset();
-}
-
 static uint16_t phys_section_add(MemoryRegionSection *section)
 {
     /* The physical section number is ORed with a page-aligned
@@ -820,6 +789,7 @@  static void phys_sections_clear(void)
         MemoryRegionSection *section = &phys_sections[--phys_sections_nb];
         phys_section_destroy(section->mr);
     }
+    phys_map_nodes_nb = 0;
 }
 
 static void register_subpage(AddressSpaceDispatch *d, MemoryRegionSection *section)
@@ -1724,7 +1694,6 @@  static void mem_begin(MemoryListener *listener)
 {
     AddressSpaceDispatch *d = container_of(listener, AddressSpaceDispatch, listener);
 
-    destroy_all_mappings(d);
     d->phys_map.ptr = PHYS_MAP_NODE_NIL;
 }
 
@@ -1791,7 +1760,6 @@  void address_space_destroy_dispatch(AddressSpace *as)
     AddressSpaceDispatch *d = as->dispatch;
 
     memory_listener_unregister(&d->listener);
-    destroy_l2_mapping(&d->phys_map, P_L2_LEVELS - 1);
     g_free(d);
     as->dispatch = NULL;
 }