Re: [Qemu-devel] [PATCH v3 07/12] memory: Introduce memory listener hook log_clear()

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Peter Xu (peterx@redhat.com) wrote:
> Introduce a new memory region listener hook log_clear() to allow the
> listeners to hook onto the points where the dirty bitmap is cleared by
> the bitmap users.
> 
> Previously log_sync() contains two operations:
> 
>   - dirty bitmap collection, and,
>   - dirty bitmap clear on remote site.
> 
> Let's take KVM as example - log_sync() for KVM will first copy the
> kernel dirty bitmap to userspace, and at the same time we'll clear the
> dirty bitmap there along with re-protecting all the guest pages again.
> 
> We add this new log_clear() interface only to split the old log_sync()
> into two separated procedures:
> 
>   - use log_sync() to collect the collection only, and,
>   - use log_clear() to clear the remote dirty bitmap.
> 
> With the new interface, the memory listener users will still be able
> to decide how to implement the log synchronization procedure, e.g.,
> they can still only provide log_sync() method only and put all the two
> procedures within log_sync() (that's how the old KVM works before
> KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2 is introduced).  However with this
> new interface the memory listener users will start to have a chance to
> postpone the log clear operation explicitly if the module supports.
> That can really benefit users like KVM at least for host kernels that
> support KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2.
> 
> There are three places that can clear dirty bits in any one of the
> dirty bitmap in the ram_list.dirty_memory[3] array:
> 
>         cpu_physical_memory_snapshot_and_clear_dirty
>         cpu_physical_memory_test_and_clear_dirty
>         cpu_physical_memory_sync_dirty_bitmap
> 
> Currently we hook directly into each of the functions to notify about
> the log_clear().
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  exec.c                  | 12 ++++++++++
>  include/exec/memory.h   | 17 ++++++++++++++
>  include/exec/ram_addr.h |  3 +++
>  memory.c                | 51 +++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 83 insertions(+)
> 
> diff --git a/exec.c b/exec.c
> index 2615b4cfed..ab595e1e4b 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1355,6 +1355,8 @@ bool cpu_physical_memory_test_and_clear_dirty(ram_addr_t start,
>      DirtyMemoryBlocks *blocks;
>      unsigned long end, page;
>      bool dirty = false;
> +    RAMBlock *ramblock;
> +    uint64_t mr_offset, mr_size;
>  
>      if (length == 0) {
>          return false;
> @@ -1366,6 +1368,10 @@ bool cpu_physical_memory_test_and_clear_dirty(ram_addr_t start,
>      rcu_read_lock();
>  
>      blocks = atomic_rcu_read(&ram_list.dirty_memory[client]);
> +    ramblock = qemu_get_ram_block(start);
> +    /* Range sanity check on the ramblock */
> +    assert(start >= ramblock->offset &&
> +           start + length <= ramblock->offset + ramblock->used_length);
>  
>      while (page < end) {
>          unsigned long idx = page / DIRTY_MEMORY_BLOCK_SIZE;
> @@ -1377,6 +1383,10 @@ bool cpu_physical_memory_test_and_clear_dirty(ram_addr_t start,
>          page += num;
>      }
>  
> +    mr_offset = (ram_addr_t)(page << TARGET_PAGE_BITS) - ramblock->offset;
> +    mr_size = (end - page) << TARGET_PAGE_BITS;
> +    memory_region_clear_dirty_bitmap(ramblock->mr, mr_offset, mr_size);
> +
>      rcu_read_unlock();
>  
>      if (dirty && tcg_enabled()) {
> @@ -1432,6 +1442,8 @@ DirtyBitmapSnapshot *cpu_physical_memory_snapshot_and_clear_dirty
>          tlb_reset_dirty_range_all(start, length);
>      }
>  
> +    memory_region_clear_dirty_bitmap(mr, addr, length);
> +
>      return snap;
>  }
>  
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index f29300c54d..d752b2a758 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -416,6 +416,7 @@ struct MemoryListener {
>      void (*log_stop)(MemoryListener *listener, MemoryRegionSection *section,
>                       int old, int new);
>      void (*log_sync)(MemoryListener *listener, MemoryRegionSection *section);
> +    void (*log_clear)(MemoryListener *listener, MemoryRegionSection *section);
>      void (*log_global_start)(MemoryListener *listener);
>      void (*log_global_stop)(MemoryListener *listener);
>      void (*eventfd_add)(MemoryListener *listener, MemoryRegionSection *section,
> @@ -1269,6 +1270,22 @@ void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client);
>  void memory_region_set_dirty(MemoryRegion *mr, hwaddr addr,
>                               hwaddr size);
>  
> +/**
> + * memory_region_clear_dirty_bitmap - clear dirty bitmap for memory range
> + *
> + * This function is called when the caller wants to clear the remote
> + * dirty bitmap of a memory range within the memory region.  This can
> + * be used by e.g. KVM to manually clear dirty log when
> + * KVM_CAP_MANUAL_DIRTY_LOG_PROTECT is declared support by the host
> + * kernel.
> + *
> + * @mr:     the memory region to clear the dirty log upon
> + * @start:  start address offset within the memory region
> + * @len:    length of the memory region to clear dirty bitmap
> + */
> +void memory_region_clear_dirty_bitmap(MemoryRegion *mr, hwaddr start,
> +                                      hwaddr len);
> +
>  /**
>   * memory_region_snapshot_and_clear_dirty: Get a snapshot of the dirty
>   *                                         bitmap and clear it.
> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> index f8ee011d3c..f8930914cd 100644
> --- a/include/exec/ram_addr.h
> +++ b/include/exec/ram_addr.h
> @@ -461,6 +461,9 @@ uint64_t cpu_physical_memory_sync_dirty_bitmap(RAMBlock *rb,
>                  idx++;
>              }
>          }
> +
> +        /* TODO: split the huge bitmap into smaller chunks */
> +        memory_region_clear_dirty_bitmap(rb->mr, start, length);
>      } else {
>          ram_addr_t offset = rb->offset;
>  
> diff --git a/memory.c b/memory.c
> index 84bba7b65c..a051025dd1 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -2064,6 +2064,57 @@ static void memory_region_sync_dirty_bitmap(MemoryRegion *mr)
>      }
>  }
>  
> +void memory_region_clear_dirty_bitmap(MemoryRegion *mr, hwaddr start,
> +                                      hwaddr len)
> +{
> +    MemoryRegionSection mrs;
> +    MemoryListener *listener;
> +    AddressSpace *as;
> +    FlatView *view;
> +    FlatRange *fr;
> +    hwaddr sec_start, sec_end, sec_size;
> +
> +    QTAILQ_FOREACH(listener, &memory_listeners, link) {
> +        if (!listener->log_clear) {
> +            continue;
> +        }
> +        as = listener->address_space;
> +        view = address_space_get_flatview(as);
> +        FOR_EACH_FLAT_RANGE(fr, view) {
> +            if (!fr->dirty_log_mask || fr->mr != mr) {
> +                /*
> +                 * Clear dirty bitmap operation only applies to those
> +                 * regions whose dirty logging is at least enabled
> +                 */
> +                continue;
> +            }
> +
> +            mrs = section_from_flat_range(fr, view);
> +
> +            sec_start = MAX(mrs.offset_within_region, start);
> +            sec_end = mrs.offset_within_region + int128_get64(mrs.size);
> +            sec_end = MIN(sec_end, start + len);
> +
> +            if (sec_start >= sec_end) {
> +                /*
> +                 * If this memory region section has no intersection
> +                 * with the requested range, skip.
> +                 */
> +                continue;
> +            }
> +
> +            /* Valid case; shrink the section if needed */
> +            mrs.offset_within_address_space +=
> +                sec_start - mrs.offset_within_region;
> +            mrs.offset_within_region = sec_start;
> +            sec_size = sec_end - sec_start;
> +            mrs.size = int128_make64(sec_size);
> +            listener->log_clear(listener, &mrs);
> +        }
> +        flatview_unref(view);
> +    }
> +}
> +
>  DirtyBitmapSnapshot *memory_region_snapshot_and_clear_dirty(MemoryRegion *mr,
>                                                              hwaddr addr,
>                                                              hwaddr size,

I think that's ok (although some of the size juggling I only think I've
got).

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK