Re: [PATCH 10/14] migration: Make PageSearchStatus part of RAMState

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> We used to allocate PSS structure on the stack for precopy when sending
> pages.  Make it static, so as to describe per-channel ram migration status.
> 
> Here we declared RAM_CHANNEL_MAX instances, preparing for postcopy to use
> it, even though this patch has not yet to start using the 2nd instance.
> 
> This should not have any functional change per se, but it already starts to
> export PSS information via the RAMState, so that e.g. one PSS channel can
> start to reference the other PSS channel.
> 
> Always protect PSS access using the same RAMState.bitmap_mutex.  We already
> do so, so no code change needed, just some comment update.  Maybe we should
> consider renaming bitmap_mutex some day as it's going to be a more commonly
> and big mutex we use for ram states, but just leave it for later.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/ram.c | 112 ++++++++++++++++++++++++++----------------------
>  1 file changed, 61 insertions(+), 51 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index b4b36ca59e..dbe11e1ace 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -85,6 +85,46 @@
>  
>  XBZRLECacheStats xbzrle_counters;
>  
> +/* used by the search for pages to send */
> +struct PageSearchStatus {
> +    /* The migration channel used for a specific host page */
> +    QEMUFile    *pss_channel;
> +    /* Current block being searched */
> +    RAMBlock    *block;
> +    /* Current page to search from */
> +    unsigned long page;
> +    /* Set once we wrap around */
> +    bool         complete_round;
> +    /*
> +     * [POSTCOPY-ONLY] Whether current page is explicitly requested by
> +     * postcopy.  When set, the request is "urgent" because the dest QEMU
> +     * threads are waiting for us.
> +     */
> +    bool         postcopy_requested;
> +    /*
> +     * [POSTCOPY-ONLY] The target channel to use to send current page.
> +     *
> +     * Note: This may _not_ match with the value in postcopy_requested
> +     * above. Let's imagine the case where the postcopy request is exactly
> +     * the page that we're sending in progress during precopy. In this case
> +     * we'll have postcopy_requested set to true but the target channel
> +     * will be the precopy channel (so that we don't split brain on that
> +     * specific page since the precopy channel already contains partial of
> +     * that page data).
> +     *
> +     * Besides that specific use case, postcopy_target_channel should
> +     * always be equal to postcopy_requested, because by default we send
> +     * postcopy pages via postcopy preempt channel.
> +     */
> +    bool         postcopy_target_channel;
> +    /* Whether we're sending a host page */
> +    bool          host_page_sending;
> +    /* The start/end of current host page.  Invalid if 
> host_page_sending==false */
> +    unsigned long host_page_start;
> +    unsigned long host_page_end;
> +};
> +typedef struct PageSearchStatus PageSearchStatus;
> +
>  /* struct contains XBZRLE cache and a static page
>     used by the compression */
>  static struct {
> @@ -319,6 +359,11 @@ typedef struct {
>  struct RAMState {
>      /* QEMUFile used for this migration */
>      QEMUFile *f;
> +    /*
> +     * PageSearchStatus structures for the channels when send pages.
> +     * Protected by the bitmap_mutex.
> +     */
> +    PageSearchStatus pss[RAM_CHANNEL_MAX];

Why statically size this rather than allocate it in ram_state_init ?

Dave

>      /* UFFD file descriptor, used in 'write-tracking' migration */
>      int uffdio_fd;
>      /* Last block that we have visited searching for dirty pages */
> @@ -362,7 +407,12 @@ struct RAMState {
>      uint64_t target_page_count;
>      /* number of dirty bits in the bitmap */
>      uint64_t migration_dirty_pages;
> -    /* Protects modification of the bitmap and migration dirty pages */
> +    /*
> +     * Protects:
> +     * - dirty/clear bitmap
> +     * - migration_dirty_pages
> +     * - pss structures
> +     */
>      QemuMutex bitmap_mutex;
>      /* The RAMBlock used in the last src_page_requests */
>      RAMBlock *last_req_rb;
> @@ -444,46 +494,6 @@ void dirty_sync_missed_zero_copy(void)
>      ram_counters.dirty_sync_missed_zero_copy++;
>  }
>  
> -/* used by the search for pages to send */
> -struct PageSearchStatus {
> -    /* The migration channel used for a specific host page */
> -    QEMUFile    *pss_channel;
> -    /* Current block being searched */
> -    RAMBlock    *block;
> -    /* Current page to search from */
> -    unsigned long page;
> -    /* Set once we wrap around */
> -    bool         complete_round;
> -    /*
> -     * [POSTCOPY-ONLY] Whether current page is explicitly requested by
> -     * postcopy.  When set, the request is "urgent" because the dest QEMU
> -     * threads are waiting for us.
> -     */
> -    bool         postcopy_requested;
> -    /*
> -     * [POSTCOPY-ONLY] The target channel to use to send current page.
> -     *
> -     * Note: This may _not_ match with the value in postcopy_requested
> -     * above. Let's imagine the case where the postcopy request is exactly
> -     * the page that we're sending in progress during precopy. In this case
> -     * we'll have postcopy_requested set to true but the target channel
> -     * will be the precopy channel (so that we don't split brain on that
> -     * specific page since the precopy channel already contains partial of
> -     * that page data).
> -     *
> -     * Besides that specific use case, postcopy_target_channel should
> -     * always be equal to postcopy_requested, because by default we send
> -     * postcopy pages via postcopy preempt channel.
> -     */
> -    bool         postcopy_target_channel;
> -    /* Whether we're sending a host page */
> -    bool          host_page_sending;
> -    /* The start/end of current host page.  Only valid if 
> host_page_sending==true */
> -    unsigned long host_page_start;
> -    unsigned long host_page_end;
> -};
> -typedef struct PageSearchStatus PageSearchStatus;
> -
>  CompressionStats compression_counters;
>  
>  struct CompressParam {
> @@ -2627,7 +2637,7 @@ static int ram_save_host_page(RAMState *rs, 
> PageSearchStatus *pss)
>   */
>  static int ram_find_and_save_block(RAMState *rs)
>  {
> -    PageSearchStatus pss;
> +    PageSearchStatus *pss = &rs->pss[RAM_CHANNEL_PRECOPY];
>      int pages = 0;
>      bool again, found;
>  
> @@ -2648,11 +2658,11 @@ static int ram_find_and_save_block(RAMState *rs)
>          rs->last_page = 0;
>      }
>  
> -    pss_init(&pss, rs->last_seen_block, rs->last_page);
> +    pss_init(pss, rs->last_seen_block, rs->last_page);
>  
>      do {
>          again = true;
> -        found = get_queued_page(rs, &pss);
> +        found = get_queued_page(rs, pss);
>  
>          if (!found) {
>              /*
> @@ -2660,27 +2670,27 @@ static int ram_find_and_save_block(RAMState *rs)
>               * preempted precopy.  Otherwise find the next dirty bit.
>               */
>              if (postcopy_preempt_triggered(rs)) {
> -                postcopy_preempt_restore(rs, &pss, false);
> +                postcopy_preempt_restore(rs, pss, false);
>                  found = true;
>              } else {
>                  /* priority queue empty, so just search for something dirty 
> */
> -                found = find_dirty_block(rs, &pss, &again);
> +                found = find_dirty_block(rs, pss, &again);
>              }
>          }
>  
>          if (found) {
>              /* Update rs->f with correct channel */
>              if (postcopy_preempt_active()) {
> -                postcopy_preempt_choose_channel(rs, &pss);
> +                postcopy_preempt_choose_channel(rs, pss);
>              }
>              /* Cache rs->f in pss_channel (TODO: remove rs->f) */
> -            pss.pss_channel = rs->f;
> -            pages = ram_save_host_page(rs, &pss);
> +            pss->pss_channel = rs->f;
> +            pages = ram_save_host_page(rs, pss);
>          }
>      } while (!pages && again);
>  
> -    rs->last_seen_block = pss.block;
> -    rs->last_page = pss.page;
> +    rs->last_seen_block = pss->block;
> +    rs->last_page = pss->page;
>  
>      return pages;
>  }
> -- 
> 2.32.0
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK