Re: [PATCH v3 08/13] migration/ram: Simplify host page handling in ram_load_postcopy()

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

* David Hildenbrand (david@redhat.com) wrote:
> On 06.03.20 17:20, David Hildenbrand wrote:
> > On 06.03.20 17:05, Dr. David Alan Gilbert wrote:
> >> * David Hildenbrand (david@redhat.com) wrote:
> >>> Add two new helper functions. This will in come handy once we want to
> >>> handle ram block resizes while postcopy is active.
> >>>
> >>> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >>> Cc: Juan Quintela <quintela@redhat.com>
> >>> Cc: Peter Xu <peterx@redhat.com>
> >>> Signed-off-by: David Hildenbrand <david@redhat.com>
> >>> ---
> >>>  migration/ram.c | 54 ++++++++++++++++++++++++++++---------------------
> >>>  1 file changed, 31 insertions(+), 23 deletions(-)
> >>>
> >>> diff --git a/migration/ram.c b/migration/ram.c
> >>> index d5a4d69e1c..f815f4e532 100644
> >>> --- a/migration/ram.c
> >>> +++ b/migration/ram.c
> >>> @@ -2734,6 +2734,20 @@ static inline void *host_from_ram_block_offset(RAMBlock *block,
> >>>      return block->host + offset;
> >>>  }
> >>>  
> >>> +static void *host_page_from_ram_block_offset(RAMBlock *block,
> >>> +                                             ram_addr_t offset)
> >>> +{
> >>> +    /* Note: Explicitly no check against offset_in_ramblock(). */
> >>> +    return (void *)QEMU_ALIGN_DOWN((uintptr_t)block->host + offset,
> >>> +                                   block->page_size);
> >>> +}
> >>> +
> >>> +static ram_addr_t host_page_offset_from_ram_block_offset(RAMBlock *block,
> >>> +                                                         ram_addr_t offset)
> >>> +{
> >>> +    return ((uintptr_t)block->host + offset) & (block->page_size - 1);
> >>> +}
> >>> +
> >>>  static inline void *colo_cache_from_block_offset(RAMBlock *block,
> >>>                                                   ram_addr_t offset)
> >>>  {
> >>> @@ -3111,13 +3125,12 @@ static int ram_load_postcopy(QEMUFile *f)
> >>>      MigrationIncomingState *mis = migration_incoming_get_current();
> >>>      /* Temporary page that is later 'placed' */
> >>>      void *postcopy_host_page = mis->postcopy_tmp_page;
> >>> -    void *this_host = NULL;
> >>> +    void *host_page = NULL;
> >>>      bool all_zero = false;
> >>>      int target_pages = 0;
> >>>  
> >>>      while (!ret && !(flags & RAM_SAVE_FLAG_EOS)) {
> >>>          ram_addr_t addr;
> >>> -        void *host = NULL;
> >>>          void *page_buffer = NULL;
> >>>          void *place_source = NULL;
> >>>          RAMBlock *block = NULL;
> >>> @@ -3143,9 +3156,12 @@ static int ram_load_postcopy(QEMUFile *f)
> >>>          if (flags & (RAM_SAVE_FLAG_ZERO | RAM_SAVE_FLAG_PAGE |
> >>>                       RAM_SAVE_FLAG_COMPRESS_PAGE)) {
> >>>              block = ram_block_from_stream(f, flags);
> >>> +            if (!block) {
> >>> +                ret = -EINVAL;
> >>
> >> Could we have an error_report there, at the moment it would trigger
> >> the one below.
> > 
> > Makes sense, I'll add one!
> 
> My memory kicks in: This was dropped on purpose. ram_block_from_stream()
> will print proper errors already.

OK!

Dave

> Cheers!
> 
> 
> -- 
> Thanks,
> 
> David / dhildenb
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK