Re: [PATCH RFC 3/9] memory: Introduce log_sync_global() to memory listener

2020-03-25 Thread Peter Xu
On Wed, Mar 25, 2020 at 04:52:52PM +, Dr. David Alan Gilbert wrote:
> * Peter Xu (pet...@redhat.com) wrote:
> > Some of the memory listener may want to do log synchronization without
> > being able to specify a range of memory to sync but always globally.
> > Such a memory listener should provide this new method instead of the
> > log_sync() method.
> > 
> > Obviously we can also achieve similar thing when we put the global
> > sync logic into a log_sync() handler. However that's not efficient
> > enough because otherwise memory_global_dirty_log_sync() may do the
> > global sync N times, where N is the number of flat views.
> > 
> > Make this new method be exclusive to log_sync().
> > 
> > Signed-off-by: Peter Xu 
> 
> OK, so I guess the idea here is that when you have a ring with dirty
> pages on it, you just need to clear all outstanding things on the ring
> whereever they came from.

Yeah, or say it's about granularity of log_sync(), and the ring layout
will only be able to sync in a global manner rather than per-memslot.
It's actually not good when we only want to sync some specific small
memory region (e.g., the VGA code wants to only sync the VGA ram), but
we don't have much choice...

> 
> Reviewed-by: Dr. David Alan Gilbert 

Thanks!

-- 
Peter Xu




Re: [PATCH RFC 3/9] memory: Introduce log_sync_global() to memory listener

2020-03-25 Thread Dr. David Alan Gilbert
* Peter Xu (pet...@redhat.com) wrote:
> Some of the memory listener may want to do log synchronization without
> being able to specify a range of memory to sync but always globally.
> Such a memory listener should provide this new method instead of the
> log_sync() method.
> 
> Obviously we can also achieve similar thing when we put the global
> sync logic into a log_sync() handler. However that's not efficient
> enough because otherwise memory_global_dirty_log_sync() may do the
> global sync N times, where N is the number of flat views.
> 
> Make this new method be exclusive to log_sync().
> 
> Signed-off-by: Peter Xu 

OK, so I guess the idea here is that when you have a ring with dirty
pages on it, you just need to clear all outstanding things on the ring
whereever they came from.

Reviewed-by: Dr. David Alan Gilbert 

> ---
>  include/exec/memory.h | 12 
>  memory.c  | 33 +++--
>  2 files changed, 35 insertions(+), 10 deletions(-)
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index e85b7de99a..c4427094bb 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -533,6 +533,18 @@ struct MemoryListener {
>   */
>  void (*log_sync)(MemoryListener *listener, MemoryRegionSection *section);
>  
> +/**
> + * @log_sync_global:
> + *
> + * This is the global version of @log_sync when the listener does
> + * not have a way to synchronize the log with finer granularity.
> + * When the listener registers with @log_sync_global defined, then
> + * its @log_sync must be NULL.  Vice versa.
> + *
> + * @listener: The #MemoryListener.
> + */
> +void (*log_sync_global)(MemoryListener *listener);
> +
>  /**
>   * @log_clear:
>   *
> diff --git a/memory.c b/memory.c
> index aeaa8dcc9e..53828ba00c 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -2016,6 +2016,10 @@ void memory_region_set_dirty(MemoryRegion *mr, hwaddr 
> addr,
>  
> memory_region_get_dirty_log_mask(mr));
>  }
>  
> +/*
> + * If memory region `mr' is NULL, do global sync.  Otherwise, sync
> + * dirty bitmap for the specified memory region.
> + */
>  static void memory_region_sync_dirty_bitmap(MemoryRegion *mr)
>  {
>  MemoryListener *listener;
> @@ -2029,18 +2033,24 @@ static void 
> memory_region_sync_dirty_bitmap(MemoryRegion *mr)
>   * address space once.
>   */
>  QTAILQ_FOREACH(listener, _listeners, link) {
> -if (!listener->log_sync) {
> -continue;
> -}
> -as = listener->address_space;
> -view = address_space_get_flatview(as);
> -FOR_EACH_FLAT_RANGE(fr, view) {
> -if (fr->dirty_log_mask && (!mr || fr->mr == mr)) {
> -MemoryRegionSection mrs = section_from_flat_range(fr, view);
> -listener->log_sync(listener, );
> +if (listener->log_sync) {
> +as = listener->address_space;
> +view = address_space_get_flatview(as);
> +FOR_EACH_FLAT_RANGE(fr, view) {
> +if (fr->dirty_log_mask && (!mr || fr->mr == mr)) {
> +MemoryRegionSection mrs = section_from_flat_range(fr, 
> view);
> +listener->log_sync(listener, );
> +}
>  }
> +flatview_unref(view);
> +} else if (listener->log_sync_global) {
> +/*
> + * No matter whether MR is specified, what we can do here
> + * is to do a global sync, because we are not capable to
> + * sync in a finer granularity.
> + */
> +listener->log_sync_global(listener);
>  }
> -flatview_unref(view);
>  }
>  }
>  
> @@ -2727,6 +2737,9 @@ void memory_listener_register(MemoryListener *listener, 
> AddressSpace *as)
>  {
>  MemoryListener *other = NULL;
>  
> +/* Only one of them can be defined for a listener */
> +assert(!(listener->log_sync && listener->log_sync_global));
> +
>  listener->address_space = as;
>  if (QTAILQ_EMPTY(_listeners)
>  || listener->priority >= QTAILQ_LAST(_listeners)->priority) {
> -- 
> 2.24.1
> 
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK