Re: [Qemu-devel] [PATCH 14/29] exec: use memory_region_get_dirty_log_mask to optimize dirty tracking

[Fam Zheng <famz@redhat.com>]

On Mon, 04/27 18:28, Paolo Bonzini wrote:
> The memory API can now return the exact set of bitmaps that have to
> be tracked.  Use it instead of the in_migration variable.
> 
> In the next patches, we will also use it to set only DIRTY_MEMORY_VGA
> or DIRTY_MEMORY_MIGRATION if necessary.  This can make a difference
> for dataplane, especially after the dirty bitmap is changed to use
> more expensive atomic operations.
> 
> Of some interest is the change to stl_phys_notdirty.  When migration
> was introduced, stl_phys_notdirty was changed to effectively behave
> as stl_phys during migration.  In fact, if one looks at the function as it
> was in the beginning (commit 8df1cd0, physical memory access functions,
> 2005-01-28), at the time the dirty bitmap was the equivalent of
> DIRTY_MEMORY_CODE nowadays; hence, the function simply should not touch
> the dirty code bits.  This patch changes it to do the intended thing.

There are three changes in this patch:

 1) Removal of core_memory_listener;
 2) Test of dirty log mask bits in invalidate_and_set_dirty;
 3) Test of dirty log mask bits in stl_phys_notdirty.

1) and 3) are connected by in_migration, so they belong to the same patch. But
I'm not sure about 2). Is it required by 1) and 3), or it's changed because it
also touches the condition of tb_invalidate_phys_range?

> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  exec.c | 59 +++++++++++++++++++----------------------------------------
>  1 file changed, 19 insertions(+), 40 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 083d65d..6e83161 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -59,8 +59,6 @@
>  //#define DEBUG_SUBPAGE
>  
>  #if !defined(CONFIG_USER_ONLY)
> -static bool in_migration;
> -
>  /* ram_list is read under rcu_read_lock()/rcu_read_unlock().  Writes
>   * are protected by the ramlist lock.
>   */
> @@ -871,11 +869,6 @@ void cpu_physical_memory_reset_dirty(ram_addr_t start, ram_addr_t length,
>      }
>  }
>  
> -static void cpu_physical_memory_set_dirty_tracking(bool enable)
> -{
> -    in_migration = enable;
> -}
> -
>  /* Called from RCU critical section */
>  hwaddr memory_region_section_get_iotlb(CPUState *cpu,
>                                         MemoryRegionSection *section,
> @@ -2140,22 +2133,6 @@ static void tcg_commit(MemoryListener *listener)
>      }
>  }
>  
> -static void core_log_global_start(MemoryListener *listener)
> -{
> -    cpu_physical_memory_set_dirty_tracking(true);
> -}
> -
> -static void core_log_global_stop(MemoryListener *listener)
> -{
> -    cpu_physical_memory_set_dirty_tracking(false);
> -}
> -
> -static MemoryListener core_memory_listener = {
> -    .log_global_start = core_log_global_start,
> -    .log_global_stop = core_log_global_stop,
> -    .priority = 1,
> -};
> -
>  void address_space_init_dispatch(AddressSpace *as)
>  {
>      as->dispatch = NULL;
> @@ -2195,8 +2172,6 @@ static void memory_map_init(void)
>      memory_region_init_io(system_io, NULL, &unassigned_io_ops, NULL, "io",
>                            65536);
>      address_space_init(&address_space_io, system_io, "I/O");
> -
> -    memory_listener_register(&core_memory_listener, &address_space_memory);
>  }
>  
>  MemoryRegion *get_system_memory(void)
> @@ -2254,12 +2229,18 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
>  
>  #else
>  
> -static void invalidate_and_set_dirty(hwaddr addr,
> +static void invalidate_and_set_dirty(MemoryRegion *mr, hwaddr addr,
>                                       hwaddr length)
>  {
>      if (cpu_physical_memory_range_includes_clean(addr, length)) {
> -        tb_invalidate_phys_range(addr, addr + length, 0);
> -        cpu_physical_memory_set_dirty_range_nocode(addr, length);
> +        uint8_t dirty_log_mask = memory_region_get_dirty_log_mask(mr);
> +        if (dirty_log_mask & (1 << DIRTY_MEMORY_CODE)) {
> +            tb_invalidate_phys_range(addr, addr + length, 0);
> +            dirty_log_mask &= ~(1 << DIRTY_MEMORY_CODE);
> +        }
> +        if (dirty_log_mask) {
> +            cpu_physical_memory_set_dirty_range_nocode(addr, length);
> +        }
>      } else {
>          xen_modified_memory(addr, length);
>      }
> @@ -2342,7 +2323,7 @@ bool address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
>                  /* RAM case */
>                  ptr = qemu_get_ram_ptr(addr1);
>                  memcpy(ptr, buf, l);
> -                invalidate_and_set_dirty(addr1, l);
> +                invalidate_and_set_dirty(mr, addr1, l);
>              }
>          } else {
>              if (!memory_access_is_direct(mr, is_write)) {
> @@ -2431,7 +2412,7 @@ static inline void cpu_physical_memory_write_rom_internal(AddressSpace *as,
>              switch (type) {
>              case WRITE_DATA:
>                  memcpy(ptr, buf, l);
> -                invalidate_and_set_dirty(addr1, l);
> +                invalidate_and_set_dirty(mr, addr1, l);
>                  break;
>              case FLUSH_CACHE:
>                  flush_icache_range((uintptr_t)ptr, (uintptr_t)ptr + l);
> @@ -2647,7 +2628,7 @@ void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
>          mr = qemu_ram_addr_from_host(buffer, &addr1);
>          assert(mr != NULL);
>          if (is_write) {
> -            invalidate_and_set_dirty(addr1, access_len);
> +            invalidate_and_set_dirty(mr, addr1, access_len);
>          }
>          if (xen_enabled()) {
>              xen_invalidate_map_cache_entry(buffer);
> @@ -2871,6 +2852,7 @@ void stl_phys_notdirty(AddressSpace *as, hwaddr addr, uint32_t val)
>      MemoryRegion *mr;
>      hwaddr l = 4;
>      hwaddr addr1;
> +    uint8_t dirty_log_mask;
>  
>      mr = address_space_translate(as, addr, &addr1, &l,
>                                   true);
> @@ -2881,13 +2863,10 @@ void stl_phys_notdirty(AddressSpace *as, hwaddr addr, uint32_t val)
>          ptr = qemu_get_ram_ptr(addr1);
>          stl_p(ptr, val);
>  
> -        if (unlikely(in_migration)) {
> -            if (cpu_physical_memory_is_clean(addr1)) {
> -                /* invalidate code */
> -                tb_invalidate_phys_page_range(addr1, addr1 + 4, 0);
> -                /* set dirty bit */
> -                cpu_physical_memory_set_dirty_range_nocode(addr1, 4);
> -            }
> +        dirty_log_mask = memory_region_get_dirty_log_mask(mr);
> +        dirty_log_mask &= ~(1 << DIRTY_MEMORY_CODE);
> +        if (dirty_log_mask) {
> +            cpu_physical_memory_set_dirty_range_nocode(addr1, 4);

Looks OK.

A side question, it seems cpu_physical_memory_is_clean returns true if *any* of
three bitmaps is clean:

    static inline bool cpu_physical_memory_is_clean(ram_addr_t addr)
    {
        bool vga = cpu_physical_memory_get_dirty_flag(addr, DIRTY_MEMORY_VGA);
        bool code = cpu_physical_memory_get_dirty_flag(addr, DIRTY_MEMORY_CODE);
        bool migration =
            cpu_physical_memory_get_dirty_flag(addr, DIRTY_MEMORY_MIGRATION);
->      return !(vga && code && migration);
    }

It's counter-intuitive. Why is that?

Fam

>          }
>      }
>  }
> @@ -2930,7 +2909,7 @@ static inline void stl_phys_internal(AddressSpace *as,
>              stl_p(ptr, val);
>              break;
>          }
> -        invalidate_and_set_dirty(addr1, 4);
> +        invalidate_and_set_dirty(mr, addr1, 4);
>      }
>  }
>  
> @@ -2993,7 +2972,7 @@ static inline void stw_phys_internal(AddressSpace *as,
>              stw_p(ptr, val);
>              break;
>          }
> -        invalidate_and_set_dirty(addr1, 2);
> +        invalidate_and_set_dirty(mr, addr1, 2);
>      }
>  }
>  
> -- 
> 1.8.3.1
> 
>