Re: [Qemu-devel] [PATCH 27/29] memory: replace cpu_physical_memory_reset_dirty() with test-and-clear

[Fam Zheng <famz@redhat.com>]

On Mon, 04/27 18:28, Paolo Bonzini wrote:
> From: Stefan Hajnoczi <stefanha@redhat.com>
> 
> The cpu_physical_memory_reset_dirty() function is sometimes used
> together with cpu_physical_memory_get_dirty().  This is not atomic since
> two separate accesses to the dirty memory bitmap are made.
> 
> Turn cpu_physical_memory_reset_dirty() and
> cpu_physical_memory_clear_dirty_range_type() into the atomic
> cpu_physical_memory_test_and_clear_dirty().
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> Message-Id: <1417519399-3166-6-git-send-email-stefanha@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  cputlb.c                |  4 ++--
>  exec.c                  | 23 +++++++++++++++++------
>  include/exec/ram_addr.h | 33 ++++++++++-----------------------
>  memory.c                | 11 ++++-------
>  4 files changed, 33 insertions(+), 38 deletions(-)
> 
> diff --git a/cputlb.c b/cputlb.c
> index a86e76f..75b2dcf 100644
> --- a/cputlb.c
> +++ b/cputlb.c
> @@ -125,8 +125,8 @@ void tlb_flush_page(CPUState *cpu, target_ulong addr)
>     can be detected */
>  void tlb_protect_code(ram_addr_t ram_addr)
>  {
> -    cpu_physical_memory_reset_dirty(ram_addr, TARGET_PAGE_SIZE,
> -                                    DIRTY_MEMORY_CODE);
> +    cpu_physical_memory_test_and_clear_dirty(ram_addr, TARGET_PAGE_SIZE,
> +                                             DIRTY_MEMORY_CODE);
>  }
>  
>  /* update the TLB so that writes in physical page 'phys_addr' are no longer
> diff --git a/exec.c b/exec.c
> index b7aebbf..daed5a7 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -857,16 +857,27 @@ static void tlb_reset_dirty_range_all(ram_addr_t start, ram_addr_t length)
>  }
>  
>  /* Note: start and end must be within the same ram block.  */
> -void cpu_physical_memory_reset_dirty(ram_addr_t start, ram_addr_t length,
> -                                     unsigned client)
> +bool cpu_physical_memory_test_and_clear_dirty(ram_addr_t start,
> +                                              ram_addr_t length,
> +                                              unsigned client)
>  {
> -    if (length == 0)
> -        return;
> -    cpu_physical_memory_clear_dirty_range_type(start, length, client);
> +    unsigned long end, page;
> +    bool dirty;
> +
> +    if (length == 0) {
> +        return false;
> +    }
>  
> -    if (tcg_enabled()) {
> +    end = TARGET_PAGE_ALIGN(start + length) >> TARGET_PAGE_BITS;
> +    page = start >> TARGET_PAGE_BITS;
> +    dirty = bitmap_test_and_clear_atomic(ram_list.dirty_memory[client],
> +                                         page, end - page);
> +
> +    if (dirty && tcg_enabled()) {
>          tlb_reset_dirty_range_all(start, length);
>      }
> +
> +    return dirty;
>  }
>  
>  /* Called from RCU critical section */
> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> index 9bd7827..ea77187 100644
> --- a/include/exec/ram_addr.h
> +++ b/include/exec/ram_addr.h
> @@ -194,30 +194,19 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
>  }
>  #endif /* not _WIN32 */
>  
> -static inline void cpu_physical_memory_clear_dirty_range_type(ram_addr_t start,
> -                                                              ram_addr_t length,
> -                                                              unsigned client)
> -{
> -    unsigned long end, page;
> -
> -    assert(client < DIRTY_MEMORY_NUM);
> -    end = TARGET_PAGE_ALIGN(start + length) >> TARGET_PAGE_BITS;
> -    page = start >> TARGET_PAGE_BITS;
> -    bitmap_clear(ram_list.dirty_memory[client], page, end - page);
> -}
> +bool cpu_physical_memory_test_and_clear_dirty(ram_addr_t start,
> +                                              ram_addr_t length,
> +                                              unsigned client);
>  
>  static inline void cpu_physical_memory_clear_dirty_range(ram_addr_t start,
>                                                           ram_addr_t length)
>  {
> -    cpu_physical_memory_clear_dirty_range_type(start, length, DIRTY_MEMORY_MIGRATION);
> -    cpu_physical_memory_clear_dirty_range_type(start, length, DIRTY_MEMORY_VGA);
> -    cpu_physical_memory_clear_dirty_range_type(start, length, DIRTY_MEMORY_CODE);
> +    cpu_physical_memory_test_and_clear_dirty(start, length, DIRTY_MEMORY_MIGRATION);

Is this line too long? Otherwise,

Reviewed-by: Fam Zheng <famz@redhat.com>

> +    cpu_physical_memory_test_and_clear_dirty(start, length, DIRTY_MEMORY_VGA);
> +    cpu_physical_memory_test_and_clear_dirty(start, length, DIRTY_MEMORY_CODE);
>  }
>  
>  
> -void cpu_physical_memory_reset_dirty(ram_addr_t start, ram_addr_t length,
> -                                     unsigned client);
> -
>  static inline
>  uint64_t cpu_physical_memory_sync_dirty_bitmap(unsigned long *dest,
>                                                 ram_addr_t start,
> @@ -245,12 +234,10 @@ uint64_t cpu_physical_memory_sync_dirty_bitmap(unsigned long *dest,
>          }
>      } else {
>          for (addr = 0; addr < length; addr += TARGET_PAGE_SIZE) {
> -            if (cpu_physical_memory_get_dirty(start + addr,
> -                                              TARGET_PAGE_SIZE,
> -                                              DIRTY_MEMORY_MIGRATION)) {
> -                cpu_physical_memory_reset_dirty(start + addr,
> -                                                TARGET_PAGE_SIZE,
> -                                                DIRTY_MEMORY_MIGRATION);
> +            if (cpu_physical_memory_test_and_clear_dirty(
> +                        start + addr,
> +                        TARGET_PAGE_SIZE,
> +                        DIRTY_MEMORY_MIGRATION)) {
>                  long k = (start + addr) >> TARGET_PAGE_BITS;
>                  if (!test_and_set_bit(k, dest)) {
>                      num_dirty++;
> diff --git a/memory.c b/memory.c
> index 7422790..bb86b4b 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1397,13 +1397,9 @@ void memory_region_set_dirty(MemoryRegion *mr, hwaddr addr,
>  bool memory_region_test_and_clear_dirty(MemoryRegion *mr, hwaddr addr,
>                                          hwaddr size, unsigned client)
>  {
> -    bool ret;
>      assert(mr->terminates);
> -    ret = cpu_physical_memory_get_dirty(mr->ram_addr + addr, size, client);
> -    if (ret) {
> -        cpu_physical_memory_reset_dirty(mr->ram_addr + addr, size, client);
> -    }
> -    return ret;
> +    return cpu_physical_memory_test_and_clear_dirty(mr->ram_addr + addr,
> +                                                    size, client);
>  }
>  
>  
> @@ -1447,7 +1443,8 @@ void memory_region_reset_dirty(MemoryRegion *mr, hwaddr addr,
>                                 hwaddr size, unsigned client)
>  {
>      assert(mr->terminates);
> -    cpu_physical_memory_reset_dirty(mr->ram_addr + addr, size, client);
> +    cpu_physical_memory_test_and_clear_dirty(mr->ram_addr + addr, size,
> +                                             client);
>  }
>  
>  int memory_region_get_fd(MemoryRegion *mr)
> -- 
> 1.8.3.1
> 
> 
>