Re: [PATCH RFC 04/15] migration: Add postcopy_has_request()

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> Add a helper to detect whether postcopy has pending request.
> 
> Since at it, cleanup the code a bit, e.g. in unqueue_page() we shouldn't need
> to check it again on queue empty because we're the only one (besides cleanup
> code, which should never run during this process) that will take a request off
> the list, so the request list can only grow but not shrink under the hood.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

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

> ---
>  migration/ram.c | 45 ++++++++++++++++++++++++++++-----------------
>  1 file changed, 28 insertions(+), 17 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 94b0ad4234..dc6ba041fa 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -354,6 +354,12 @@ static RAMState *ram_state;
>  
>  static NotifierWithReturnList precopy_notifier_list;
>  
> +/* Whether postcopy has queued requests? */
> +static bool postcopy_has_request(RAMState *rs)
> +{
> +    return !QSIMPLEQ_EMPTY_ATOMIC(&rs->src_page_requests);
> +}
> +
>  void precopy_infrastructure_init(void)
>  {
>      notifier_with_return_list_init(&precopy_notifier_list);
> @@ -1533,28 +1539,33 @@ static bool find_dirty_block(RAMState *rs, 
> PageSearchStatus *pss, bool *again)
>   */
>  static RAMBlock *unqueue_page(RAMState *rs, ram_addr_t *offset)
>  {
> +    struct RAMSrcPageRequest *entry;
>      RAMBlock *block = NULL;
>  
> -    if (QSIMPLEQ_EMPTY_ATOMIC(&rs->src_page_requests)) {
> +    if (!postcopy_has_request(rs)) {
>          return NULL;
>      }
>  
>      QEMU_LOCK_GUARD(&rs->src_page_req_mutex);
> -    if (!QSIMPLEQ_EMPTY(&rs->src_page_requests)) {
> -        struct RAMSrcPageRequest *entry =
> -                                QSIMPLEQ_FIRST(&rs->src_page_requests);
> -        block = entry->rb;
> -        *offset = entry->offset;
> -
> -        if (entry->len > TARGET_PAGE_SIZE) {
> -            entry->len -= TARGET_PAGE_SIZE;
> -            entry->offset += TARGET_PAGE_SIZE;
> -        } else {
> -            memory_region_unref(block->mr);
> -            QSIMPLEQ_REMOVE_HEAD(&rs->src_page_requests, next_req);
> -            g_free(entry);
> -            migration_consume_urgent_request();
> -        }
> +
> +    /*
> +     * This should _never_ change even after we take the lock, because no one
> +     * should be taking anything off the request list other than us.
> +     */
> +    assert(postcopy_has_request(rs));
> +
> +    entry = QSIMPLEQ_FIRST(&rs->src_page_requests);
> +    block = entry->rb;
> +    *offset = entry->offset;
> +
> +    if (entry->len > TARGET_PAGE_SIZE) {
> +        entry->len -= TARGET_PAGE_SIZE;
> +        entry->offset += TARGET_PAGE_SIZE;
> +    } else {
> +        memory_region_unref(block->mr);
> +        QSIMPLEQ_REMOVE_HEAD(&rs->src_page_requests, next_req);
> +        g_free(entry);
> +        migration_consume_urgent_request();
>      }
>  
>      return block;
> @@ -2996,7 +3007,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>          t0 = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
>          i = 0;
>          while ((ret = qemu_file_rate_limit(f)) == 0 ||
> -                !QSIMPLEQ_EMPTY(&rs->src_page_requests)) {
> +               postcopy_has_request(rs)) {
>              int pages;
>  
>              if (qemu_file_get_error(f)) {
> -- 
> 2.32.0
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK