Re: [Qemu-devel] [PATCH 14/22] exec: only check relevant bitmaps for cleanliness

[Fam Zheng <famz@redhat.com>]

On Thu, 03/26 18:38, Paolo Bonzini wrote:
> Most of the time, not all bitmaps have to be marked as dirty;
> do not do anything if the interesting ones are already dirty.
> Previously, any clean bitmap would have cause all the bitmaps to be
> marked dirty.
> 
> In fact, unless running TCG most of the time bitmap operations
> are not done at all because memory_region_is_logging returns
> zero.  In this case, skip cpu_physical_memory_range_includes_clean
> as well.
> 
> With this patch, cpu_physical_memory_set_dirty_range is called
> unconditionally, so there need not be anymore a separate call to
> xen_modified_memory.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  exec.c                  | 18 +++++++++---------
>  include/exec/ram_addr.h | 19 +++++++++++++------
>  2 files changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 94b7644..83c58ff 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2236,16 +2236,16 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
>  static void invalidate_and_set_dirty(MemoryRegion *mr, hwaddr addr,
>                                       hwaddr length)
>  {
> -    if (cpu_physical_memory_range_includes_clean(addr, length)) {
> -        uint8_t dirty_log_mask = memory_region_is_logging(mr);
> -        if (dirty_log_mask & (1 << DIRTY_MEMORY_CODE)) {
> -            tb_invalidate_phys_range(addr, addr + length, 0);
> -            dirty_log_mask &= ~(1 << DIRTY_MEMORY_CODE);
> -        }
> -        cpu_physical_memory_set_dirty_range(addr, length, dirty_log_mask);
> -    } else {
> -        xen_modified_memory(addr, length);
> +    uint8_t dirty_log_mask = memory_region_is_logging(mr);
> +    if (dirty_log_mask &&
> +        !cpu_physical_memory_range_includes_clean(addr, length, dirty_log_mask)) {
> +        dirty_log_mask = 0;
> +    }
> +    if (dirty_log_mask & (1 << DIRTY_MEMORY_CODE)) {
> +        tb_invalidate_phys_range(addr, addr + length, 0);
> +        dirty_log_mask &= ~(1 << DIRTY_MEMORY_CODE);
>      }
> +    cpu_physical_memory_set_dirty_range(addr, length, dirty_log_mask);
>  }
>  
>  static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> index 6c3b74e..b408408 100644
> --- a/include/exec/ram_addr.h
> +++ b/include/exec/ram_addr.h
> @@ -90,13 +90,20 @@ static inline bool cpu_physical_memory_is_clean(ram_addr_t addr)
>  }
>  
>  static inline bool cpu_physical_memory_range_includes_clean(ram_addr_t start,
> -                                                            ram_addr_t length)
> +                                                            ram_addr_t length,
> +                                                            uint8_t mask)
>  {
> -    bool vga = cpu_physical_memory_get_clean(start, length, DIRTY_MEMORY_VGA);
> -    bool code = cpu_physical_memory_get_clean(start, length, DIRTY_MEMORY_CODE);
> -    bool migration =
> -        cpu_physical_memory_get_clean(start, length, DIRTY_MEMORY_MIGRATION);
> -    return vga || code || migration;
> +    bool clean = false;
> +    if (mask & (1 << DIRTY_MEMORY_VGA)) {
> +        clean = cpu_physical_memory_get_clean(start, length, DIRTY_MEMORY_VGA);
> +    }
> +    if (!clean && (mask & (1 << DIRTY_MEMORY_CODE))) {
> +        clean = cpu_physical_memory_get_clean(start, length, DIRTY_MEMORY_CODE);
> +    }
> +    if (!clean && (mask & (1 << DIRTY_MEMORY_MIGRATION))) {
> +        clean = cpu_physical_memory_get_clean(start, length, DIRTY_MEMORY_MIGRATION);
> +    }
> +    return clean;
>  }

Out of curiosity, is it valid that a mask bit is cleared but the corresponding
dirty bit is set?

>  
>  static inline void cpu_physical_memory_set_dirty_flag(ram_addr_t addr,
> -- 
> 2.3.3
> 
> 
>