Re: [Qemu-devel] [PATCH 19/29] exec: pass client mask to cpu_physical_memory_set_dirty_range

[Fam Zheng]

On Mon, 04/27 18:28, Paolo Bonzini wrote:
> This cuts in half the cost of bitmap operations (which will become more
> expensive when made atomic) during migration on non-VRAM regions.
> 
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  exec.c                  | 20 +++++++++++---------
>  include/exec/ram_addr.h | 33 ++++++++++++++++-----------------
>  memory.c                |  3 ++-
>  3 files changed, 29 insertions(+), 27 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 1c75dd1..cd8665a 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1356,7 +1356,8 @@ int qemu_ram_resize(ram_addr_t base, ram_addr_t 
> newsize, Error **errp)
>  
>      cpu_physical_memory_clear_dirty_range(block->offset, block->used_length);
>      block->used_length = newsize;
> -    cpu_physical_memory_set_dirty_range(block->offset, block->used_length);
> +    cpu_physical_memory_set_dirty_range(block->offset, block->used_length,
> +                                        DIRTY_CLIENTS_ALL);
>      memory_region_set_size(block->mr, newsize);
>      if (block->resized) {
>          block->resized(block->idstr, newsize, block->host);
> @@ -1430,7 +1431,8 @@ static ram_addr_t ram_block_add(RAMBlock *new_block, 
> Error **errp)
>         }
>      }
>      cpu_physical_memory_set_dirty_range(new_block->offset,
> -                                        new_block->used_length);
> +                                        new_block->used_length,
> +                                        DIRTY_CLIENTS_ALL);
>  
>      if (new_block->host) {
>          qemu_ram_setup_dump(new_block->host, new_block->max_length);
> @@ -1818,7 +1820,11 @@ static void notdirty_mem_write(void *opaque, hwaddr 
> ram_addr,
>      default:
>          abort();
>      }
> -    cpu_physical_memory_set_dirty_range_nocode(ram_addr, size);
> +    /* Set both VGA and migration bits for simplicity and to remove
> +     * the notdirty callback faster.
> +     */
> +    cpu_physical_memory_set_dirty_range(ram_addr, size,
> +                                        DIRTY_CLIENTS_NOCODE);
>      /* we remove the notdirty callback only if the code has been
>         flushed */
>      if (!cpu_physical_memory_is_clean(ram_addr)) {
> @@ -2238,9 +2244,7 @@ static void invalidate_and_set_dirty(MemoryRegion *mr, 
> hwaddr addr,
>              tb_invalidate_phys_range(addr, addr + length);
>              dirty_log_mask &= ~(1 << DIRTY_MEMORY_CODE);
>          }
> -        if (dirty_log_mask) {
> -            cpu_physical_memory_set_dirty_range_nocode(addr, length);
> -        }
> +        cpu_physical_memory_set_dirty_range(addr, length, dirty_log_mask);
>      } else {
>          xen_modified_memory(addr, length);
>      }
> @@ -2865,9 +2869,7 @@ void stl_phys_notdirty(AddressSpace *as, hwaddr addr, 
> uint32_t val)
>  
>          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);
> -        }
> +        cpu_physical_memory_set_dirty_range(addr1, 4, dirty_log_mask);
>      }
>  }
>  
> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> index 7f6e928..29b89d2 100644
> --- a/include/exec/ram_addr.h
> +++ b/include/exec/ram_addr.h
> @@ -41,6 +41,9 @@ void qemu_ram_free_from_ptr(ram_addr_t addr);
>  
>  int qemu_ram_resize(ram_addr_t base, ram_addr_t newsize, Error **errp);
>  
> +#define DIRTY_CLIENTS_ALL      ((1 << DIRTY_MEMORY_NUM) - 1)
> +#define DIRTY_CLIENTS_NOCODE   (DIRTY_CLIENTS_ALL & ~(1 << 
> DIRTY_MEMORY_CODE))
> +
>  static inline bool cpu_physical_memory_get_dirty(ram_addr_t start,
>                                                   ram_addr_t length,
>                                                   unsigned client)
> @@ -103,28 +106,23 @@ static inline void 
> cpu_physical_memory_set_dirty_flag(ram_addr_t addr,
>      set_bit(addr >> TARGET_PAGE_BITS, ram_list.dirty_memory[client]);
>  }
>  
> -static inline void cpu_physical_memory_set_dirty_range_nocode(ram_addr_t 
> start,
> -                                                              ram_addr_t 
> length)
> -{
> -    unsigned long end, page;
> -
> -    end = TARGET_PAGE_ALIGN(start + length) >> TARGET_PAGE_BITS;
> -    page = start >> TARGET_PAGE_BITS;
> -    bitmap_set(ram_list.dirty_memory[DIRTY_MEMORY_MIGRATION], page, end - 
> page);
> -    bitmap_set(ram_list.dirty_memory[DIRTY_MEMORY_VGA], page, end - page);
> -    xen_modified_memory(start, length);
> -}
> -
>  static inline void cpu_physical_memory_set_dirty_range(ram_addr_t start,
> -                                                       ram_addr_t length)
> +                                                       ram_addr_t length,
> +                                                       uint8_t mask)
>  {
>      unsigned long end, page;
>  
>      end = TARGET_PAGE_ALIGN(start + length) >> TARGET_PAGE_BITS;
>      page = start >> TARGET_PAGE_BITS;
> -    bitmap_set(ram_list.dirty_memory[DIRTY_MEMORY_MIGRATION], page, end - 
> page);
> -    bitmap_set(ram_list.dirty_memory[DIRTY_MEMORY_VGA], page, end - page);
> -    bitmap_set(ram_list.dirty_memory[DIRTY_MEMORY_CODE], page, end - page);
> +    if (likely(mask & (1 << DIRTY_MEMORY_MIGRATION))) {
> +        bitmap_set(ram_list.dirty_memory[DIRTY_MEMORY_MIGRATION], page, end 
> - page);

Wrap the line? Other than that:

Reviewed-by: Fam Zheng <address@hidden>

> +    }
> +    if (unlikely(mask & (1 << DIRTY_MEMORY_VGA))) {
> +        bitmap_set(ram_list.dirty_memory[DIRTY_MEMORY_VGA], page, end - 
> page);
> +    }
> +    if (unlikely(mask & (1 << DIRTY_MEMORY_CODE))) {
> +        bitmap_set(ram_list.dirty_memory[DIRTY_MEMORY_CODE], page, end - 
> page);
> +    }
>      xen_modified_memory(start, length);
>  }
>  
> @@ -172,7 +170,8 @@ static inline void 
> cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
>                      addr = page_number * TARGET_PAGE_SIZE;
>                      ram_addr = start + addr;
>                      cpu_physical_memory_set_dirty_range(ram_addr,
> -                                       TARGET_PAGE_SIZE * hpratio);
> +                                       TARGET_PAGE_SIZE * hpratio,
> +                                       DIRTY_CLIENTS_ALL);
>                  } while (c != 0);
>              }
>          }
> diff --git a/memory.c b/memory.c
> index 174cd15..7422790 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1390,7 +1390,8 @@ void memory_region_set_dirty(MemoryRegion *mr, hwaddr 
> addr,
>                               hwaddr size)
>  {
>      assert(mr->terminates);
> -    cpu_physical_memory_set_dirty_range(mr->ram_addr + addr, size);
> +    cpu_physical_memory_set_dirty_range(mr->ram_addr + addr, size,
> +                                        
> memory_region_get_dirty_log_mask(mr));
>  }
>  
>  bool memory_region_test_and_clear_dirty(MemoryRegion *mr, hwaddr addr,
> -- 
> 1.8.3.1
> 
> 
>